Re: [PATCH v4 2/3] cxlflash: Superpipe support

[Manoj Kumar <manoj@linux.vnet.ibm.com>]

Ben:

Comments inline below.


On 8/11/2015 12:29 AM, Benjamin Herrenschmidt wrote:
>   So in a similar vein to the previous review, I am missing a lot of
> context here but a few things did spring to me eyes:

Thanks for your review.

>> +	list_for_each_entry_safe(lli, temp, &cfg->lluns, list)
>> +		if (!memcmp(lli->wwid, wwid, DK_CXLFLASH_MANAGE_LUN_WWID_LEN)) {
>> +			lli->newly_created = false;
>
> This is weird ... a lookup effectively changes the state of the object
> looked up... what for ? There is something oddball here.
>
> It might be legit but in that case, you should really add a comment
> explaining the logic around that 'newly_created' field.

As suggested later, will rename these functions in v5.

> Also you drop the lock right below but you have no refcounting, are
> these objects ever disposed of ?

These objects are long lived. The local lun info structure lives as
long as the card is available, and the global lun info is kept around
as long as the module is loaded.

> In general, can you provide a clearer explanation of what are "global"
> vs "local" LUNs ?

This is a good idea. Will clarify in v5.

> Same ...

Same as above. Will address by renaming.

> Make the function name more explicit: find_or_create_lun() for example.
> I very much dislike a function called "lookup" that has side effects.

Good point. Will rename in v5.


>> +	lli = create_local(sdev, wwid);
>> +	if (unlikely(!lli))
>> +		goto out;
>
> Similar question to earlier, you have no refcounting, should I assume
> these things never get removed ?

Right, these are long lived.


>> +	lli->parent = gli;
>> +	spin_lock_irqsave(&cfg->slock, lock_flags);
>> +	list_add(&lli->list, &cfg->lluns);
>> +	spin_unlock_irqrestore(&cfg->slock, lock_flags);
>> +
>> +	spin_lock_irqsave(&global.slock, lock_flags);
>> +	list_add(&gli->list, &global.gluns);
>> +	spin_unlock_irqrestore(&global.slock, lock_flags);
>
> Your locks are extremely fine grained... too much ? Any reason why you
> don't have a simple/single lock handling all these ? IE, do you expect
> frequent accesses ?
>
> Also, a function called "lookup_something" that has the side effect of
> adding that something to two lists doesn't look great to me. You may
> want to review the function naming a bit.
>
> Finally, what happens if two processes call this trying to effectively
> create the same global LUN simultaneously ?
>
> IE, can't you have a case where both lookup fail, then they both hit the
> create_global() case for the same WWID ? Should you have a single lock
> or a mutex wrapping the whole thing ? That would make the code a lot
> simpler to review as well...

Good catch. Will look into simplifying to a mutex in v5, wrapping the
whole lookup/create sequence.


>> +void cxlflash_list_init(void)
>> +{
>> +	INIT_LIST_HEAD(&global.gluns);
>> +	spin_lock_init(&global.slock);
>> +	global.err_page = NULL;
>> +}
>
> Wouldn't it make the code nicer to have all that LUN management in a
> separate file ?

Good suggestion. Will look at moving these LUN management to a separate
file.

>> +		rc = mutex_lock_interruptible(&cfg->ctx_tbl_list_mutex);
>> +		if (rc)
>> +			goto out;
>
> This can be interrupted by any signal, I assume your userspace deals
> with it ?

That is correct. We do want to be interrupted by any signal (SIGTERM, 
SIGKILL, SIGINT etc.) at this point. We fail the context validation, and 
ultimately the ioctl and leave it to user-space to deal with it.


>> +		rc = mutex_trylock(&ctxi->mutex);
>> +		mutex_unlock(&cfg->ctx_tbl_list_mutex);
>> +		if (!rc)
>> +			goto retry;
>
> Ouch.. that's a nasty one. Are you using the above construct to avoid
> an A->B/B->A deadlock scenario where somebody else might be taking
> the list mutex while holding the context one ?

No, this is not addressing a lock ordering issue. Will clarify with a
comment why the list mutex is being released and reacquired in the
retry loop.