Re: [PATCH 6/7] null_blk: allow disacrd on non-membacked mode

From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>
Cc: "paolo.valente@linaro.org" <paolo.valente@linaro.org>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"rostedt@goodmis.org" <rostedt@goodmis.org>,
	"mingo@redhat.com" <mingo@redhat.com>,
	Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
	"bvanassche@acm.org" <bvanassche@acm.org>,
	"dongli.zhang@oracle.com" <dongli.zhang@oracle.com>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	Aravind Ramesh <Aravind.Ramesh@wdc.com>,
	"joshi.k@samsung.com" <joshi.k@samsung.com>,
	Niklas Cassel <Niklas.Cassel@wdc.com>,
	"hare@suse.de" <hare@suse.de>, "colyli@suse.de" <colyli@suse.de>,
	"tj@kernel.org" <tj@kernel.org>,
	"rdunlap@infradead.org" <rdunlap@infradead.org>,
	"jack@suse.cz" <jack@suse.cz>, "hch@lst.de" <hch@lst.de>
Subject: Re: [PATCH 6/7] null_blk: allow disacrd on non-membacked mode
Date: Tue, 2 Feb 2021 09:08:53 +0000	[thread overview]
Message-ID: <BL0PR04MB6514BB3CE744EDC43058B456E7B59@BL0PR04MB6514.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20210202052544.4108-7-chaitanya.kulkarni@wdc.com

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@wdc.com>
> ---
> # 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