e2fsck: Fix directory with holes even when i_size is wrong

PATCH New Theodore Ts'o 1 participant 4 messages Nov 28, 2009 – Nov 29, 2009 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Theodore Ts'o

author

2009-11-28 15:30 UTC #1 lore raw

PATCH 1/3 e2fsck: Fix directory with holes even when i_size is wrong

The old method for detecting directories with holes depended on i_size being correct, even though the correct value of i_size hadn't been calculated yet. Hence, a directory inode with holes and an i_size of 0 would require two e2fsck passes to fix completely.

The replacement method for determining whether or not ext2fs_add_dir_block() should be called is more reliable, and reduces the size of e2fsck and makes the code more readable as a bonus.

Thanks to Mikulas Patocka for providing a sample file system which demonstrated this problem.

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 e2fsck/pass1.c |   61 ++++++++++++++-----------------------------------------
 1 files changed, 16 insertions(+), 45 deletions(-)

e2fsck/pass1.c +16 ~~-45~~

index 2531e57..f87d3ed 100644
@@ -83,6 +83,7 @@ struct process_block_struct {
 	blk_t		num_blocks;
 	blk_t		max_blocks;
 	e2_blkcnt_t	last_block;
+	e2_blkcnt_t	last_db_block;
 	int		num_illegal_blocks;
 	blk_t		previous_block;
 	struct ext2_inode *inode;

@@ -767,6 +768,7 @@ void e2fsck_pass1(e2fsck_t ctx)
 			}
 			pb.ino = EXT2_BAD_INO;
 			pb.num_blocks = pb.last_block = 0;
+			pb.last_db_block = -1;
 			pb.num_illegal_blocks = 0;
 			pb.suppress = 0; pb.clear = 0; pb.is_dir = 0;
 			pb.is_reg = 0; pb.fragmented = 0; pb.bbcheck = 0;

@@ -1820,6 +1822,7 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 	pb.ino = ino;
 	pb.num_blocks = 0;
 	pb.last_block = -1;
+	pb.last_db_block = -1;
 	pb.num_illegal_blocks = 0;
 	pb.suppress = 0; pb.clear = 0;
 	pb.fragmented = 0;

@@ -1883,26 +1886,6 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 		return;
 	}
 
-	if (pb.is_dir) {
-		while (1) {
-			struct ext2_db_entry *entry;
-
-			if (ext2fs_dblist_get_last(fs->dblist, &entry) ||
-			    (entry->ino != ino) ||
-			    (entry->blk != 0) ||
-			    (entry->blockcnt == 0))
-				break;
-			/* printf("Dropping ino %lu blk %lu blockcnt %d\n",
-				  entry->ino, entry->blk, entry->blockcnt); */
-			ext2fs_dblist_drop_last(fs->dblist);
-			if (ext2fs_dblist_get_last(fs->dblist, &entry) ||
-			    (entry->ino != ino))
-				pb.last_block--;
-			else
-				pb.last_block = entry->blockcnt;
-		}
-	}
-
 	if (inode->i_flags & EXT2_INDEX_FL) {
 		if (handle_htree(ctx, pctx, ino, inode, block_buf)) {
 			inode->i_flags &= ~EXT2_INDEX_FL;

@@ -2094,31 +2077,8 @@ static int process_block(ext2_filsys fs,
 		return 0;
 	}
 
-	if (blk == 0) {
-		if (p->is_dir == 0) {
-			/*
-			 * Should never happen, since only directories
-			 * get called with BLOCK_FLAG_HOLE
-			 */
-#if DEBUG_E2FSCK
-			printf("process_block() called with blk == 0, "
-			       "blockcnt=%d, inode %lu???\n",
-			       blockcnt, p->ino);
-#endif
-			return 0;
-		}
-		if (blockcnt < 0)
-			return 0;
-		if (blockcnt * fs->blocksize < p->inode->i_size) {
-#if 0
-			printf("Missing block (#%d) in directory inode %lu!\n",
-			       blockcnt, p->ino);
-#endif
-			p->last_block = blockcnt;
-			goto mark_dir;
-		}
-		return 0;
-	}
+	if (blk == 0)
+		return;
 
 #if 0
 	printf("Process_block, inode %lu, block %u, #%d\n", p->ino, blk,

@@ -2203,11 +2163,22 @@ static int process_block(ext2_filsys fs,
 		p->last_block = blockcnt;
 mark_dir:
 	if (p->is_dir && (blockcnt >= 0)) {
+		while (++p->last_db_block < blockcnt) {
+			pctx->errcode = ext2fs_add_dir_block(fs->dblist,
+							     p->ino, 0,
+							     p->last_db_block);
+			if (pctx->errcode) {
+				pctx->blk = 0;
+				pctx->num = p->last_db_block;
+				goto failed_add_dir_block;
+			}
+		}
 		pctx->errcode = ext2fs_add_dir_block(fs->dblist, p->ino,
 						    blk, blockcnt);
 		if (pctx->errcode) {
 			pctx->blk = blk;
 			pctx->num = blockcnt;
+		failed_add_dir_block:
 			fix_problem(ctx, PR_1_ADD_DBLOCK, pctx);
 			/* Should never get here */
 			ctx->flags |= E2F_FLAG_ABORT;

Signature · 1 line

1.6.5.216.g5288a.dirty

Theodore Ts'o

author

2009-11-29 20:31 UTC #2 lore raw

PATCH 1/1 e2fsck: Fix directory with holes even when i_size is wrong

The replacement method for determining whether or not ext2fs_add_dir_block() should be called is more reliable, and reduces the size of e2fsck and makes the code more readable as a bonus.

Thanks to Mikulas Patocka for providing a sample file system which demonstrated this problem.

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 e2fsck/pass1.c |   59 ++++++++++++++-----------------------------------------
 1 files changed, 15 insertions(+), 44 deletions(-)

e2fsck/pass1.c +15 ~~-44~~

index 2531e57..b534841 100644
@@ -83,6 +83,7 @@ struct process_block_struct {
 	blk_t		num_blocks;
 	blk_t		max_blocks;
 	e2_blkcnt_t	last_block;
+	e2_blkcnt_t	last_db_block;
 	int		num_illegal_blocks;
 	blk_t		previous_block;
 	struct ext2_inode *inode;

@@ -767,6 +768,7 @@ void e2fsck_pass1(e2fsck_t ctx)
 			}
 			pb.ino = EXT2_BAD_INO;
 			pb.num_blocks = pb.last_block = 0;
+			pb.last_db_block = -1;
 			pb.num_illegal_blocks = 0;
 			pb.suppress = 0; pb.clear = 0; pb.is_dir = 0;
 			pb.is_reg = 0; pb.fragmented = 0; pb.bbcheck = 0;

@@ -1820,6 +1822,7 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 	pb.ino = ino;
 	pb.num_blocks = 0;
 	pb.last_block = -1;
+	pb.last_db_block = -1;
 	pb.num_illegal_blocks = 0;
 	pb.suppress = 0; pb.clear = 0;
 	pb.fragmented = 0;

@@ -1883,26 +1886,6 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 		return;
 	}
 
-	if (pb.is_dir) {
-		while (1) {
-			struct ext2_db_entry *entry;
-
-			if (ext2fs_dblist_get_last(fs->dblist, &entry) ||
-			    (entry->ino != ino) ||
-			    (entry->blk != 0) ||
-			    (entry->blockcnt == 0))
-				break;
-			/* printf("Dropping ino %lu blk %lu blockcnt %d\n",
-				  entry->ino, entry->blk, entry->blockcnt); */
-			ext2fs_dblist_drop_last(fs->dblist);
-			if (ext2fs_dblist_get_last(fs->dblist, &entry) ||
-			    (entry->ino != ino))
-				pb.last_block--;
-			else
-				pb.last_block = entry->blockcnt;
-		}
-	}
-
 	if (inode->i_flags & EXT2_INDEX_FL) {
 		if (handle_htree(ctx, pctx, ino, inode, block_buf)) {
 			inode->i_flags &= ~EXT2_INDEX_FL;

@@ -2094,31 +2077,8 @@ static int process_block(ext2_filsys fs,
 		return 0;
 	}
 
-	if (blk == 0) {
-		if (p->is_dir == 0) {
-			/*
-			 * Should never happen, since only directories
-			 * get called with BLOCK_FLAG_HOLE
-			 */
-#if DEBUG_E2FSCK
-			printf("process_block() called with blk == 0, "
-			       "blockcnt=%d, inode %lu???\n",
-			       blockcnt, p->ino);
-#endif
-			return 0;
-		}
-		if (blockcnt < 0)
-			return 0;
-		if (blockcnt * fs->blocksize < p->inode->i_size) {
-#if 0
-			printf("Missing block (#%d) in directory inode %lu!\n",
-			       blockcnt, p->ino);
-#endif
-			p->last_block = blockcnt;
-			goto mark_dir;
-		}
+	if (blk == 0)
 		return 0;
-	}
 
 #if 0
 	printf("Process_block, inode %lu, block %u, #%d\n", p->ino, blk,

@@ -2203,11 +2163,22 @@ static int process_block(ext2_filsys fs,
 		p->last_block = blockcnt;
 mark_dir:
 	if (p->is_dir && (blockcnt >= 0)) {
+		while (++p->last_db_block < blockcnt) {
+			pctx->errcode = ext2fs_add_dir_block(fs->dblist,
+							     p->ino, 0,
+							     p->last_db_block);
+			if (pctx->errcode) {
+				pctx->blk = 0;
+				pctx->num = p->last_db_block;
+				goto failed_add_dir_block;
+			}
+		}
 		pctx->errcode = ext2fs_add_dir_block(fs->dblist, p->ino,
 						    blk, blockcnt);
 		if (pctx->errcode) {
 			pctx->blk = blk;
 			pctx->num = blockcnt;
+		failed_add_dir_block:
 			fix_problem(ctx, PR_1_ADD_DBLOCK, pctx);
 			/* Should never get here */
 			ctx->flags |= E2F_FLAG_ABORT;

Signature · 1 line

1.6.5.216.g5288a.dirty

Theodore Ts'o

author

2009-11-28 15:30 UTC #3 lore raw

PATCH 2/3 e2fsck: Move check to add an index to a directory after fixing i_size

In reply to a message not in the archive

The check that determines whether an directory needs to be have an index added to it depends on i_size. So move it after we have fixed up i_size so that we reliable will rehash a directory that needs it, even if its i_size field was originally incorrect. Otherwise, a second run of e2fsck would be needed before the directory gets an index added.

Thanks to Mikulas Patocka for providing a sample file system which demonstrated this problem.

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 e2fsck/pass1.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

e2fsck/pass1.c +6 -4

index f87d3ed..40948e7 100644
@@ -1896,10 +1896,6 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 #endif
 		}
 	}
-	if (ctx->dirs_to_hash && pb.is_dir &&
-	    !(inode->i_flags & EXT2_INDEX_FL) &&
-	    ((inode->i_size / fs->blocksize) >= 3))
-		ext2fs_u32_list_add(ctx->dirs_to_hash, ino);
 
 	if (!pb.num_blocks && pb.is_dir) {
 		if (fix_problem(ctx, PR_1_ZERO_LENGTH_DIR, pctx)) {

@@ -1977,6 +1973,12 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 		}
 		pctx->num = 0;
 	}
+
+	if (ctx->dirs_to_hash && pb.is_dir &&
+	    !(inode->i_flags & EXT2_INDEX_FL) &&
+	    ((inode->i_size / fs->blocksize) >= 3))
+		ext2fs_u32_list_add(ctx->dirs_to_hash, ino);
+
 out:
 	if (dirty_inode)
 		e2fsck_write_inode(ctx, ino, inode, "check_blocks");

Signature · 1 line

1.6.5.216.g5288a.dirty

Theodore Ts'o

author

2009-11-28 15:31 UTC #4 lore raw

PATCH 3/3 filefrag: Fix a core dump on sparc32 platforms with 8k file systems

In reply to a message not in the archive

On 32-bit platforms where the file system block size is 8k or greater, the calculation bpib*bpib*bpib* will overflow a 32-bit calculation, leading to a divide by zero error. Fix this.

Thanks to Mikulas Patocka for pointing this out.

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 misc/filefrag.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

misc/filefrag.c +1 -1

index 11783fd..1611f90 100644
@@ -341,7 +341,7 @@ static void frag_report(const char *filename)
 				if (((i-EXT2_DIRECT-bpib) % (bpib*bpib)) == 0)
 					last_block++;
 				if (((i-EXT2_DIRECT-bpib-bpib*bpib) %
-							(bpib*bpib*bpib)) == 0)
+				     (((__u64) bpib)*bpib*bpib)) == 0)
 					last_block++;
 			}
 			rc = get_bmap(fd, i, &block);

Signature · 1 line

1.6.5.216.g5288a.dirty