Re: [PATCH 5/6] target: Allocate se_luns only when used

[Andy Grover <agrover@redhat.com>]

On 03/07/2014 02:41 AM, Christoph Hellwig wrote:
>> -	se_tpg->tpg_lun_list = array_zalloc(TRANSPORT_MAX_LUNS_PER_TPG,
>> -			sizeof(struct se_lun), GFP_KERNEL);
>> +	se_tpg->tpg_lun_list = kzalloc(TRANSPORT_MAX_LUNS_PER_TPG * sizeof(void*),
>> +				       GFP_KERNEL);
>
> Again seems like the pointer array should just be embedded instead of
> dynamically allocated.  Also given that it's properly typdef the kcalloc
> should have used the type instead of void *.  And it should have been
> kcalloc to start with..

Yup. I posted a "7 of 6"  patch that folds the arrays into their parent 
structures.

>> +void core_tpg_free_lun(
>> +	struct se_portal_group *tpg,
>> +	struct se_lun *lun)
>> +{
>>   	spin_lock(&tpg->tpg_lun_lock);
>> -	lun->lun_status = TRANSPORT_LUN_STATUS_FREE;
>> +	tpg->tpg_lun_list[lun->unpacked_lun] = NULL;
>>   	spin_unlock(&tpg->tpg_lun_lock);
>> +
>> +	kfree(lun);
>
> I can't see how the synchronization can work without refcounting the lun
> structure.  The lock just protectes the assignment, but you free it
> right after.  What happens to how jsut dereferenced it under the lock
> but then uses it outside (e.g. core_dev_add_initiator_node_lun_acl).

Well you're right, but this is one instance of a larger lio 
locking/refcounting hairball. This will be addressed in a separate patch 
series.

Regards  -- Andy