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
h...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
