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?
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
