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

[Bart Van Assche <bvanassche@acm.org>]

On 11/16/22 02:32, Martin Wilck wrote:
> On Tue, 2022-11-15 at 14:49 -0800, Bart Van Assche wrote:
> 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 */

Although I like the approach of this patch, how about the implementation below?
I think the implementation below is a little cleaner but that might be subjective ...

Thanks,

Bart.


diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index bd4ee294f5c7..d5a7d6ed5c63 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 (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 */
@@ -1246,8 +1257,8 @@ static void alua_bus_detach(struct scsi_device *sdev)
  		spin_unlock_irq(&pg->lock);
  		kref_put(&pg->kref, release_port_group);
  	}
-	sdev->handler_data = NULL;
  	synchronize_rcu();
+	sdev->handler_data = NULL;
  	kfree(h);
  }