[prev in list] [next in list] [prev in thread] [next in thread] 

List:       ocfs2-devel
Subject:    Re: [Ocfs2-devel] [PATCH 1/1] ocfs2: Add extended attribute support
From:       Tiger Yang <tiger.yang () oracle ! com>
Date:       2008-07-31 9:37:12
Message-ID: 489187C8.7030708 () oracle ! com
[Download RAW message or body]

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


["0009-2.patch" (text/x-patch)]

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 0b34db1..93e46ee 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -1596,12 +1596,16 @@ static int ocfs2_xattr_ibody_find(struct inode *inode,
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
 	int ret;
+	int has_space = 0;
 
 	if (inode->i_sb->s_blocksize == OCFS2_MIN_BLOCKSIZE)
 		return 0;
 
 	if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
-		if (!ocfs2_xattr_has_space_inline(inode, di))
+		down_read(&oi->ip_alloc_sem);
+		has_space = ocfs2_xattr_has_space_inline(inode, di);
+		up_read(&oi->ip_alloc_sem);
+		if (!has_space)
 			return 0;
 	}
 
@@ -1644,13 +1648,18 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
 	if (inode->i_sb->s_blocksize == OCFS2_MIN_BLOCKSIZE)
 		return -ENOSPC;
 
+	down_write(&oi->ip_alloc_sem);
 	if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
-		if (!ocfs2_xattr_has_space_inline(inode, di))
-			return -ENOSPC;
+		if (!ocfs2_xattr_has_space_inline(inode, di)) {
+			ret = -ENOSPC;
+			goto out;
+		}
 	}
 
 	ret = ocfs2_xattr_set_entry(inode, xi, xs,
 				(OCFS2_INLINE_XATTR_FL | OCFS2_HAS_XATTR_FL));
+out:
+	up_write(&oi->ip_alloc_sem);
 
 	return ret;
 }


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel

[prev in list] [next in list] [prev in thread] [next in thread]