Re: [PATCH 07/16] scsi_dh_alua: Use separate alua_port_group structure

[Hannes Reinecke <hare@suse.de>]

On 02/14/2014 12:56 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> +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);
>> +	spin_lock(&port_group_lock);
>> +	list_del(&pg->node);
>> +	spin_unlock(&port_group_lock);
>> +	if (pg->buff && pg->inq != pg->buff)
>> +		kfree(pg->buff);
>> +	kfree(pg);
>> +}
> 
> Does that message really need level "WARNING" ?
No, probably not.

> 
>> +	sdev_printk(KERN_INFO, sdev,
>> +		    "%s: port group %02x rel port %02x\n",
>> +		    ALUA_DH_NAME, group_id, h->rel_port);
>> +	spin_lock(&port_group_lock);
>> +	pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
>> +	if (!pg) {
>> +		sdev_printk(KERN_WARNING, sdev,
>> +			    "%s: kzalloc port group failed\n",
>> +			    ALUA_DH_NAME);
>> +		/* Temporary failure, bypass */
>> +		spin_unlock(&port_group_lock);
>> +		err = SCSI_DH_DEV_TEMP_BUSY;
>> +		goto out;
>> +	}
>> +	pg->group_id = group_id;
>> +	pg->buff = pg->inq;
>> +	pg->bufflen = ALUA_INQUIRY_SIZE;
>> +	pg->tpgs = h->tpgs;
>> +	pg->state = TPGS_STATE_OPTIMIZED;
>> +	pg->flags = h->flags;
>> +	kref_init(&pg->kref);
>> +	list_add(&pg->node, &port_group_list);
>> +	h->pg = pg;
>> +	spin_unlock(&port_group_lock);
>> +	err = SCSI_DH_OK;
> 
> A GFP_KERNEL allocation with a spin lock held ?? Please move that
> allocation outside the critical section and also the code for
> initializing *pg. What's not clear to me is why no uniqueness check is
> performed before invoking list_add() ? Does that mean that information
> for the same port group ID can end up multiple times in the
> port_group_list ? Such a uniqueness check can only be performed if a
> storage array identification is available so patches 07 and 08 may have
> to be swapped.
> 
The reason I did this was that I don't have to allocate memory
unnecesarily.
If I move the allocation out of the spinlock I'll have to recheck
the list upon insertion to ensure no duplicates are present.
Upon hitting a duplicate I would have to release the memory again.

I do agree that GFP_KERNEL is probably not the correct thing here;
so either I move it to GFP_ATOMIC or we may run into a chance of
having to release the memory again afterwards.

Personally I'm inclined to use GFP_ATOMIC, but I'm not sure what'd
be best.

What would you suggest 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