Skip to contentSkip to conversation

Add extended attributes to ext2/3

DISCUSSION Andreas Gruenbacher 4 participants 7 messages Oct 15, 2002 – Oct 21, 2002 ext2 file system

lore
Share & tools
lore

Andreas Gruenbacher

author
#0 lore raw

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.

fs/ext3/ialloc.c +3 -7
@@ -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;
fs/ext2/ialloc.c +0 -1
diff -Nur --exclude='*.orig' linux-2.4.19.old/fs/ext2/ialloc.c linux-2.4.19.new/fs/ext2/ialloc.c
@@ -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;
fs/ext2/xattr.c +2 -2
@@ -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;
fs/ext3/xattr.c +2 -3
@@ -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)
#1 lore raw
Quoted 5 lines · Andreas Gruenbacher

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.

Quoted 1 line

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

author
#2 lore raw
Quoted 3 lines · Theodore Ts'o

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.

Quoted 3 lines

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.

fs/ext3/ialloc.c +3 -7
@@ -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;
fs/ext2/ialloc.c +0 -1
diff -Nur --exclude='*.orig' linux-2.4.19.old/fs/ext2/ialloc.c linux-2.4.19.new/fs/ext2/ialloc.c
@@ -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;
fs/ext3/file.c +1 -0
@@ -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>
include/linux/ext2_acl.h +1 -0
@@ -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;
 }
include/linux/ext3_acl.h +1 -0
@@ -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;
 }
#3 lore raw
Quoted 4 lines · Andreas Gruenbacher

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/

author
#4 lore raw
Quoted 11 lines · Andreas Dilger
Quoted 5 lines · Andreas Gruenbacher

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.

author
#5 lore raw
Quoted 6 lines · Andreas Gruenbacher
Quoted 4 lines

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.

#6 lore raw
In reply to Andreas Dilger
Quoted 4 lines · Andreas Dilger

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.