[v4,17/28] null_blk: Introduce fua attribute

Commit Message

Damien Le Moal April 2, 2024, 12:38 p.m. UTC

Add the fua configfs attribute and module parameter to allow
configuring if the device supports FUA or not. Using this attribute
has an effect on the null_blk device only if memory backing is enabled
together with a write cache (cache_size option).

This new attribute allows configuring a null_blk device with a write
cache but without FUA support. This is convenient to test the block
layer flush machinery.

Signed-off-by: Damien Le Moal <[e-mail]>
Reviewed-by: Hannes Reinecke <[e-mail]>
Reviewed-by: Christoph Hellwig <[e-mail]>
Reviewed-by: Bart Van Assche <[e-mail]>
Reviewed-by: Chaitanya Kulkarni <[e-mail]>
---
 drivers/block/null_blk/main.c     | 12 ++++++++++--
 drivers/block/null_blk/null_blk.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

Comments

Chaitanya Kulkarni April 3, 2024, 3:56 a.m. UTC | #1

Damien,

On 4/2/24 05:38, Damien Le Moal wrote:
>   
> +static bool g_fua = true;
> +module_param_named(fua, g_fua, bool, S_IRUGO);
> +MODULE_PARM_DESC(zoned, "Enable/disable FUA support when cache_size is used. Default: true");
> +

checkpatch is generating warning on this patch, please check :-

WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider 
using octal permissions '0444'.
#31: FILE: drivers/block/null_blk/main.c:229:
+module_param_named(fua, g_fua, bool, S_IRUGO);

Also, I noticed that for zone_append_max_sectors attribute patch
you are using 0444 but for fua you are using S_IRUGO, any specific
reason ?

-ck

Damien Le Moal April 3, 2024, 3:58 a.m. UTC | #2

On 4/3/24 12:56, Chaitanya Kulkarni wrote:
> Damien,
> 
> On 4/2/24 05:38, Damien Le Moal wrote:
>>   +static bool g_fua = true;
>> +module_param_named(fua, g_fua, bool, S_IRUGO);
>> +MODULE_PARM_DESC(zoned, "Enable/disable FUA support when cache_size is used.
>> Default: true");
>> +
> 
> checkpatch is generating warning on this patch, please check :-
> 
> WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal
> permissions '0444'.
> #31: FILE: drivers/block/null_blk/main.c:229:
> +module_param_named(fua, g_fua, bool, S_IRUGO);
> 
> Also, I noticed that for zone_append_max_sectors attribute patch
> you are using 0444 but for fua you are using S_IRUGO, any specific
> reason ?

No particular reason. I probably followed the pattern around the code when I
added it.

Personnally, I find this checkpatch warning about S_IRUGO silly as it is far
more readable than 0444... Just my 2 cents. I can make the change if you insist.

> 
> -ck
> 
>

Christoph Hellwig April 3, 2024, 4:43 a.m. UTC | #3

On Wed, Apr 03, 2024 at 12:58:51PM +0900, Damien Le Moal wrote:
> No particular reason. I probably followed the pattern around the code when I
> added it.
> 
> Personnally, I find this checkpatch warning about S_IRUGO silly as it is far
> more readable than 0444... Just my 2 cents. I can make the change if you insist.

Checkpatch is useless.  Follow the code around it.

Patch

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index a5a50ba6ad9f..1dfaf2628007 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -225,6 +225,10 @@  static unsigned long g_cache_size;
 module_param_named(cache_size, g_cache_size, ulong, 0444);
 MODULE_PARM_DESC(mbps, "Cache size in MiB for memory-backed device. Default: 0 (none)");
 
+static bool g_fua = true;
+module_param_named(fua, g_fua, bool, S_IRUGO);
+MODULE_PARM_DESC(zoned, "Enable/disable FUA support when cache_size is used. Default: true");
+
 static unsigned int g_mbps;
 module_param_named(mbps, g_mbps, uint, 0444);
 MODULE_PARM_DESC(mbps, "Limit maximum bandwidth (in MiB/s). Default: 0 (no limit)");
@@ -446,6 +450,7 @@  NULLB_DEVICE_ATTR(virt_boundary, bool, NULL);
 NULLB_DEVICE_ATTR(no_sched, bool, NULL);
 NULLB_DEVICE_ATTR(shared_tags, bool, NULL);
 NULLB_DEVICE_ATTR(shared_tag_bitmap, bool, NULL);
+NULLB_DEVICE_ATTR(fua, bool, NULL);
 
 static ssize_t nullb_device_power_show(struct config_item *item, char *page)
 {
@@ -593,6 +598,7 @@  static struct configfs_attribute *nullb_device_attrs[] = {
 	&nullb_device_attr_no_sched,
 	&nullb_device_attr_shared_tags,
 	&nullb_device_attr_shared_tag_bitmap,
+	&nullb_device_attr_fua,
 	NULL,
 };
 
@@ -671,7 +677,7 @@  nullb_group_drop_item(struct config_group *group, struct config_item *item)
 static ssize_t memb_group_features_show(struct config_item *item, char *page)
 {
 	return snprintf(page, PAGE_SIZE,
-			"badblocks,blocking,blocksize,cache_size,"
+			"badblocks,blocking,blocksize,cache_size,fua,"
 			"completion_nsec,discard,home_node,hw_queue_depth,"
 			"irqmode,max_sectors,mbps,memory_backed,no_sched,"
 			"poll_queues,power,queue_mode,shared_tag_bitmap,"
@@ -763,6 +769,8 @@  static struct nullb_device *null_alloc_dev(void)
 	dev->no_sched = g_no_sched;
 	dev->shared_tags = g_shared_tags;
 	dev->shared_tag_bitmap = g_shared_tag_bitmap;
+	dev->fua = g_fua;
+
 	return dev;
 }
 
@@ -1920,7 +1928,7 @@  static int null_add_dev(struct nullb_device *dev)
 
 	if (dev->cache_size > 0) {
 		set_bit(NULLB_DEV_FL_CACHE, &nullb->dev->flags);
-		blk_queue_write_cache(nullb->q, true, true);
+		blk_queue_write_cache(nullb->q, true, dev->fua);
 	}
 
 	nullb->q->queuedata = nullb;
diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h
index a9c5df650ddb..3234e6c85eed 100644
--- a/drivers/block/null_blk/null_blk.h
+++ b/drivers/block/null_blk/null_blk.h
@@ -105,6 +105,7 @@  struct nullb_device {
 	bool no_sched; /* no IO scheduler for the device */
 	bool shared_tags; /* share tag set between devices for blk-mq */
 	bool shared_tag_bitmap; /* use hostwide shared tags */
+	bool fua; /* Support FUA */
 };
 
 struct nullb {