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

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

[Mark Fasheh]

Ok, your handling of symlinks looks fine. Thanks very much for doing this.
Other comments on this patch below.

On Fri, Jul 25, 2008 at 05:03:06PM +0800, Tiger Yang wrote:
> diff --git a/fs/ocfs2/inode.h b/fs/ocfs2/inode.h
> index 390a855..0977569 100644
> --- a/fs/ocfs2/inode.h
> +++ b/fs/ocfs2/inode.h
> @@ -40,6 +40,14 @@ struct ocfs2_inode_info
>  	/* protects allocation changes on this inode. */
>  	struct rw_semaphore		ip_alloc_sem;
>  
> +	/*
> +	 * Extended attributes can be read independently of the main file
> +	 * data. Taking i_mutex even when reading would cause contention
> +	 * between readers of EAs and writers of regular file data, so
> +	 * instead we synchronize on xattr_sem when reading or changing EAs.
> +	 */
> +	struct rw_semaphore		xattr_sem;

Nice comment - thanks for making this clear. I hate to say this though,
but... Could you please remove it and go back to i_mutex / ip_alloc_sem
locking for now?

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.


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.


>   * On disk structure for xattr block.
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index 00d0fff..d521362 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -532,6 +532,45 @@ bail:
>  	return status;
>  }
>  
> +int ocfs2_reserve_new_metadata_blocks(struct ocfs2_super *osb,
> +				      int blocks,
> +				      struct ocfs2_alloc_context **ac)
> +{
> +	int status;
> +	u32 slot;
> +
> +	*ac = kzalloc(sizeof(struct ocfs2_alloc_context), GFP_KERNEL);
> +	if (!(*ac)) {
> +		status = -ENOMEM;
> +		mlog_errno(status);
> +		goto bail;
> +	}
> +
> +	(*ac)->ac_bits_wanted = blocks;
> +	(*ac)->ac_which = OCFS2_AC_USE_META;
> +	slot = osb->slot_num;
> +	(*ac)->ac_group_search = ocfs2_block_group_search;
> +
> +	status = ocfs2_reserve_suballoc_bits(osb, (*ac),
> +					     EXTENT_ALLOC_SYSTEM_INODE,
> +					     slot, ALLOC_NEW_GROUP);
> +	if (status < 0) {
> +		if (status != -ENOSPC)
> +			mlog_errno(status);
> +		goto bail;
> +	}
> +
> +	status = 0;
> +bail:
> +	if ((status < 0) && *ac) {
> +		ocfs2_free_alloc_context(*ac);
> +		*ac = NULL;
> +	}
> +
> +	mlog_exit(status);
> +	return status;
> +}


Ok, I see what you're doing here, but you should just refactor
ocfs2_reserve_new_metadata() as a small wrapper around this function:

int ocfs2_reserve_new_metadata(struct ocfs2_super *osb,
			       struct ocfs2_extent_list *root_el,
			       struct ocfs2_alloc_context **ac)
{
	return ocfs2_reserve_new_metadata_blocks(osb, ocfs2_extend_meta_needed(root_el), ac);
}

Thanks,
	--Mark

--
Mark Fasheh

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

[Tiger Yang]

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?


> 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 */

Best regards,
tiger

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-2.patch
Type: text/x-patch
Size: 1253 bytes
Desc: not available
Url : http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20080731/7cf7eb39/attachment.bin 

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

[Mark Fasheh]

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.


> >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.
	--Mark

--
Mark Fasheh

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

[Tao Ma]

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

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

[Mark Fasheh]

On Tue, Aug 05, 2008 at 10:39:39AM +0800, Tao Ma wrote:
> >>>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.

Ok, if that's the case then we can certainly keep xattr_sem. I'm curious
though - what tests are we talking about here?
	--Mark

--
Mark Fasheh

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

[Tao Ma]

Hi Mark,

Mark Fasheh wrote:
> On Tue, Aug 05, 2008 at 10:39:39AM +0800, Tao Ma wrote:
>>>>> 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.
> 
> Ok, if that's the case then we can certainly keep xattr_sem. I'm curious
> though - what tests are we talking about here?
http://oss.oracle.com/osswiki/OCFS2/XattrTest

btw, tristan.ye from oracle will focus on ocfs2 test in the following 
year. Thank you, tristan.

Regards,
Tao

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

[Tiger Yang]

Mark Fasheh wrote:
> 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.

Thanks the guide. I think I have done what your said.
In ocfs2_ibody_find(), I checked the l_count/id_count with readable 
semaphore. And in ocfs2_xattr_set_ibody, I checked it again and follow 
by ocfs2_xattr_set_entry() with writable semaphore. If mmap change 
l_count/id_count between those two function, I still have chance to know 
it. And data in struct ocfs2_xattr_search which filled in 
ocfs2_ibody_find() are not affected by mmap operations.

Best regards,
tiger

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

[Mark Fasheh]

On Wed, Aug 06, 2008 at 10:34:53AM +0800, Tiger Yang wrote:
> 
> Mark Fasheh wrote:
> >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.
> 
> Thanks the guide. I think I have done what your said.
> In ocfs2_ibody_find(), I checked the l_count/id_count with readable 
> semaphore. And in ocfs2_xattr_set_ibody, I checked it again and follow 
> by ocfs2_xattr_set_entry() with writable semaphore. If mmap change 
> l_count/id_count between those two function, I still have chance to know 
> it. And data in struct ocfs2_xattr_search which filled in 
> ocfs2_ibody_find() are not affected by mmap operations.

Ok, sounds good to me.
	--Mark

--
Mark Fasheh