[v2] scsi: add non-sleeping variant of scsi_device_put() and use it in alua

From: Martin Wilck <mwilck@suse.com>

Since the might_sleep() annotation was added in scsi_device_put() and
alua_rtpg_queue(), we have seen repeated reports of "BUG: sleeping function
called from invalid context" [1], [2]. alua_rtpg_queue() is always called
from contexts where the caller must hold at least one reference to the scsi
device in question. This means that the reference taken by
alua_rtpg_queue() itself can't be the last one, and thus can be dropped
without entering the code path in which scsi_device_put() might actually
sleep.

Add a new helper function, scsi_device_put_nosleep() for cases like this,
where a device reference is put from atomic context, and at the same time
it is certain that this reference is not the last one, and use it from
alua_rtpg_queue().

[1] https://lore.kernel.org/linux-scsi/b49e37d5-edfb-4c56-3eeb-62c7d5855c00@linux.ibm.com/
[2] https://lore.kernel.org/linux-scsi/55c35e64-a7d4-9072-46fd-e8eae6a90e96@linux.ibm.com/

Fixes: f93ed747e2c7 ("scsi: core: Release SCSI devices synchronously")
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Sachin Sant <sachinp@linux.ibm.com>
Cc: Benjamin Block <bblock@linux.ibm.com>
Reported-by: Steffen Maier <maier@linux.ibm.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>

----
Changes since v1:
 - rebased onto the scsi-fixes branch.
 - fixed several mistakes pointed out  by Steffen Maier.
---
 drivers/scsi/device_handler/scsi_dh_alua.c |  2 +-
 drivers/scsi/scsi.c                        | 23 ++++++++++++++++++----
 include/scsi/scsi_device.h                 |  1 +
 3 files changed, 21 insertions(+), 5 deletions(-)

On 1/24/23 06:30, mwilck@suse.com wrote:
> +/**
> + * scsi_device_put_nosleep  -  release a reference to a scsi_device
> + * @sdev:	device to release a reference on.
> + *
> + * Description: Release a reference to the scsi_device and decrements the use
> + * count of the underlying LLDD module. This function may only be called from
> + * a call context where it is certain that the reference dropped is not the
> + * last one.
> + */

The above comment does not cover the call from scsi_device_put(). That 
could be addressed by adding the following text at the end of the above 
comment: " or from a context in which it is allowed to sleep". However, 
if that text would be added the function name 
("scsi_device_put_nosleep()") would become confusing. How about 
open-coding scsi_device_put_nosleep() in scsi_device_put() to prevent 
this confusion?

Thanks,

Bart.

On 1/24/23 07:26, Martin Wilck wrote:
> Or simply mentioning that the call from scsi_device_put() is legal?

That's fine with me.

Thanks,

Bart.

On Tue, Jan 24, 2023 at 03:30:25PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Since the might_sleep() annotation was added in scsi_device_put() and
> alua_rtpg_queue(), we have seen repeated reports of "BUG: sleeping function
> called from invalid context" [1], [2]. alua_rtpg_queue() is always called
> from contexts where the caller must hold at least one reference to the scsi
> device in question. This means that the reference taken by
> alua_rtpg_queue() itself can't be the last one, and thus can be dropped
> without entering the code path in which scsi_device_put() might actually
> sleep.

If there is always guaranteed to be another reference, why does this
code even grab one?  The pattern of dropping a reference that can't be
the last is pretty nonsensical.

On Tue, 2023-01-24 at 22:49 -0800, Christoph Hellwig wrote:
> On Tue, Jan 24, 2023 at 03:30:25PM +0100, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > Since the might_sleep() annotation was added in scsi_device_put()
> > and
> > alua_rtpg_queue(), we have seen repeated reports of "BUG: sleeping
> > function
> > called from invalid context" [1], [2]. alua_rtpg_queue() is always
> > called
> > from contexts where the caller must hold at least one reference to
> > the scsi
> > device in question. This means that the reference taken by
> > alua_rtpg_queue() itself can't be the last one, and thus can be
> > dropped
> > without entering the code path in which scsi_device_put() might
> > actually
> > sleep.
> 
> If there is always guaranteed to be another reference, why does this
> code even grab one?  The pattern of dropping a reference that can't
> be
> the last is pretty nonsensical.
> 

It's because the sdev is passed to the work queue to execute the RTPG.
To my understanding, the rationale is that the caller's ref may be
given up before the workqueue starts running, thus an additional ref is
needed to make sure the sdev isn't freed before the workqueue accesses
it. But if queue_delayed_work() fails (e.g. because the item is already
queued) this additional ref must be given up.

Regards,
Martin