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

[Hannes Reinecke <hare@suse.de>]

On 12/30/2015 02:19 PM, Christoph Hellwig wrote:
> This looks good in general, but a couple nitpicks below:
>
>> +static uint optimize_stpg;
>> +module_param(optimize_stpg, uint, S_IRUGO|S_IWUSR);
>> +MODULE_PARM_DESC(optimize_stpg, "Allow use of a non-optimized path, rather than sending a STPG, when implicit TPGS is supported (0=No,1=Yes). Default is 0.");
>
> why is this moved around in this patch?  It doesn't seem related to the
> rest of it, and isn't documented in the changelog either.
>
Yeah, can move it to a separate patch.

>>   {
>> -	struct alua_port_group *pg = NULL;
>> +	struct alua_port_group *pg;
>
> looks like this should be folded into the patch that introduces the
> unessecary NULL assignment.
>
Ok.

>>   	list_for_each_entry(pg, &port_group_list, node) {
>>   		if (pg->group_id != group_id)
>> @@ -214,18 +240,26 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
>>   	pg->group_id = group_id;
>>   	pg->tpgs = tpgs;
>>   	pg->state = TPGS_STATE_OPTIMIZED;
>> +	if (optimize_stpg)
>> +		pg->flags |= ALUA_OPTIMIZE_STPG;
>
>
> why is this moved earlier here?  Doing it from the beginning seems
> useful to me, but I'd expect it in a separate patch with a proper
> description.
>
Ok.

>>   	kref_init(&pg->kref);
>> +	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
>> +	INIT_LIST_HEAD(&pg->rtpg_list);
>> +	INIT_LIST_HEAD(&pg->node);
>> +	spin_lock_init(&pg->lock);
>>
>>   	/* Re-check list again to catch concurrent updates */
>>   	spin_lock(&port_group_lock);
>>   	tmp_pg = alua_lookup_pg(id_str, id_size, group_id);
>>   	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.

> While we're at it, there are 6 calls to
> 'kref_put(&pg->kref, release_port_group)' in addition to this one,
> so it might make sense to add a helper for it to the patch introducing
> struct alua_port_group.
>
Ok.

>>    * Extract the relative target port and the target port group
>>    * descriptor from the list of identificators.
>>    *
>> - * Returns 0 or SCSI_DH_ error code on failure.
>> + * Returns the target port group id or -1 on failure
>
> That's not how I interpret the code below, it seems to always return
> SCSI_DH_* values.
>
>> +	struct alua_port_group *pg = NULL, *old_pg = NULL;
>> +	bool pg_found = false;
>
>> +	pg = alua_get_pg(sdev, group_id, tpgs, id_str, id_size);
>> +	if (!pg)
>>   		return SCSI_DH_NOMEM;
>> +	/* Check for existing port_group references */
>> +	spin_lock(&h->pg_lock);
>> +	if (h->pg) {
>> +		old_pg = pg;
>> +		/* port_group has changed. Update to new port group */
>> +		if (h->pg != pg) {
>> +			old_pg = h->pg;
>> +			rcu_assign_pointer(h->pg, pg);
>> +			h->pg->expiry = 0;
>
> why do we set expiry to 0 here, but not if we didn't have a pg
> yet?  This could use a comment not just here but in all the places
> that set it to zero.
>
>> +			pg_found = true;
>
> 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.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html