[PATCH 0/2] Rework how the ALUA driver calls scsi_device_put()

[PATCH 0/2] Rework how the ALUA driver calls scsi_device_put()

[Bart Van Assche]

Hi Martin,

This patch series reworks how the SCSI ALUA device handler calls
scsi_device_put(). Please consider this patch series for the next merge
window.

Thanks,

Bart.

Bart Van Assche (2):
  scsi: alua: Revert "Move a scsi_device_put() call out of
    alua_check_vpd()"
  scsi: alua: Call scsi_device_put() from non-atomic context

 drivers/scsi/device_handler/scsi_dh_alua.c | 46 ++++++++++++----------
 1 file changed, 25 insertions(+), 21 deletions(-)

[PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()"

[Bart Van Assche]

There is a bug in commit 0b25e17e9018 ("scsi: alua: Move a
scsi_device_put() call out of alua_check_vpd()"): that patch may cause
alua_rtpg_queue() callers to call scsi_device_put() even if that function
should not be called. Revert that commit to prepare for a different
solution.

Cc: Hannes Reinecke <hare@suse.de>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Sachin Sant <sachinp@linux.ibm.com>
Cc: Benjamin Block <bblock@linux.ibm.com>
Reported-by: Sachin Sant <sachinp@linux.ibm.com>
Reported-by: Benjamin Block <bblock@linux.ibm.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 23 ++++++++--------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 693cd827e138..bd4ee294f5c7 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -324,7 +324,6 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 	struct alua_port_group *pg, *old_pg = NULL;
 	bool pg_updated = false;
 	unsigned long flags;
-	bool put_sdev;
 
 	group_id = scsi_vpd_tpg_id(sdev, &rel_port);
 	if (group_id < 0) {
@@ -374,14 +373,11 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 		list_add_rcu(&h->node, &pg->dh_list);
 	spin_unlock_irqrestore(&pg->lock, flags);
 
-	put_sdev = alua_rtpg_queue(rcu_dereference_protected(h->pg,
+	alua_rtpg_queue(rcu_dereference_protected(h->pg,
 						  lockdep_is_held(&h->pg_lock)),
 			sdev, NULL, true);
 	spin_unlock(&h->pg_lock);
 
-	if (put_sdev)
-		scsi_device_put(sdev);
-
 	if (old_pg)
 		kref_put(&old_pg->kref, release_port_group);
 
@@ -982,10 +978,9 @@ static void alua_rtpg_work(struct work_struct *work)
  *         RTPG already has been scheduled.
  *
  * Returns true if and only if alua_rtpg_work() will be called asynchronously.
- * That function is responsible for calling @qdata->fn(). If this function
- * returns true, the caller is responsible for invoking scsi_device_put(@sdev).
+ * That function is responsible for calling @qdata->fn().
  */
-static bool __must_check alua_rtpg_queue(struct alua_port_group *pg,
+static bool alua_rtpg_queue(struct alua_port_group *pg,
 			    struct scsi_device *sdev,
 			    struct alua_queue_data *qdata, bool force)
 {
@@ -1024,6 +1019,8 @@ static bool __must_check alua_rtpg_queue(struct alua_port_group *pg,
 		else
 			kref_put(&pg->kref, release_port_group);
 	}
+	if (sdev)
+		scsi_device_put(sdev);
 
 	return true;
 }
@@ -1130,12 +1127,10 @@ static int alua_activate(struct scsi_device *sdev,
 	rcu_read_unlock();
 	mutex_unlock(&h->init_mutex);
 
-	if (alua_rtpg_queue(pg, sdev, qdata, true)) {
-		scsi_device_put(sdev);
+	if (alua_rtpg_queue(pg, sdev, qdata, true))
 		fn = NULL;
-	} else {
+	else
 		err = SCSI_DH_DEV_OFFLINED;
-	}
 	kref_put(&pg->kref, release_port_group);
 out:
 	if (fn)
@@ -1161,9 +1156,7 @@ static void alua_check(struct scsi_device *sdev, bool force)
 		return;
 	}
 	rcu_read_unlock();
-
-	if (alua_rtpg_queue(pg, sdev, NULL, force))
-		scsi_device_put(sdev);
+	alua_rtpg_queue(pg, sdev, NULL, force);
 	kref_put(&pg->kref, release_port_group);
 }
 

Re: [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()"

[Sachin Sant]

> On 18-Nov-2022, at 12:06 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> There is a bug in commit 0b25e17e9018 ("scsi: alua: Move a
> scsi_device_put() call out of alua_check_vpd()"): that patch may cause
> alua_rtpg_queue() callers to call scsi_device_put() even if that function
> should not be called. Revert that commit to prepare for a different
> solution.
> 
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Sachin Sant <sachinp@linux.ibm.com>
> Cc: Benjamin Block <bblock@linux.ibm.com>
> Reported-by: Sachin Sant <sachinp@linux.ibm.com>
> Reported-by: Benjamin Block <bblock@linux.ibm.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/scsi/device_handler/scsi_dh_alua.c | 23 ++++++++--------------
> 1 file changed, 8 insertions(+), 15 deletions(-)

Thanks for the patch. 
Tested it on top of next-20221117. The reported warning is not seen.

Tested-by: Sachin Sant <sachinp@linux.ibm.com <mailto:sachinp@linux.ibm.com>>

- Sachin

Re: [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()"

[Bart Van Assche]

On 11/17/22 22:11, Sachin Sant wrote:
> Tested it on top of next-20221117. The reported warning is not seen.
> 
> Tested-by: Sachin Sant <sachinp@linux.ibm.com <mailto:sachinp@linux.ibm.com>>

Hi Sachin,

Thank you for the help with testing.

Can you also test patch 2/2 from this series 
(https://lore.kernel.org/all/20221117183626.2656196-3-bvanassche@acm.org/)?

Thanks,

Bart.

Re: [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()"

[Sachin Sant]

> On 18-Nov-2022, at 8:37 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> On 11/17/22 22:11, Sachin Sant wrote:
>> Tested it on top of next-20221117. The reported warning is not seen.
>> Tested-by: Sachin Sant <sachinp@linux.ibm.com <mailto:sachinp@linux.ibm.com>>
> 
> Hi Sachin,
> 
> Thank you for the help with testing.
> 
> Can you also test patch 2/2 from this series (https://lore.kernel.org/all/20221117183626.2656196-3-bvanassche@acm.org/)?

I tested with both the patches applied on top of next-20221117.

- Sachin

Re: [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()"

[Bart Van Assche]

On 11/18/22 08:03, Sachin Sant wrote:
>> On 18-Nov-2022, at 8:37 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>> Can you also test patch 2/2 from this series (https://lore.kernel.org/all/20221117183626.2656196-3-bvanassche@acm.org/)?
> 
> I tested with both the patches applied on top of next-20221117.

Thank you Sachin for having confirmed this. In the future when testing an
entire patch series, consider replying with "Tested-by:" to the cover letter
instead of the first patch. I think that is the conventional way to indicate
that a patch series has been tested in its entirety instead of a single
patch from a series.

Bart.

Re: [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()"

[Sachin Sant]

> On 19-Nov-2022, at 12:24 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> On 11/18/22 08:03, Sachin Sant wrote:
>>> On 18-Nov-2022, at 8:37 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>>> Can you also test patch 2/2 from this series (https://lore.kernel.org/all/20221117183626.2656196-3-bvanassche@acm.org/)?
>> I tested with both the patches applied on top of next-20221117.
> 
> Thank you Sachin for having confirmed this. In the future when testing an
> entire patch series, consider replying with "Tested-by:" to the cover letter
> instead of the first patch. I think that is the conventional way to indicate
> that a patch series has been tested in its entirety instead of a single
> patch from a series.
> 

I did not receive the cover letter so replied to the first patch. 
I should have explicitly called out that I have tested both the patches.

Sorry about the confusion.

- Sachin

[PATCH 2/2] scsi: alua: Call scsi_device_put() from non-atomic context

[Bart Van Assche]

Since commit f93ed747e2c7 ("scsi: core: Release SCSI devices
synchronously"), scsi_device_put() might sleep. Avoid calling it from
alua_rtpg_queue() with the pg_lock held. The lock only pretects h->pg,
anyway. To avoid the pg being freed under us, because of a race with
another thread, take a temporary reference. In alua_rtpg_queue(), verify
that the pg still belongs to the sdev being passed before actually
queueing the RTPG.

This patch fixes the following smatch warning:

drivers/scsi/device_handler/scsi_dh_alua.c:1013 alua_rtpg_queue() warn: sleeping in atomic context

alua_check_vpd() <- disables preempt
-> alua_rtpg_queue()
   -> scsi_device_put()

Cc: Martin Wilck <mwilck@suse.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Sachin Sant <sachinp@linux.ibm.com>
Cc: Benjamin Block <bblock@linux.ibm.com>
Suggested-by: Martin Wilck <mwilck@suse.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 27 +++++++++++++++-------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index bd4ee294f5c7..49cc18a87473 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -354,6 +354,8 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 			    "%s: port group %x rel port %x\n",
 			    ALUA_DH_NAME, group_id, rel_port);
 
+	kref_get(&pg->kref);
+
 	/* Check for existing port group references */
 	spin_lock(&h->pg_lock);
 	old_pg = rcu_dereference_protected(h->pg, lockdep_is_held(&h->pg_lock));
@@ -373,11 +375,11 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 		list_add_rcu(&h->node, &pg->dh_list);
 	spin_unlock_irqrestore(&pg->lock, flags);
 
-	alua_rtpg_queue(rcu_dereference_protected(h->pg,
-						  lockdep_is_held(&h->pg_lock)),
-			sdev, NULL, true);
 	spin_unlock(&h->pg_lock);
 
+	alua_rtpg_queue(pg, sdev, NULL, true);
+	kref_put(&pg->kref, release_port_group);
+
 	if (old_pg)
 		kref_put(&old_pg->kref, release_port_group);
 
@@ -986,6 +988,9 @@ static bool alua_rtpg_queue(struct alua_port_group *pg,
 {
 	int start_queue = 0;
 	unsigned long flags;
+
+	might_sleep();
+
 	if (WARN_ON_ONCE(!pg) || scsi_device_get(sdev))
 		return false;
 
@@ -996,11 +1001,17 @@ static bool alua_rtpg_queue(struct alua_port_group *pg,
 		force = true;
 	}
 	if (pg->rtpg_sdev == NULL) {
-		pg->interval = 0;
-		pg->flags |= ALUA_PG_RUN_RTPG;
-		kref_get(&pg->kref);
-		pg->rtpg_sdev = sdev;
-		start_queue = 1;
+		struct alua_dh_data *h = sdev->handler_data;
+
+		rcu_read_lock();
+		if (h && rcu_dereference(h->pg) == pg) {
+			pg->interval = 0;
+			pg->flags |= ALUA_PG_RUN_RTPG;
+			kref_get(&pg->kref);
+			pg->rtpg_sdev = sdev;
+			start_queue = 1;
+		}
+		rcu_read_unlock();
 	} else if (!(pg->flags & ALUA_PG_RUN_RTPG) && force) {
 		pg->flags |= ALUA_PG_RUN_RTPG;
 		/* Do not queue if the worker is already running */

Re: [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put()

[Martin K. Petersen]

Bart,

> This patch series reworks how the SCSI ALUA device handler calls
> scsi_device_put(). Please consider this patch series for the next
> merge window.

Applied to 6.2/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put()

[Martin K. Petersen]

On Thu, 17 Nov 2022 10:36:24 -0800, Bart Van Assche wrote:

> This patch series reworks how the SCSI ALUA device handler calls
> scsi_device_put(). Please consider this patch series for the next merge
> window.
> 
> Thanks,
> 
> Bart.
> 
> [...]

Applied to 6.2/scsi-queue, thanks!

[1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()"
      https://git.kernel.org/mkp/scsi/c/a500c4cc06cd
[2/2] scsi: alua: Call scsi_device_put() from non-atomic context
      https://git.kernel.org/mkp/scsi/c/50759b881e1d

-- 
Martin K. Petersen	Oracle Linux Engineering