Linux Virtualization - Apr 2020 - [PATCH RFC v2 02/24] scsi: allocate separate queue for reserved commands

On Tue, Mar 10, 2020 at 09:08:56PM +0000, John Garry
wrote:
> On 10/03/2020 18:32, Christoph Hellwig wrote:
> > On Wed, Mar 11, 2020 at 12:25:28AM +0800, John Garry wrote:
> > > From: Hannes Reinecke <hare at suse.com>
> > > 
> > > Allocate a separate 'reserved_cmd_q' for sending reserved
commands.
> > 
> > Why?  Reserved command specifically are not in any way tied to queues.
> > .
> > 
> 
> So the v1 series used a combination of the sdev queue and the per-host
> reserved_cmd_q. Back then you questioned using the sdev queue for virtio
> scsi, and the unconfirmed conclusion was to use a common per-host q. This
is
> the best link I can find now:
> 
> https://www.mail-archive.com/linux-scsi at vger.kernel.org/msg83177.html
That was just a question on why virtio uses the per-device tags, which
didn't look like it made any sense.  What I'm worried about here is
mixing up the concept of reserved tags in the tagset, and queues to use
them.  Note that we already have the scsi_get_host_dev to allocate
a scsi_device and thus a request_queue for the host itself.  That seems
like the better interface to use a tag for a host wide command vs
introducing a parallel path.

On 3/11/20 7:22 AM, Christoph Hellwig wrote:
> On Tue, Mar 10, 2020 at 09:08:56PM +0000, John Garry wrote:
>> On 10/03/2020 18:32, Christoph Hellwig wrote:
>>> On Wed, Mar 11, 2020 at 12:25:28AM +0800, John Garry wrote:
>>>> From: Hannes Reinecke <hare at suse.com>
>>>>
>>>> Allocate a separate 'reserved_cmd_q' for sending
reserved commands.
>>>
>>> Why?  Reserved command specifically are not in any way tied to
queues.
>>> .
>>>
>>
>> So the v1 series used a combination of the sdev queue and the per-host
>> reserved_cmd_q. Back then you questioned using the sdev queue for
virtio
>> scsi, and the unconfirmed conclusion was to use a common per-host q.
This is
>> the best link I can find now:
>>
>> https://www.mail-archive.com/linux-scsi at
vger.kernel.org/msg83177.html
> 
> That was just a question on why virtio uses the per-device tags, which
> didn't look like it made any sense.  What I'm worried about here is
> mixing up the concept of reserved tags in the tagset, and queues to use
> them.  Note that we already have the scsi_get_host_dev to allocate
> a scsi_device and thus a request_queue for the host itself.  That seems
> like the better interface to use a tag for a host wide command vs
> introducing a parallel path.
> 
Ah. Right.
Will be looking into that, and convert the patchset over to it.

And the problem of the separate queue is the fact that I'll need a queue
to reserve tags from; trying to allocate a tag directly from the bitmap
turns out to be major surgery in the blocklayer with no immediate gain.
And I can't use per-device queues as for some drivers the reserved
commands are used to query the HBA itself to figure out how many devices
are present.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare at suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 N?rnberg
HRB 36809 (AG N?rnberg), GF: Felix Imend?rffer

On 3/11/20 7:22 AM, Christoph Hellwig wrote:
> On Tue, Mar 10, 2020 at 09:08:56PM +0000, John Garry wrote:
>> On 10/03/2020 18:32, Christoph Hellwig wrote:
>>> On Wed, Mar 11, 2020 at 12:25:28AM +0800, John Garry wrote:
>>>> From: Hannes Reinecke <hare at suse.com>
>>>>
>>>> Allocate a separate 'reserved_cmd_q' for sending
reserved commands.
>>>
>>> Why?  Reserved command specifically are not in any way tied to
queues.
>>> .
>>>
>>
>> So the v1 series used a combination of the sdev queue and the per-host
>> reserved_cmd_q. Back then you questioned using the sdev queue for
virtio
>> scsi, and the unconfirmed conclusion was to use a common per-host q.
This is
>> the best link I can find now:
>>
>> https://www.mail-archive.com/linux-scsi at
vger.kernel.org/msg83177.html
> 
> That was just a question on why virtio uses the per-device tags, which
> didn't look like it made any sense.  What I'm worried about here is
> mixing up the concept of reserved tags in the tagset, and queues to use
> them.  Note that we already have the scsi_get_host_dev to allocate
> a scsi_device and thus a request_queue for the host itself.  That seems
> like the better interface to use a tag for a host wide command vs
> introducing a parallel path.
> 
Thinking about it some more, I don't think that scsi_get_host_dev() is
the best way of handling it.
Problem is that it'll create a new scsi_device with
<hostno:this_id:0>,
which will then show up via eg 'lsscsi'.
This would be okay if 'this_id' would have been defined by the driver;
sadly, most drivers which are affected here do set 'this_id' to -1.
So we wouldn't have a nice target ID to allocate the device from, let
alone the problem that we would have to emulate a complete scsi device
with all required minimal command support etc.
And I'm not quite sure how well that would play with the exising SCSI
host template; the device we'll be allocating would have basically
nothing in common with the 'normal' SCSI devices.

What we could do, though, is to try it the other way round:
Lift the request queue from scsi_get_host_dev() into the scsi host
itself, so that scsi_get_host_dev() can use that queue, but we also
would be able to use it without a SCSI device attached.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare at suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 N?rnberg
HRB 36809 (AG N?rnberg), GF: Felix Imend?rffer

On 4/7/20 1:54 PM, John Garry wrote:
> On 06/04/2020 10:05, Hannes Reinecke wrote:
>> On 3/11/20 7:22 AM, Christoph Hellwig wrote:
>>> On Tue, Mar 10, 2020 at 09:08:56PM +0000, John Garry wrote:
>>>> On 10/03/2020 18:32, Christoph Hellwig wrote:
>>>>> On Wed, Mar 11, 2020 at 12:25:28AM +0800, John Garry wrote:
>>>>>> From: Hannes Reinecke <hare at suse.com>
>>>>>>
>>>>>> Allocate a separate 'reserved_cmd_q' for
sending reserved commands.
>>>>>
>>>>> Why?? Reserved command specifically are not in any way tied
to queues.
>>>>> .
>>>>>
>>>>
>>>> So the v1 series used a combination of the sdev queue and the
per-host
>>>> reserved_cmd_q. Back then you questioned using the sdev queue
for
>>>> virtio
>>>> scsi, and the unconfirmed conclusion was to use a common
per-host q.
>>>> This is
>>>> the best link I can find now:
>>>>
>>>> https://www.mail-archive.com/linux-scsi at
vger.kernel.org/msg83177.html
>>>
>>> That was just a question on why virtio uses the per-device tags,
which
>>> didn't look like it made any sense.? What I'm worried about
here is
>>> mixing up the concept of reserved tags in the tagset, and queues to
use
>>> them.? Note that we already have the scsi_get_host_dev to allocate
>>> a scsi_device and thus a request_queue for the host itself.? That
seems
>>> like the better interface to use a tag for a host wide command vs
>>> introducing a parallel path.
>>>
>> Thinking about it some more, I don't think that scsi_get_host_dev()
is
>> the best way of handling it.
>> Problem is that it'll create a new scsi_device with
<hostno:this_id:0>,
>> which will then show up via eg 'lsscsi'.
> 
> are you sure? Doesn't this function just allocate the sdev, but do 
> nothing with it, like probing it?
> 
> I bludgeoned it in here for PoC:
> 
>
https://github.com/hisilicon/kernel-dev/commit/ef0ae8540811e32776f64a5b42bd76cbed17ba47
> 
> 
> And then still:
> 
> john at ubuntu:~$ lsscsi
> [0:0:0:0] disk SEAGATE? ST2000NM0045? N004? /dev/sda
> [0:0:1:0] disk SEAGATE? ST2000NM0045? N004? /dev/sdb
> [0:0:2:0] disk ATASAMSUNG HM320JI? 0_01? /dev/sdc
> [0:0:3:0] disk SEAGATE? ST1000NM0023? 0006? /dev/sdd
> [0:0:4:0] enclosu HUAWEIExpander 12Gx16? 128-
> john at ubuntu:~$
> 
> Some proper plumbing would be needed, though.
> 
>> This would be okay if 'this_id' would have been defined by the
driver;
>> sadly, most drivers which are affected here do set 'this_id' to
-1.
>> So we wouldn't have a nice target ID to allocate the device from,
let
>> alone the problem that we would have to emulate a complete scsi device
>> with all required minimal command support etc.
>> And I'm not quite sure how well that would play with the exising
SCSI
>> host template; the device we'll be allocating would have basically
>> nothing in common with the 'normal' SCSI devices.
>>
>> What we could do, though, is to try it the other way round:
>> Lift the request queue from scsi_get_host_dev() into the scsi host
>> itself, so that scsi_get_host_dev() can use that queue, but we also
>> would be able to use it without a SCSI device attached.
> 
> wouldn't that limit 1x scsi device per host, not that I know if any
more
> would ever be required? But it does still seem better to use the request 
> queue in the scsi device.
> 
My concern is this:

struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
{
	[ .. ]
	starget = scsi_alloc_target(&shost->shost_gendev, 0, shost->this_id);
	[ .. ]

and we have typically:

drivers/scsi/hisi_sas/hisi_sas_v3_hw.c: .this_id                = -1,

It's _very_ uncommon to have a negative number as the SCSI target 
device; in fact, it _is_ an unsigned int already.

But alright, I'll give it a go; let's see what I'll end up with.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 N?rnberg
HRB 36809 (AG N?rnberg), Gesch?ftsf?hrer: Felix Imend?rffer

On 4/7/20 4:35 PM, John Garry wrote:
> On 07/04/2020 15:00, Hannes Reinecke wrote:
>> On 4/7/20 1:54 PM, John Garry wrote:
>>> On 06/04/2020 10:05, Hannes Reinecke wrote:
[ .. ]
>>>> This would be okay if 'this_id' would have been defined
by the driver;
>>>> sadly, most drivers which are affected here do set
'this_id' to -1.
>>>> So we wouldn't have a nice target ID to allocate the device
from, let
>>>> alone the problem that we would have to emulate a complete scsi
device
>>>> with all required minimal command support etc.
>>>> And I'm not quite sure how well that would play with the
exising SCSI
>>>> host template; the device we'll be allocating would have
basically
>>>> nothing in common with the 'normal' SCSI devices.
>>>>
>>>> What we could do, though, is to try it the other way round:
>>>> Lift the request queue from scsi_get_host_dev() into the scsi
host
>>>> itself, so that scsi_get_host_dev() can use that queue, but we
also
>>>> would be able to use it without a SCSI device attached.
>>>
>>> wouldn't that limit 1x scsi device per host, not that I know if
any
>>> more would ever be required? But it does still seem better to use
the
>>> request queue in the scsi device.
>>>
>> My concern is this:
>>
>> struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
>> {
>> ?????[ .. ]
>> ?????starget = scsi_alloc_target(&shost->shost_gendev, 0, 
>> shost->this_id);
>> ?????[ .. ]
>>
>> and we have typically:
>>
>> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c: .this_id??????????????? = -1,
>>
>> It's _very_ uncommon to have a negative number as the SCSI target 
>> device; in fact, it _is_ an unsigned int already.
>>
> 
> FWIW, the only other driver (gdth) which I see uses this API has this_id 
> = -1 in the scsi host template.
> 
>> But alright, I'll give it a go; let's see what I'll end up
with.
> 
> note: If we want a fixed scsi_device per host, calling 
> scsi_mq_setup_tags() -> scsi_get_host_dev() will fail as shost state is 
> not running. Maybe we need to juggle some things there to provide a 
> generic solution.
> 
It might even get worse, as during device setup things like 
'slave_alloc' etc is getting called, which has a fair chance of getting 
confused for non-existing devices.
Cf qla2xxx:qla2xx_slave_alloc() is calling starget_to_rport(), which 
will get us a nice oops when accessing a target which is _not_ the child 
of a fc remote port.
And this is why I'm not utterly keen on this approach; auditing all 
these callbacks is _not_ fun.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 N?rnberg
HRB 36809 (AG N?rnberg), Gesch?ftsf?hrer: Felix Imend?rffer

On Tue, Apr 07, 2020 at 04:00:10PM +0200, Hannes Reinecke
wrote:
> My concern is this:
> 
> struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
> {
> 	[ .. ]
> 	starget = scsi_alloc_target(&shost->shost_gendev, 0,
shost->this_id);
> 	[ .. ]
> 
> and we have typically:
> 
> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c: .this_id                = -1,
> 
> It's _very_ uncommon to have a negative number as the SCSI target
device; in
> fact, it _is_ an unsigned int already.
> 
> But alright, I'll give it a go; let's see what I'll end up
with.
But this shouldn't be exposed anywhere.  And I prefer that over having
magic requests/scsi_cmnd that do not have a valid ->device pointer.

On 4/23/20 4:13 PM, John Garry wrote:
> On 07/04/2020 17:30, Christoph Hellwig wrote:
>> On Tue, Apr 07, 2020 at 04:00:10PM +0200, Hannes Reinecke wrote:
>>> My concern is this:
>>>
>>> struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
>>> {
>>> ????[ .. ]
>>> ????starget = scsi_alloc_target(&shost->shost_gendev, 0, 
>>> shost->this_id);
>>> ????[ .. ]
>>>
>>> and we have typically:
>>>
>>> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c: .this_id??????????????? =
-1,
>>>
>>> It's _very_ uncommon to have a negative number as the SCSI
target
>>> device; in
>>> fact, it _is_ an unsigned int already.
>>>
>>> But alright, I'll give it a go; let's see what I'll end
up with.
>>
>> But this shouldn't be exposed anywhere.? And I prefer that over
having
>> magic requests/scsi_cmnd that do not have a valid ->device pointer.
>> .
>>
> 
> (just looking at this again)
> 
> Hi Christoph,
> 
> So how would this look added in scsi_lib.c:
> 
> struct scsi_cmnd *scsi_get_reserved_cmd(struct Scsi_Host *shost)
> {
>  ????struct scsi_cmnd *scmd;
>  ????struct request *rq;
>  ????struct scsi_device *sdev = scsi_get_host_dev(shost);
> 
>  ????if (!sdev)
>  ??????? return NULL;
> 
>  ????rq = blk_mq_alloc_request(sdev->request_queue,
>  ????????????????? REQ_OP_DRV_OUT | REQ_NOWAIT,
>  ????????????????? BLK_MQ_REQ_RESERVED);
>  ????if (IS_ERR(rq)) // fix tidy-up
>  ??????? return NULL;
>  ????WARN_ON(rq->tag == -1);
>  ????scmd = blk_mq_rq_to_pdu(rq);
>  ????scmd->request = rq;
>  ????scmd->device = sdev;
> 
>  ????return scmd;
> }
> EXPORT_SYMBOL_GPL(scsi_get_reserved_cmd);
> 
> void scsi_put_reserved_cmd(struct scsi_cmnd *scmd)
> {
>  ????struct request *rq = blk_mq_rq_from_pdu(scmd);
> 
>  ????if (blk_mq_rq_is_reserved(rq)) {
>  ??????? struct scsi_device *sdev = scmd->device;
>  ??????? blk_mq_free_request(rq);
>  ??????? scsi_free_host_dev(sdev);
>  ????}
> }
> EXPORT_SYMBOL_GPL(scsi_put_reserved_cmd);
> 
> Not sure if we want a static scsi_device per host, or alloc and free 
> dynamically.
> 
> (@Hannes, I also have some proper patches for libsas if you want to add it)
> 
Hold your horses.
I'm currently preparing a patchset implementing things by improving the 
current scsi_get_host_dev() etc.

RFC should be ready by the end of the week.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 N?rnberg
HRB 36809 (AG N?rnberg), Gesch?ftsf?hrer: Felix Imend?rffer