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

[Martin Wilck <mwilck@suse.com>]

On Wed, 2022-11-16 at 13:22 -0800, Bart Van Assche wrote:
> On 11/16/22 02:32, Martin Wilck wrote:
> 
> 
> [...]
> 
> 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 ...
> 

Fine with me, absolutely, and indeed better than mine. I don't quite
understand the last hook - sdev->handler_data isn't protected by rcu in
any way, is it? But that doesn't matter much.

Regards,
Martin


> 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);
>   }
>