| Message ID | 20240226071355.16723-1-kch@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: add simple write-zeroes support | expand |
On 26.02.24 08:15, Chaitanya Kulkarni wrote: > @@ -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; Please use SECTOR_SHIFT instead of the magic '9'.
On 2024/02/25 23:13, Chaitanya Kulkarni wrote: > 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 <kch@nvidia.com> > --- > drivers/block/null_blk/main.c | 14 ++++++++++++-- > drivers/block/null_blk/null_blk.h | 1 + > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index 71c39bcd872c..b454f6e6c60a 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -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. > + 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...) > 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) > diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h > index 477b97746823..8518b4bf2530 100644 > --- a/drivers/block/null_blk/null_blk.h > +++ b/drivers/block/null_blk/null_blk.h > @@ -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 */
On 2/26/24 04:20, Johannes Thumshirn wrote: > On 26.02.24 08:15, Chaitanya Kulkarni wrote: >> @@ -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; > 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; }
On 2/26/24 05:07, Damien Le Moal wrote: > On 2024/02/25 23:13, Chaitanya Kulkarni wrote: >> 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 <kch@nvidia.com> >> --- >> drivers/block/null_blk/main.c | 14 ++++++++++++-- >> drivers/block/null_blk/null_blk.h | 1 + >> 2 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c >> index 71c39bcd872c..b454f6e6c60a 100644 >> --- a/drivers/block/null_blk/main.c >> +++ b/drivers/block/null_blk/main.c >> @@ -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. 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(). >> + > 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 .. > 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; }
On 26.02.24 23:03, Chaitanya Kulkarni wrote: > On 2/26/24 04:20, Johannes Thumshirn wrote: >> On 26.02.24 08:15, Chaitanya Kulkarni wrote: >>> @@ -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; >> 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 :(. 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 :/
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 71c39bcd872c..b454f6e6c60a 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -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) diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h index 477b97746823..8518b4bf2530 100644 --- a/drivers/block/null_blk/null_blk.h +++ b/drivers/block/null_blk/null_blk.h @@ -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 */
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 <kch@nvidia.com> --- drivers/block/null_blk/main.c | 14 ++++++++++++-- drivers/block/null_blk/null_blk.h | 1 + 2 files changed, 13 insertions(+), 2 deletions(-)