Re: [PATCH 13/23] scsi_dh_alua: Use separate alua_port_group structure

[Hannes Reinecke <hare@suse.de>]

On 09/01/2015 12:20 PM, Christoph Hellwig wrote:
>> +struct alua_dh_data {
>> +	struct alua_port_group	*pg;
>> +	int			rel_port;
>> +	int			tpgs;
> 
> The tpgs file doesn't make much sense in the new alua_dh_data
> structure.  Please return it explicitly from alua_check_tpgs and
> assign it directly to the member in struct alua_port_group.
> 
Okay.

>> +static void release_port_group(struct kref *kref)
>> +{
>> +	struct alua_port_group *pg;
>> +
>> +	pg = container_of(kref, struct alua_port_group, kref);
>> +	printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);
> 
> Why is this a warning?  I'd consider it nothing but debug noise.
> 
Yes, indeed it is. I'll be removing it.

>> -	err = alua_rtpg(sdev, h, 0);
>> -	if (err != SCSI_DH_OK)
>> +	if (err != SCSI_DH_OK || !h->pg)
>>  		goto out;
>>  
>> +	kref_get(&h->pg->kref);
> 
> What prevents this kref_get from racing with the last kref_put?
> 
> I think all the kref_get calls in this patch need to be
> kref_get_unless_zero with proper error handling.
> 
Okay.

>> +	rcu_read_lock();
>> +	pg = rcu_dereference(h->pg);
>> +	if (!pg) {
>> +		rcu_read_unlock();
>> +		return -ENXIO;
>> +	}
> 
> What is this rcu_read_lock supposed to protect against given
> that struct alua_port_group isn't RCU freed?  I think this needs
> to be another kref_get_unless_zero.
> 
It's not freed now, but it'll be with one of the next patches (ie with
the 'rescan' patch).
I just kept it here for consistency, following the rule to always
enclose 'rcu_dereference' with 'rcu_read_lock()/rcu_read_unlock()'
pairs. Irrespective on whether it's being freed or not.

>> +
>>  	if (optimize)
>> -		h->flags |= ALUA_OPTIMIZE_STPG;
>> +		pg->flags |= ALUA_OPTIMIZE_STPG;
>>  	else
>> -		h->flags &= ~ALUA_OPTIMIZE_STPG;
>> +		pg->flags |= ~ALUA_OPTIMIZE_STPG;
>> +	rcu_read_unlock();
> 
> The read-modify-write of pg->flags will need some sort of locking.
> Seems like most modifications of it are under pg->rtpg_lock, so
> I'd suggest to enforce that as a strict rule.
> 
Yes, of course the 'rtpg_lock' should have been taken here.

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