Re: [PATCH] null_blk: add simple write-zeroes support

[Chaitanya Kulkarni <chaitanyak@xxxxxxxxxx>]

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@xxxxxxxxxx>
>> ---
>>   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;
}