[PATCH v2 0/3] scsi: fixes for targets with many LUNs

[PATCH v2 0/3] scsi: fixes for targets with many LUNs

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This patch series addresses some issues we saw in a test setup
with a large number of SCSI LUNs. The first two patches simply
increase the number of available sg and bsg devices. The last one
fixes an large delay we encountered between blocking a Fibre Channel
remote port and the dev_loss_tmo.

Changes v1 -> v2:
 - call blk_mq_wait_quiesce_done() from scsi_target_block() to
   cover the case where BLK_MQ_F_BLOCKING is set (Bart van Assche)

Hannes Reinecke (3):
  bsg: increase number of devices
  scsi: sg: increase number of devices
  scsi: simplify scsi_stop_queue()

 block/bsg.c             |  2 +-
 drivers/scsi/scsi_lib.c | 28 ++++++++++++++--------------
 drivers/scsi/sg.c       |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

-- 
2.40.1

[PATCH v2 1/3] bsg: increase number of devices

[mwilck]

From: Hannes Reinecke <hare@suse.de>

Larger setups may need to allocate more than 32k bsg devices, so
increase the number of devices to the full range of minor device
numbers.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 block/bsg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/bsg.c b/block/bsg.c
index 7eca43f33d7f..c53f24243bf2 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -36,7 +36,7 @@ static inline struct bsg_device *to_bsg_device(struct inode *inode)
 }
 
 #define BSG_DEFAULT_CMDS	64
-#define BSG_MAX_DEVS		32768
+#define BSG_MAX_DEVS		(1 << MINORBITS)
 
 static DEFINE_IDA(bsg_minor_ida);
 static struct class *bsg_class;
-- 
2.40.1

[PATCH v2 2/3] scsi: sg: increase number of devices

[mwilck]

From: Hannes Reinecke <hare@suse.de>

Larger setups may need to allocate more than 32k sg devices, so
increase the number of devices to the full range of minor device
numbers.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Acked-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 037f8c98a6d3..6c04cf941dac 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -71,7 +71,7 @@ static int sg_proc_init(void);
 
 #define SG_ALLOW_DIO_DEF 0
 
-#define SG_MAX_DEVS 32768
+#define SG_MAX_DEVS (1 << MINORBITS)
 
 /* SG_MAX_CDB_SIZE should be 260 (spc4r37 section 3.1.30) however the type
  * of sg_io_hdr::cmd_len can only represent 255. All SCSI commands greater
-- 
2.40.1

[PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[mwilck]

From: Hannes Reinecke <hare@suse.de>

scsi_target_block() calls scsi_stop_queue() for each scsi_device
and scsi_stop_queue() calls blk_mq_wait_quiesce_done() for each LUN.
As blk_mq_wait_quiesce_done() comes down to synchronize_rcu() for
SCSI queues, this can cause substantial delay for scsi_target_block()
on a target with a lot of logical units (we measured more than 100s
delay for blocking a FC rport with 2048 LUNs).

Simplify scsi_stop_queue(), which is only called in this code path, to never
wait for the quiescing to finish. Rather call blk_mq_wait_quiesce_done()
from scsi_target_block() after iterating over all devices.

Also, move the call to scsi_stop_queue() in scsi_internal_device_block()
out of the code section where the state_mutex is held.

This patch uses the same basic idea as f983622ae605 ("scsi: core: Avoid
calling synchronize_rcu() for each device in scsi_host_block()").

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_lib.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 25489fbd94c6..bc78bea62755 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2726,24 +2726,18 @@ void scsi_start_queue(struct scsi_device *sdev)
 		blk_mq_unquiesce_queue(sdev->request_queue);
 }
 
-static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
+static void scsi_stop_queue(struct scsi_device *sdev)
 {
 	/*
 	 * The atomic variable of ->queue_stopped covers that
 	 * blk_mq_quiesce_queue* is balanced with blk_mq_unquiesce_queue.
 	 *
 	 * However, we still need to wait until quiesce is done
-	 * in case that queue has been stopped.
+	 * in case that queue has been stopped. This is done in
+	 * scsi_target_block() for all devices of the target.
 	 */
-	if (!cmpxchg(&sdev->queue_stopped, 0, 1)) {
-		if (nowait)
-			blk_mq_quiesce_queue_nowait(sdev->request_queue);
-		else
-			blk_mq_quiesce_queue(sdev->request_queue);
-	} else {
-		if (!nowait)
-			blk_mq_wait_quiesce_done(sdev->request_queue->tag_set);
-	}
+	if (!cmpxchg(&sdev->queue_stopped, 0, 1))
+		blk_mq_quiesce_queue_nowait(sdev->request_queue);
 }
 
 /**
@@ -2770,7 +2764,7 @@ int scsi_internal_device_block_nowait(struct scsi_device *sdev)
 	 * request queue.
 	 */
 	if (!ret)
-		scsi_stop_queue(sdev, true);
+		scsi_stop_queue(sdev);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait);
@@ -2796,9 +2790,9 @@ static int scsi_internal_device_block(struct scsi_device *sdev)
 
 	mutex_lock(&sdev->state_mutex);
 	err = __scsi_internal_device_block_nowait(sdev);
-	if (err == 0)
-		scsi_stop_queue(sdev, false);
 	mutex_unlock(&sdev->state_mutex);
+	if (err == 0)
+		scsi_stop_queue(sdev);
 
 	return err;
 }
@@ -2906,11 +2900,17 @@ target_block(struct device *dev, void *data)
 void
 scsi_target_block(struct device *dev)
 {
+	struct Scsi_Host *shost = dev_to_shost(dev);
+
 	if (scsi_is_target_device(dev))
 		starget_for_each_device(to_scsi_target(dev), NULL,
 					device_block);
 	else
 		device_for_each_child(dev, NULL, target_block);
+
+	/* Wait for ongoing scsi_queue_rq() calls to finish. */
+	if (!WARN_ON_ONCE(!shost))
+		blk_mq_wait_quiesce_done(&shost->tag_set);
 }
 EXPORT_SYMBOL_GPL(scsi_target_block);
 
-- 
2.40.1

Re: [PATCH v2 0/3] scsi: fixes for targets with many LUNs

[Bart Van Assche]

On 6/6/23 12:38, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This patch series addresses some issues we saw in a test setup
> with a large number of SCSI LUNs. The first two patches simply
> increase the number of available sg and bsg devices. The last one
> fixes an large delay we encountered between blocking a Fibre Channel
> remote port and the dev_loss_tmo.
> 
> Changes v1 -> v2:
>   - call blk_mq_wait_quiesce_done() from scsi_target_block() to
>     cover the case where BLK_MQ_F_BLOCKING is set (Bart van Assche)

For the entire series:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 0/3] scsi: fixes for targets with many LUNs

[Ming Lei]

On Wed, Jun 7, 2023 at 3:38 AM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> This patch series addresses some issues we saw in a test setup
> with a large number of SCSI LUNs. The first two patches simply
> increase the number of available sg and bsg devices. The last one
> fixes an large delay we encountered between blocking a Fibre Channel
> remote port and the dev_loss_tmo.
>
> Changes v1 -> v2:
>  - call blk_mq_wait_quiesce_done() from scsi_target_block() to
>    cover the case where BLK_MQ_F_BLOCKING is set (Bart van Assche)


Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,

Re: [PATCH v2 1/3] bsg: increase number of devices

[Christoph Hellwig]

On Tue, Jun 06, 2023 at 09:38:43PM +0200, mwilck@suse.com wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Larger setups may need to allocate more than 32k bsg devices, so
> increase the number of devices to the full range of minor device
> numbers.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Martin, if you send this on it also needs your signoff.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/3] scsi: sg: increase number of devices

[Christoph Hellwig]

On Tue, Jun 06, 2023 at 09:38:44PM +0200, mwilck@suse.com wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Larger setups may need to allocate more than 32k sg devices, so
> increase the number of devices to the full range of minor device
> numbers.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Acked-by: Douglas Gilbert <dgilbert@interlog.com>

Same comment about the signoff.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[Christoph Hellwig]

On Tue, Jun 06, 2023 at 09:38:45PM +0200, mwilck@suse.com wrote:
> Simplify scsi_stop_queue(), which is only called in this code path, to never
> wait for the quiescing to finish. Rather call blk_mq_wait_quiesce_done()
> from scsi_target_block() after iterating over all devices.

I don't think simplify is the right word here.  The code isn't in any
way simpler, it just is more efficient an shifts work from
scsi_stop_queue to scsi_internal_device_block and scsi_target_block.

But the whole transformation is very confusing to me even if it looks
correct in the end, and it took me quite a while to understand it.

I'd suggest to further split this up and include some additional
cleanups:

  1) remove scsi_internal_device_block and fold it into device_block
  2) move the scsi_internal_device_block in what was
     scsi_internal_device_block and now is device_block out
     of state_mutex (and document in the commit log why this is safe)
  3) remove scsi_stop_queue and open code it in the two callers, one
     of which currently wants nowait semantics, and one that doesn't.
  4) move the quiesce wait to scsi_target_block and make it per-tagset

>  scsi_target_block(struct device *dev)
>  {
> +	struct Scsi_Host *shost = dev_to_shost(dev);
> +
>  	if (scsi_is_target_device(dev))
>  		starget_for_each_device(to_scsi_target(dev), NULL,
>  					device_block);
>  	else
>  		device_for_each_child(dev, NULL, target_block);
> +
> +	/* Wait for ongoing scsi_queue_rq() calls to finish. */
> +	if (!WARN_ON_ONCE(!shost))

How could host ever be NULL here?  I can't see why we'd want this
check.

Btw, as far as I can tell scsi_target_block is never called for
a device that is a target device.  It might be worth throwing in
another patch to remove support for that case and simplify things
further.

Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[Martin Wilck]

On Wed, 2023-06-07 at 07:27 +0200, Christoph Hellwig wrote:
> On Tue, Jun 06, 2023 at 09:38:45PM +0200, mwilck@suse.com wrote:
> > Simplify scsi_stop_queue(), which is only called in this code path,
> > to never
> > wait for the quiescing to finish. Rather call
> > blk_mq_wait_quiesce_done()
> > from scsi_target_block() after iterating over all devices.
> 
> I don't think simplify is the right word here.  The code isn't in any
> way simpler, it just is more efficient an shifts work from
> scsi_stop_queue to scsi_internal_device_block and scsi_target_block.
> 
> But the whole transformation is very confusing to me even if it looks
> correct in the end, and it took me quite a while to understand it.
> 
> I'd suggest to further split this up and include some additional
> cleanups:
> 
>   1) remove scsi_internal_device_block and fold it into device_block

ok

>   2) move the scsi_internal_device_block in what was

You mean scsi_stop_queue() here, right?

>      scsi_internal_device_block and now is device_block out
>      of state_mutex (and document in the commit log why this is safe)
>   3) remove scsi_stop_queue and open code it in the two callers, one
>      of which currently wants nowait semantics, and one that doesn't.
ok

>   4) move the quiesce wait to scsi_target_block and make it per-
> tagset

ok

> 
> >  scsi_target_block(struct device *dev)
> >  {
> > +       struct Scsi_Host *shost = dev_to_shost(dev);
> > +
> >         if (scsi_is_target_device(dev))
> >                 starget_for_each_device(to_scsi_target(dev), NULL,
> >                                         device_block);
> >         else
> >                 device_for_each_child(dev, NULL, target_block);
> > +
> > +       /* Wait for ongoing scsi_queue_rq() calls to finish. */
> > +       if (!WARN_ON_ONCE(!shost))
> 
> How could host ever be NULL here?  I can't see why we'd want this
> check.
> 

The reason is simple: I wasn't certain if dev_to_shost() could return
NULL, and preferred skipping the wait over an Oops. I hear you say that
dev_to_shost() can't go wrong, so I'll remove the NULL test.

> Btw, as far as I can tell scsi_target_block is never called for
> a device that is a target device.  It might be worth throwing in
> another patch to remove support for that case and simplify things
> further.

Can we do this separately, maybe? 

Thanks,
Martin

Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[Martin Wilck]

On Wed, 2023-06-07 at 11:26 +0200, Martin Wilck wrote:
> On Wed, 2023-06-07 at 07:27 +0200, Christoph Hellwig wrote:
> > 
> >   3) remove scsi_stop_queue and open code it in the two callers,
> > one
> >      of which currently wants nowait semantics, and one that
> > doesn't.
> ok

Hm, scsi_stop_queue() pairs with scsi_start_queue(), do we really want
to open-code it?

Martin

Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[Christoph Hellwig]

On Wed, Jun 07, 2023 at 11:26:04AM +0200, Martin Wilck wrote:
> > cleanups:
> > 
> >   1) remove scsi_internal_device_block and fold it into device_block
> 
> ok
> 
> >   2) move the scsi_internal_device_block in what was
> 
> You mean scsi_stop_queue() here, right?

Yes.

> The reason is simple: I wasn't certain if dev_to_shost() could return
> NULL, and preferred skipping the wait over an Oops. I hear you say that
> dev_to_shost() can't go wrong, so I'll remove the NULL test.

Well, the parent device can't really go away while we have a reference
to a child.  So the only case where it can return NULL is if the
passed in device isn't the child of a SCSI host, which would be a grave
programming error.

> 
> > Btw, as far as I can tell scsi_target_block is never called for
> > a device that is a target device.  It might be worth throwing in
> > another patch to remove support for that case and simplify things
> > further.
> 
> Can we do this separately, maybe? 

Sure.  Would be nice to just tack into onto the end of this series
if you touch the area, though.

> On Wed, 2023-06-07 at 11:26 +0200, Martin Wilck wrote:
> > On Wed, 2023-06-07 at 07:27 +0200, Christoph Hellwig wrote:
> > > 
> > >   3) remove scsi_stop_queue and open code it in the two callers,
> > > one
> > >      of which currently wants nowait semantics, and one that
> > > doesn't.
> > ok
> 
> Hm, scsi_stop_queue() pairs with scsi_start_queue(), do we really want
> to open-code it?

Oh well, feel free to keep it if you prefer it that way.

Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[Bart Van Assche]

On 6/7/23 02:26, Martin Wilck wrote:
> On Wed, 2023-06-07 at 07:27 +0200, Christoph Hellwig wrote:
>> On Tue, Jun 06, 2023 at 09:38:45PM +0200, mwilck@suse.com wrote:
>>>   scsi_target_block(struct device *dev)
>>>   {
>>> +       struct Scsi_Host *shost = dev_to_shost(dev);
>>> +
>>>          if (scsi_is_target_device(dev))
>>>                  starget_for_each_device(to_scsi_target(dev), NULL,
>>>                                          device_block);
>>>          else
>>>                  device_for_each_child(dev, NULL, target_block);
>>> +
>>> +       /* Wait for ongoing scsi_queue_rq() calls to finish. */
>>> +       if (!WARN_ON_ONCE(!shost))
>>
>> How could host ever be NULL here?  I can't see why we'd want this
>> check.
> 
> The reason is simple: I wasn't certain if dev_to_shost() could return
> NULL, and preferred skipping the wait over an Oops. I hear you say that
> dev_to_shost() can't go wrong, so I'll remove the NULL test.

I propose to pass shost as the first argument to scsi_target_block() 
instead of using dev_to_shost() inside scsi_target_block(). Except in 
__iscsi_block_session(), shost is already available as a local variable.

Bart.

Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[Christoph Hellwig]

On Wed, Jun 07, 2023 at 07:05:34AM -0700, Bart Van Assche wrote:
>> The reason is simple: I wasn't certain if dev_to_shost() could return
>> NULL, and preferred skipping the wait over an Oops. I hear you say that
>> dev_to_shost() can't go wrong, so I'll remove the NULL test.
>
> I propose to pass shost as the first argument to scsi_target_block() 
> instead of using dev_to_shost() inside scsi_target_block(). Except in 
> __iscsi_block_session(), shost is already available as a local variable.

That sounds like a good idea to me.

Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[Martin Wilck]

On Wed, 2023-06-07 at 07:05 -0700, Bart Van Assche wrote:
> On 6/7/23 02:26, Martin Wilck wrote:
> > On Wed, 2023-06-07 at 07:27 +0200, Christoph Hellwig wrote:
> > > On Tue, Jun 06, 2023 at 09:38:45PM +0200, mwilck@suse.com wrote:
> > > >   scsi_target_block(struct device *dev)
> > > >   {
> > > > +       struct Scsi_Host *shost = dev_to_shost(dev);
> > > > +
> > > >          if (scsi_is_target_device(dev))
> > > >                  starget_for_each_device(to_scsi_target(dev),
> > > > NULL,
> > > >                                          device_block);
> > > >          else
> > > >                  device_for_each_child(dev, NULL,
> > > > target_block);
> > > > +
> > > > +       /* Wait for ongoing scsi_queue_rq() calls to finish. */
> > > > +       if (!WARN_ON_ONCE(!shost))
> > > 
> > > How could host ever be NULL here?  I can't see why we'd want this
> > > check.
> > 
> > The reason is simple: I wasn't certain if dev_to_shost() could
> > return
> > NULL, and preferred skipping the wait over an Oops. I hear you say
> > that
> > dev_to_shost() can't go wrong, so I'll remove the NULL test.
> 
> I propose to pass shost as the first argument to scsi_target_block() 
> instead of using dev_to_shost() inside scsi_target_block(). Except in
> __iscsi_block_session(), shost is already available as a local
> variable.

If we do this, it might actually be cleaner to just pass the tag set to
wait for.

Martin

Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[Bart Van Assche]

On 6/7/23 08:38, Martin Wilck wrote:
> On Wed, 2023-06-07 at 07:05 -0700, Bart Van Assche wrote:
>> On 6/7/23 02:26, Martin Wilck wrote:
>>> On Wed, 2023-06-07 at 07:27 +0200, Christoph Hellwig wrote:
>>>> On Tue, Jun 06, 2023 at 09:38:45PM +0200, mwilck@suse.com wrote:
>>>>>    scsi_target_block(struct device *dev)
>>>>>    {
>>>>> +       struct Scsi_Host *shost = dev_to_shost(dev);
>>>>> +
>>>>>           if (scsi_is_target_device(dev))
>>>>>                   starget_for_each_device(to_scsi_target(dev),
>>>>> NULL,
>>>>>                                           device_block);
>>>>>           else
>>>>>                   device_for_each_child(dev, NULL,
>>>>> target_block);
>>>>> +
>>>>> +       /* Wait for ongoing scsi_queue_rq() calls to finish. */
>>>>> +       if (!WARN_ON_ONCE(!shost))
>>>>
>>>> How could host ever be NULL here?  I can't see why we'd want this
>>>> check.
>>>
>>> The reason is simple: I wasn't certain if dev_to_shost() could
>>> return
>>> NULL, and preferred skipping the wait over an Oops. I hear you say
>>> that
>>> dev_to_shost() can't go wrong, so I'll remove the NULL test.
>>
>> I propose to pass shost as the first argument to scsi_target_block()
>> instead of using dev_to_shost() inside scsi_target_block(). Except in
>> __iscsi_block_session(), shost is already available as a local
>> variable.
> 
> If we do this, it might actually be cleaner to just pass the tag set to
> wait for.

Wouldn't that be close to a layering violation? Shouldn't SCSI APIs accept
pointers to SCSI objects instead of pointers to block layer abstractions?

Thanks,

Bart.

Re: [PATCH v2 3/3] scsi: simplify scsi_stop_queue()

[Martin Wilck]

On Wed, 2023-06-07 at 09:39 -0700, Bart Van Assche wrote:
> On 6/7/23 08:38, Martin Wilck wrote:
> > On Wed, 2023-06-07 at 07:05 -0700, Bart Van Assche wrote:
> > > On 6/7/23 02:26, Martin Wilck wrote:
> > > > On Wed, 2023-06-07 at 07:27 +0200, Christoph Hellwig wrote:
> > > > > On Tue, Jun 06, 2023 at 09:38:45PM +0200,
> > > > > mwilck@suse.com wrote:
> > > > > >    scsi_target_block(struct device *dev)
> > > > > >    {
> > > > > > +       struct Scsi_Host *shost = dev_to_shost(dev);
> > > > > > +
> > > > > >           if (scsi_is_target_device(dev))
> > > > > >                   starget_for_each_device(to_scsi_target(de
> > > > > > v),
> > > > > > NULL,
> > > > > >                                           device_block);
> > > > > >           else
> > > > > >                   device_for_each_child(dev, NULL,
> > > > > > target_block);
> > > > > > +
> > > > > > +       /* Wait for ongoing scsi_queue_rq() calls to
> > > > > > finish. */
> > > > > > +       if (!WARN_ON_ONCE(!shost))
> > > > > 
> > > > > How could host ever be NULL here?  I can't see why we'd want
> > > > > this
> > > > > check.
> > > > 
> > > > The reason is simple: I wasn't certain if dev_to_shost() could
> > > > return
> > > > NULL, and preferred skipping the wait over an Oops. I hear you
> > > > say
> > > > that
> > > > dev_to_shost() can't go wrong, so I'll remove the NULL test.
> > > 
> > > I propose to pass shost as the first argument to
> > > scsi_target_block()
> > > instead of using dev_to_shost() inside scsi_target_block().
> > > Except in
> > > __iscsi_block_session(), shost is already available as a local
> > > variable.
> > 
> > If we do this, it might actually be cleaner to just pass the tag
> > set to
> > wait for.
> 
> Wouldn't that be close to a layering violation? Shouldn't SCSI APIs
> accept
> pointers to SCSI objects instead of pointers to block layer
> abstractions?

My thought was that quiescing is based on tag sets in current kernels,
and passing in the tag set to scsi_target_block() would make that
explicit.

But you've got a point. I'll resubmit the with a Scsi_Host argument and
see how it goes.

Thanks,
Martin