Re: Add extended attributes to ext2/3

Re: Add extended attributes to ext2/3

[Andreas Gruenbacher]

[-- Attachment #1: Type: text/plain, Size: 203 bytes --]

Hello,

Here are two fixes as incrementals to the xattr/acl patches:

fix-xattr.diff: The bad_block bug Andreas Dilger has reported
fix-acl.diff: Make change in ext[23]_new_inode() less intrusive.

[-- Attachment #2: fix-acl.diff --]
[-- Type: text/x-diff, Size: 1083 bytes --]

--- linux-2.4.19.old/fs/ext3/ialloc.c	2002-10-15 14:18:59.000000000 +0200
+++ linux-2.4.19.new/fs/ext3/ialloc.c	2002-10-15 14:16:34.000000000 +0200
@@ -510,7 +510,9 @@
 	inode->i_generation = sb->u.ext3_sb.s_next_generation++;
 
 	inode->u.ext3_i.i_state = EXT3_STATE_NEW;
-
+	err = ext3_mark_inode_dirty(handle, inode);
+	if (err) goto fail;
+	
 	unlock_super (sb);
 	if(DQUOT_ALLOC_INODE(inode)) {
 		DQUOT_DROP(inode);
@@ -522,12 +524,6 @@
 		DQUOT_FREE_INODE(inode);
 		goto fail2;
 	}
-	err = ext3_mark_inode_dirty(handle, inode);
-	if (err) {
-		DQUOT_FREE_INODE(inode);
-		goto fail2;
-	}
-
 	ext3_debug ("allocating inode %lu\n", inode->i_ino);
 	return inode;
 
diff -Nur --exclude='*.orig' linux-2.4.19.old/fs/ext2/ialloc.c linux-2.4.19.new/fs/ext2/ialloc.c
--- linux-2.4.19.old/fs/ext2/ialloc.c	2002-10-15 15:32:24.000000000 +0200
+++ linux-2.4.19.new/fs/ext2/ialloc.c	2002-10-15 15:32:16.000000000 +0200
@@ -410,7 +410,6 @@
 		DQUOT_FREE_INODE(inode);
 		goto fail3;
 	}
-	mark_inode_dirty(inode);
 
 	ext2_debug ("allocating inode %lu\n", inode->i_ino);
 	return inode;

[-- Attachment #3: fix-xattr.diff --]
[-- Type: text/x-diff, Size: 1323 bytes --]

--- linux-2.4.19.old/fs/ext2/xattr.c	2002-10-15 13:57:44.000000000 +0200
+++ linux-2.4.19.new/fs/ext2/xattr.c	2002-10-15 13:59:57.000000000 +0200
@@ -587,6 +587,7 @@
 	struct ext2_xattr_header *header = NULL;
 	struct ext2_xattr_entry *here, *last;
 	unsigned int name_len;
+	int block = EXT2_I(inode)->i_file_acl;
 	int min_offs = sb->s_blocksize, not_found = 1, free, error;
 	char *end;
 	
@@ -618,9 +619,8 @@
 		return -ERANGE;
 	ext2_xattr_lock();
 
-	if (EXT2_I(inode)->i_file_acl) {
+	if (block) {
 		/* The inode already has an extended attribute block. */
-		int block = EXT2_I(inode)->i_file_acl;
 
 		bh = sb_bread(sb, block);
 		error = -EIO;
--- linux-2.4.19.old/fs/ext3/xattr.c	2002-10-15 14:24:10.000000000 +0200
+++ linux-2.4.19.new/fs/ext3/xattr.c	2002-10-15 14:24:01.000000000 +0200
@@ -587,6 +587,7 @@
 	struct ext3_xattr_header *header = NULL;
 	struct ext3_xattr_entry *here, *last;
 	unsigned int name_len;
+	int block = EXT3_I(inode)->i_file_acl;
 	int min_offs = sb->s_blocksize, not_found = 1, free, error;
 	char *end;
 	
@@ -618,10 +619,8 @@
 		return -ERANGE;
 	ext3_xattr_lock();
 
-	if (EXT3_I(inode)->i_file_acl) {
+	if (block) {
 		/* The inode already has an extended attribute block. */
-		int block = EXT3_I(inode)->i_file_acl;
-
 		bh = sb_bread(sb, block);
 		error = -EIO;
 		if (!bh)

Re: Add extended attributes to ext2/3

[Theodore Ts'o]

On Tue, Oct 15, 2002 at 04:40:15PM +0200, Andreas Gruenbacher wrote:
> Hello,
> 
> Here are two fixes as incrementals to the xattr/acl patches:
> 
> fix-xattr.diff: The bad_block bug Andreas Dilger has reported

I've fixed this already in my patches, thanks.

> fix-acl.diff: Make change in ext[23]_new_inode() less intrusive.

Uh, I must be missing something.

It looks like the ext3 change in fix-acl.diff was to revert a change
that I never had; it's not in the 2.4 0.8.50 patches, and it wasn't in
my patches.  So I'm not sure what's going on there.

The ext2 change in fix-acl.diff looks *wrong*.  It removes a call to
mark_inode_dirty which was there in the original, and which is
necessary.

Are you sure you sent me the right diff, or that you diffed the
correct set of trees/files?

						- Ted

Re: Add extended attributes to ext2/3

[Andreas Gruenbacher]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

On Tuesday 15 October 2002 20:29, Theodore Ts'o wrote:
> It looks like the ext3 change in fix-acl.diff was to revert a change
> that I never had; it's not in the 2.4 0.8.50 patches, and it wasn't in
> my patches.  So I'm not sure what's going on there.

Utter confusion has arisen. Can you put the current versions of your patches 
at a well known location (web/ftp/cvs), so we can more easily check and patch 
against them? That would be great.

The fix-acl patch is on top of either 0.8.50 or 8.0.51. The x_init_acl() dirty 
the inode themselves (at least they should). Initially the x_dirty_inode() 
calls had been moved below the x_init_acl() calls, but this is no longer 
necessary, and so the patch moved them up again.

BUG: I have overlooked the dummy implementation of ext[23]_init_acl(). Please 
find attached a corrected version.

> The ext2 change in fix-acl.diff looks *wrong*.  It removes a call to
> mark_inode_dirty which was there in the original, and which is
> necessary.

The original ext2_new_inode with no xattr/acl patches calls mark_inode_dirty 
before unlock_super. This call is not removed in 0.8.50 or 0.8.51, but a 
second call is added below ext2_init_acl. Since ext2_init_acl takes care of 
dirtying the inode itself this second call is no longer needed (I hope!)

--Andreas.

[-- Attachment #2: fix-acl2.diff --]
[-- Type: text/x-diff, Size: 2118 bytes --]

--- linux-2.4.19.old/fs/ext3/ialloc.c	2002-10-15 14:18:59.000000000 +0200
+++ linux-2.4.19.new/fs/ext3/ialloc.c	2002-10-15 14:16:34.000000000 +0200
@@ -510,7 +510,9 @@
 	inode->i_generation = sb->u.ext3_sb.s_next_generation++;
 
 	inode->u.ext3_i.i_state = EXT3_STATE_NEW;
-
+	err = ext3_mark_inode_dirty(handle, inode);
+	if (err) goto fail;
+	
 	unlock_super (sb);
 	if(DQUOT_ALLOC_INODE(inode)) {
 		DQUOT_DROP(inode);
@@ -522,12 +524,6 @@
 		DQUOT_FREE_INODE(inode);
 		goto fail2;
 	}
-	err = ext3_mark_inode_dirty(handle, inode);
-	if (err) {
-		DQUOT_FREE_INODE(inode);
-		goto fail2;
-	}
-
 	ext3_debug ("allocating inode %lu\n", inode->i_ino);
 	return inode;
 
diff -Nur --exclude='*.orig' linux-2.4.19.old/fs/ext2/ialloc.c linux-2.4.19.new/fs/ext2/ialloc.c
--- linux-2.4.19.old/fs/ext2/ialloc.c	2002-10-15 15:32:24.000000000 +0200
+++ linux-2.4.19.new/fs/ext2/ialloc.c	2002-10-15 15:32:16.000000000 +0200
@@ -410,7 +410,6 @@
 		DQUOT_FREE_INODE(inode);
 		goto fail3;
 	}
-	mark_inode_dirty(inode);
 
 	ext2_debug ("allocating inode %lu\n", inode->i_ino);
 	return inode;
--- linux-2.4.19/fs/ext3/file.c	2002-10-15 22:39:06.000000000 +0200
+++ linux-2.4.19.new/fs/ext3/file.c	2002-10-15 22:34:04.000000000 +0200
@@ -21,6 +21,7 @@
 #include <linux/sched.h>
 #include <linux/fs.h>
 #include <linux/locks.h>
+#include <linux/ext3_jbd.h>
 #include <linux/jbd.h>
 #include <linux/ext3_fs.h>
 #include <linux/ext3_xattr.h>
--- linux-2.4.19/include/linux/ext2_acl.h	2002-10-15 22:47:09.000000000 +0200
+++ linux-2.4.19.new/include/linux/ext2_acl.h	2002-10-15 22:30:58.000000000 +0200
@@ -87,6 +87,7 @@
 static inline int ext2_init_acl (struct inode *inode, struct inode *dir)
 {
 	inode->i_mode &= ~current->fs->umask;
+	mark_inode_dirty(inode);
 	return 0;
 }
 
--- linux-2.4.19/include/linux/ext3_acl.h	2002-10-15 22:47:16.000000000 +0200
+++ linux-2.4.19.new/include/linux/ext3_acl.h	2002-10-15 22:30:37.000000000 +0200
@@ -90,6 +90,7 @@
 ext3_init_acl(handle_t *handle, struct inode *inode, struct inode *dir)
 {
 	inode->i_mode &= ~current->fs->umask;
+	ext3_mark_inode_dirty(handle, inode);
 	return 0;
 }
 

Re: Add extended attributes to ext2/3

[Andreas Dilger]

On Oct 15, 2002  23:00 +0200, Andreas Gruenbacher wrote:
> The original ext2_new_inode with no xattr/acl patches calls mark_inode_dirty 
> before unlock_super. This call is not removed in 0.8.50 or 0.8.51, but a 
> second call is added below ext2_init_acl. Since ext2_init_acl takes care of 
> dirtying the inode itself this second call is no longer needed (I hope!)

Just as an FYI - marking ext3 inodes dirty is an expensive operation,
and should be done only once if at all possible (not sure if the same
code applies to ext3 as you are discussing ext2, but I thought I should
mention it).

Cheers, Andreas
--
Andreas Dilger
http://www-mddsp.enel.ucalgary.ca/People/adilger/
http://sourceforge.net/projects/ext2resize/

Re: Add extended attributes to ext2/3

[Andreas Gruenbacher]

On Wednesday 16 October 2002 00:01, Andreas Dilger wrote:
> On Oct 15, 2002  23:00 +0200, Andreas Gruenbacher wrote:
> > The original ext2_new_inode with no xattr/acl patches calls
> > mark_inode_dirty before unlock_super. This call is not removed in 0.8.50
> > or 0.8.51, but a second call is added below ext2_init_acl. Since
> > ext2_init_acl takes care of dirtying the inode itself this second call is
> > no longer needed (I hope!)
>
> Just as an FYI - marking ext3 inodes dirty is an expensive operation,
> and should be done only once if at all possible (not sure if the same
> code applies to ext3 as you are discussing ext2, but I thought I should
> mention it).

Then I should really think out something to improve that. Thanks.

--Andreas.

Re: Add extended attributes to ext2/3

[Andreas Gruenbacher]

On Wednesday 16 October 2002 00:09, Andreas Gruenbacher wrote:
> > Just as an FYI - marking ext3 inodes dirty is an expensive operation,
> > and should be done only once if at all possible (not sure if the same
> > code applies to ext3 as you are discussing ext2, but I thought I should
> > mention it).
>
> Then I should really think out something to improve that. Thanks.

Can I be sure that it's safe to:
 - move mark_inode_dirty() below unlock_super() in ext2
 - move ext3_mark_inode_dirty() below unlock_super() ext3
in ext[23]_new_inode()?

Thanks.

Re: Add extended attributes to ext2/3

[Alan Cox]

On Tue, 2002-10-15 at 23:01, Andreas Dilger wrote:
> Just as an FYI - marking ext3 inodes dirty is an expensive operation,
> and should be done only once if at all possible (not sure if the same
> code applies to ext3 as you are discussing ext2, but I thought I should
> mention it).

Yes its noticable that 2.4 makes ext3 inodes dirty unneccessarily in
some cases. Its one of the things Pete Zaitsev of mysql pointed out to
me.