[PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Ming Lei]

With the introduced module parameter of 'use_blk_mq', it is easy
to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
module, so that we can test scsi_mq/blk_mq related regressions easily.

Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Omar Sandoval <osandov@fb.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/scsi_debug.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 24d7496cd9e2..236cfb669df3 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -144,6 +144,7 @@ static const char *sdebug_version_date = "20180128";
 #define DEF_SUBMIT_QUEUES 1
 #define DEF_UUID_CTL 0
 #define JDELAY_OVERRIDDEN -9999
+#define DEF_USE_BLK_MQ  0
 
 #define SDEBUG_LUN_0_VAL 0
 
@@ -671,6 +672,7 @@ static bool sdebug_verbose;
 static bool have_dif_prot;
 static bool write_since_sync;
 static bool sdebug_statistics = DEF_STATISTICS;
+static bool sdebug_use_blk_mq = DEF_USE_BLK_MQ;
 
 static unsigned int sdebug_store_sectors;
 static sector_t sdebug_capacity;	/* in sectors */
@@ -4537,6 +4539,7 @@ module_param_named(vpd_use_hostno, sdebug_vpd_use_hostno, int,
 		   S_IRUGO | S_IWUSR);
 module_param_named(write_same_length, sdebug_write_same_length, int,
 		   S_IRUGO | S_IWUSR);
+module_param_named(use_blk_mq, sdebug_use_blk_mq, bool, S_IRUGO | S_IWUSR);
 
 MODULE_AUTHOR("Eric Youngdale + Douglas Gilbert");
 MODULE_DESCRIPTION("SCSI debug adapter driver");
@@ -5849,6 +5852,8 @@ static int sdebug_driver_probe(struct device *dev)
 	sdebug_driver_template.can_queue = sdebug_max_queue;
 	if (sdebug_clustering)
 		sdebug_driver_template.use_clustering = ENABLE_CLUSTERING;
+	if (sdebug_use_blk_mq)
+		sdebug_driver_template.force_blk_mq = 1;
 	hpnt = scsi_host_alloc(&sdebug_driver_template, sizeof(sdbg_host));
 	if (NULL == hpnt) {
 		pr_err("scsi_host_alloc failed\n");
-- 
2.9.5

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Christoph Hellwig]

On Mon, Jul 02, 2018 at 01:06:42PM +0800, Ming Lei wrote:
> With the introduced module parameter of 'use_blk_mq', it is easy
> to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
> module, so that we can test scsi_mq/blk_mq related regressions easily.

No.  We should not make a per driver choice.

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Johannes Thumshirn]

On Mon, Jul 02, 2018 at 05:54:22AM -0700, Christoph Hellwig wrote:
> On Mon, Jul 02, 2018 at 01:06:42PM +0800, Ming Lei wrote:
> > With the introduced module parameter of 'use_blk_mq', it is easy
> > to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
> > module, so that we can test scsi_mq/blk_mq related regressions easily.
> 
> No.  We should not make a per driver choice.

Btw, wouldn't it be time for:

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 35c909bbf8ba..bd115bab162e 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -49,6 +49,7 @@ config SCSI_NETLINK
 
 config SCSI_MQ_DEFAULT
 	bool "SCSI: use blk-mq I/O path by default"
+	default y
 	depends on SCSI
 	---help---
 	  This option enables the new blk-mq based I/O path for SCSI

again and see how we've improved in the last year and a half?

      Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Christoph Hellwig]

On Mon, Jul 02, 2018 at 02:56:52PM +0200, Johannes Thumshirn wrote:
> On Mon, Jul 02, 2018 at 05:54:22AM -0700, Christoph Hellwig wrote:
> > On Mon, Jul 02, 2018 at 01:06:42PM +0800, Ming Lei wrote:
> > > With the introduced module parameter of 'use_blk_mq', it is easy
> > > to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
> > > module, so that we can test scsi_mq/blk_mq related regressions easily.
> > 
> > No.  We should not make a per driver choice.
> 
> Btw, wouldn't it be time for:
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 35c909bbf8ba..bd115bab162e 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -49,6 +49,7 @@ config SCSI_NETLINK
>  
>  config SCSI_MQ_DEFAULT
>  	bool "SCSI: use blk-mq I/O path by default"
> +	default y
>  	depends on SCSI
>  	---help---
>  	  This option enables the new blk-mq based I/O path for SCSI
> 
> again and see how we've improved in the last year and a half?

Yes, totally.  We'll just need to sort out the error handling with
scsi and mq first (for 4.18 in fact).

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Johannes Thumshirn]

On Mon, Jul 02, 2018 at 06:03:10AM -0700, Christoph Hellwig wrote:
> Yes, totally.  We'll just need to sort out the error handling with
> scsi and mq first (for 4.18 in fact).

Sure.

	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Ming Lei]

On Mon, Jul 2, 2018 at 8:54 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jul 02, 2018 at 01:06:42PM +0800, Ming Lei wrote:
>> With the introduced module parameter of 'use_blk_mq', it is easy
>> to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
>> module, so that we can test scsi_mq/blk_mq related regressions easily.
>
> No.  We should not make a per driver choice.

Could you share us why we can't?

BTW, this patch can make scsi_mq/blk_mq regression tests to
be implemented very easily, such as, run one test in non-mq mode,
and run it in mq mode, then compare the two.

Thanks,
Ming Lei

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Ming Lei]

On Mon, Jul 2, 2018 at 9:03 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jul 02, 2018 at 02:56:52PM +0200, Johannes Thumshirn wrote:
>> On Mon, Jul 02, 2018 at 05:54:22AM -0700, Christoph Hellwig wrote:
>> > On Mon, Jul 02, 2018 at 01:06:42PM +0800, Ming Lei wrote:
>> > > With the introduced module parameter of 'use_blk_mq', it is easy
>> > > to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
>> > > module, so that we can test scsi_mq/blk_mq related regressions easily.
>> >
>> > No.  We should not make a per driver choice.
>>
>> Btw, wouldn't it be time for:
>>
>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>> index 35c909bbf8ba..bd115bab162e 100644
>> --- a/drivers/scsi/Kconfig
>> +++ b/drivers/scsi/Kconfig
>> @@ -49,6 +49,7 @@ config SCSI_NETLINK
>>
>>  config SCSI_MQ_DEFAULT
>>       bool "SCSI: use blk-mq I/O path by default"
>> +     default y
>>       depends on SCSI
>>       ---help---
>>         This option enables the new blk-mq based I/O path for SCSI
>>
>> again and see how we've improved in the last year and a half?
>
> Yes, totally.  We'll just need to sort out the error handling with
> scsi and mq first (for 4.18 in fact).

IMO, this two aren't contradictory, or not related, because this patch's
motivation is for doing scsi_mq/blk_mq regression test.

thanks,
Ming Lei

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Christoph Hellwig]

On Mon, Jul 02, 2018 at 09:13:35PM +0800, Ming Lei wrote:
> On Mon, Jul 2, 2018 at 8:54 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Mon, Jul 02, 2018 at 01:06:42PM +0800, Ming Lei wrote:
> >> With the introduced module parameter of 'use_blk_mq', it is easy
> >> to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
> >> module, so that we can test scsi_mq/blk_mq related regressions easily.
> >
> > No.  We should not make a per driver choice.
> 
> Could you share us why we can't?

Because it really is not a driver propery what code we use, it
is one of the core scsi code.  And intended as a temporary one,
although it already has lasted far too long.

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Douglas Gilbert]

On 2018-07-02 07:06 AM, Ming Lei wrote:
> With the introduced module parameter of 'use_blk_mq', it is easy
> to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
> module, so that we can test scsi_mq/blk_mq related regressions easily.
> 
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Cc: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Omar Sandoval <osandov@fb.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/scsi/scsi_debug.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 24d7496cd9e2..236cfb669df3 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -144,6 +144,7 @@ static const char *sdebug_version_date = "20180128";
>   #define DEF_SUBMIT_QUEUES 1
>   #define DEF_UUID_CTL 0
>   #define JDELAY_OVERRIDDEN -9999
> +#define DEF_USE_BLK_MQ  0
>   
>   #define SDEBUG_LUN_0_VAL 0
>   
> @@ -671,6 +672,7 @@ static bool sdebug_verbose;
>   static bool have_dif_prot;
>   static bool write_since_sync;
>   static bool sdebug_statistics = DEF_STATISTICS;
> +static bool sdebug_use_blk_mq = DEF_USE_BLK_MQ;
>   
>   static unsigned int sdebug_store_sectors;
>   static sector_t sdebug_capacity;	/* in sectors */
> @@ -4537,6 +4539,7 @@ module_param_named(vpd_use_hostno, sdebug_vpd_use_hostno, int,
>   		   S_IRUGO | S_IWUSR);
>   module_param_named(write_same_length, sdebug_write_same_length, int,
>   		   S_IRUGO | S_IWUSR);
> +module_param_named(use_blk_mq, sdebug_use_blk_mq, bool, S_IRUGO | S_IWUSR);
>   
>   MODULE_AUTHOR("Eric Youngdale + Douglas Gilbert");
>   MODULE_DESCRIPTION("SCSI debug adapter driver");
> @@ -5849,6 +5852,8 @@ static int sdebug_driver_probe(struct device *dev)
>   	sdebug_driver_template.can_queue = sdebug_max_queue;
>   	if (sdebug_clustering)
>   		sdebug_driver_template.use_clustering = ENABLE_CLUSTERING;
> +	if (sdebug_use_blk_mq)
> +		sdebug_driver_template.force_blk_mq = 1;
>   	hpnt = scsi_host_alloc(&sdebug_driver_template, sizeof(sdbg_host));
>   	if (NULL == hpnt) {
>   		pr_err("scsi_host_alloc failed\n");
> 

Hi,
It is up to others whether this patch goes through. It seems the default
associated with blk_mq may soon change. So two things:
   - I'd like to see a MODULE_PARM_DESC(use_blk_mq, ...) entry in patch
   - perhaps a 3 valued use_blk_mq: 1 --> force blk_mq; 0 (def) --> accept
     default mq setting; -1 --> turn off mq (if possible, if not it's a
     NOP)

Doug Gilbert

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Jens Axboe]

On 7/2/18 2:28 PM, Douglas Gilbert wrote:
> On 2018-07-02 07:06 AM, Ming Lei wrote:
>> With the introduced module parameter of 'use_blk_mq', it is easy
>> to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
>> module, so that we can test scsi_mq/blk_mq related regressions easily.
>>
>> Cc: Douglas Gilbert <dgilbert@interlog.com>
>> Cc: Bart Van Assche <bart.vanassche@wdc.com>
>> Cc: Jens Axboe <axboe@kernel.dk>
>> Cc: Omar Sandoval <osandov@fb.com>
>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>> ---
>>   drivers/scsi/scsi_debug.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>> index 24d7496cd9e2..236cfb669df3 100644
>> --- a/drivers/scsi/scsi_debug.c
>> +++ b/drivers/scsi/scsi_debug.c
>> @@ -144,6 +144,7 @@ static const char *sdebug_version_date = "20180128";
>>   #define DEF_SUBMIT_QUEUES 1
>>   #define DEF_UUID_CTL 0
>>   #define JDELAY_OVERRIDDEN -9999
>> +#define DEF_USE_BLK_MQ  0
>>   
>>   #define SDEBUG_LUN_0_VAL 0
>>   
>> @@ -671,6 +672,7 @@ static bool sdebug_verbose;
>>   static bool have_dif_prot;
>>   static bool write_since_sync;
>>   static bool sdebug_statistics = DEF_STATISTICS;
>> +static bool sdebug_use_blk_mq = DEF_USE_BLK_MQ;
>>   
>>   static unsigned int sdebug_store_sectors;
>>   static sector_t sdebug_capacity;	/* in sectors */
>> @@ -4537,6 +4539,7 @@ module_param_named(vpd_use_hostno, sdebug_vpd_use_hostno, int,
>>   		   S_IRUGO | S_IWUSR);
>>   module_param_named(write_same_length, sdebug_write_same_length, int,
>>   		   S_IRUGO | S_IWUSR);
>> +module_param_named(use_blk_mq, sdebug_use_blk_mq, bool, S_IRUGO | S_IWUSR);
>>   
>>   MODULE_AUTHOR("Eric Youngdale + Douglas Gilbert");
>>   MODULE_DESCRIPTION("SCSI debug adapter driver");
>> @@ -5849,6 +5852,8 @@ static int sdebug_driver_probe(struct device *dev)
>>   	sdebug_driver_template.can_queue = sdebug_max_queue;
>>   	if (sdebug_clustering)
>>   		sdebug_driver_template.use_clustering = ENABLE_CLUSTERING;
>> +	if (sdebug_use_blk_mq)
>> +		sdebug_driver_template.force_blk_mq = 1;
>>   	hpnt = scsi_host_alloc(&sdebug_driver_template, sizeof(sdbg_host));
>>   	if (NULL == hpnt) {
>>   		pr_err("scsi_host_alloc failed\n");
>>
> 
> Hi,
> It is up to others whether this patch goes through. It seems the default
> associated with blk_mq may soon change. So two things:
>    - I'd like to see a MODULE_PARM_DESC(use_blk_mq, ...) entry in patch
>    - perhaps a 3 valued use_blk_mq: 1 --> force blk_mq; 0 (def) --> accept
>      default mq setting; -1 --> turn off mq (if possible, if not it's a
>      NOP)

The point is that the parameter is going to be short lived, since the
non-mq path will be going away. We don't want drivers exposing this
sort of thing, just to remove the option shortly again. Once we're
happy with scsi-mq for a release or two, the old code should be
deleted. Once that happens, then the option will have no purpose.

-- 
Jens Axboe

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Ming Lei]

On Tue, Jul 3, 2018 at 4:37 AM, Jens Axboe <axboe@kernel.dk> wrote:
> On 7/2/18 2:28 PM, Douglas Gilbert wrote:
>> On 2018-07-02 07:06 AM, Ming Lei wrote:
>>> With the introduced module parameter of 'use_blk_mq', it is easy
>>> to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
>>> module, so that we can test scsi_mq/blk_mq related regressions easily.
>>>
>>> Cc: Douglas Gilbert <dgilbert@interlog.com>
>>> Cc: Bart Van Assche <bart.vanassche@wdc.com>
>>> Cc: Jens Axboe <axboe@kernel.dk>
>>> Cc: Omar Sandoval <osandov@fb.com>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>   drivers/scsi/scsi_debug.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>>> index 24d7496cd9e2..236cfb669df3 100644
>>> --- a/drivers/scsi/scsi_debug.c
>>> +++ b/drivers/scsi/scsi_debug.c
>>> @@ -144,6 +144,7 @@ static const char *sdebug_version_date = "20180128";
>>>   #define DEF_SUBMIT_QUEUES 1
>>>   #define DEF_UUID_CTL 0
>>>   #define JDELAY_OVERRIDDEN -9999
>>> +#define DEF_USE_BLK_MQ  0
>>>
>>>   #define SDEBUG_LUN_0_VAL 0
>>>
>>> @@ -671,6 +672,7 @@ static bool sdebug_verbose;
>>>   static bool have_dif_prot;
>>>   static bool write_since_sync;
>>>   static bool sdebug_statistics = DEF_STATISTICS;
>>> +static bool sdebug_use_blk_mq = DEF_USE_BLK_MQ;
>>>
>>>   static unsigned int sdebug_store_sectors;
>>>   static sector_t sdebug_capacity;   /* in sectors */
>>> @@ -4537,6 +4539,7 @@ module_param_named(vpd_use_hostno, sdebug_vpd_use_hostno, int,
>>>                 S_IRUGO | S_IWUSR);
>>>   module_param_named(write_same_length, sdebug_write_same_length, int,
>>>                 S_IRUGO | S_IWUSR);
>>> +module_param_named(use_blk_mq, sdebug_use_blk_mq, bool, S_IRUGO | S_IWUSR);
>>>
>>>   MODULE_AUTHOR("Eric Youngdale + Douglas Gilbert");
>>>   MODULE_DESCRIPTION("SCSI debug adapter driver");
>>> @@ -5849,6 +5852,8 @@ static int sdebug_driver_probe(struct device *dev)
>>>      sdebug_driver_template.can_queue = sdebug_max_queue;
>>>      if (sdebug_clustering)
>>>              sdebug_driver_template.use_clustering = ENABLE_CLUSTERING;
>>> +    if (sdebug_use_blk_mq)
>>> +            sdebug_driver_template.force_blk_mq = 1;
>>>      hpnt = scsi_host_alloc(&sdebug_driver_template, sizeof(sdbg_host));
>>>      if (NULL == hpnt) {
>>>              pr_err("scsi_host_alloc failed\n");
>>>
>>
>> Hi,
>> It is up to others whether this patch goes through. It seems the default
>> associated with blk_mq may soon change. So two things:
>>    - I'd like to see a MODULE_PARM_DESC(use_blk_mq, ...) entry in patch
>>    - perhaps a 3 valued use_blk_mq: 1 --> force blk_mq; 0 (def) --> accept
>>      default mq setting; -1 --> turn off mq (if possible, if not it's a
>>      NOP)
>
> The point is that the parameter is going to be short lived, since the
> non-mq path will be going away. We don't want drivers exposing this
> sort of thing, just to remove the option shortly again. Once we're
> happy with scsi-mq for a release or two, the old code should be
> deleted. Once that happens, then the option will have no purpose.

Not like other drivers, scsi_debug is a bit special, since it is for
test purpose. We may switch to scsi_mq at default soon, but the
whole MQ(block, scsi_mq) code may take ages to be removed,
maybe never.

We may use the per-driver parameter to test both mq and non-mq
path, especially for comparing both, then speed up to make MQ code
mature & stable. That is why I think this patch is very useful.

For example, we may write a test case to capture the recent scsi_mq
long boot delay regression easily with the introduced parameter.


Thanks,
Ming Lei

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Hannes Reinecke]

On 07/02/2018 03:03 PM, Christoph Hellwig wrote:
> On Mon, Jul 02, 2018 at 02:56:52PM +0200, Johannes Thumshirn wrote:
>> On Mon, Jul 02, 2018 at 05:54:22AM -0700, Christoph Hellwig wrote:
>>> On Mon, Jul 02, 2018 at 01:06:42PM +0800, Ming Lei wrote:
>>>> With the introduced module parameter of 'use_blk_mq', it is easy
>>>> to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
>>>> module, so that we can test scsi_mq/blk_mq related regressions easily.
>>>
>>> No.  We should not make a per driver choice.
>>
>> Btw, wouldn't it be time for:
>>
>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>> index 35c909bbf8ba..bd115bab162e 100644
>> --- a/drivers/scsi/Kconfig
>> +++ b/drivers/scsi/Kconfig
>> @@ -49,6 +49,7 @@ config SCSI_NETLINK
>>  
>>  config SCSI_MQ_DEFAULT
>>  	bool "SCSI: use blk-mq I/O path by default"
>> +	default y
>>  	depends on SCSI
>>  	---help---
>>  	  This option enables the new blk-mq based I/O path for SCSI
>>
>> again and see how we've improved in the last year and a half?
> 
> Yes, totally.  We'll just need to sort out the error handling with
> scsi and mq first (for 4.18 in fact).
> 
Ah. Seem to have missed that.
Care to elaborate?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Jens Axboe]

On 7/2/18 5:43 PM, Ming Lei wrote:
> On Tue, Jul 3, 2018 at 4:37 AM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 7/2/18 2:28 PM, Douglas Gilbert wrote:
>>> On 2018-07-02 07:06 AM, Ming Lei wrote:
>>>> With the introduced module parameter of 'use_blk_mq', it is easy
>>>> to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
>>>> module, so that we can test scsi_mq/blk_mq related regressions easily.
>>>>
>>>> Cc: Douglas Gilbert <dgilbert@interlog.com>
>>>> Cc: Bart Van Assche <bart.vanassche@wdc.com>
>>>> Cc: Jens Axboe <axboe@kernel.dk>
>>>> Cc: Omar Sandoval <osandov@fb.com>
>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>> ---
>>>>   drivers/scsi/scsi_debug.c | 5 +++++
>>>>   1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>>>> index 24d7496cd9e2..236cfb669df3 100644
>>>> --- a/drivers/scsi/scsi_debug.c
>>>> +++ b/drivers/scsi/scsi_debug.c
>>>> @@ -144,6 +144,7 @@ static const char *sdebug_version_date = "20180128";
>>>>   #define DEF_SUBMIT_QUEUES 1
>>>>   #define DEF_UUID_CTL 0
>>>>   #define JDELAY_OVERRIDDEN -9999
>>>> +#define DEF_USE_BLK_MQ  0
>>>>
>>>>   #define SDEBUG_LUN_0_VAL 0
>>>>
>>>> @@ -671,6 +672,7 @@ static bool sdebug_verbose;
>>>>   static bool have_dif_prot;
>>>>   static bool write_since_sync;
>>>>   static bool sdebug_statistics = DEF_STATISTICS;
>>>> +static bool sdebug_use_blk_mq = DEF_USE_BLK_MQ;
>>>>
>>>>   static unsigned int sdebug_store_sectors;
>>>>   static sector_t sdebug_capacity;   /* in sectors */
>>>> @@ -4537,6 +4539,7 @@ module_param_named(vpd_use_hostno, sdebug_vpd_use_hostno, int,
>>>>                 S_IRUGO | S_IWUSR);
>>>>   module_param_named(write_same_length, sdebug_write_same_length, int,
>>>>                 S_IRUGO | S_IWUSR);
>>>> +module_param_named(use_blk_mq, sdebug_use_blk_mq, bool, S_IRUGO | S_IWUSR);
>>>>
>>>>   MODULE_AUTHOR("Eric Youngdale + Douglas Gilbert");
>>>>   MODULE_DESCRIPTION("SCSI debug adapter driver");
>>>> @@ -5849,6 +5852,8 @@ static int sdebug_driver_probe(struct device *dev)
>>>>      sdebug_driver_template.can_queue = sdebug_max_queue;
>>>>      if (sdebug_clustering)
>>>>              sdebug_driver_template.use_clustering = ENABLE_CLUSTERING;
>>>> +    if (sdebug_use_blk_mq)
>>>> +            sdebug_driver_template.force_blk_mq = 1;
>>>>      hpnt = scsi_host_alloc(&sdebug_driver_template, sizeof(sdbg_host));
>>>>      if (NULL == hpnt) {
>>>>              pr_err("scsi_host_alloc failed\n");
>>>>
>>>
>>> Hi,
>>> It is up to others whether this patch goes through. It seems the default
>>> associated with blk_mq may soon change. So two things:
>>>    - I'd like to see a MODULE_PARM_DESC(use_blk_mq, ...) entry in patch
>>>    - perhaps a 3 valued use_blk_mq: 1 --> force blk_mq; 0 (def) --> accept
>>>      default mq setting; -1 --> turn off mq (if possible, if not it's a
>>>      NOP)
>>
>> The point is that the parameter is going to be short lived, since the
>> non-mq path will be going away. We don't want drivers exposing this
>> sort of thing, just to remove the option shortly again. Once we're
>> happy with scsi-mq for a release or two, the old code should be
>> deleted. Once that happens, then the option will have no purpose.
> 
> Not like other drivers, scsi_debug is a bit special, since it is for
> test purpose. We may switch to scsi_mq at default soon, but the
> whole MQ(block, scsi_mq) code may take ages to be removed,
> maybe never.

It'll definitely get removed, the motivation to do that is strong. I'm
not in violent disagreement with you, my main point is just that it's
going to be a temporary option. Once we flip the switch again and remove
the non-mq path for SCSI a few releases later, then the option is dead.

> We may use the per-driver parameter to test both mq and non-mq
> path, especially for comparing both, then speed up to make MQ code
> mature & stable. That is why I think this patch is very useful.

Also possible with just a reboot, or fiddle with the scsi_mod.use_blk_mq
option...

-- 
Jens Axboe

Re: [PATCH] scsi: scsi_debug: introduce module parameter of 'use_blk_mq'

[Jens Axboe]

On 7/3/18 8:05 AM, Jens Axboe wrote:
> On 7/2/18 5:43 PM, Ming Lei wrote:
>> On Tue, Jul 3, 2018 at 4:37 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 7/2/18 2:28 PM, Douglas Gilbert wrote:
>>>> On 2018-07-02 07:06 AM, Ming Lei wrote:
>>>>> With the introduced module parameter of 'use_blk_mq', it is easy
>>>>> to switch between 'blk_mq' and 'non_blk_mq' by reloading scsi_debug
>>>>> module, so that we can test scsi_mq/blk_mq related regressions easily.
>>>>>
>>>>> Cc: Douglas Gilbert <dgilbert@interlog.com>
>>>>> Cc: Bart Van Assche <bart.vanassche@wdc.com>
>>>>> Cc: Jens Axboe <axboe@kernel.dk>
>>>>> Cc: Omar Sandoval <osandov@fb.com>
>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>>> ---
>>>>>   drivers/scsi/scsi_debug.c | 5 +++++
>>>>>   1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>>>>> index 24d7496cd9e2..236cfb669df3 100644
>>>>> --- a/drivers/scsi/scsi_debug.c
>>>>> +++ b/drivers/scsi/scsi_debug.c
>>>>> @@ -144,6 +144,7 @@ static const char *sdebug_version_date = "20180128";
>>>>>   #define DEF_SUBMIT_QUEUES 1
>>>>>   #define DEF_UUID_CTL 0
>>>>>   #define JDELAY_OVERRIDDEN -9999
>>>>> +#define DEF_USE_BLK_MQ  0
>>>>>
>>>>>   #define SDEBUG_LUN_0_VAL 0
>>>>>
>>>>> @@ -671,6 +672,7 @@ static bool sdebug_verbose;
>>>>>   static bool have_dif_prot;
>>>>>   static bool write_since_sync;
>>>>>   static bool sdebug_statistics = DEF_STATISTICS;
>>>>> +static bool sdebug_use_blk_mq = DEF_USE_BLK_MQ;
>>>>>
>>>>>   static unsigned int sdebug_store_sectors;
>>>>>   static sector_t sdebug_capacity;   /* in sectors */
>>>>> @@ -4537,6 +4539,7 @@ module_param_named(vpd_use_hostno, sdebug_vpd_use_hostno, int,
>>>>>                 S_IRUGO | S_IWUSR);
>>>>>   module_param_named(write_same_length, sdebug_write_same_length, int,
>>>>>                 S_IRUGO | S_IWUSR);
>>>>> +module_param_named(use_blk_mq, sdebug_use_blk_mq, bool, S_IRUGO | S_IWUSR);
>>>>>
>>>>>   MODULE_AUTHOR("Eric Youngdale + Douglas Gilbert");
>>>>>   MODULE_DESCRIPTION("SCSI debug adapter driver");
>>>>> @@ -5849,6 +5852,8 @@ static int sdebug_driver_probe(struct device *dev)
>>>>>      sdebug_driver_template.can_queue = sdebug_max_queue;
>>>>>      if (sdebug_clustering)
>>>>>              sdebug_driver_template.use_clustering = ENABLE_CLUSTERING;
>>>>> +    if (sdebug_use_blk_mq)
>>>>> +            sdebug_driver_template.force_blk_mq = 1;
>>>>>      hpnt = scsi_host_alloc(&sdebug_driver_template, sizeof(sdbg_host));
>>>>>      if (NULL == hpnt) {
>>>>>              pr_err("scsi_host_alloc failed\n");
>>>>>
>>>>
>>>> Hi,
>>>> It is up to others whether this patch goes through. It seems the default
>>>> associated with blk_mq may soon change. So two things:
>>>>    - I'd like to see a MODULE_PARM_DESC(use_blk_mq, ...) entry in patch
>>>>    - perhaps a 3 valued use_blk_mq: 1 --> force blk_mq; 0 (def) --> accept
>>>>      default mq setting; -1 --> turn off mq (if possible, if not it's a
>>>>      NOP)
>>>
>>> The point is that the parameter is going to be short lived, since the
>>> non-mq path will be going away. We don't want drivers exposing this
>>> sort of thing, just to remove the option shortly again. Once we're
>>> happy with scsi-mq for a release or two, the old code should be
>>> deleted. Once that happens, then the option will have no purpose.
>>
>> Not like other drivers, scsi_debug is a bit special, since it is for
>> test purpose. We may switch to scsi_mq at default soon, but the
>> whole MQ(block, scsi_mq) code may take ages to be removed,
>> maybe never.
> 
> It'll definitely get removed, the motivation to do that is strong. I'm
> not in violent disagreement with you, my main point is just that it's
> going to be a temporary option. Once we flip the switch again and remove
> the non-mq path for SCSI a few releases later, then the option is dead.
> 
>> We may use the per-driver parameter to test both mq and non-mq
>> path, especially for comparing both, then speed up to make MQ code
>> mature & stable. That is why I think this patch is very useful.
> 
> Also possible with just a reboot, or fiddle with the scsi_mod.use_blk_mq
> option...

Just to drive the point home, you can easily do:

echo N/Y > /sys/module/scsi_mod/parameters/use_blk_mq

to make scsi_debug default to one or the other, without adding this
module specific parameter.

-- 
Jens Axboe