quota-related e2fsck fixes and tests

In "Pass 3A: Optimizing directories", a directory may have it's size reduced. If that happens and quota is enabled in the filesystem, the quota information will be incorrect because it doesn't take the rehash into account. This issue was detected by running fstest ext4/014.

This patch simply updates the quota data accordingly, after the directory is written and it's size has been updated.

Link https://bugzilla.kernel.org/show_bug.cgi?id=218626

Signed-off-by Luis Henriques (SUSE) <[email protected]>

---
e2fsck/rehash.c | 27 +++++++++++++++++++++------

1 file changed, 21 insertions(+), 6 deletions(-)

e2fsck/rehash.c +21 -6

index c1da7d52724e..4847d172e5fe 100644
@@ -987,14 +987,18 @@ errcode_t e2fsck_rehash_dir(e2fsck_t ctx, ext2_ino_t ino,
 {
 	ext2_filsys 		fs = ctx->fs;
 	errcode_t		retval;
-	struct ext2_inode 	inode;
+	struct ext2_inode_large	inode;
 	char			*dir_buf = 0;
 	struct fill_dir_struct	fd = { NULL, NULL, 0, 0, 0, NULL,
 				       0, 0, 0, 0, 0, 0 };
 	struct out_dir		outdir = { 0, 0, 0, 0 };
-	struct name_cmp_ctx name_cmp_ctx = {0, NULL};
+	struct name_cmp_ctx	name_cmp_ctx = {0, NULL};
+	__u64			osize;
 
-	e2fsck_read_inode(ctx, ino, &inode, "rehash_dir");
+	e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode),
+			       sizeof(inode), "rehash_dir");
+
+	osize = EXT2_I_SIZE(&inode);
 
 	if (ext2fs_has_feature_inline_data(fs->super) &&
 	   (inode.i_flags & EXT4_INLINE_DATA_FL))

@@ -1013,7 +1017,7 @@ errcode_t e2fsck_rehash_dir(e2fsck_t ctx, ext2_ino_t ino,
 	fd.ino = ino;
 	fd.ctx = ctx;
 	fd.buf = dir_buf;
-	fd.inode = &inode;
+	fd.inode = EXT2_INODE(&inode);
 	fd.dir = ino;
 	if (!ext2fs_has_feature_dir_index(fs->super) ||
 	    (inode.i_size / fs->blocksize) < 2)

@@ -1092,14 +1096,25 @@ resort:
 			goto errout;
 	}
 
-	retval = write_directory(ctx, fs, &outdir, ino, &inode, fd.compress);
+	retval = write_directory(ctx, fs, &outdir, ino, EXT2_INODE(&inode),
+				 fd.compress);
 	if (retval)
 		goto errout;
 
+	if ((osize > EXT2_I_SIZE(&inode)) &&
+	    (ino != quota_type2inum(PRJQUOTA, fs->super)) &&
+	    (ino != fs->super->s_orphan_file_inum) &&
+	    (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super)) &&
+	    !(inode.i_flags & EXT4_EA_INODE_FL)) {
+		quota_data_sub(ctx->qctx, &inode,
+			       ino, osize - EXT2_I_SIZE(&inode));
+	}
+
 	if (ctx->options & E2F_OPT_CONVERT_BMAP)
 		retval = e2fsck_rebuild_extents_later(ctx, ino);
 	else
-		retval = e2fsck_check_rebuild_extents(ctx, ino, &inode, pctx);
+		retval = e2fsck_check_rebuild_extents(ctx, ino,
+						      EXT2_INODE(&inode), pctx);

errout:

	ext2fs_free_mem(&dir_buf);
	ext2fs_free_mem(&fd.harray);

If a bad inode is found it will be deallocated. However, if the filesystem has quota enabled, the quota information isn't being updated accordingly. This issue was detected by running fstest ext4/019.

This patch fixes the issue by decreasing the inode count from the quota and, if blocks are also being released, also subtract them as well.

Signed-off-by Luis Henriques (SUSE) <[email protected]>

---
e2fsck/pass2.c | 33 +++++++++++++++++++++++----------

1 file changed, 23 insertions(+), 10 deletions(-)

e2fsck/pass2.c +21 -8

index b91628567a7f..e16b488af643 100644
@@ -1859,12 +1859,13 @@ static int deallocate_inode_block(ext2_filsys fs,
 static void deallocate_inode(e2fsck_t ctx, ext2_ino_t ino, char* block_buf)
 {
 	ext2_filsys fs = ctx->fs;
-	struct ext2_inode	inode;
+	struct ext2_inode_large	inode;
 	struct problem_context	pctx;
 	__u32			count;
 	struct del_block	del_block;
 
-	e2fsck_read_inode(ctx, ino, &inode, "deallocate_inode");
+	e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode),
+			       sizeof(inode), "deallocate_inode");
 	clear_problem_context(&pctx);
 	pctx.ino = ino;
 

@@ -1874,29 +1875,29 @@ static void deallocate_inode(e2fsck_t ctx, ext2_ino_t ino, char* block_buf)
 	e2fsck_read_bitmaps(ctx);
 	ext2fs_inode_alloc_stats2(fs, ino, -1, LINUX_S_ISDIR(inode.i_mode));
 
-	if (ext2fs_file_acl_block(fs, &inode) &&
+	if (ext2fs_file_acl_block(fs, EXT2_INODE(&inode)) &&
 	    ext2fs_has_feature_xattr(fs->super)) {
 		pctx.errcode = ext2fs_adjust_ea_refcount3(fs,
-				ext2fs_file_acl_block(fs, &inode),
+				ext2fs_file_acl_block(fs, EXT2_INODE(&inode)),
 				block_buf, -1, &count, ino);
 		if (pctx.errcode == EXT2_ET_BAD_EA_BLOCK_NUM) {
 			pctx.errcode = 0;
 			count = 1;
 		}
 		if (pctx.errcode) {
-			pctx.blk = ext2fs_file_acl_block(fs, &inode);
+			pctx.blk = ext2fs_file_acl_block(fs, EXT2_INODE(&inode));
 			fix_problem(ctx, PR_2_ADJ_EA_REFCOUNT, &pctx);
 			ctx->flags |= E2F_FLAG_ABORT;
 			return;
 		}
 		if (count == 0) {
 			ext2fs_block_alloc_stats2(fs,
-				  ext2fs_file_acl_block(fs, &inode), -1);
+				  ext2fs_file_acl_block(fs, EXT2_INODE(&inode)), -1);
 		}
-		ext2fs_file_acl_block_set(fs, &inode, 0);
+		ext2fs_file_acl_block_set(fs, EXT2_INODE(&inode), 0);
 	}
 
-	if (!ext2fs_inode_has_valid_blocks2(fs, &inode))
+	if (!ext2fs_inode_has_valid_blocks2(fs, EXT2_INODE(&inode)))
 		goto clear_inode;
 
 	/* Inline data inodes don't have blocks to iterate */

@@ -1921,10 +1922,22 @@ static void deallocate_inode(e2fsck_t ctx, ext2_ino_t ino, char* block_buf)
 		ctx->flags |= E2F_FLAG_ABORT;
 		return;
 	}
+
+	if ((ino != quota_type2inum(PRJQUOTA, fs->super)) &&
+	    (ino != fs->super->s_orphan_file_inum) &&
+	    (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super)) &&
+	    !(inode.i_flags & EXT4_EA_INODE_FL)) {
+		if (del_block.num > 0)
+			quota_data_sub(ctx->qctx, &inode, ino,
+				       del_block.num * EXT2_CLUSTER_SIZE(fs->super));
+		quota_data_inodes(ctx->qctx, (struct ext2_inode_large *)&inode,
+				  ino, -1);
+	}
+

clear_inode:

	/* Inode may have changed by block_iterate, so reread it */

e2fsck/pass2.c +2 -2

-	e2fsck_read_inode(ctx, ino, &inode, "deallocate_inode");
-	e2fsck_clear_inode(ctx, ino, &inode, 0, "deallocate_inode");
+	e2fsck_read_inode(ctx, ino, EXT2_INODE(&inode), "deallocate_inode");
+	e2fsck_clear_inode(ctx, ino, EXT2_INODE(&inode), 0, "deallocate_inode");

}

/*

This new test validates e2fsck by verifying that quota data is updated after a directory optimization is performed. It mimics fstest ext4/014 by including a filesystem image where a file is created inside a new directory on the filesystem root and then root block 0 is wiped:

 # debugfs -w -R 'zap -f / 0' f_testnew/image

Signed-off-by Luis Henriques (SUSE) <[email protected]>

---

tests/f_quota_shrinkdir/expect.1 |  40 +++++++++++++++++++++++++++++++
tests/f_quota_shrinkdir/expect.2 |   7 ++++++
tests/f_quota_shrinkdir/image.gz | Bin 0 -> 11453 bytes
tests/f_quota_shrinkdir/name     |   1 +
4 files changed, 48 insertions(+)

create mode 100644 tests/f_quota_shrinkdir/expect.1 create mode 100644 tests/f_quota_shrinkdir/expect.2 create mode 100644 tests/f_quota_shrinkdir/image.gz create mode 100644 tests/f_quota_shrinkdir/name

tests/f_quota_shrinkdir/expect.1 new file +40 -0

new file mode 100644
index 000000000000..812fe44b887d
@@ -0,0 +1,40 @@
+Pass 1: Checking inodes, blocks, and sizes
+Pass 2: Checking directory structure
+Directory inode 2, block #0, offset 0: directory corrupted
+Salvage? yes
+
+Missing '.' in directory inode 2.
+Fix? yes
+
+Missing '..' in directory inode 2.
+Fix? yes
+
+Pass 3: Checking directory connectivity
+'..' in / (2) is <The NULL inode> (0), should be / (2).
+Fix? yes
+
+Unconnected directory inode 11 (was in /)
+Connect to /lost+found? yes
+
+/lost+found not found.  Create? yes
+
+Unconnected directory inode 12 (was in /)
+Connect to /lost+found? yes
+
+Pass 3A: Optimizing directories
+Pass 4: Checking reference counts
+Inode 11 ref count is 3, should be 2.  Fix? yes
+
+Inode 12 ref count is 3, should be 2.  Fix? yes
+
+Pass 5: Checking group summary information
+[QUOTA WARNING] Usage inconsistent for ID 0:actual (4096, 5) != expected (14336, 4)
+Update quota info for quota type 0? yes
+
+[QUOTA WARNING] Usage inconsistent for ID 0:actual (4096, 5) != expected (14336, 4)
+Update quota info for quota type 1? yes
+
+
+test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
+test_filesys: 14/256 files (14.3% non-contiguous), 1146/8192 blocks
+Exit status is 1

tests/f_quota_shrinkdir/expect.2 new file +7 -0

new file mode 100644
index 000000000000..814f84a54fd6
@@ -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: 14/256 files (14.3% non-contiguous), 1146/8192 blocks
+Exit status is 0

tests/f_quota_shrinkdir/image.gz new file binary +0 -0

new file mode 100644
index 0000000000000000000000000000000000000000..753774f6a11f8a60fdbdf2ce205e64a36d753893
GIT binary patch

tests/f_quota_shrinkdir/name new file +0 -0

new file mode 100644
index 000000000000..8772ae5c814b

Quoted 6 lines · Luis Henriques (SUSE)

This new test validates e2fsck by verifying that quota data is updated after a directory optimization is performed. It mimics fstest ext4/014 by including a filesystem image where a file is created inside a new directory on the filesystem root and then root block 0 is wiped:

Quoted 1 line

 # debugfs -w -R 'zap -f / 0' f_testnew/image

I appreciate the test case, and I hate to be difficult, but IMHO this test case is not ideal. It is *still* reporting quota inconsistency at the end, so it is difficult to see whether the patch is actually improving anything or not?

This is because the image is testing a number of different things at once (repairing the root inode, superblock, etc). IMHO, it would be better to have this test be specific to the directory shrink issue (e.g. a large directory is created, many files are deleted from it, then optimized), and ideally have a non-root user, group, and project involved so that it is verifying that all of the quotas are updated.

Quoted 7 lines · Andreas Dilger

Quoted 6 lines · Luis Henriques (SUSE)

This new test validates e2fsck by verifying that quota data is updated after a directory optimization is performed. It mimics fstest ext4/014 by including a filesystem image where a file is created inside a new directory on the filesystem root and then root block 0 is wiped:

Quoted 6 lines

Quoted 1 line

 # debugfs -w -R 'zap -f / 0' f_testnew/image

I appreciate the test case, and I hate to be difficult, but IMHO this test case is not ideal. It is *still* reporting quota inconsistency at the end, so it is difficult to see whether the patch is actually improving anything or not?

Maybe I misunderstood how the tests really work. Here's what I understood:

e2fsck is run twice. During the first run the filesystem is recovered. And that's the output of expect.1 -- it reports the quota inconsistency because quota data needs to be fixed. And it is fixed in that first run, where e2fsck returns '1' ("File system errors corrected"). The second time e2fsck is run (expect.2) it will do nothing, and '0' is returned because the filesystem hasn't been modified.

Without the first patch in this series the second time e2fsck is executed it will still fail and report inconsistencies because the first time the fix wasn't correct. (And after this second time the filesystem should actually be corrected, a third run of e2fsck should return '0'.)

Quoted 6 lines

This is because the image is testing a number of different things at once (repairing the root inode, superblock, etc). IMHO, it would be better to have this test be specific to the directory shrink issue (e.g. a large directory is created, many files are deleted from it, then optimized), and ideally have a non-root user, group, and project involved so that it is verifying that all of the quotas are updated.

Right, that makes sense. However, I'm failing to narrow the test to that specific case. I've tried to create a bunch of files in a directory and used the debugfs 'kill_file' to remove files from that directory. However, in that case e2fsck isn't reporting quota inconsistencies as I would expect. Which may hint at yet more quota-related bugs. But I'm still looking.