Re: [PATCHv4 13/23] scsi_dh_alua: Use workqueue for RTPG

[Christoph Hellwig <hch@infradead.org>]

>  static void release_port_group(struct kref *kref)
>  {
>  	struct alua_port_group *pg;
>  
> +	synchronize_rcu();
>  	pg = container_of(kref, struct alua_port_group, kref);
> +	if (pg->rtpg_sdev)
> +		flush_delayed_work(&pg->rtpg_work);
>  	spin_lock(&port_group_lock);
>  	list_del(&pg->node);
>  	spin_unlock(&port_group_lock);

I don't think this is correct - we need a grace period after the
list_del to prevent new lookups.  I suspect the right thing to do here
is to simply use kfree_rcu for the pg.  This also avoids waiting for
whole grace periods for each deleted port_group, which might be rather
expensive.

> +	/* Check for existing port group references */
> +	spin_lock(&h->pg_lock);
> +	if (h->pg) {
> +		old_pg = pg;
> +		if (h->pg != pg) {
> +			/* port group has changed. Update to new port group */
> +			old_pg = h->pg;
> +			rcu_assign_pointer(h->pg, pg);
> +		}
> +	} else {
> +		rcu_assign_pointer(h->pg, pg);
> +	}

This looks confusing - pg is the structure we just allocated, so why
do we assign it to old_pg?  It seems like the above could be written
similar and more clear as:


	olg_pg = h->pg;
	if (pg != old_pg)
		rcu_assign_pointer(h->pg, pg);

> +	if (!sdev) {
> +		WARN_ON(pg->flags & ALUA_PG_RUN_RTPG ||
> +			pg->flags & ALUA_PG_RUN_STPG);

Please use two separate WARN_ONs here to know which one triggered from
the line in the kernel log.