Re: [PATCH 11/20] scsi_dh_alua: Use workqueue for RTPG

[Christoph Hellwig <hch@lst.de>]

On Thu, Dec 31, 2015 at 02:01:43PM +0100, Hannes Reinecke wrote:
>>>   	if (tmp_pg) {
>>>   		spin_unlock(&port_group_lock);
>>> -		kfree(pg);
>>> -		return tmp_pg;
>>> +		kref_put(&pg->kref, release_port_group);
>>> +		pg = tmp_pg;
>>> +		tmp_pg = NULL;
>>
>> The only thing release_port_group does in addition to the kfree
>> is a list_del on pg->entry.  But given that we never added
>> the pg to a list it doesn't need to be deleted, and it can't
>> have another reference.  Why this change?
>>
> Mainly for symmetry; with this patch we're always calling kref_put() to 
> delete the port_group structure.

Given that it has never been added it's not actuallt symmetric.

Either way it shouldn't be added in this patch - either use kref_put
from the patch introducing the kref or not use it at all.  I would
prefer the second option.

>>
>> pg_found should be pg_updated, right?
>>
>>> +	if (pg_found)
>>> +		synchronize_rcu();
>>> +	if (old_pg) {
>>> +		if (old_pg->rtpg_sdev)
>>> +			flush_delayed_work(&old_pg->rtpg_work);
>>> +		kref_put(&old_pg->kref, release_port_group);
>>> +	}
>>
>> This code looks odd.  I can't see why we need a synchronize_rcu here.
>> The only thing we should need is a kfree_rcu for the final free in
>> release_port_group.  I also don't quite understand the flush_delayed_work.
>> As far as I can tell we only need it if rtpg_sdev is the sdev passed
>> in, so it we probably should check for that.
>>
> rtpg_sdev is set whenever the port_group structure needs or is scheduled 
> for alua_rtpg_work(), ie whenever some action needs to be taken.
> As the sdev might be a different sdev than that one we've been called from 
> (a port_group might have several sdevs pointing to it), the existence of an 
> sdev signals that we need to flush the workqueue item.

So why do we only need to flush it for some kref_put callers and not the
others?