null_blk: add simple write-zeroes support

PATCH In review Chaitanya Kulkarni 4 participants 6 messages Feb 26, 2024 – Feb 27, 2024 Block layer

Share & tools

Chaitanya Kulkarni

author

2024-02-26 07:15 UTC #0 lore raw

PATCH 1/1 null_blk: add simple write-zeroes support

To test the REQ_OP_WRITE_ZEROES command and fatal signal handling in the code path starting from blkdev_issue_zeroout(), add a new module parameter when null_blk module is loaded in non-memory-backed mode.

Disable REQ_OP_WRITE_ZEROES by default to retain the existing behavior.

without this patch :-

linux-block (for-next) # modprobe null_blk
linux-block (for-next) # blkdiscard -z -o 0 -l 40960 /dev/nullb0 linux-block (for-next) # dmesg -c

[24977.282226] null_blk: null_process_cmd 1337 WRITE

with this patch :-

linux-block (for-next) # modprobe null_blk write_zeroes=1 linux-block (for-next) # blkdiscard -z -o 0 -l 40960 /dev/nullb0 linux-block (for-next) # dmesg -c

[25009.164341] null_blk: null_process_cmd 1337 WRITE_ZEROES

Signed-off-by Chaitanya Kulkarni <[email protected]>

---
 drivers/block/null_blk/main.c     | 14 ++++++++++++--
 drivers/block/null_blk/null_blk.h |  1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

drivers/block/null_blk/main.c +12 -2

index 71c39bcd872c..b454f6e6c60a 100644
@@ -221,6 +221,10 @@ static bool g_discard;
 module_param_named(discard, g_discard, bool, 0444);
 MODULE_PARM_DESC(discard, "Support discard operations (requires memory-backed null_blk device). Default: false");
 
+static bool g_write_zeroes;
+module_param_named(write_zeroes, g_write_zeroes, bool, 0444);
+MODULE_PARM_DESC(write_zeroes, "Support write-zeroes operations (requires non-memory-backed null_blk device). Default: false");
+
 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)");

@@ -742,6 +746,7 @@ static struct nullb_device *null_alloc_dev(void)
 	dev->blocking = g_blocking;
 	dev->memory_backed = g_memory_backed;
 	dev->discard = g_discard;
+	dev->write_zeroes = g_write_zeroes;
 	dev->cache_size = g_cache_size;
 	dev->mbps = g_mbps;
 	dev->use_per_node_hctx = g_use_per_node_hctx;

@@ -1684,8 +1689,13 @@ static void null_del_dev(struct nullb *nullb)
 	dev->nullb = NULL;
 }
 
-static void null_config_discard(struct nullb *nullb, struct queue_limits *lim)
+static void null_config_discard_write_zeroes(struct nullb *nullb,
+					     struct queue_limits *lim)
 {
+	/* REQ_OP_WRITE_ZEROES only supported in non memory backed mode */
+	if (!nullb->dev->memory_backed && nullb->dev->write_zeroes)
+		lim->max_write_zeroes_sectors = UINT_MAX >> 9;
+
 	if (nullb->dev->discard == false)
 		return;

@@ -1891,7 +1901,7 @@ static int null_add_dev(struct nullb_device *dev)
 
 	if (dev->virt_boundary)
 		lim.virt_boundary_mask = PAGE_SIZE - 1;
-	null_config_discard(nullb, &lim);
+	null_config_discard_write_zeroes(nullb, &lim);
 	if (dev->zoned) {
 		rv = null_init_zoned_dev(dev, &lim);
 		if (rv)

drivers/block/null_blk/null_blk.h +1 -0

index 477b97746823..8518b4bf2530 100644
@@ -99,6 +99,7 @@ struct nullb_device {
 	bool power; /* power on/off the device */
 	bool memory_backed; /* if data is stored in memory */
 	bool discard; /* if support discard */
+	bool write_zeroes; /* if support write_zeroes */
 	bool zoned; /* if device is zoned */
 	bool virt_boundary; /* virtual boundary on/off for the device */
 	bool no_sched; /* no IO scheduler for the device */

Signature · 1 line

2.40.0

Johannes Thumshirn

2024-02-26 12:20 UTC #1 lore raw

Quoted 4 lines · Chaitanya Kulkarni

drivers/block/null_blk/main.c +0 -0

@@ -1684,8 +1689,13 @@ static void null_del_dev(struct nullb *nullb)
  	dev->nullb = NULL;
  }

drivers/block/null_blk/main.c static void null_del_dev(struct nullb *nullb) · line 1700

Quoted 10 lines

drivers/block/null_blk/main.c +6 -1

-static void null_config_discard(struct nullb *nullb, struct queue_limits *lim)
+static void null_config_discard_write_zeroes(struct nullb *nullb,
+					     struct queue_limits *lim)
  {
+	/* REQ_OP_WRITE_ZEROES only supported in non memory backed mode */
+	if (!nullb->dev->memory_backed && nullb->dev->write_zeroes)
+		lim->max_write_zeroes_sectors = UINT_MAX >> 9;
+
  	if (nullb->dev->discard == false)
  		return;

Please use SECTOR_SHIFT instead of the magic '9'.

Chaitanya Kulkarni

2024-02-26 22:02 UTC #2 lore raw

Quoted 5 lines · Johannes Thumshirn

Quoted 4 lines · Chaitanya Kulkarni
drivers/block/null_blk/main.c +0 -0
@@ -1684,8 +1689,13 @@ static void null_del_dev(struct nullb *nullb)
   	dev->nullb = NULL;
   }
 

Quoted 11 lines

drivers/block/null_blk/main.c static void null_del_dev(struct nullb *nullb) · line 1700

Quoted 10 lines

drivers/block/null_blk/main.c +6 -1

-static void null_config_discard(struct nullb *nullb, struct queue_limits *lim)
+static void null_config_discard_write_zeroes(struct nullb *nullb,
+					     struct queue_limits *lim)
   {
+	/* REQ_OP_WRITE_ZEROES only supported in non memory backed mode */
+	if (!nullb->dev->memory_backed && nullb->dev->write_zeroes)
+		lim->max_write_zeroes_sectors = UINT_MAX >> 9;
+
   	if (nullb->dev->discard == false)
   		return;

Please use SECTOR_SHIFT instead of the magic '9'.

In past I've been told explicitly to not use SECTOR_SHIFT. That also follows existing code in the function where SECTOR_SHIFT is not used [1].

Now I'm really confused :(.

-ck

[1]

static void null_config_discard(struct nullb *nullb, struct queue_limits *lim)
{
if (nullb->dev->discard == false)
return;

        if (!nullb->dev->memory_backed) { nullb->dev->discard = false; pr_info("discard option is ignored without memory
backing\n");
                return;
        }

        if (nullb->dev->zoned) {
                nullb->dev->discard = false; pr_info("discard option is ignored in zoned mode\n"); return;
        }

lim->max_hw_discard_sectors = UINT_MAX >> 9;
}

Johannes Thumshirn

2024-02-27 11:21 UTC #3 lore raw

Quoted 18 lines · Chaitanya Kulkarni

Quoted 5 lines · Johannes Thumshirn
Quoted 4 lines · Chaitanya Kulkarni
drivers/block/null_blk/main.c +0 -0
@@ -1684,8 +1689,13 @@ static void null_del_dev(struct nullb *nullb)
    	dev->nullb = NULL;
    }
 
Quoted 11 lines
drivers/block/null_blk/main.c static void null_del_dev(struct nullb *nullb) · line 1700
Quoted 10 lines
drivers/block/null_blk/main.c +6 -1
-static void null_config_discard(struct nullb *nullb, struct queue_limits *lim)
+static void null_config_discard_write_zeroes(struct nullb *nullb,
+					     struct queue_limits *lim)
    {
+	/* REQ_OP_WRITE_ZEROES only supported in non memory backed mode */
+	if (!nullb->dev->memory_backed && nullb->dev->write_zeroes)
+		lim->max_write_zeroes_sectors = UINT_MAX >> 9;
+
    	if (nullb->dev->discard == false)
    		return;
Please use SECTOR_SHIFT instead of the magic '9'.

Quoted 5 lines

In past I've been told explicitly to not use SECTOR_SHIFT. That also follows existing code in the function where SECTOR_SHIFT is not used [1].
Now I'm really confused :(.

I'd really prefer not to have the magic 9 but SECTOR_SHIFT, OTOH as null_blk is a debug driver and not something people unfamiliar with the block layer are looking at I guess we can keep the 9s :/

Damien Le Moal

2024-02-26 13:07 UTC #4 lore raw

In reply to Chaitanya Kulkarni

Quoted 31 lines · Chaitanya Kulkarni

To test the REQ_OP_WRITE_ZEROES command and fatal signal handling in the code path starting from blkdev_issue_zeroout(), add a new module parameter when null_blk module is loaded in non-memory-backed mode.
Disable REQ_OP_WRITE_ZEROES by default to retain the existing behavior.
without this patch :-
linux-block (for-next) # modprobe null_blk
linux-block (for-next) # blkdiscard -z -o 0 -l 40960 /dev/nullb0 linux-block (for-next) # dmesg -c
[24977.282226] null_blk: null_process_cmd 1337 WRITE
with this patch :-
linux-block (for-next) # modprobe null_blk write_zeroes=1 linux-block (for-next) # blkdiscard -z -o 0 -l 40960 /dev/nullb0 linux-block (for-next) # dmesg -c
[25009.164341] null_blk: null_process_cmd 1337 WRITE_ZEROES
Signed-off-by Chaitanya Kulkarni <[email protected]>
---
 drivers/block/null_blk/main.c     | 14 ++++++++++++--
 drivers/block/null_blk/null_blk.h |  1 +
 2 files changed, 13 insertions(+), 2 deletions(-)
drivers/block/null_blk/main.c +0 -0
index 71c39bcd872c..b454f6e6c60a 100644

drivers/block/null_blk/main.c static bool g_discard; · line 226

Quoted 6 lines

drivers/block/null_blk/main.c +3 -0

@@ -221,6 +221,10 @@ static bool g_discard;
 module_param_named(discard, g_discard, bool, 0444);
 MODULE_PARM_DESC(discard, "Support discard operations (requires memory-backed null_blk device). Default: false");
 
+static bool g_write_zeroes;
+module_param_named(write_zeroes, g_write_zeroes, bool, 0444);
+MODULE_PARM_DESC(write_zeroes, "Support write-zeroes operations (requires non-memory-backed null_blk device). Default: false");

Supporting memory backed devices is not that hard. Why not add it ? And while at it, we could add discard support for memory backed devices as well.

drivers/block/null_blk/main.c static bool g_discard; · line 227

Quoted 1 line

drivers/block/null_blk/main.c +1 -0
+

No need for the whiteline here to stay consistent with style.

Please also add the equivalent parameter for the configfs interface so that the same devices can be created with both modprobe and through configfs.

Also, instead of a bool argument, why not define this as a "write_zeroes_sectors" which defaults to 0 (disable) ? Doing so, the property is more generic and not only allows defining thr write zeroes suported (write zeroes sectors > 0) and not supported (write zeroes sectors == 0) but also set the maximum size of write zeroes operations.

(note that the same could be said for discard...)

Quoted 47 lines

drivers/block/null_blk/main.c +8 -2

 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)");

@@ -742,6 +746,7 @@ static struct nullb_device *null_alloc_dev(void)
 	dev->blocking = g_blocking;
 	dev->memory_backed = g_memory_backed;
 	dev->discard = g_discard;
+	dev->write_zeroes = g_write_zeroes;
 	dev->cache_size = g_cache_size;
 	dev->mbps = g_mbps;
 	dev->use_per_node_hctx = g_use_per_node_hctx;

@@ -1684,8 +1689,13 @@ static void null_del_dev(struct nullb *nullb)
 	dev->nullb = NULL;
 }
 
-static void null_config_discard(struct nullb *nullb, struct queue_limits *lim)
+static void null_config_discard_write_zeroes(struct nullb *nullb,
+					     struct queue_limits *lim)
 {
+	/* REQ_OP_WRITE_ZEROES only supported in non memory backed mode */
+	if (!nullb->dev->memory_backed && nullb->dev->write_zeroes)
+		lim->max_write_zeroes_sectors = UINT_MAX >> 9;
+
 	if (nullb->dev->discard == false)
 		return;

@@ -1891,7 +1901,7 @@ static int null_add_dev(struct nullb_device *dev)
 
 	if (dev->virt_boundary)
 		lim.virt_boundary_mask = PAGE_SIZE - 1;
-	null_config_discard(nullb, &lim);
+	null_config_discard_write_zeroes(nullb, &lim);
 	if (dev->zoned) {
 		rv = null_init_zoned_dev(dev, &lim);
 		if (rv)

drivers/block/null_blk/null_blk.h +1 -0

index 477b97746823..8518b4bf2530 100644
@@ -99,6 +99,7 @@ struct nullb_device {
 	bool power; /* power on/off the device */
 	bool memory_backed; /* if data is stored in memory */
 	bool discard; /* if support discard */
+	bool write_zeroes; /* if support write_zeroes */
 	bool zoned; /* if device is zoned */
 	bool virt_boundary; /* virtual boundary on/off for the device */
 	bool no_sched; /* no IO scheduler for the device */

Signature · 2 lines

Damien Le Moal
Western Digital Research

Chaitanya Kulkarni

2024-02-26 22:18 UTC #5 lore raw

Quoted 35 lines · Damien Le Moal

Quoted 34 lines · Chaitanya Kulkarni
To test the REQ_OP_WRITE_ZEROES command and fatal signal handling in the code path starting from blkdev_issue_zeroout(), add a new module parameter when null_blk module is loaded in non-memory-backed mode.
Disable REQ_OP_WRITE_ZEROES by default to retain the existing behavior.
without this patch :-
linux-block (for-next) # modprobe null_blk
linux-block (for-next) # blkdiscard -z -o 0 -l 40960 /dev/nullb0 linux-block (for-next) # dmesg -c
[24977.282226] null_blk: null_process_cmd 1337 WRITE
with this patch :-
linux-block (for-next) # modprobe null_blk write_zeroes=1 linux-block (for-next) # blkdiscard -z -o 0 -l 40960 /dev/nullb0 linux-block (for-next) # dmesg -c
[25009.164341] null_blk: null_process_cmd 1337 WRITE_ZEROES
Signed-off-by Chaitanya Kulkarni <[email protected]>
---
  drivers/block/null_blk/main.c     | 14 ++++++++++++--
  drivers/block/null_blk/null_blk.h |  1 +
  2 files changed, 13 insertions(+), 2 deletions(-)
drivers/block/null_blk/main.c +0 -0
index 71c39bcd872c..b454f6e6c60a 100644
@@ -221,6 +221,10 @@ static bool g_discard;
  module_param_named(discard, g_discard, bool, 0444);
  MODULE_PARM_DESC(discard, "Support discard operations (requires memory-backed null_blk device). Default: false");
 

Quoted 5 lines

drivers/block/null_blk/main.c static bool g_discard; · line 226
Quoted 3 lines
drivers/block/null_blk/main.c +3 -0
+static bool g_write_zeroes;
+module_param_named(write_zeroes, g_write_zeroes, bool, 0444);
+MODULE_PARM_DESC(write_zeroes, "Support write-zeroes operations (requires non-memory-backed null_blk device). Default: false");
Supporting memory backed devices is not that hard. Why not add it ? And while at it, we could add discard support for memory backed devices as well.

Agree, I just needed something to add a test fatal signgl for write-zeroes that doesn't require membacked write-zeroes support, I'll add it to V2.

For null_blk, discard is only supported in membacked mode [1], by any chance you meant we should make discard support for non membacked mode ?

This will also help fatal signal testing and for __blkdev_issue_discard().

Quoted 5 lines

drivers/block/null_blk/main.c static bool g_discard; · line 227
Quoted 1 line
drivers/block/null_blk/main.c +1 -0
+
No need for the whiteline here to stay consistent with style.
Please also add the equivalent parameter for the configfs interface so that the same devices can be created with both modprobe and through configfs.

I'll add in V2 ..

Quoted 8 lines

Also, instead of a bool argument, why not define this as a "write_zeroes_sectors" which defaults to 0 (disable) ? Doing so, the property is more generic and not only allows defining thr write zeroes suported (write zeroes sectors > 0) and not supported (write zeroes sectors == 0) but also set the maximum size of write zeroes operations.
(note that the same could be said for discard...)

trying to keep existing implementation consistent with discard for now, how about we add a separate patch to make discard and write-zeroes to accept the sectors, with maintaining the backward compatibility for the discard ?

Thanks for the reviews Damien and Johannes, I'll send out V2 once we resolve write_zeroes_sectors comment.

-ck

[1]
static void null_config_discard(struct nullb *nullb, struct queue_limits *lim)
{
if (nullb->dev->discard == false)
return;

        if (!nullb->dev->memory_backed) { nullb->dev->discard = false; pr_info("discard option is ignored without memory
backing\n");
                return;
        }

        if (nullb->dev->zoned) {
                nullb->dev->discard = false; pr_info("discard option is ignored in zoned mode\n"); return;
        }

lim->max_hw_discard_sectors = UINT_MAX >> 9;
}