Re: [PATCH v3] ubifs: make ubifs_[get|set]xattr atomic

[Dongsheng Yang <yangds.fnst@cn.fujitsu.com>]

On 08/10/2015 04:05 PM, Artem Bityutskiy wrote:
> Right now, AFAICS, UBIFS uses the "host" inode mutex to serialize xattr
> operations. May be the terminology is not the best, but "host" is the
> non-xattr inode (say, a file inode) which "carries" the extended
> attributes. So one "host" may have many xattrs.
>
> Now, obviously, locking the entire "host" to change one xattr is very
> coarse, because operations on other xattrs get blocked by operations on
> other, unrelated xattrs.
>
> So the host inode has fields like "xattr_size" - this needs to be
> protected with the host mutex, but operations on individual xattrs may
> be protected by xattr mutexes. However, this would be a more complex
> locking scheme.

Hi Atem, thanx for your explanation. :)

But I am afraid the current locking scheme is what you want that:

(host inode) xattr_size, xattr_cnt, xattr_names ------- protected by 
host->ui_mutex
(xattr inode) data ------ protected by ui->ui_mutex (with my patch applied)

So, I think fields in host are all protected by host->ui_mutex and
fields in xattr inodes are all protected by individual ui_mutex.

Am I missing something?

BTW, I found I have to add ui_mutex in create_xattr() even if you
agree with it. :)

Yang
>
>
> So my point is - use the "host->ui_mutex", not the xattr's mutex,
> because this is how it is implemented now.
>
> Re-working and improving locking could be a separate piece of job.
>
> On Fri, 2015-08-07 at 14:07 +0800, Dongsheng Yang wrote:
>> This commit make the ubifs_[get|set]xattr protected by ui_mutex.
>>
>> Originally, there is a possibility that ubifs_getxattr to get
>> a wrong value.
>>
>>    P1                                  P2
>> ----------                  	----------
>> ubifs_getxattr                      ubifs_setxattr
>> 					- kfree()
>> - memcpy()
>> 					- kmemdup()
>>
>
> ...
>
>>
>> +	mutex_lock(&ui->ui_mutex);
>
> Or to put it differently, you need to use 'host->ui_mutex', not 'ui
> ->ui_mutex', because 'ui' is the xattr inode here.
>
> .
>