[Ocfs2-devel] [PATCH 1/1] ocfs2: Add extended attribute support v3

[Tao Ma <tao.ma@oracle.com>]

Hi Mark,

Mark Fasheh wrote:
> On Thu, Jul 31, 2008 at 05:37:12PM +0800, Tiger Yang wrote:
>> Mark Fasheh wrote:
>>> The thing is, the locking as you have it here isn't protecting against
>>> racing a data write, which is reading l_count on the extent list (or 
>>> id_count on
>>> inline data) and an xattr write which might want to shrink those. You'll
>>> need at least ip_alloc_sem around those, since ocfs2_page_mkwrite() doesn't
>>> take i_mutex because it doesn't want to deadlock against the mmap
>>> semaphore.
>> Thanks, You point out a potential bug in my patch. I didn't protect
>> reading/writing xattr against file data.
>>
>> My patch check l_count/id_count in ocfs2_xattr_has_space_inline() and
>> may change it in ocfs2_xattr_ibody_set().
>> is patch attached fix this problem?
> 
> 
> In order to protect against mmap changing the inline data state, you should
> hold a write lock on ip_alloc_sem across the call to ocfs2_xattr_ibody_find()
> and until the return from ocfs2_xattr_set_entry.
> 
> So basically, ocfs2_xattr_set() should look like:
> 
> ...
> down_write(&oi->ip_alloc_sem);
> ocfs2_ibody_find();
> ...
> ocfs2_xattr_set_entry();
> up_write(&oi->ip_alloc_sem);
> ...
> 
> 
> This way mmap can't change inline data state in between the calls to
> ibody_find and xattr_set_entry.
> 
> By the way, are operations that change or add xattrs protected by i_mutex?
> It looks like we might want that too.
i_mutex is already taken by vfs in vfs_setxattr.
> 
> 
>>> Or are you trying to protect xattr against itself? If that's the case, you
>>> could push this lock up to the top and take it around entire operations.
>> Actually I am trying to protect xattr read/write by this semaphore,
>> since we found a bug about it.
>> http://oss.oracle.com/bugzilla/show_bug.cgi?id=990
>>
>> So I need change comment about xattr semaphore.
>> /* protects extended attribute change on this inode */
> 
> You could, or how about we just take i_mutex at the top of our xattr
> operations for now? If we need the extra performance that more complicated
> locking gives us, we can add this later.
We can't use i_mutex because of the performance issue. And actually 
xattr_sem is done by me at the very beginning and I think it should be 
included in Tiger's patch, so asked him to merge it to his patch. ;)
I originally used i_mutex in get and list to protect xattr, but as 
tristan tested the patch, he told me that the performance is very bad 
compared with ext3, so I looked at how ext3 implemented it and there 
comes out the usage of xattr_sem.

Regards,
Tao