Skip to contentSkip to conversation

e2fsck: add support for large xattrs in external inodes

PATCH In review Tahsin Erdogan 3 participants 25 messages Jun 26, 2017 – Jul 5, 2017

lore
Share & tools
lore
#0 lore raw

PATCH 1/12 e2fsck: add support for large xattrs in external inodes

From: Andreas Dilger <[email protected]>

Add support for the INCOMPAT_EA_INODE feature, which stores large extended attributes into an external inode instead of data blocks. The inode is referenced by the e_value_inum field (formerly the unused e_value_block field) from the extent header, and stores the xattr data starting at byte offset 0 in the inode data block.

The xattr inode stores the referring inode number in its i_mtime, and the parent i_generation in its own i_generation, so that there is a solid linkage between the two that e2fsck can verify. The xattr inode is itself marked with EXT4_EA_INODE_FL as well.

---
 e2fsck/e2fsck.h            |   1 +
 e2fsck/pass1.c             | 128 ++++++++++++++++++++++++++++++++++-----------
 e2fsck/pass4.c             |  18 +++++++
 e2fsck/problem.c           |  22 ++++++++
 e2fsck/problem.h           |  14 +++++
 lib/ext2fs/ext2_ext_attr.h |   2 +-
 lib/ext2fs/ext2_fs.h       |   3 +-
 lib/ext2fs/ext2fs.h        |   1 +
 lib/ext2fs/ext_attr.c      | 116 +++++++++++++++++++++++++---------------
 lib/ext2fs/swapfs.c        |   2 +-
 misc/mke2fs.c              |   1 +
 misc/tune2fs.c             |   2 +
 12 files changed, 235 insertions(+), 75 deletions(-)
e2fsck/e2fsck.h +1 -0
index fd12024c2cd7..c19cdfdd733e 100644
@@ -254,6 +254,7 @@ struct e2fsck_struct {
 	ext2fs_inode_bitmap inode_bb_map; /* Inodes which are in bad blocks */
 	ext2fs_inode_bitmap inode_imagic_map; /* AFS inodes */
 	ext2fs_inode_bitmap inode_reg_map; /* Inodes which are regular files*/
+	ext2fs_inode_bitmap inode_ea_map; /* EA inodes which are non-orphan */
 
 	ext2fs_block_bitmap block_found_map; /* Blocks which are in use */
 	ext2fs_block_bitmap block_dup_map; /* Blks referenced more than once */
e2fsck/pass1.c +97 -31

160 lines not on this page · raw message

e2fsck/pass4.c +18 -0
index 8c101fd2805c..e44fc69c1613 100644
@@ -11,6 +11,7 @@
  * Pass 4 frees the following data structures:
  * 	- A bitmap of which inodes are in bad blocks.	(inode_bb_map)
  * 	- A bitmap of which inodes are imagic inodes.	(inode_imagic_map)
+ *	- A bitmap of EA inodes.			(inode_ea_map)
  */
 
 #include "config.h"
@@ -38,6 +39,21 @@ static int disconnect_inode(e2fsck_t ctx, ext2_ino_t i,
 			       "pass4: disconnect_inode");
 	if (EXT2_INODE_SIZE(fs->super) > EXT2_GOOD_OLD_INODE_SIZE)
 		extra_size = inode->i_extra_isize;
+
+	if (inode->i_flags & EXT4_EA_INODE_FL) {
+		if (ext2fs_test_inode_bitmap2(ctx->inode_ea_map, i)) {
+			ext2fs_icount_store(ctx->inode_count, i, 1);
+			return 0;
+		} else {
+			/* Zero the link count so that when inode is linked to
+			 * lost+found it has correct link count */
+			inode->i_links_count = 0;
+			e2fsck_write_inode(ctx, i, EXT2_INODE(inode),
+					   "disconnect_inode");
+			ext2fs_icount_store(ctx->inode_link_info, i, 0);
+		}
+	}
+
 	clear_problem_context(&pctx);
 	pctx.ino = i;
 	pctx.inode = EXT2_INODE(inode);
@@ -195,6 +211,8 @@ void e2fsck_pass4(e2fsck_t ctx)
 	}
 	ext2fs_free_icount(ctx->inode_link_info); ctx->inode_link_info = 0;
 	ext2fs_free_icount(ctx->inode_count); ctx->inode_count = 0;
+	ext2fs_free_inode_bitmap(ctx->inode_ea_map);
+	ctx->inode_ea_map = 0;
 	ext2fs_free_inode_bitmap(ctx->inode_bb_map);
 	ctx->inode_bb_map = 0;
 	ext2fs_free_inode_bitmap(ctx->inode_imagic_map);
e2fsck/problem.c +22 -0
index 206a8a0b0639..dcdf9abe1abd 100644
@@ -1145,6 +1145,28 @@ static struct e2fsck_problem problem_table[] = {
 	  N_("Timestamp(s) on @i %i beyond 2310-04-04 are likely pre-1970.\n"),
 	  PROMPT_FIX, PR_PREEN_OK | PR_NO_OK },
 
+	/* Inode has illegal extended attribute value inode */
+	{ PR_1_ATTR_VALUE_EA_INODE,
+	  N_("@i %i has @I @a value @i %N.\n"),
+	  PROMPT_CLEAR, PR_PREEN_OK },
+
+	/* Invalid backpointer from extended attribute inode to parent inode */
+	{ PR_1_ATTR_INVAL_EA_INODE,
+	  N_("@n backpointer from @a @i %N to parent @i %i.\n"),
+	  PROMPT_CLEAR, PR_PREEN_OK },
+
+	/* Inode has invalid extended attribute. EA inode missing
+	 * EA_INODE flag. */
+	{ PR_1_ATTR_NO_EA_INODE_FL,
+	  N_("@i %i has @n @a. EA @i %N missing EA_INODE flag.\n"),
+	  PROMPT_CLEAR, PR_PREEN_OK },
+
+	/* EA inode for parent inode missing EA_INODE flag. */
+	{ PR_1_ATTR_SET_EA_INODE_FL,
+	  N_("EA @i %N for parent @i %i missing EA_INODE flag.\n "),
+	  PROMPT_FIX, PR_PREEN_OK },
+
+
 	/* Pass 1b errors */
 
 	/* Pass 1B: Rescan for duplicate/bad blocks */
e2fsck/problem.h +14 -0
index f8650b941b9a..dad608c94fd0 100644
@@ -675,6 +675,20 @@ struct problem_context {
 /* Timestamp(s) on inode beyond 2310-04-04 are likely pre-1970. */
 #define PR_1_EA_TIME_OUT_OF_RANGE		0x010082
 
+/* Inode has illegal EA value inode */
+#define PR_1_ATTR_VALUE_EA_INODE		0x010083
+
+/* Invalid backpointer from EA inode to parent inode */
+#define PR_1_ATTR_INVAL_EA_INODE		0x010084
+
+/* Parent inode has invalid EA entry. EA inode does not have
+ * EXT4_EA_INODE_FL flag. Delete EA entry? */
+#define PR_1_ATTR_NO_EA_INODE_FL		0x010085
+
+/* EA inode for parent inode does not have EXT4_EA_INODE_FL flag */
+#define PR_1_ATTR_SET_EA_INODE_FL		0x010086
+
+
 /*
  * Pass 1b errors
  */
lib/ext2fs/ext2_ext_attr.h +1 -1
index bbb0aaa97bca..f2042ed56cf9 100644
@@ -29,7 +29,7 @@ struct ext2_ext_attr_entry {
 	__u8	e_name_len;	/* length of name */
 	__u8	e_name_index;	/* attribute name index */
 	__u16	e_value_offs;	/* offset in disk block of value */
-	__u32	e_value_block;	/* disk block attribute is stored on (n/i) */
+	__u32	e_value_inum;	/* inode in which the value is stored */
 	__u32	e_value_size;	/* size of attribute value */
 	__u32	e_hash;		/* hash value of name and value */
 #if 0
lib/ext2fs/ext2_fs.h +2 -1
index 66b7058c3993..3b55000e3a90 100644
@@ -922,7 +922,8 @@ EXT4_FEATURE_INCOMPAT_FUNCS(encrypt,		4, ENCRYPT)
 #define EXT2_FEATURE_COMPAT_SUPP	0
 #define EXT2_FEATURE_INCOMPAT_SUPP    (EXT2_FEATURE_INCOMPAT_FILETYPE| \
 				       EXT4_FEATURE_INCOMPAT_MMP| \
-				       EXT4_FEATURE_INCOMPAT_LARGEDIR)
+				       EXT4_FEATURE_INCOMPAT_LARGEDIR| \
+				       EXT4_FEATURE_INCOMPAT_EA_INODE)
 #define EXT2_FEATURE_RO_COMPAT_SUPP	(EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER| \
 					 EXT2_FEATURE_RO_COMPAT_LARGE_FILE| \
 					 EXT4_FEATURE_RO_COMPAT_DIR_NLINK| \
lib/ext2fs/ext2fs.h +1 -0
index c18ea5f827ad..f4131a640f9c 100644
@@ -598,6 +598,7 @@ typedef struct ext2_icount *ext2_icount_t;
 					 EXT3_FEATURE_INCOMPAT_RECOVER|\
 					 EXT3_FEATURE_INCOMPAT_EXTENTS|\
 					 EXT4_FEATURE_INCOMPAT_FLEX_BG|\
+					 EXT4_FEATURE_INCOMPAT_EA_INODE|\
 					 EXT4_LIB_INCOMPAT_MMP|\
 					 EXT4_FEATURE_INCOMPAT_64BIT|\
 					 EXT4_FEATURE_INCOMPAT_INLINE_DATA|\
lib/ext2fs/ext_attr.c +75 -41

153 lines not on this page · raw message

lib/ext2fs/swapfs.c +1 -1
index 2d05ee7b995b..23a8570c2a3a 100644
@@ -170,7 +170,7 @@ void ext2fs_swap_ext_attr_entry(struct ext2_ext_attr_entry *to_entry,
 				struct ext2_ext_attr_entry *from_entry)
 {
 	to_entry->e_value_offs  = ext2fs_swab16(from_entry->e_value_offs);
-	to_entry->e_value_block = ext2fs_swab32(from_entry->e_value_block);
+	to_entry->e_value_inum  = ext2fs_swab32(from_entry->e_value_inum);
 	to_entry->e_value_size  = ext2fs_swab32(from_entry->e_value_size);
 	to_entry->e_hash	= ext2fs_swab32(from_entry->e_hash);
 }
misc/mke2fs.c +1 -0
index 6cbba33b31d8..1ef46f44f35d 100644
@@ -1082,6 +1082,7 @@ static __u32 ok_features[3] = {
 		EXT3_FEATURE_INCOMPAT_JOURNAL_DEV|
 		EXT2_FEATURE_INCOMPAT_META_BG|
 		EXT4_FEATURE_INCOMPAT_FLEX_BG|
+		EXT4_FEATURE_INCOMPAT_EA_INODE|
 		EXT4_FEATURE_INCOMPAT_MMP |
 		EXT4_FEATURE_INCOMPAT_64BIT|
 		EXT4_FEATURE_INCOMPAT_INLINE_DATA|
misc/tune2fs.c +2 -0
index dc5872934427..90746707064b 100644
@@ -153,6 +153,7 @@ static __u32 ok_features[3] = {
 	EXT2_FEATURE_INCOMPAT_FILETYPE |
 		EXT3_FEATURE_INCOMPAT_EXTENTS |
 		EXT4_FEATURE_INCOMPAT_FLEX_BG |
+		EXT4_FEATURE_INCOMPAT_EA_INODE|
 		EXT4_FEATURE_INCOMPAT_MMP |
 		EXT4_FEATURE_INCOMPAT_64BIT |
 		EXT4_FEATURE_INCOMPAT_ENCRYPT |
@@ -179,6 +180,7 @@ static __u32 clear_ok_features[3] = {
 	/* Incompat */
 	EXT2_FEATURE_INCOMPAT_FILETYPE |
 		EXT4_FEATURE_INCOMPAT_FLEX_BG |
+		EXT4_FEATURE_INCOMPAT_EA_INODE|
 		EXT4_FEATURE_INCOMPAT_MMP |
 		EXT4_FEATURE_INCOMPAT_64BIT |
 		EXT4_FEATURE_INCOMPAT_CSUM_SEED,
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#1 lore raw

PATCH 2/12 tune2fs: do not allow disabling ea_inode feature

Disabling ea_inode feature would require inlining all the existing xattr values that are currently stored in external inodes. This is not always possible. Just disallow it.

---
 misc/tune2fs.c | 1 -
 1 file changed, 1 deletion(-)
misc/tune2fs.c +0 -1
index 90746707064b..2a437b9fc7b7 100644
@@ -180,7 +180,6 @@ static __u32 clear_ok_features[3] = {
 	/* Incompat */
 	EXT2_FEATURE_INCOMPAT_FILETYPE |
 		EXT4_FEATURE_INCOMPAT_FLEX_BG |
-		EXT4_FEATURE_INCOMPAT_EA_INODE|
 		EXT4_FEATURE_INCOMPAT_MMP |
 		EXT4_FEATURE_INCOMPAT_64BIT |
 		EXT4_FEATURE_INCOMPAT_CSUM_SEED,
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#2 lore raw

PATCH 3/12 e2fsck: ea_inode hash validation

In reply to Tahsin Erdogan

In original implementation of ea_inode feature, each xattr inode had a single parent. Child inode tracked the parent by storing its inode number in i_mtime field. Also, child's i_generation matched parent's i_generation.

With deduplication support, xattr inodes can now be shared so a single backpointer is not sufficient to achieve strong binding. This is now replaced by hash validation.

---
 e2fsck/pass1.c        | 91 +++++++++++++++++++++++++++++++++------------------
 e2fsck/problem.c      |  5 ---
 e2fsck/problem.h      |  7 ++--
 lib/ext2fs/ext2fs.h   |  3 ++
 lib/ext2fs/ext_attr.c | 67 +++++++++++++++++++++++++++++++++++--
 5 files changed, 128 insertions(+), 45 deletions(-)
e2fsck/pass1.c +59 -32
index 1c68af8990b4..32152f3ec926 100644
@@ -360,27 +360,54 @@ static problem_t check_large_ea_inode(e2fsck_t ctx,
 				      struct problem_context *pctx)
 {
 	struct ext2_inode inode;
+	__u32 hash;
+	errcode_t retval;
 
 	/* Check if inode is within valid range */
 	if ((entry->e_value_inum < EXT2_FIRST_INODE(ctx->fs->super)) ||
-	    (entry->e_value_inum > ctx->fs->super->s_inodes_count))
+	    (entry->e_value_inum > ctx->fs->super->s_inodes_count)) {
+		pctx->num = entry->e_value_inum;
 		return PR_1_ATTR_VALUE_EA_INODE;
+	}
 
 	e2fsck_read_inode(ctx, entry->e_value_inum, &inode, "pass1");
+
+	retval = ext2fs_ext_attr_hash_entry2(ctx->fs, entry, NULL, &hash);
+	if (retval) {
+		com_err("check_large_ea_inode", retval,
+			_("while hashing entry with e_value_inum = %u"),
+			entry->e_value_inum);
+		fatal_error(ctx, 0);
+	}
+
+	if (hash != entry->e_hash) {
+		/* This might be an old Lustre-style ea_inode reference. */
+		if (inode.i_mtime != pctx->ino ||
+		    inode.i_generation != pctx->inode->i_generation) {
+
+			/* If target inode is also missing EA_INODE flag,
+			 * this is likely to be a bad reference.
+			 */
+			if (!(inode.i_flags & EXT4_EA_INODE_FL)) {
+				pctx->num = entry->e_value_inum;
+				return PR_1_ATTR_VALUE_EA_INODE;
+			} else {
+				pctx->num = entry->e_hash;
+				return PR_1_ATTR_HASH;
+			}
+		}
+	}
+
 	if (!(inode.i_flags & EXT4_EA_INODE_FL)) {
-		/* If EXT4_EA_INODE_FL flag is not present but back-pointer
-		 * matches then we should set this flag */
-		if (inode.i_mtime == pctx->ino &&
-		    inode.i_generation == pctx->inode->i_generation &&
-		    fix_problem(ctx, PR_1_ATTR_SET_EA_INODE_FL, pctx)) {
+		pctx->num = entry->e_value_inum;
+		if (fix_problem(ctx, PR_1_ATTR_SET_EA_INODE_FL, pctx)) {
 			inode.i_flags |= EXT4_EA_INODE_FL;
-			ext2fs_write_inode(ctx->fs, entry->e_value_inum,&inode);
-		} else
+			ext2fs_write_inode(ctx->fs, entry->e_value_inum,
+					   &inode);
+		} else {
 			return PR_1_ATTR_NO_EA_INODE_FL;
-	} else if (inode.i_mtime != pctx->ino ||
-		   inode.i_generation != pctx->inode->i_generation)
-		return PR_1_ATTR_INVAL_EA_INODE;
-
+		}
+	}
 	return 0;
 }
 
@@ -458,6 +485,16 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx)
 				problem = PR_1_INODE_EA_ALLOC_COLLISION;
 				goto fix;
 			}
+
+			hash = ext2fs_ext_attr_hash_entry(entry,
+							  start + entry->e_value_offs);
+
+			/* e_hash may be 0 in older inode's ea */
+			if (entry->e_hash != 0 && entry->e_hash != hash) {
+				pctx->num = entry->e_hash;
+				problem = PR_1_ATTR_HASH;
+				goto fix;
+			}
 		} else {
 			problem = check_large_ea_inode(ctx, entry, pctx);
 			if (problem != 0)
@@ -466,16 +503,6 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx)
 			mark_inode_ea_map(ctx, pctx, entry->e_value_inum);
 		}
 
-		hash = ext2fs_ext_attr_hash_entry(entry,
-						  start + entry->e_value_offs);
-
-		/* e_hash may be 0 in older inode's ea */
-		if (entry->e_hash != 0 && entry->e_hash != hash) {
-			pctx->num = entry->e_hash;
-			problem = PR_1_ATTR_HASH;
-			goto fix;
-		}
-
 		/* If EA value is stored in external inode then it does not
 		 * consume space here */
 		if (entry->e_value_inum == 0)
@@ -2439,6 +2466,16 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
 						pctx))
 					goto clear_extattr;
 			}
+
+			hash = ext2fs_ext_attr_hash_entry(entry, block_buf +
+							  entry->e_value_offs);
+
+			if (entry->e_hash != hash) {
+				pctx->num = entry->e_hash;
+				if (fix_problem(ctx, PR_1_ATTR_HASH, pctx))
+					goto clear_extattr;
+				entry->e_hash = hash;
+			}
 		} else {
 			problem_t problem;
 
@@ -2450,16 +2487,6 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
 				goto clear_extattr;
 		}
 
-		hash = ext2fs_ext_attr_hash_entry(entry, block_buf +
-							 entry->e_value_offs);
-
-		if (entry->e_hash != hash) {
-			pctx->num = entry->e_hash;
-			if (fix_problem(ctx, PR_1_ATTR_HASH, pctx))
-				goto clear_extattr;
-			entry->e_hash = hash;
-		}
-
 		entry = EXT2_EXT_ATTR_NEXT(entry);
 	}
 	if (region_allocate(region, (char *)entry - (char *)header, 4)) {
e2fsck/problem.c +0 -5
index dcdf9abe1abd..deffa4d66a89 100644
@@ -1150,11 +1150,6 @@ static struct e2fsck_problem problem_table[] = {
 	  N_("@i %i has @I @a value @i %N.\n"),
 	  PROMPT_CLEAR, PR_PREEN_OK },
 
-	/* Invalid backpointer from extended attribute inode to parent inode */
-	{ PR_1_ATTR_INVAL_EA_INODE,
-	  N_("@n backpointer from @a @i %N to parent @i %i.\n"),
-	  PROMPT_CLEAR, PR_PREEN_OK },
-
 	/* Inode has invalid extended attribute. EA inode missing
 	 * EA_INODE flag. */
 	{ PR_1_ATTR_NO_EA_INODE_FL,
e2fsck/problem.h +2 -5
index dad608c94fd0..8e07c10895c1 100644
@@ -678,15 +678,12 @@ struct problem_context {
 /* Inode has illegal EA value inode */
 #define PR_1_ATTR_VALUE_EA_INODE		0x010083
 
-/* Invalid backpointer from EA inode to parent inode */
-#define PR_1_ATTR_INVAL_EA_INODE		0x010084
-
 /* Parent inode has invalid EA entry. EA inode does not have
  * EXT4_EA_INODE_FL flag. Delete EA entry? */
-#define PR_1_ATTR_NO_EA_INODE_FL		0x010085
+#define PR_1_ATTR_NO_EA_INODE_FL		0x010084
 
 /* EA inode for parent inode does not have EXT4_EA_INODE_FL flag */
-#define PR_1_ATTR_SET_EA_INODE_FL		0x010086
+#define PR_1_ATTR_SET_EA_INODE_FL		0x010085
 
 
 /*
lib/ext2fs/ext2fs.h +3 -0
index f4131a640f9c..f72cbe17a8cd 100644
@@ -1198,6 +1198,9 @@ extern errcode_t ext2fs_expand_dir(ext2_filsys fs, ext2_ino_t dir);
 /* ext_attr.c */
 extern __u32 ext2fs_ext_attr_hash_entry(struct ext2_ext_attr_entry *entry,
 					void *data);
+extern errcode_t ext2fs_ext_attr_hash_entry2(ext2_filsys fs,
+					     struct ext2_ext_attr_entry *entry,
+					     void *data, __u32 *hash);
 extern errcode_t ext2fs_read_ext_attr(ext2_filsys fs, blk_t block, void *buf);
 extern errcode_t ext2fs_read_ext_attr2(ext2_filsys fs, blk64_t block,
 				       void *buf);
lib/ext2fs/ext_attr.c +64 -3

85 lines not on this page · raw message

Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#3 lore raw

Re: [PATCH 03/12] e2fsck: ea_inode hash validation

Quoted 25 lines · Tahsin Erdogan

In original implementation of ea_inode feature, each xattr inode had a single parent. Child inode tracked the parent by storing its inode number in i_mtime field. Also, child's i_generation matched parent's i_generation.

With deduplication support, xattr inodes can now be shared so a single backpointer is not sufficient to achieve strong binding. This is now replaced by hash validation.

---

e2fsck/pass1.c        | 91 +++++++++++++++++++++++++++++++++------------------
e2fsck/problem.c      |  5 ---
e2fsck/problem.h      |  7 ++--
lib/ext2fs/ext2fs.h   |  3 ++
lib/ext2fs/ext_attr.c | 67 +++++++++++++++++++++++++++++++++++--
5 files changed, 128 insertions(+), 45 deletions(-)
e2fsck/pass1.c +0 -0
index 1c68af8990b4..32152f3ec926 100644
@@ -2439,6 +2466,16 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
 						pctx))

e2fsck/pass1.c static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx, · line 2476

Quoted 10 lines
e2fsck/pass1.c +8 -0
 					goto clear_extattr;
 			}
+
+			hash = ext2fs_ext_attr_hash_entry(entry, block_buf +
+							  entry->e_value_offs);
+
+			if (entry->e_hash != hash) {
+				pctx->num = entry->e_hash;
+				if (fix_problem(ctx, PR_1_ATTR_HASH, pctx))
+					goto clear_extattr;

Shouldn't this be "if (!fix_problem(...))" ?

Quoted 9 lines
e2fsck/problem.h +0 -1
index dad608c94fd0..8e07c10895c1 100644
@@ -678,15 +678,12 @@ struct problem_context {
 /* Inode has illegal EA value inode */
 #define PR_1_ATTR_VALUE_EA_INODE		0x010083
 
-/* Invalid backpointer from EA inode to parent inode */

e2fsck/problem.h struct problem_context { · line 686

Quoted 10 lines
e2fsck/problem.h +2 -4
-#define PR_1_ATTR_INVAL_EA_INODE		0x010084
-
 /* Parent inode has invalid EA entry. EA inode does not have
 * EXT4_EA_INODE_FL flag. Delete EA entry? */
-#define PR_1_ATTR_NO_EA_INODE_FL		0x010085
+#define PR_1_ATTR_NO_EA_INODE_FL		0x010084
 
 /* EA inode for parent inode does not have EXT4_EA_INODE_FL flag */
-#define PR_1_ATTR_SET_EA_INODE_FL		0x010086
+#define PR_1_ATTR_SET_EA_INODE_FL		0x010085

It would also be OK to keep the old values, and just skip 0x10084.

Quoted 13 lines
lib/ext2fs/ext_attr.c +2 -3
index 14d05c7e57fb..d48ab5c55270 100644
@@ -914,9 +955,29 @@ static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle,
 			void *data = (entry->e_value_inum != 0) ?
 					0 : value_start + entry->e_value_offs;
 
-			hash = ext2fs_ext_attr_hash_entry(entry, data);
-			if (entry->e_hash != hash)
-				return EXT2_ET_BAD_EA_HASH;
+			err = ext2fs_ext_attr_hash_entry2(handle->fs, entry,
+							  data, &hash);

lib/ext2fs/ext_attr.c static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle, · line 969

Quoted 10 lines
lib/ext2fs/ext_attr.c +10 -0
+			if (err)
+				return err;
+			if (entry->e_hash != hash) {
+				struct ext2_inode parent, child;
+
+				/* Check whether this is an old Lustre-style
+				 * ea_inode reference.
+				 */
+				err = ext2fs_read_inode(handle->fs, handle->ino,
+							&parent);

Rather than reading the inode again here, it would make more sense to pass the parent inode from the caller.

Quoted 15 lines
lib/ext2fs/ext_attr.c +11 -1
+				if (err)
+					return err;
+				err = ext2fs_read_inode(handle->fs,
+							entry->e_value_inum,
+							&child);
+				if (err)
+					return err;
+				if (child.i_mtime != handle->ino ||
+				    child.i_generation != parent.i_generation)
+					return EXT2_ET_BAD_EA_HASH;
+			}
 		}
 
 		x++;
--
Quoted 2 lines

2.13.1.611.g7e3b11ae1-goog

Cheers, Andreas

#4 lore raw

PATCH v2 3/12 e2fsck: ea_inode hash validation

In original implementation of ea_inode feature, each xattr inode had a single parent. Child inode tracked the parent by storing its inode number in i_mtime field. Also, child's i_generation matched parent's i_generation.

With deduplication support, xattr inodes can now be shared so a single backpointer is not sufficient to achieve strong binding. This is now replaced by hash validation.

---
v2:
- restored original problem number definitions
- pass parent inode to read_xattrs_from_buffer() instead of reading it again

 e2fsck/pass1.c        | 91 +++++++++++++++++++++++++++++++++------------------
 e2fsck/problem.c      |  5 ---
 e2fsck/problem.h      |  3 --
 lib/ext2fs/ext2fs.h   |  3 ++
 lib/ext2fs/ext_attr.c | 68 +++++++++++++++++++++++++++++++++++---
 5 files changed, 125 insertions(+), 45 deletions(-)
e2fsck/pass1.c +59 -32
index 1c68af8990b4..32152f3ec926 100644
@@ -360,27 +360,54 @@ static problem_t check_large_ea_inode(e2fsck_t ctx,
 				      struct problem_context *pctx)
 {
 	struct ext2_inode inode;
+	__u32 hash;
+	errcode_t retval;
 
 	/* Check if inode is within valid range */
 	if ((entry->e_value_inum < EXT2_FIRST_INODE(ctx->fs->super)) ||
-	    (entry->e_value_inum > ctx->fs->super->s_inodes_count))
+	    (entry->e_value_inum > ctx->fs->super->s_inodes_count)) {
+		pctx->num = entry->e_value_inum;
 		return PR_1_ATTR_VALUE_EA_INODE;
+	}
 
 	e2fsck_read_inode(ctx, entry->e_value_inum, &inode, "pass1");
+
+	retval = ext2fs_ext_attr_hash_entry2(ctx->fs, entry, NULL, &hash);
+	if (retval) {
+		com_err("check_large_ea_inode", retval,
+			_("while hashing entry with e_value_inum = %u"),
+			entry->e_value_inum);
+		fatal_error(ctx, 0);
+	}
+
+	if (hash != entry->e_hash) {
+		/* This might be an old Lustre-style ea_inode reference. */
+		if (inode.i_mtime != pctx->ino ||
+		    inode.i_generation != pctx->inode->i_generation) {
+
+			/* If target inode is also missing EA_INODE flag,
+			 * this is likely to be a bad reference.
+			 */
+			if (!(inode.i_flags & EXT4_EA_INODE_FL)) {
+				pctx->num = entry->e_value_inum;
+				return PR_1_ATTR_VALUE_EA_INODE;
+			} else {
+				pctx->num = entry->e_hash;
+				return PR_1_ATTR_HASH;
+			}
+		}
+	}
+
 	if (!(inode.i_flags & EXT4_EA_INODE_FL)) {
-		/* If EXT4_EA_INODE_FL flag is not present but back-pointer
-		 * matches then we should set this flag */
-		if (inode.i_mtime == pctx->ino &&
-		    inode.i_generation == pctx->inode->i_generation &&
-		    fix_problem(ctx, PR_1_ATTR_SET_EA_INODE_FL, pctx)) {
+		pctx->num = entry->e_value_inum;
+		if (fix_problem(ctx, PR_1_ATTR_SET_EA_INODE_FL, pctx)) {
 			inode.i_flags |= EXT4_EA_INODE_FL;
-			ext2fs_write_inode(ctx->fs, entry->e_value_inum,&inode);
-		} else
+			ext2fs_write_inode(ctx->fs, entry->e_value_inum,
+					   &inode);
+		} else {
 			return PR_1_ATTR_NO_EA_INODE_FL;
-	} else if (inode.i_mtime != pctx->ino ||
-		   inode.i_generation != pctx->inode->i_generation)
-		return PR_1_ATTR_INVAL_EA_INODE;
-
+		}
+	}
 	return 0;
 }
 
@@ -458,6 +485,16 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx)
 				problem = PR_1_INODE_EA_ALLOC_COLLISION;
 				goto fix;
 			}
+
+			hash = ext2fs_ext_attr_hash_entry(entry,
+							  start + entry->e_value_offs);
+
+			/* e_hash may be 0 in older inode's ea */
+			if (entry->e_hash != 0 && entry->e_hash != hash) {
+				pctx->num = entry->e_hash;
+				problem = PR_1_ATTR_HASH;
+				goto fix;
+			}
 		} else {
 			problem = check_large_ea_inode(ctx, entry, pctx);
 			if (problem != 0)
@@ -466,16 +503,6 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx)
 			mark_inode_ea_map(ctx, pctx, entry->e_value_inum);
 		}
 
-		hash = ext2fs_ext_attr_hash_entry(entry,
-						  start + entry->e_value_offs);
-
-		/* e_hash may be 0 in older inode's ea */
-		if (entry->e_hash != 0 && entry->e_hash != hash) {
-			pctx->num = entry->e_hash;
-			problem = PR_1_ATTR_HASH;
-			goto fix;
-		}
-
 		/* If EA value is stored in external inode then it does not
 		 * consume space here */
 		if (entry->e_value_inum == 0)
@@ -2439,6 +2466,16 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
 						pctx))
 					goto clear_extattr;
 			}
+
+			hash = ext2fs_ext_attr_hash_entry(entry, block_buf +
+							  entry->e_value_offs);
+
+			if (entry->e_hash != hash) {
+				pctx->num = entry->e_hash;
+				if (fix_problem(ctx, PR_1_ATTR_HASH, pctx))
+					goto clear_extattr;
+				entry->e_hash = hash;
+			}
 		} else {
 			problem_t problem;
 
@@ -2450,16 +2487,6 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
 				goto clear_extattr;
 		}
 
-		hash = ext2fs_ext_attr_hash_entry(entry, block_buf +
-							 entry->e_value_offs);
-
-		if (entry->e_hash != hash) {
-			pctx->num = entry->e_hash;
-			if (fix_problem(ctx, PR_1_ATTR_HASH, pctx))
-				goto clear_extattr;
-			entry->e_hash = hash;
-		}
-
 		entry = EXT2_EXT_ATTR_NEXT(entry);
 	}
 	if (region_allocate(region, (char *)entry - (char *)header, 4)) {
e2fsck/problem.c +0 -5
index dcdf9abe1abd..deffa4d66a89 100644
@@ -1150,11 +1150,6 @@ static struct e2fsck_problem problem_table[] = {
 	  N_("@i %i has @I @a value @i %N.\n"),
 	  PROMPT_CLEAR, PR_PREEN_OK },
 
-	/* Invalid backpointer from extended attribute inode to parent inode */
-	{ PR_1_ATTR_INVAL_EA_INODE,
-	  N_("@n backpointer from @a @i %N to parent @i %i.\n"),
-	  PROMPT_CLEAR, PR_PREEN_OK },
-
 	/* Inode has invalid extended attribute. EA inode missing
 	 * EA_INODE flag. */
 	{ PR_1_ATTR_NO_EA_INODE_FL,
e2fsck/problem.h +0 -3
index dad608c94fd0..60cc764fd2df 100644
@@ -678,9 +678,6 @@ struct problem_context {
 /* Inode has illegal EA value inode */
 #define PR_1_ATTR_VALUE_EA_INODE		0x010083
 
-/* Invalid backpointer from EA inode to parent inode */
-#define PR_1_ATTR_INVAL_EA_INODE		0x010084
-
 /* Parent inode has invalid EA entry. EA inode does not have
  * EXT4_EA_INODE_FL flag. Delete EA entry? */
 #define PR_1_ATTR_NO_EA_INODE_FL		0x010085
lib/ext2fs/ext2fs.h +3 -0
index f4131a640f9c..f72cbe17a8cd 100644
@@ -1198,6 +1198,9 @@ extern errcode_t ext2fs_expand_dir(ext2_filsys fs, ext2_ino_t dir);
 /* ext_attr.c */
 extern __u32 ext2fs_ext_attr_hash_entry(struct ext2_ext_attr_entry *entry,
 					void *data);
+extern errcode_t ext2fs_ext_attr_hash_entry2(ext2_filsys fs,
+					     struct ext2_ext_attr_entry *entry,
+					     void *data, __u32 *hash);
 extern errcode_t ext2fs_read_ext_attr(ext2_filsys fs, blk_t block, void *buf);
 extern errcode_t ext2fs_read_ext_attr2(ext2_filsys fs, blk64_t block,
 				       void *buf);
lib/ext2fs/ext_attr.c +63 -5

104 lines not on this page · raw message

Signature · 1 line
2.13.2.725.g09c95d1e9-goog
#5 lore raw

Re: [PATCH v2 03/12] e2fsck: ea_inode hash validation

Quoted 4 lines

e2fsck/pass1.c static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx, · line 2476

Quoted 2 lines
e2fsck/pass1.c +2 -0
+                             if (fix_problem(ctx, PR_1_ATTR_HASH, pctx))
+                                     goto clear_extattr;

Shouldn't this be "if (!fix_problem(...))" ?

Unfortunately here "fix" means clearing all entries so it is as intended. This is the way it was before, I haven't changed the logic.

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +1 -1
-#define PR_1_ATTR_SET_EA_INODE_FL            0x010086
+#define PR_1_ATTR_SET_EA_INODE_FL            0x010085

It would also be OK to keep the old values, and just skip 0x10084.

Done.

Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+                             err = ext2fs_read_inode(handle->fs, handle->ino,
+                                                     &parent);

Rather than reading the inode again here, it would make more sense to pass the parent inode from the caller.

Done.

#6 lore raw

PATCH 4/12 e2fsck: do not early terminate extra space check

In reply to Tahsin Erdogan

When check_inode_extra_space() detects a problem with the value of i_extra_isize, it adjusts it and then returns without further validation of contents in the inode body. Change this so that it will proceed to check inline extended attributes.

---
 e2fsck/pass1.c | 1 -
 1 file changed, 1 deletion(-)
e2fsck/pass1.c +0 -1
index 32152f3ec926..1532fd2067f2 100644
@@ -582,7 +582,6 @@ static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx)
 			inode->i_extra_isize = (inode->i_extra_isize + 3) & ~3;
 		e2fsck_write_inode_full(ctx, pctx->ino, pctx->inode,
 					EXT2_INODE_SIZE(sb), "pass1");
-		return;
 	}
 
 	/* check if there is no place for an EA header */
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#7 lore raw

Re: [PATCH 04/12] e2fsck: do not early terminate extra space check

Quoted 18 lines · Tahsin Erdogan

When check_inode_extra_space() detects a problem with the value of i_extra_isize, it adjusts it and then returns without further validation of contents in the inode body. Change this so that it will proceed to check inline extended attributes.

---
e2fsck/pass1.c | 1 -

1 file changed, 1 deletion(-)
e2fsck/pass1.c +0 -0
index 32152f3ec926..1532fd2067f2 100644

e2fsck/pass1.c static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx) · line 587

Quoted 7 lines
e2fsck/pass1.c +0 -1
@@ -582,7 +582,6 @@ static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx)
 			inode->i_extra_isize = (inode->i_extra_isize + 3) & ~3;
 		e2fsck_write_inode_full(ctx, pctx->ino, pctx->inode,
 					EXT2_INODE_SIZE(sb), "pass1");
-		return;
 	}
 
 	/* check if there is no place for an EA header */

The problem is that if i_extra_isize is changed, then the EA magic and data will no longer be aligned properly, so there isn't anything to check?

Cheers, Andreas

#8 lore raw

Re: [PATCH 04/12] e2fsck: do not early terminate extra space check

Quoted 7 lines
Quoted 7 lines
e2fsck/pass1.c +0 -1
@@ -582,7 +582,6 @@ static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx)
                      inode->i_extra_isize = (inode->i_extra_isize + 3) & ~3;
              e2fsck_write_inode_full(ctx, pctx->ino, pctx->inode,
                                      EXT2_INODE_SIZE(sb), "pass1");
-             return;
      }
 
Quoted 5 lines

e2fsck/pass1.c static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx) · line 587

Quoted 1 line
e2fsck/pass1.c +0 -0
      /* check if there is no place for an EA header */

The problem is that if i_extra_isize is changed, then the EA magic and data will no longer be aligned properly, so there isn't anything to check?

In the rest of the function, inline extended attributes and i_*time_extra fields are checked. If i_extra_size moves from its original location, magic field won't be found and inline extended attribute check will be skipped. But if it happens to be corrected to its original location, then we should check the ea contents. Also, we should check the time extra fields.

I don't really see a good reason to return early. Please let me know if I missing something.

#9 lore raw

PATCH 5/12 e2fsck: generalize ea_refcount

In reply to Tahsin Erdogan

Currently ea_refcount is only used to track ea block refcounts. By generalizing it, we could use it for ea quota tracking and also ea_inode refcounts.

---
 e2fsck/e2fsck.h      |  21 ++++---
 e2fsck/ea_refcount.c | 160 +++++++++++++++++++++++++++------------------------
 e2fsck/pass1.c       |   4 +-
 3 files changed, 99 insertions(+), 86 deletions(-)
e2fsck/e2fsck.h +13 -8
index c19cdfdd733e..f9285d01978c 100644
@@ -466,18 +466,23 @@ extern int e2fsck_get_num_dx_dirinfo(e2fsck_t ctx);
 extern struct dx_dir_info *e2fsck_dx_dir_info_iter(e2fsck_t ctx, int *control);
 
 /* ea_refcount.c */
-extern errcode_t ea_refcount_create(int size, ext2_refcount_t *ret);
+typedef __u64 ea_key_t;
+typedef __u64 ea_value_t;
+
+extern errcode_t ea_refcount_create(size_t size, ext2_refcount_t *ret);
 extern void ea_refcount_free(ext2_refcount_t refcount);
-extern errcode_t ea_refcount_fetch(ext2_refcount_t refcount, blk64_t blk, int *ret);
+extern errcode_t ea_refcount_fetch(ext2_refcount_t refcount, ea_key_t ea_key,
+				   ea_value_t *ret);
 extern errcode_t ea_refcount_increment(ext2_refcount_t refcount,
-				       blk64_t blk, int *ret);
+				       ea_key_t ea_key, ea_value_t *ret);
 extern errcode_t ea_refcount_decrement(ext2_refcount_t refcount,
-				       blk64_t blk, int *ret);
-extern errcode_t ea_refcount_store(ext2_refcount_t refcount,
-				   blk64_t blk, int count);
-extern blk_t ext2fs_get_refcount_size(ext2_refcount_t refcount);
+				       ea_key_t ea_key, ea_value_t *ret);
+extern errcode_t ea_refcount_store(ext2_refcount_t refcount, ea_key_t ea_key,
+				   ea_value_t count);
+extern size_t ext2fs_get_refcount_size(ext2_refcount_t refcount);
 extern void ea_refcount_intr_begin(ext2_refcount_t refcount);
-extern blk64_t ea_refcount_intr_next(ext2_refcount_t refcount, int *ret);
+extern ea_key_t ea_refcount_intr_next(ext2_refcount_t refcount,
+				      ea_value_t *ret);
 
 /* ehandler.c */
 extern const char *ehandler_operation(const char *op);
e2fsck/ea_refcount.c +84 -76

353 lines not on this page · raw message

e2fsck/pass1.c +2 -2
index 1532fd2067f2..9ee2c5e89a61 100644
@@ -2269,7 +2269,7 @@ static void adjust_extattr_refcount(e2fsck_t ctx, ext2_refcount_t refcount,
 	ext2_filsys			fs = ctx->fs;
 	blk64_t				blk;
 	__u32				should_be;
-	int				count;
+	ea_value_t			count;
 
 	clear_problem_context(&pctx);
 
@@ -2286,7 +2286,7 @@ static void adjust_extattr_refcount(e2fsck_t ctx, ext2_refcount_t refcount,
 		}
 		header = (struct ext2_ext_attr_header *) block_buf;
 		pctx.blkcount = header->h_refcount;
-		should_be = header->h_refcount + adjust_sign * count;
+		should_be = header->h_refcount + adjust_sign * (int)count;
 		pctx.num = should_be;
 		if (fix_problem(ctx, PR_1_EXTATTR_REFCOUNT, &pctx)) {
 			header->h_refcount = should_be;
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#10 lore raw

PATCH 6/12 e2fsck: update i_blocks accounting for ea_inode feature

In reply to Tahsin Erdogan

With ea_inode feature, i_blocks include the disk space used by referenced xattr inodes. Make e2fsck aware of that.

---
 e2fsck/e2fsck.c |   4 ++
 e2fsck/e2fsck.h |   5 +++
 e2fsck/pass1.c  | 124 +++++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 100 insertions(+), 33 deletions(-)
e2fsck/e2fsck.c +4 -0
index 0f9da46a5a12..e0f449ee2047 100644
@@ -98,6 +98,10 @@ errcode_t e2fsck_reset_context(e2fsck_t ctx)
 		ea_refcount_free(ctx->refcount_extra);
 		ctx->refcount_extra = 0;
 	}
+	if (ctx->ea_block_quota) {
+		ea_refcount_free(ctx->ea_block_quota);
+		ctx->ea_block_quota = 0;
+	}
 	if (ctx->block_dup_map) {
 		ext2fs_free_block_bitmap(ctx->block_dup_map);
 		ctx->block_dup_map = 0;
e2fsck/e2fsck.h +5 -0
index f9285d01978c..79eeda9fbafb 100644
@@ -269,6 +269,11 @@ struct e2fsck_struct {
 	ext2_refcount_t	refcount;
 	ext2_refcount_t refcount_extra;
 
+	/*
+	 * Quota blocks to be charged for each ea block.
+	 */
+	ext2_refcount_t ea_block_quota;
+
 	/*
 	 * Array of flags indicating whether an inode bitmap, block
 	 * bitmap, or inode table is invalid
e2fsck/pass1.c +91 -33

313 lines not on this page · raw message

Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#11 lore raw

Re: [PATCH 06/12] e2fsck: update i_blocks accounting for ea_inode feature

Quoted 3 lines · Tahsin Erdogan

With ea_inode feature, i_blocks include the disk space used by referenced xattr inodes. Make e2fsck aware of that.

This makes it all the more important that the "fast symlink" checking be divorced from i_blocks, and instead depend on i_size. Otherwise, we will have yet another bunch of problems as large xattrs on a fast symlink will incorrectly result in problems with the symlink.

This was most recently discussed in the thread:
[PATCH] libext2fs: correctly subtract xattr blocks on bigalloc filesystems
https://www.spinics.net/lists/linux-ext4/msg56601.html

All indications are that using i_size to distinguish fast symlinks is safe, while using i_blocks has repeatedly caused breakage as new blocks are allocated to the inode.

Cheers, Andreas

Quoted 382 lines

---

e2fsck/e2fsck.c |   4 ++
e2fsck/e2fsck.h |   5 +++
e2fsck/pass1.c  | 124 +++++++++++++++++++++++++++++++++++++++++---------------
3 files changed, 100 insertions(+), 33 deletions(-)
e2fsck/e2fsck.c +4 -0
index 0f9da46a5a12..e0f449ee2047 100644
@@ -98,6 +98,10 @@ errcode_t e2fsck_reset_context(e2fsck_t ctx)
 		ea_refcount_free(ctx->refcount_extra);
 		ctx->refcount_extra = 0;
 	}
+	if (ctx->ea_block_quota) {
+		ea_refcount_free(ctx->ea_block_quota);
+		ctx->ea_block_quota = 0;
+	}
 	if (ctx->block_dup_map) {
 		ext2fs_free_block_bitmap(ctx->block_dup_map);
 		ctx->block_dup_map = 0;
e2fsck/e2fsck.h +5 -0
index f9285d01978c..79eeda9fbafb 100644
@@ -269,6 +269,11 @@ struct e2fsck_struct {
 	ext2_refcount_t	refcount;
 	ext2_refcount_t refcount_extra;
 
+	/*
+	 * Quota blocks to be charged for each ea block.
+	 */
+	ext2_refcount_t ea_block_quota;
+
 	/*
 	 * Array of flags indicating whether an inode bitmap, block
 	 * bitmap, or inode table is invalid
e2fsck/pass1.c +91 -34
index 9ee2c5e89a61..cc88a7d05cef 100644
@@ -66,7 +66,7 @@ static int process_bad_block(ext2_filsys fs, blk64_t *block_nr,
 			     e2_blkcnt_t blockcnt, blk64_t ref_blk,
 			     int ref_offset, void *priv_data);
 static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
-			 char *block_buf);
+			 char *block_buf, blk64_t ea_ibody_quota_blocks);
 static void mark_table_blocks(e2fsck_t ctx);
 static void alloc_bb_map(e2fsck_t ctx);
 static void alloc_imagic_map(e2fsck_t ctx);
@@ -103,6 +103,7 @@ struct process_block_struct {
 
 struct process_inode_block {
 	ext2_ino_t ino;
+	blk64_t ea_ibody_quota_blocks;
 	struct ext2_inode_large inode;
 };
 
@@ -351,13 +352,25 @@ static void mark_inode_ea_map(e2fsck_t ctx, struct problem_context *pctx,
 	ext2fs_mark_inode_bitmap2(ctx->inode_ea_map, ino);
 }
 
+/*
+ * For a given size, calculate how many blocks would be charged towards quota.
+ */
+static blk64_t size_to_quota_blocks(ext2_filsys fs, size_t size)
+{
+	blk64_t clusters;
+
+	clusters = DIV_ROUND_UP(size, fs->blocksize << fs->cluster_ratio_bits);
+	return EXT2FS_C2B(fs, clusters);
+}
+
 /*
 * Check validity of EA inode. Return 0 if EA inode is valid, otherwise return
 * the problem code.
 */
 static problem_t check_large_ea_inode(e2fsck_t ctx,
 				      struct ext2_ext_attr_entry *entry,
-				      struct problem_context *pctx)
+				      struct problem_context *pctx,
+				      blk64_t *quota_blocks)
 {
 	struct ext2_inode inode;
 	__u32 hash;
@@ -380,11 +393,15 @@ static problem_t check_large_ea_inode(e2fsck_t ctx,
 		fatal_error(ctx, 0);
 	}
 
-	if (hash != entry->e_hash) {
+	if (hash == entry->e_hash) {
+		*quota_blocks = size_to_quota_blocks(ctx->fs,
+						     entry->e_value_size);
+	} else {
 		/* This might be an old Lustre-style ea_inode reference. */
-		if (inode.i_mtime != pctx->ino ||
-		    inode.i_generation != pctx->inode->i_generation) {
-
+		if (inode.i_mtime == pctx->ino &&
+		    inode.i_generation == pctx->inode->i_generation) {
+			*quota_blocks = 0;
+		} else {
 			/* If target inode is also missing EA_INODE flag,
 			 * this is likely to be a bad reference.
 			 */
@@ -411,7 +428,8 @@ static problem_t check_large_ea_inode(e2fsck_t ctx,
 	return 0;
 }
 
-static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx)
+static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx,
+			      blk64_t *ea_ibody_quota_blocks)
 {
 	struct ext2_super_block *sb = ctx->fs->super;
 	struct ext2_inode_large *inode;
@@ -420,6 +438,9 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx)
 	unsigned int storage_size, remain;
 	problem_t problem = 0;
 	region_t region = 0;
+	blk64_t quota_blocks = 0;
+
+	*ea_ibody_quota_blocks = 0;
 
 	inode = (struct ext2_inode_large *) pctx->inode;
 	storage_size = EXT2_INODE_SIZE(ctx->fs->super) - EXT2_GOOD_OLD_INODE_SIZE -
@@ -496,11 +517,15 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx)
 				goto fix;
 			}
 		} else {
-			problem = check_large_ea_inode(ctx, entry, pctx);
+			blk64_t entry_quota_blocks;
+
+			problem = check_large_ea_inode(ctx, entry, pctx,
+						       &entry_quota_blocks);
 			if (problem != 0)
 				goto fix;
 
 			mark_inode_ea_map(ctx, pctx, entry->e_value_inum);
+			quota_blocks += entry_quota_blocks;
 		}
 
 		/* If EA value is stored in external inode then it does not
@@ -523,8 +548,10 @@ fix:
 	 * it seems like a corruption. it's very unlikely we could repair
 	 * EA(s) in automatic fashion -bzzz
 	 */
-	if (problem == 0 || !fix_problem(ctx, problem, pctx))
+	if (problem == 0 || !fix_problem(ctx, problem, pctx)) {
+		*ea_ibody_quota_blocks = quota_blocks;
 		return;
+	}
 
 	/* simply remove all possible EA(s) */
 	*((__u32 *)header) = 0UL;
@@ -547,13 +574,16 @@ static int check_inode_extra_negative_epoch(__u32 xtime, __u32 extra) {
 */
 #define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 2 * (1LL << 32)
 
-static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx)
+static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx,
+				    blk64_t *ea_ibody_quota_blocks)
 {
 	struct ext2_super_block *sb = ctx->fs->super;
 	struct ext2_inode_large *inode;
 	__u32 *eamagic;
 	int min, max;
 
+	*ea_ibody_quota_blocks = 0;
+
 	inode = (struct ext2_inode_large *) pctx->inode;
 	if (EXT2_INODE_SIZE(sb) == EXT2_GOOD_OLD_INODE_SIZE) {
 		/* this isn't large inode. so, nothing to check */
@@ -592,7 +622,7 @@ static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx)
 			inode->i_extra_isize);
 	if (*eamagic == EXT2_EXT_ATTR_MAGIC) {
 		/* it seems inode has an extended attribute(s) in body */
-		check_ea_in_inode(ctx, pctx);
+		check_ea_in_inode(ctx, pctx, ea_ibody_quota_blocks);
 	}
 
 	/*
@@ -1150,6 +1180,7 @@ void e2fsck_pass1(e2fsck_t ctx)
 	int		failed_csum = 0;
 	ext2_ino_t	ino_threshold = 0;
 	dgrp_t		ra_group = 0;
+	blk64_t		ea_ibody_quota_blocks;
 
 	init_resource_track(&rtrack, ctx->fs->io);
 	clear_problem_context(&pctx);
@@ -1695,7 +1726,7 @@ void e2fsck_pass1(e2fsck_t ctx)
 							   "pass1");
 					failed_csum = 0;
 				}
-				check_blocks(ctx, &pctx, block_buf);
+				check_blocks(ctx, &pctx, block_buf, 0);
 				FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
 				continue;
 			}
@@ -1722,7 +1753,7 @@ void e2fsck_pass1(e2fsck_t ctx)
 							"pass1");
 					failed_csum = 0;
 				}
-				check_blocks(ctx, &pctx, block_buf);
+				check_blocks(ctx, &pctx, block_buf, 0);
 				FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
 				continue;
 			}
@@ -1760,7 +1791,7 @@ void e2fsck_pass1(e2fsck_t ctx)
 					failed_csum = 0;
 				}
 			}
-			check_blocks(ctx, &pctx, block_buf);
+			check_blocks(ctx, &pctx, block_buf, 0);
 			FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
 			continue;
 		}
@@ -1825,7 +1856,7 @@ void e2fsck_pass1(e2fsck_t ctx)
 			}
 		}
 
-		check_inode_extra_space(ctx, &pctx);
+		check_inode_extra_space(ctx, &pctx, &ea_ibody_quota_blocks);
 		check_is_really_dir(ctx, &pctx, block_buf);
 
 		/*
@@ -1872,7 +1903,8 @@ void e2fsck_pass1(e2fsck_t ctx)
 				continue;
 			} else if (ext2fs_inode_data_blocks(fs, inode) == 0) {
 				ctx->fs_fast_symlinks_count++;
-				check_blocks(ctx, &pctx, block_buf);
+				check_blocks(ctx, &pctx, block_buf,
+					     ea_ibody_quota_blocks);
 				FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
 				continue;
 			}
@@ -1906,17 +1938,19 @@ void e2fsck_pass1(e2fsck_t ctx)
 		     inode->i_block[EXT2_DIND_BLOCK] ||
 		     inode->i_block[EXT2_TIND_BLOCK] ||
 		     ext2fs_file_acl_block(fs, inode))) {
-			struct ext2_inode_large *ip;
+			struct process_inode_block *itp;
 
-			inodes_to_process[process_inode_count].ino = ino;
-			ip = &inodes_to_process[process_inode_count].inode;
+			itp = &inodes_to_process[process_inode_count];
+			itp->ino = ino;
+			itp->ea_ibody_quota_blocks = ea_ibody_quota_blocks;
 			if (inode_size < sizeof(struct ext2_inode_large))
-				memcpy(ip, inode, inode_size);
+				memcpy(&itp->inode, inode, inode_size);
 			else
-				memcpy(ip, inode, sizeof(*ip));
+				memcpy(&itp->inode, inode, sizeof(itp->inode));
 			process_inode_count++;
 		} else
-			check_blocks(ctx, &pctx, block_buf);
+			check_blocks(ctx, &pctx, block_buf,
+				     ea_ibody_quota_blocks);
 
 		FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
 
@@ -2086,7 +2120,8 @@ static void process_inodes(e2fsck_t ctx, char *block_buf)
 		sprintf(buf, _("reading indirect blocks of inode %u"),
 			pctx.ino);
 		ehandler_operation(buf);
-		check_blocks(ctx, &pctx, block_buf);
+		check_blocks(ctx, &pctx, block_buf,
+			     inodes_to_process[i].ea_ibody_quota_blocks);
 		if (ctx->flags & E2F_FLAG_SIGNAL_MASK)
 			break;
 	}
@@ -2306,7 +2341,7 @@ static void adjust_extattr_refcount(e2fsck_t ctx, ext2_refcount_t refcount,
 * Handle processing the extended attribute blocks
 */
 static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
-			   char *block_buf)
+			   char *block_buf, blk64_t *ea_block_quota_blocks)
 {
 	ext2_filsys fs = ctx->fs;
 	ext2_ino_t	ino = pctx->ino;
@@ -2315,7 +2350,7 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
 	char *		end;
 	struct ext2_ext_attr_header *header;
 	struct ext2_ext_attr_entry *entry;
-	int		count;
+	blk64_t		quota_blocks = EXT2FS_C2B(fs, 1);
 	region_t	region = 0;
 	int		failed_csum = 0;
 
@@ -2369,6 +2404,12 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
 
 	/* Have we seen this EA block before? */
 	if (ext2fs_fast_test_block_bitmap2(ctx->block_ea_map, blk)) {
+		if (ctx->ea_block_quota)
+			ea_refcount_fetch(ctx->ea_block_quota, blk,
+					  ea_block_quota_blocks);
+		else
+			*ea_block_quota_blocks = 0;
+
 		if (ea_refcount_decrement(ctx->refcount, blk, 0) == 0)
 			return 1;
 		/* Ooops, this EA was referenced more than it stated */
@@ -2477,13 +2518,17 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
 			}
 		} else {
 			problem_t problem;
+			blk64_t entry_quota_blocks;
 
-			problem = check_large_ea_inode(ctx, entry, pctx);
+			problem = check_large_ea_inode(ctx, entry, pctx,
+						       &entry_quota_blocks);
 			if (problem == 0)
 				mark_inode_ea_map(ctx, pctx,
 						  entry->e_value_inum);
 			else if (fix_problem(ctx, problem, pctx))
 				goto clear_extattr;
+
+			quota_blocks += entry_quota_blocks;
 		}
 
 		entry = EXT2_EXT_ATTR_NEXT(entry);
@@ -2506,9 +2551,21 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
 			return 0;
 	}
 
-	count = header->h_refcount - 1;
-	if (count)
-		ea_refcount_store(ctx->refcount, blk, count);
+	*ea_block_quota_blocks = quota_blocks;
+	if (quota_blocks) {
+		if (!ctx->ea_block_quota) {
+			pctx->errcode = ea_refcount_create(0,
+							&ctx->ea_block_quota);
+			if (pctx->errcode) {
+				pctx->num = 3;
+				fix_problem(ctx, PR_1_ALLOCATE_REFCOUNT, pctx);
+				ctx->flags |= E2F_FLAG_ABORT;
+				return 0;
+			}
+		}
+		ea_refcount_store(ctx->ea_block_quota, blk, quota_blocks);
+	}
+	ea_refcount_store(ctx->refcount, blk, header->h_refcount - 1);
 	mark_block_used(ctx, blk);
 	ext2fs_fast_mark_block_bitmap2(ctx->block_ea_map, blk);
 	return 1;
@@ -3162,7 +3219,7 @@ err:
 * blocks used by that inode.
 */
 static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
-			 char *block_buf)
+			 char *block_buf, blk64_t ea_ibody_quota_blocks)
 {
 	ext2_filsys fs = ctx->fs;
 	struct process_block_struct pb;
@@ -3173,9 +3230,10 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 	int		extent_fs;
 	int		inlinedata_fs;
 	__u64		size;
+	blk64_t		ea_block_quota_blocks = 0;
 
 	pb.ino = ino;
-	pb.num_blocks = 0;
+	pb.num_blocks = EXT2FS_B2C(ctx->fs, ea_ibody_quota_blocks);
 	pb.last_block = ~0;
 	pb.last_init_lblock = -1;
 	pb.last_db_block = -1;
@@ -3198,10 +3256,10 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 	extent_fs = ext2fs_has_feature_extents(ctx->fs->super);
 	inlinedata_fs = ext2fs_has_feature_inline_data(ctx->fs->super);
 
-	if (check_ext_attr(ctx, pctx, block_buf)) {
+	if (check_ext_attr(ctx, pctx, block_buf, &ea_block_quota_blocks)) {
 		if (ctx->flags & E2F_FLAG_SIGNAL_MASK)
 			goto out;
-		pb.num_blocks++;
+		pb.num_blocks += EXT2FS_B2C(ctx->fs, ea_block_quota_blocks);
 	}
 
 	if (inlinedata_fs && (inode->i_flags & EXT4_INLINE_DATA_FL))
--
Quoted 2 lines

2.13.1.611.g7e3b11ae1-goog

Cheers, Andreas

#12 lore raw

Re: [PATCH 06/12] e2fsck: update i_blocks accounting for ea_inode feature

Quoted 10 lines · Andreas Dilger
Quoted 3 lines · Tahsin Erdogan

With ea_inode feature, i_blocks include the disk space used by referenced xattr inodes. Make e2fsck aware of that.

This makes it all the more important that the "fast symlink" checking be divorced from i_blocks, and instead depend on i_size. Otherwise, we will have yet another bunch of problems as large xattrs on a fast symlink will incorrectly result in problems with the symlink.

Totally agree. The changes to i_blocks accounting for ea_inode feature likely broke ext4_inode_is_fast_symlink() in kernel. I will try to fix that first.

#13 lore raw

Re: [PATCH 06/12] e2fsck: update i_blocks accounting for ea_inode feature

Quoted 8 lines
Quoted 5 lines

This makes it all the more important that the "fast symlink" checking be divorced from i_blocks, and instead depend on i_size. Otherwise, we will have yet another bunch of problems as large xattrs on a fast symlink will incorrectly result in problems with the symlink.

Totally agree. The changes to i_blocks accounting for ea_inode feature likely broke ext4_inode_is_fast_symlink() in kernel. I will try to fix that first.

FYI, I sent out a patch to fix e2fsprogs part too: ("[PATCH]
e2fsprogs: add ext2fs_is_fast_symlink() function")

#14 lore raw

PATCH 7/12 e2fsck: track ea_inode references

In reply to Tahsin Erdogan

An extended attribute inode has a ref count to track how many entries point to it. Update e2fsck to verify that the stored ref count is correct.

---
 e2fsck/e2fsck.c  |   4 +++
 e2fsck/e2fsck.h  |   6 +++-
 e2fsck/message.c |   7 ++++
 e2fsck/pass1.c   |  66 ++++++++++++++++++++++--------------
 e2fsck/pass4.c   | 101 +++++++++++++++++++++++++++++++++++++++++++++----------
 e2fsck/problem.c |   4 +++
 e2fsck/problem.h |   5 ++-
 7 files changed, 148 insertions(+), 45 deletions(-)
e2fsck/e2fsck.c +4 -0
index e0f449ee2047..63a986b92af9 100644
@@ -102,6 +102,10 @@ errcode_t e2fsck_reset_context(e2fsck_t ctx)
 		ea_refcount_free(ctx->ea_block_quota);
 		ctx->ea_block_quota = 0;
 	}
+	if (ctx->ea_inode_refs) {
+		ea_refcount_free(ctx->ea_inode_refs);
+		ctx->ea_inode_refs = 0;
+	}
 	if (ctx->block_dup_map) {
 		ext2fs_free_block_bitmap(ctx->block_dup_map);
 		ctx->block_dup_map = 0;
e2fsck/e2fsck.h +5 -1
index 79eeda9fbafb..b25c5eb9179b 100644
@@ -254,7 +254,6 @@ struct e2fsck_struct {
 	ext2fs_inode_bitmap inode_bb_map; /* Inodes which are in bad blocks */
 	ext2fs_inode_bitmap inode_imagic_map; /* AFS inodes */
 	ext2fs_inode_bitmap inode_reg_map; /* Inodes which are regular files*/
-	ext2fs_inode_bitmap inode_ea_map; /* EA inodes which are non-orphan */
 
 	ext2fs_block_bitmap block_found_map; /* Blocks which are in use */
 	ext2fs_block_bitmap block_dup_map; /* Blks referenced more than once */
@@ -274,6 +273,11 @@ struct e2fsck_struct {
 	 */
 	ext2_refcount_t ea_block_quota;
 
+	/*
+	 * ea_inode references from attr entries.
+	 */
+	ext2_refcount_t ea_inode_refs;
+
 	/*
 	 * Array of flags indicating whether an inode bitmap, block
 	 * bitmap, or inode table is invalid
e2fsck/message.c +7 -0
index 34201a37fd4b..525f4a1e7628 100644
@@ -466,6 +466,13 @@ static _INLINE_ void expand_percent_expression(FILE *f, ext2_filsys fs,
 		fprintf(f, "%*u", width, ctx->num);
 #else
 		fprintf(f, "%*llu", width, (long long)ctx->num);
+#endif
+		break;
+	case 'n':
+#ifdef EXT2_NO_64_TYPE
+		fprintf(f, "%*u", width, ctx->num2);
+#else
+		fprintf(f, "%*llu", width, (long long)ctx->num2);
 #endif
 		break;
 	case 'p':
e2fsck/pass1.c +40 -26

139 lines not on this page · raw message

e2fsck/pass4.c +84 -17

133 lines not on this page · raw message

e2fsck/problem.c +4 -0
index deffa4d66a89..97069335156c 100644
@@ -1869,6 +1869,10 @@ static struct e2fsck_problem problem_table[] = {
 	  "They @s the same!\n"),
 	  PROMPT_NONE, 0 },
 
+	{ PR_4_EA_INODE_REF_COUNT,
+	  N_("@a @i %i ref count is %N, @s %n. "),
+	  PROMPT_FIX, PR_PREEN_OK },
+
 	/* Pass 5 errors */
 
 	/* Pass 5: Checking group summary information */
e2fsck/problem.h +4 -1
index 8e07c10895c1..54eb1cf9a519 100644
@@ -20,7 +20,7 @@ struct problem_context {
 	e2_blkcnt_t	blkcount;
 	dgrp_t		group;
 	__u32		csum1, csum2;
-	__u64	num;
+	__u64		num, num2;
 	const char *str;
 };
 
@@ -1131,6 +1131,9 @@ struct problem_context {
 /* Inconsistent inode count information cached */
 #define PR_4_INCONSISTENT_COUNT		0x040004
 
+/* Extended attribute inode ref count wrong */
+#define PR_4_EA_INODE_REF_COUNT		0x040005
+
 /*
  * Pass 5 errors
  */
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#15 lore raw

Re: [PATCH 07/12] e2fsck: track ea_inode references

Quoted 13 lines · Tahsin Erdogan

An extended attribute inode has a ref count to track how many entries point to it. Update e2fsck to verify that the stored ref count is correct.

---

e2fsck/message.c +0 -0
index 34201a37fd4b..525f4a1e7628 100644
@@ -466,6 +466,13 @@ static _INLINE_ void expand_percent_expression(FILE *f, ext2_filsys fs,
 		fprintf(f, "%*u", width, ctx->num);

e2fsck/message.c static _INLINE_ void expand_percent_expression(FILE *f, ext2_filsys fs, · line 476

Quoted 10 lines
e2fsck/message.c +7 -0
 #else
 		fprintf(f, "%*llu", width, (long long)ctx->num);
+#endif
+		break;
+	case 'n':
+#ifdef EXT2_NO_64_TYPE
+		fprintf(f, "%*u", width, ctx->num2);
+#else
+		fprintf(f, "%*llu", width, (long long)ctx->num2);
 #endif

Rather than a series of "#ifdef EXT2_NO_64_TYPE" checks, it would be cleaner to have a single #define to set the printf type, like:

#ifdef EXT2_NO_64_TYPE
#define EXT2_64U "%*u"
#define EXT2_64D "%*d"
#define EXT2_64X "%*x"
#else
#define EXT2_64U "%*llu"
#define EXT2_64D "%*lld"
#define EXT2_64X "%*llx"
#endif

and then use "EXT2_64D" or "EXT2_64U" in all of these fprintf() statements.

Cheers, Andreas

#16 lore raw

Re: [PATCH 07/12] e2fsck: track ea_inode references

Quoted 9 lines
Quoted 5 lines
e2fsck/message.c +4 -0
+#ifdef EXT2_NO_64_TYPE
+             fprintf(f, "%*u", width, ctx->num2);
+#else
+             fprintf(f, "%*llu", width, (long long)ctx->num2);

#endif

Rather than a series of "#ifdef EXT2_NO_64_TYPE" checks, it would be cleaner to have a single #define to set the printf type, like:

Quoted 9 lines

#ifdef EXT2_NO_64_TYPE
#define EXT2_64U "%*u"
#define EXT2_64D "%*d"
#define EXT2_64X "%*x"
#else
#define EXT2_64U "%*llu"
#define EXT2_64D "%*lld"
#define EXT2_64X "%*llx"
#endif

I am trying to figure out the purpose of #ifdef EXT2_NO_64_TYPE checks now. Who defines it and what problem does it solve?

thanks

#17 lore raw

Re: [PATCH 07/12] e2fsck: track ea_inode references

Quoted 22 lines · Tahsin Erdogan
Quoted 10 lines
Quoted 6 lines
e2fsck/message.c +4 -0
+#ifdef EXT2_NO_64_TYPE
+             fprintf(f, "%*u", width, ctx->num2);
+#else
+             fprintf(f, "%*llu", width, (long long)ctx->num2);

#endif

Rather than a series of "#ifdef EXT2_NO_64_TYPE" checks, it would be cleaner to have a single #define to set the printf type, like:

Quoted 9 lines

#ifdef EXT2_NO_64_TYPE
#define EXT2_64U "%*u"
#define EXT2_64D "%*d"
#define EXT2_64X "%*x"
#else
#define EXT2_64U "%*llu"
#define EXT2_64D "%*lld"
#define EXT2_64X "%*llx"
#endif

Quoted 2 lines

I am trying to figure out the purpose of #ifdef EXT2_NO_64_TYPE checks now. Who defines it and what problem does it solve?

Sorry, I can't answer that. Git history shows that it has been around since 2000 but is not set by the configure script. I just thought it made sense to clean up the code, but it might be even better to get rid of it completely.

Cheers, Andreas

#18 lore raw

Re: [PATCH 07/12] e2fsck: track ea_inode references

Quoted 3 lines

Sorry, I can't answer that. Git history shows that it has been around since 2000 but is not set by the configure script. I just thought it made sense to clean up the code, but it might be even better to get rid of it completely.

Sounds good. Sent it out as a separate patch ("[PATCH] e2fsck: remove #ifdef EXT2_NO_64_TYPE blocks").

#19 lore raw

PATCH 8/12 e2fsck: add test for ea_inode feature

In reply to Tahsin Erdogan

f_ea_inode test covers the following scenarios:

- a file that contains old Lustre-style valid ea_inode references in inode body and xattr block

- a file that contains new style valid ea_inode references in inode body and xattr block

- a file with an extended attribute that references an invalid inode number (e_value_inum > s_inodes_count)

- an ea entry with bad e_hash and points to an inode that does not have EA_INODE flag set

- an ea entry with bad e_hash but points to a valid ea_inode

- an ea entry with valid e_hash that points to an inode that is missing EA_INODE flag

---
 tests/f_ea_inode/expect.1 |  31 +++++++++++++++++++++++++++++++
 tests/f_ea_inode/expect.2 |   7 +++++++
 tests/f_ea_inode/image.gz | Bin 0 -> 1389 bytes
 3 files changed, 38 insertions(+)
 create mode 100644 tests/f_ea_inode/expect.1
 create mode 100644 tests/f_ea_inode/expect.2
 create mode 100644 tests/f_ea_inode/image.gz
tests/f_ea_inode/expect.1 new file +31 -0
new file mode 100644
index 000000000000..aaa0bead9c41
@@ -0,0 +1,31 @@
+Pass 1: Checking inodes, blocks, and sizes
+Inode 17 has illegal extended attribute value inode 4008636142.
+Clear? yes
+
+Inode 17, i_blocks is 8, should be 0.  Fix? yes
+
+Inode 18 has illegal extended attribute value inode 19.
+Clear? yes
+
+Inode 18, i_blocks is 8, should be 0.  Fix? yes
+
+Extended attribute in inode 20 has a hash (1145324612) which is invalid
+Clear? yes
+
+Inode 20, i_blocks is 8, should be 0.  Fix? yes
+
+EA inode 19 for parent inode 21 missing EA_INODE flag.
+ Fix? yes
+
+Pass 2: Checking directory structure
+Pass 3: Checking directory connectivity
+Pass 4: Checking reference counts
+Extended attribute inode 16 ref count is 51, should be 2. Fix? yes
+
+Extended attribute inode 19 ref count is 2, should be 1. Fix? yes
+
+Pass 5: Checking group summary information
+
+test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
+test_filesys: 21/32 files (0.0% non-contiguous), 18/64 blocks
+Exit status is 1
tests/f_ea_inode/expect.2 new file +7 -0
new file mode 100644
index 000000000000..f9276fba341d
@@ -0,0 +1,7 @@
+Pass 1: Checking inodes, blocks, and sizes
+Pass 2: Checking directory structure
+Pass 3: Checking directory connectivity
+Pass 4: Checking reference counts
+Pass 5: Checking group summary information
+test_filesys: 21/32 files (0.0% non-contiguous), 18/64 blocks
+Exit status is 0
tests/f_ea_inode/image.gz new file binary +0 -0
new file mode 100644
index 0000000000000000000000000000000000000000..68a397580d2ecca9fa30e33528a286419c757c7e
GIT binary patch
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#20 lore raw

PATCH 9/12 tune2fs: update ea_inode hashes when fs uuid changes

In reply to Tahsin Erdogan

Extended attribute inodes maintain a crc32c hash that is used for deduplication. The crc seed derives from uuid so ea_inode hashes must be updated when uuid changes.

The ea_inode hash is also incorporated into the xattr entry e_hash so the entries that reference the inode also must be updated.

---
 lib/ext2fs/csum.c     |   3 +-
 lib/ext2fs/ext2fs.h   |   4 +
 lib/ext2fs/ext_attr.c |  44 +++++++-
 misc/tune2fs.c        | 299 ++++++++++++++++++++++++++++++++++----------------
 4 files changed, 249 insertions(+), 101 deletions(-)
lib/ext2fs/csum.c +2 -1
index e67850fa47af..214a099762ee 100644
@@ -34,7 +34,8 @@ void ext2fs_init_csum_seed(ext2_filsys fs)
 {
 	if (ext2fs_has_feature_csum_seed(fs->super))
 		fs->csum_seed = fs->super->s_checksum_seed;
-	else if (ext2fs_has_feature_metadata_csum(fs->super))
+	else if (ext2fs_has_feature_metadata_csum(fs->super) ||
+		 ext2fs_has_feature_ea_inode(fs->super))
 		fs->csum_seed = ext2fs_crc32c_le(~0, fs->super->s_uuid,
 						 sizeof(fs->super->s_uuid));
 }
lib/ext2fs/ext2fs.h +4 -0
index f72cbe17a8cd..90df2182bd7b 100644
@@ -1247,6 +1247,10 @@ errcode_t ext2fs_xattr_inode_max_size(ext2_filsys fs, ext2_ino_t ino,
 #define XATTR_HANDLE_FLAG_RAW	0x0001
 errcode_t ext2fs_xattrs_flags(struct ext2_xattr_handle *handle,
 			      unsigned int *new_flags, unsigned int *old_flags);
+extern void ext2fs_ext_attr_block_rehash(struct ext2_ext_attr_header *header,
+					 struct ext2_ext_attr_entry *end);
+extern __u32 ext2fs_get_ea_inode_hash(struct ext2_inode *inode);
+extern void ext2fs_set_ea_inode_hash(struct ext2_inode *inode, __u32 hash);
 
 /* extent.c */
 extern errcode_t ext2fs_extent_header_verify(void *ptr, int size);
lib/ext2fs/ext_attr.c +40 -4

62 lines not on this page · raw message

misc/tune2fs.c +203 -96

337 lines not on this page · raw message

Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#21 lore raw

PATCH 10/12 fuse2fs: refuse to mount fs with ea_inode feature

In reply to Tahsin Erdogan

ext2fs_xattr_set() currently does not support creating xattr inodes, so allowing fuse2fs to mount a filesystem with ea_inode feature could lead to corruption. Refuse to mount if the ea_inode feature is set.

---
 misc/fuse2fs.c | 6 ++++++
 1 file changed, 6 insertions(+)
misc/fuse2fs.c +6 -0
index b5897685c466..956348f8f876 100644
@@ -3786,6 +3786,12 @@ int main(int argc, char *argv[])
 	global_fs->priv_data = &fctx;
 
 	ret = 3;
+	if (ext2fs_has_feature_ea_inode(global_fs->super)) {
+		printf(_("%s: fuse2fs does not support ea_inode feature.\n"),
+		       fctx.device);
+		goto out;
+	}
+
 	if (ext2fs_has_feature_journal_needs_recovery(global_fs->super)) {
 		if (!fctx.ro) {
 			printf(_("%s: recovering journal\n"), fctx.device);
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#22 lore raw

PATCH 11/12 mke2fs: ea_inode is not supported for hurd

In reply to Tahsin Erdogan

Extended attribute inodes store their refcount in inode.l_i_version field which is not available for Hurd. Fail mke2fs for this combination.

---
 misc/mke2fs.c | 6 ++++++
 1 file changed, 6 insertions(+)
misc/mke2fs.c +6 -0
index 1ef46f44f35d..85d88edc2375 100644
@@ -1992,6 +1992,7 @@ profile_error:
 		ext2fs_clear_feature_filetype(&fs_param);
 		ext2fs_clear_feature_huge_file(&fs_param);
 		ext2fs_clear_feature_metadata_csum(&fs_param);
+		ext2fs_clear_feature_ea_inode(&fs_param);
 	}
 	edit_feature(fs_features ? fs_features : tmp,
 		     &fs_param.s_feature_compat);
@@ -2017,6 +2018,11 @@ profile_error:
 						"metadata_csum feature.\n"));
 			exit(1);
 		}
+		if (ext2fs_has_feature_ea_inode(&fs_param)) {
+			fprintf(stderr, "%s", _("The HURD does not support the "
+						"ea_inode feature.\n"));
+			exit(1);
+		}
 	}
 
 	/* Get the hardware sector sizes, if available */
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
#23 lore raw

PATCH 12/12 resize2fs: moving xattr inodes is not supported

In reply to Tahsin Erdogan

In some cases, resize2fs needs to move inodes because their inode number is greater than the maximum allowed. Moving extended attribute inodes would require updating all the references to them. This is currently not supported.

---
 lib/ext2fs/ext2_err.et.in | 3 +++
 resize/resize2fs.c        | 9 +++++++++
 2 files changed, 12 insertions(+)
lib/ext2fs/ext2_err.et.in +3 -0
index ac96964d93d0..c5a9ffcc420c 100644
@@ -542,4 +542,7 @@ ec	EXT2_ET_CORRUPT_JOURNAL_SB,
 ec	EXT2_ET_INODE_CORRUPTED,
 	"Inode is corrupted"
 
+ec	EXT2_ET_CANNOT_MOVE_EA_INODE,
+	"Cannot move extended attribute inode"
+
 	end
resize/resize2fs.c +9 -0
index 8f6d95e76dc8..a54564f08ae5 100644
@@ -2057,6 +2057,15 @@ static errcode_t inode_scan_and_fix(ext2_resize_t rfs)
 		if (ino <= start_to_move)
 			goto remap_blocks; /* Don't need to move inode. */
 
+		/*
+		 * Moving an extended attribute inode requires updating all inodes
+		 * that reference it which is a lot more involved.
+		 */
+		if (inode->i_flags & EXT4_EA_INODE_FL) {
+			retval = EXT2_ET_CANNOT_MOVE_EA_INODE;
+			goto errout;
+		}
+
 		/*
 		 * Find a new inode.  Now that extents and directory blocks
 		 * are tied to the inode number through the checksum, we must
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog