| Message ID | 1583857550-12049-3-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: enable reserved commands for LLDDs | expand |
On Wed, Mar 11, 2020 at 12:25:28AM +0800, John Garry wrote: > From: Hannes Reinecke <hare@suse.com> > > Allocate a separate 'reserved_cmd_q' for sending reserved commands. Why? Reserved command specifically are not in any way tied to queues.
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@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@vger.kernel.org/msg83177.html " >> My implementation actually allows for per-device reserved tags (eg for >> virtio). But some drivers require to use internal commands prior to any >> device setup, so they have to use a separate reserved command queue just to >> be able to allocate tags. > > Why would virtio-scsi need per-device reserved commands? It currently uses > a global mempool to allocate the reset commands. > Oh, I'm perfectly fine with dropping the per-device reserved commands, and use the host-wide queue in general. It turns out most of the drivers use it that way already. Will be doing so for the next iteration. " Cheers
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@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@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@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@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
On 11/03/2020 06:58, 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@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@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. > Hannes, I assume that you will take back over this series now. Let me know. The patches are here, if not in patchwork: https://github.com/hisilicon/kernel-dev/tree/private-topic-sas-5.6-resv-commands-v2 > 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. Right, we still need to be able to allocate somewhere apart from sdev queue > Thanks
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@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@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
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@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@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@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@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. > cheers, John
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@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@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@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@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
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: >>> 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@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@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@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@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. > 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. thanks
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
>>> >> >> 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. Yes, something similar happens for libsas [hence my hack], where sas_alloc_target()->sas_find_dev_by_rphy() fails as it cannot handle rphy for scsi host as parent properly. > And this is why I'm not utterly keen on this approach; auditing all > these callbacks is _not_ fun. > Understood. And if you can't test them, then a change like this is too risky for those drivers. Cheers, John
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 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) Cheers, John
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
>> 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 wasn't going to do anything else... > I'm currently preparing a patchset implementing things by improving the > current scsi_get_host_dev() etc. OK, great, all you have to do is say. > > RFC should be ready by the end of the week. > Thanks, John
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 2967325df7a0..e809b0e30a11 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1881,6 +1881,7 @@ struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev) int scsi_mq_setup_tags(struct Scsi_Host *shost) { unsigned int cmd_size, sgl_size; + int ret; sgl_size = max_t(unsigned int, sizeof(struct scatterlist), scsi_mq_inline_sgl_size(shost)); @@ -1904,11 +1905,25 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) BLK_ALLOC_POLICY_TO_MQ_FLAG(shost->hostt->tag_alloc_policy); shost->tag_set.driver_data = shost; - return blk_mq_alloc_tag_set(&shost->tag_set); + ret = blk_mq_alloc_tag_set(&shost->tag_set); + if (ret) + return ret; + + if (shost->nr_reserved_cmds) { + shost->reserved_cmd_q = blk_mq_init_queue(&shost->tag_set); + if (IS_ERR(shost->reserved_cmd_q)) { + blk_mq_free_tag_set(&shost->tag_set); + ret = PTR_ERR(shost->reserved_cmd_q); + shost->reserved_cmd_q = NULL; + } + } + return ret; } void scsi_mq_destroy_tags(struct Scsi_Host *shost) { + if (shost->reserved_cmd_q) + blk_cleanup_queue(shost->reserved_cmd_q); blk_mq_free_tag_set(&shost->tag_set); } diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 3f860c8ad623..2258a4f7b4d8 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -604,6 +604,7 @@ struct Scsi_Host { * Number of reserved commands, if any. */ unsigned nr_reserved_cmds; + struct request_queue *reserved_cmd_q; unsigned active_mode:2; unsigned unchecked_isa_dma:1;