e2fsck: fix quota accounting to use cluster units

PATCH Applied Eric Whitney 3 participants 4 messages Apr 2, 2017 – Apr 13, 2017

Share & tools

Eric Whitney

author

2017-04-02 16:55 UTC #0 lore raw

PATCH 1/1 e2fsck: fix quota accounting to use cluster units

The quota accounting code in e2fsck's pass 1 and pass 3 uses block units rather than cluster units when recording the allocated space consumed by files and directories. In pass 1, this causes a large undercount of actual quota results and test failures for xfstests generic/383, /384, /385, and /386 on bigalloc file systems. In pass 3, this results in quota accounting errors when the lost+found directory is either extended or recreated on a bigalloc file system.

Use clusters rather than blocks when accounting for allocated space, and correct a related header comment in the quota code.

Note that pass 1b also contains call sites for quota_data_sub() that also need to be addressed. However, it appears that more than just unit conversion may be needed, so that will be left to a future patch.

Signed-off-by Eric Whitney <[email protected]>

---
 e2fsck/pass1.c        | 3 ++-
 e2fsck/pass3.c        | 5 +++--
 lib/support/quotaio.h | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

e2fsck/pass1.c +2 -1

index 1714897..188cc56 100644
@@ -3174,7 +3174,8 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 	if (ino != quota_type2inum(PRJQUOTA, fs->super) &&
 	    (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super))) {
 		quota_data_add(ctx->qctx, (struct ext2_inode_large *) inode,
-			       ino, pb.num_blocks * fs->blocksize);
+			       ino,
+			       pb.num_blocks * EXT2_CLUSTER_SIZE(fs->super));
 		quota_data_inodes(ctx->qctx, (struct ext2_inode_large *) inode,
 				  ino, +1);
 	}

e2fsck/pass3.c +3 -2

index 44203ca..4d9430b 100644
@@ -578,7 +578,7 @@ link_error:
 	ext2fs_icount_store(ctx->inode_count, ino, 2);
 	ext2fs_icount_store(ctx->inode_link_info, ino, 2);
 	ctx->lost_and_found = ino;
-	quota_data_add(ctx->qctx, &inode, ino, fs->blocksize);
+	quota_data_add(ctx->qctx, &inode, ino, EXT2_CLUSTER_SIZE(fs->super));
 	quota_data_inodes(ctx->qctx, &inode, ino, +1);
 #if 0
 	printf("/lost+found created; inode #%lu\n", ino);

@@ -899,7 +899,8 @@ errcode_t e2fsck_expand_directory(e2fsck_t ctx, ext2_ino_t dir,
 	if (retval)
 		return retval;
 	ext2fs_iblk_add_blocks(fs, EXT2_INODE(&inode), es.newblocks);
-	quota_data_add(ctx->qctx, &inode, dir, es.newblocks * fs->blocksize);
+	quota_data_add(ctx->qctx, &inode, dir,
+		       es.newblocks * EXT2_CLUSTER_SIZE(fs->super));
 
 	e2fsck_write_inode_full(ctx, dir, EXT2_INODE(&inode),
 				sizeof(inode), "expand_directory");

lib/support/quotaio.h +1 -1

index 486c2a5..f89536e 100644
@@ -12,7 +12,7 @@
  *
  *	quota_init_context(&qctx, fs, QUOTA_ALL_BIT);
  *	{
- *		quota_compute_usage(qctx, QUOTA_ALL_BIT);
+ *		quota_compute_usage(qctx);
  *		AND/OR
  *		quota_data_add/quota_data_sub/quota_data_inodes();
  *	}

Signature · 1 line

2.1.4

Alexey Lyashkov

2017-04-04 18:03 UTC #1 lore raw

Hi Eric,

Quoted 29 lines

2 апр. 2017 г., в 19:57, Eric Whitney <[email protected]> написал(а):
The quota accounting code in e2fsck's pass 1 and pass 3 uses block units rather than cluster units when recording the allocated space consumed by files and directories. In pass 1, this causes a large undercount of actual quota results and test failures for xfstests generic/383, /384, /385, and /386 on bigalloc file systems. In pass 3, this results in quota accounting errors when the lost+found directory is either extended or recreated on a bigalloc file system.
Use clusters rather than blocks when accounting for allocated space, and correct a related header comment in the quota code.
Note that pass 1b also contains call sites for quota_data_sub() that also need to be addressed. However, it appears that more than just unit conversion may be needed, so that will be left to a future patch.
Signed-off-by Eric Whitney <[email protected]>
---
e2fsck/pass1.c        | 3 ++-
e2fsck/pass3.c        | 5 +++--
lib/support/quotaio.h | 2 +-
3 files changed, 6 insertions(+), 4 deletions(-)
e2fsck/pass1.c +0 -0
index 1714897..188cc56 100644

e2fsck/pass1.c static void check_blocks(e2fsck_t ctx, struct problem_context *pctx, · line 3181

Quoted 9 lines

e2fsck/pass1.c +2 -1

@@ -3174,7 +3174,8 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 	if (ino != quota_type2inum(PRJQUOTA, fs->super) &&
 	    (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super))) {
 		quota_data_add(ctx->qctx, (struct ext2_inode_large *) inode,
-			       ino, pb.num_blocks * fs->blocksize);
+			       ino,
+			       pb.num_blocks * EXT2_CLUSTER_SIZE(fs->super));
 		quota_data_inodes(ctx->qctx, (struct ext2_inode_large *) inode,
 				  ino, +1);
 	}

I think it part may rewrite easy.

e2fsck/pass1.c +3 -1

index 98125bf..cd83156 100644 (file)
@@ -2700,6 +2700,8 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
               }
       }
 
+       pb.num_blocks *= EXT2FS_CLUSTER_RATIO(fs);
+
       if (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super)) {
               quota_data_add(ctx->qctx, inode, ino,
                              pb.num_blocks * fs->blocksize);

@@ -2710,7 +2712,7 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
             EXT4_FEATURE_RO_COMPAT_HUGE_FILE) ||
           !(inode->i_flags & EXT4_HUGE_FILE_FL))
               pb.num_blocks *= (fs->blocksize / 512);
-       pb.num_blocks *= EXT2FS_CLUSTER_RATIO(fs);
+

Eric Whitney

author

2017-04-05 20:54 UTC #2 lore raw

* Alexey Lyashkov <[email protected]>:

Quoted 56 lines

Hi Eric,
Quoted 29 lines
2 апр. 2017 г., в 19:57, Eric Whitney <[email protected]> написал(а):
The quota accounting code in e2fsck's pass 1 and pass 3 uses block units rather than cluster units when recording the allocated space consumed by files and directories. In pass 1, this causes a large undercount of actual quota results and test failures for xfstests generic/383, /384, /385, and /386 on bigalloc file systems. In pass 3, this results in quota accounting errors when the lost+found directory is either extended or recreated on a bigalloc file system.
Use clusters rather than blocks when accounting for allocated space, and correct a related header comment in the quota code.
Note that pass 1b also contains call sites for quota_data_sub() that also need to be addressed. However, it appears that more than just unit conversion may be needed, so that will be left to a future patch.
Signed-off-by Eric Whitney <[email protected]>
---
e2fsck/pass1.c        | 3 ++-
e2fsck/pass3.c        | 5 +++--
lib/support/quotaio.h | 2 +-
3 files changed, 6 insertions(+), 4 deletions(-)
e2fsck/pass1.c +0 -0
index 1714897..188cc56 100644
e2fsck/pass1.c static void check_blocks(e2fsck_t ctx, struct problem_context *pctx, · line 3181
Quoted 9 lines
e2fsck/pass1.c +2 -1
@@ -3174,7 +3174,8 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
 	if (ino != quota_type2inum(PRJQUOTA, fs->super) &&
 	    (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super))) {
 		quota_data_add(ctx->qctx, (struct ext2_inode_large *) inode,
-			       ino, pb.num_blocks * fs->blocksize);
+			       ino,
+			       pb.num_blocks * EXT2_CLUSTER_SIZE(fs->super));
 		quota_data_inodes(ctx->qctx, (struct ext2_inode_large *) inode,
 				  ino, +1);
 	}
I think it part may rewrite easy.
e2fsck/pass1.c +2 -0
index 98125bf..cd83156 100644 (file)
@@ -2700,6 +2700,8 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
               }
       }
 
+       pb.num_blocks *= EXT2FS_CLUSTER_RATIO(fs);
+
       if (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super)) {
               quota_data_add(ctx->qctx, inode, ino,
                              pb.num_blocks * fs->blocksize);

e2fsck/pass1.c static void check_blocks(e2fsck_t ctx, struct problem_context *pctx, · line 2717

Quoted 7 lines

e2fsck/pass1.c +1 -1

@@ -2710,7 +2712,7 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
             EXT4_FEATURE_RO_COMPAT_HUGE_FILE) ||
           !(inode->i_flags & EXT4_HUGE_FILE_FL))
               pb.num_blocks *= (fs->blocksize / 512);
-       pb.num_blocks *= EXT2FS_CLUSTER_RATIO(fs);
+

Hi Alexey:

Thanks for your comment. What you propose would certainly work, but I prefer my original patch because I think it makes the code easier to understand. Using EXT2_CLUSTER_SIZE makes the unit in which space has been allocated direct and explicit rather than asking the reader to first follow an intermediate conversion from cluster to block units. Also, other calls to the quota_data_add/sub code in this and in another patch to be posted can be handled more clearly using EXT2_CLUSTER_SIZE rather than EXT2FS_CLUSTER_RATIO, and using the same style for all is better for readability.

Thanks,
Eric

Theodore Ts'o

Applied

2017-04-13 15:47 UTC #3 lore raw

In reply to Eric Whitney

Quoted 8 lines · Eric Whitney

The quota accounting code in e2fsck's pass 1 and pass 3 uses block units rather than cluster units when recording the allocated space consumed by files and directories. In pass 1, this causes a large undercount of actual quota results and test failures for xfstests generic/383, /384, /385, and /386 on bigalloc file systems. In pass 3, this results in quota accounting errors when the lost+found directory is either extended or recreated on a bigalloc file system.

Quoted 8 lines

Use clusters rather than blocks when accounting for allocated space, and correct a related header comment in the quota code.
Note that pass 1b also contains call sites for quota_data_sub() that also need to be addressed. However, it appears that more than just unit conversion may be needed, so that will be left to a future patch.
Signed-off-by Eric Whitney <[email protected]>

Thanks, applied.

- Ted