Re: [PATCH] scsi: alua: Fix alua_rtpg_queue()

[Martin Wilck <mwilck@suse.com>]

Hello Bart,

On Tue, 2022-11-15 at 14:49 -0800, Bart Van Assche wrote:
> Modify alua_rtpg_queue() such that it only requests the caller to
> drop
> the sdev reference if necessary. This patch fixes a recently
> introduced
> regression.
> 
> Cc: Sachin Sant <sachinp@linux.ibm.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Reported-by: Sachin Sant <sachinp@linux.ibm.com>
> Fixes: 0b25e17e9018 ("scsi: alua: Move a scsi_device_put() call out
> of alua_check_vpd()")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Is this complexity really necessary, just for calling
scsi_device_put()? 0b25e17e9018 is supposed to avoid sleeping under 
&h->pg_lock. Have you considered simply not calling alua_rtpg_queue()
with this lock held? alua_rtpg_queue() accesses no data that is
protected by &h->pg_lock (which is just h->pg, AFAIU).

Would it perhaps be sufficient to just make sure we keep a kref for the
pg struct in alua_check_vpd(), like the patch below (on mkp/queue,
meant as an alternative to 0b25e17e9018)?

Regards
Martin

From 04df5933239921e7e7ac00e9ec0558124b050a51 Mon Sep 17 00:00:00 2001
From: Martin Wilck <mwilck@suse.com>
Date: Wed, 16 Nov 2022 11:24:58 +0100
Subject: [PATCH] scsi: alua: alua_check_vpd: drop pg_lock before calling
 alua_rtpg_queue

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.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 30 +++++++++++++++-------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 610a51538f03..905b49493e01 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -372,12 +372,13 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 	if (pg_updated)
 		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);
 
+	if (kref_get_unless_zero(&pg->kref)) {
+		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,11 +987,22 @@ 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;
+		struct alua_port_group *sdev_pg = NULL;
+
+		if (h) {
+			rcu_read_lock();
+			sdev_pg = rcu_dereference(h->pg);
+			rcu_read_unlock();
+		}
+
+		if (pg == sdev_pg) {
+			pg->flags |= ALUA_PG_RUN_RTPG;
+			pg->interval = 0;
+			kref_get(&pg->kref);
+			pg->rtpg_sdev = sdev;
+			start_queue = 1;
+		}
 	} else if (!(pg->flags & ALUA_PG_RUN_RTPG) && force) {
 		pg->flags |= ALUA_PG_RUN_RTPG;
 		/* Do not queue if the worker is already running */
-- 
2.38.0