diff mbox series

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

Message ID 20230124143025.3464-1-mwilck@suse.com (mailing list archive)
State Superseded
Headers show
Series [v2] scsi: add non-sleeping variant of scsi_device_put() and use it in alua | expand

Commit Message

Martin Wilck Jan. 24, 2023, 2:30 p.m. UTC
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(-)

Comments

Bart Van Assche Jan. 24, 2023, 3:23 p.m. UTC | #1
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.
Martin Wilck Jan. 24, 2023, 3:26 p.m. UTC | #2
On Tue, 2023-01-24 at 07:23 -0800, Bart Van Assche wrote:
> 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?

Or simply mentioning that the call from scsi_device_put() is legal?

Martin
Bart Van Assche Jan. 24, 2023, 3:26 p.m. UTC | #3
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.
Christoph Hellwig Jan. 25, 2023, 6:49 a.m. UTC | #4
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.
Martin Wilck Jan. 25, 2023, 8:18 a.m. UTC | #5
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
diff mbox series

Patch

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 29a2865b8e2e..8f3aac9e6a50 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -1032,7 +1032,7 @@  static bool alua_rtpg_queue(struct alua_port_group *pg,
 			kref_put(&pg->kref, release_port_group);
 	}
 	if (sdev)
-		scsi_device_put(sdev);
+		scsi_device_put_nosleep(sdev);
 
 	return true;
 }
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1426b9b03612..22b82e57dee3 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -576,6 +576,24 @@  int scsi_device_get(struct scsi_device *sdev)
 }
 EXPORT_SYMBOL(scsi_device_get);
 
+/**
+ * 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.
+ */
+void scsi_device_put_nosleep(struct scsi_device *sdev)
+{
+	struct module *mod = sdev->host->hostt->module;
+
+	put_device(&sdev->sdev_gendev);
+	module_put(mod);
+}
+EXPORT_SYMBOL(scsi_device_put_nosleep);
+
 /**
  * scsi_device_put  -  release a reference to a scsi_device
  * @sdev:	device to release a reference on.
@@ -586,12 +604,9 @@  EXPORT_SYMBOL(scsi_device_get);
  */
 void scsi_device_put(struct scsi_device *sdev)
 {
-	struct module *mod = sdev->host->hostt->module;
-
 	might_sleep();
 
-	put_device(&sdev->sdev_gendev);
-	module_put(mod);
+	scsi_device_put_nosleep(sdev);
 }
 EXPORT_SYMBOL(scsi_device_put);
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 3642b8e3928b..f1ad48c9c601 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -365,6 +365,7 @@  void scsi_attach_vpd(struct scsi_device *sdev);
 
 extern struct scsi_device *scsi_device_from_queue(struct request_queue *q);
 extern int __must_check scsi_device_get(struct scsi_device *);
+void scsi_device_put_nosleep(struct scsi_device *sdev);
 extern void scsi_device_put(struct scsi_device *);
 extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
 					      uint, uint, u64);