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

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

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

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