[PATCH] e2fsck: fix quota accounting to use cluster units

[PATCH] e2fsck: fix quota accounting to use cluster units

[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.

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 <enwlinux@gmail.com>
---
 e2fsck/pass1.c        | 3 ++-
 e2fsck/pass3.c        | 5 +++--
 lib/support/quotaio.h | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 1714897..188cc56 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -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);
 	}
diff --git a/e2fsck/pass3.c b/e2fsck/pass3.c
index 44203ca..4d9430b 100644
--- a/e2fsck/pass3.c
+++ b/e2fsck/pass3.c
@@ -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");
diff --git a/lib/support/quotaio.h b/lib/support/quotaio.h
index 486c2a5..f89536e 100644
--- a/lib/support/quotaio.h
+++ b/lib/support/quotaio.h
@@ -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();
  *	}
-- 
2.1.4

Re: [PATCH] e2fsck: fix quota accounting to use cluster units

[Alexey Lyashkov]

Hi Eric,

> 2 апр. 2017 г., в 19:57, Eric Whitney <enwlinux@gmail.com> написал(а):
> 
> 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 <enwlinux@gmail.com>
> ---
> e2fsck/pass1.c        | 3 ++-
> e2fsck/pass3.c        | 5 +++--
> lib/support/quotaio.h | 2 +-
> 3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 1714897..188cc56 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -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.

diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 98125bf..cd83156 100644 (file)
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -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);
+

Re: [PATCH] e2fsck: fix quota accounting to use cluster units

[Eric Whitney]

* Alexey Lyashkov <alexey.lyashkov@gmail.com>:
> Hi Eric,
> 
> > 2 апр. 2017 г., в 19:57, Eric Whitney <enwlinux@gmail.com> написал(а):
> > 
> > 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 <enwlinux@gmail.com>
> > ---
> > e2fsck/pass1.c        | 3 ++-
> > e2fsck/pass3.c        | 5 +++--
> > lib/support/quotaio.h | 2 +-
> > 3 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> > index 1714897..188cc56 100644
> > --- a/e2fsck/pass1.c
> > +++ b/e2fsck/pass1.c
> > @@ -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.
> 
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 98125bf..cd83156 100644 (file)
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -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);
> +
> 
> 

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

Re: [PATCH] e2fsck: fix quota accounting to use cluster units

[Theodore Ts'o]

On Sun, Apr 02, 2017 at 12:57:05PM -0400, Eric Whitney wrote:
> 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 <enwlinux@gmail.com>

Thanks, applied.

					- Ted