Typo in the patch title.
On 2021/02/02 14:26, Chaitanya Kulkarni wrote:
> null blk driver supports REQ_OP_DISACRD in membacked mode. When testing
> special requests having REQ_OP_DISCARD support is helpful to validate
> the special request path when mixed with read-write.
>
> Consider module parameter when setting the queue discard flag when
> device is not memory backed.
>
> This is needed to test the tracepoint related to REQ_OP_DISCARD.
>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@xxxxxxx>
> ---
> # modprobe -r null_blk
> # gitlog -7
> 4ac4d49e1028 (HEAD -> for-next) null_blk: add module param queue bounce limit
> 468e3617dae8 null_blk: allow disacrd on non-membacked mode
> bdc96efe9681 block: get rid of the trace rq insert wrapper
> 73bf523a7ce4 blktrace: fix blk_rq_merge documentation
> 6016632555da blktrace: fix blk_rq_issue documentation
> 58b5d7103a94 blktrace: add blk_fill_rwbs documentation comment
> 534f321f57dd block: remove superfluous param in blk_fill_rwbs()
>
> Test to exercise module parameter vs configfs discard param, set up
> nullb0 such a way that it errors out in both cases for module param
> discard :-
>
> for discard in 0 1
> do
> modprobe -r null_blk
> i=0
> echo "Module param discard ${discard}"
> modprobe null_blk nr_devices=0 discard=${discard}
> # Create dev 0 no discard
> NULLB_DIR=config/nullb/nullb${i}
> mkdir config/nullb/nullb${i}
> echo 1 > config/nullb/nullb${i}/memory_backed
> echo 512 > config/nullb/nullb${i}/blocksize
> echo 2048 > config/nullb/nullb${i}/size
> echo 1 > config/nullb/nullb${i}/power
> echo -n "$nullb${i} membacked : ";
> cat /sys/kernel/config/nullb/nullb${i}/memory_backed
> echo -n "$nullb${i} discard : ";
> cat /sys/kernel/config/nullb/nullb${i}/discard
> # Create dev 1 with discard
> i=1
> NULLB_DIR=config/nullb/nullb${i}
> mkdir config/nullb/nullb${i}
> echo 1 > config/nullb/nullb${i}/memory_backed
> echo 512 > config/nullb/nullb${i}/blocksize
> echo 2048 > config/nullb/nullb${i}/size
> echo 1 > config/nullb/nullb${i}/discard
> echo 1 > config/nullb/nullb${i}/power
> echo -n "$nullb${i} membacked : ";
> cat /sys/kernel/config/nullb/nullb${i}/memory_backed
> echo -n "$nullb${i} discard : ";
> cat /sys/kernel/config/nullb/nullb${i}/discard
>
> # should fail
> blkdiscard -o 0 -l 1024 /dev/nullb0
> # should pass
> blkdiscard -o 0 -l 1024 /dev/nullb1
>
> echo 0 > config/nullb/nullb0/power
> echo 0 > config/nullb/nullb1/power
> rmdir config/nullb/nullb*
>
> modprobe -r null_blk
> modprobe null_blk
> # should fail
> blkdiscard -o 0 -l 1024 /dev/nullb0
> modprobe -r null_blk
> modprobe null_blk discard=1
> # should pass
> blkdiscard -o 0 -l 1024 /dev/nullb0
> modprobe -r null_blk
> echo "--------------------------"
> done
>
> modprobe -r null_blk
> modprobe null_blk
> blkdiscard -o 0 -l 1024 /dev/nullb0
> modprobe -r null_blk
> modprobe null_blk discard=1
> blkdiscard -o 0 -l 1024 /dev/nullb0
> modprobe -r null_blk
>
> # ./discard_module_param_test.sh
> Module param discard 0
> 0 membacked : 1
> 0 discard : 0
> 1 membacked : 1
> 1 discard : 1
> blkdiscard: /dev/nullb0: BLKDISCARD ioctl failed: Operation not supported
> blkdiscard: /dev/nullb0: BLKDISCARD ioctl failed: Operation not supported
> --------------------------
> Module param discard 1
> 0 membacked : 1
> 0 discard : 0
> 1 membacked : 1
> 1 discard : 1
> blkdiscard: /dev/nullb0: BLKDISCARD ioctl failed: Operation not supported
> blkdiscard: /dev/nullb0: BLKDISCARD ioctl failed: Operation not supported
> --------------------------
> blkdiscard: /dev/nullb0: BLKDISCARD ioctl failed: Operation not supported
> ---
> drivers/block/null_blk/main.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index d6c821d48090..6e6cbb953a12 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -172,6 +172,10 @@ static bool g_shared_tag_bitmap;
> module_param_named(shared_tag_bitmap, g_shared_tag_bitmap, bool, 0444);
> MODULE_PARM_DESC(shared_tag_bitmap, "Use shared tag bitmap for all submission queues for blk-mq");
>
> +static bool g_discard;
> +module_param_named(discard, g_discard, bool, 0444);
> +MODULE_PARM_DESC(discard, "Enable queue discard (default: false)");
> +
> static int g_irqmode = NULL_IRQ_SOFTIRQ;
>
> static int null_set_irqmode(const char *str, const struct kernel_param *kp)
> @@ -1588,14 +1592,11 @@ static void null_del_dev(struct nullb *nullb)
>
> static void null_config_discard(struct nullb *nullb)
> {
> - if (nullb->dev->discard == false)
> + if (nullb->dev->memory_backed && nullb->dev->discard == false)
What is the point of adding nullb->dev->memory_backed to the test ? If
nullb->dev->memory_backed is true, nullb->dev->discard should be forced to true
also to retain the behavior we had until now.
> return;
>
> - if (!nullb->dev->memory_backed) {
> - nullb->dev->discard = false;
> - pr_info("discard option is ignored without memory backing\n");
> + if (!nullb->dev->memory_backed && !g_discard)
I do not understand this one either. Why look at g_discard ?
> return;
> - }
>
> if (nullb->dev->zoned) {
> nullb->dev->discard = false;
>
--
Damien Le Moal
Western Digital Research
