ocfs2: Add extended attribute support v3

fs/ocfs2/inode.h +0 -0

index 390a855..0977569 100644

fs/ocfs2/inode.h +0 -0

  * On disk structure for xattr block.

fs/ocfs2/suballoc.c +28 -0

index 00d0fff..d521362 100644
@@ -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;
+	}
+

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.

Quoted 7 lines · Mark Fasheh

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?

Quoted 2 lines

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

Quoted 8 lines · Tiger Yang

Quoted 7 lines · Mark Fasheh

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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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

Quoted 2 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 2 lines

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.

Quoted 12 lines · Tao Ma

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 7 lines

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?

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.

Quoted 15 lines · Mark Fasheh

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);
...

Quoted 2 lines

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.