All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH e2fsprogs] e2fsck: update quota accounting after directory optimization
@ 2024-03-27 15:43 Luis Henriques (SUSE)
  2024-03-27 19:25 ` Andreas Dilger
  0 siblings, 1 reply; 3+ messages in thread
From: Luis Henriques (SUSE) @ 2024-03-27 15:43 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: linux-ext4, Luis Henriques (SUSE)

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 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) <luis.henriques@linux.dev>
---
 e2fsck/rehash.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c
index c1da7d52724e..4847d172e5fe 100644
--- a/e2fsck/rehash.c
+++ b/e2fsck/rehash.c
@@ -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);

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH e2fsprogs] e2fsck: update quota accounting after directory optimization
  2024-03-27 15:43 [PATCH e2fsprogs] e2fsck: update quota accounting after directory optimization Luis Henriques (SUSE)
@ 2024-03-27 19:25 ` Andreas Dilger
  2024-03-27 21:16   ` Luis Henriques
  0 siblings, 1 reply; 3+ messages in thread
From: Andreas Dilger @ 2024-03-27 19:25 UTC (permalink / raw)
  To: Luis Henriques (SUSE); +Cc: Theodore Ts'o, Ext4 Developers List

[-- Attachment #1: Type: text/plain, Size: 4576 bytes --]

On Mar 27, 2024, at 9:43 AM, Luis Henriques (SUSE) <luis.henriques@linux.dev> wrote:
> 
> 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 patch simply updates the quota data accordingly, after the directory is
> written and it's size has been updated.

Hi Luis,
thanks for the patch.  It looks reasonable, and might (partially) explain
why quota accounting occasionally reports inconsistencies by a few blocks
after running e2fsck.  You can add my Reviewed-by: to the patch.


Could you please include an e2fsck regression test for this, to confirm
that it is working and continues to work in the future?  It should be
possible to use something like the following to create a test case:

    # cd tests
    # make testnew
    # tune2fs -O quota,project f_testnew/image
    # mkdir /mnt/tmp
    # mount -t ext4 -o loop f_testnew/image /mnt/tmp
    # mkdir /mnt/tmp/subdir
    # chattr -p 1000 -P /mnt/tmp/subdir
    # touch /mnt/tmp/subdir/long-filenames-for-test-{1..1024}
    # rm /mnt/tmp/subdir/long-filenames-for-test-{1..1024..2}
    # umount /mnt/tmp
    # echo "directory optimization updates quota" > f_testnew/name
    # make testend
    # mv f_testnew f_quota_shrinkdir

and confirm in expect.[12] that the quota did not need to be repaired
afterward by e2fsck (i.e. there shouldn't be any error messages about
inconsistent quota).  Running this test with an unpatched e2fsck should
report that the quotas had to be fixed and the test should fail.


On a related note, it would be convenient if "make testnew" passed an
environment variable (e.g. $TESTNEW_OPTS) to mke2fs so more options
could be set at format time instead of using tune2fs afterward:

    # TESTNEW_OPTS="-O quota,project" make testnew

It isn't a big deal in this case, but might be useful in the future.

Cheers, Andreas

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218626
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
> ---
> e2fsck/rehash.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c
> index c1da7d52724e..4847d172e5fe 100644
> --- a/e2fsck/rehash.c
> +++ b/e2fsck/rehash.c
> @@ -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);
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH e2fsprogs] e2fsck: update quota accounting after directory optimization
  2024-03-27 19:25 ` Andreas Dilger
@ 2024-03-27 21:16   ` Luis Henriques
  0 siblings, 0 replies; 3+ messages in thread
From: Luis Henriques @ 2024-03-27 21:16 UTC (permalink / raw)
  To: Andreas Dilger; +Cc: Theodore Ts'o, Ext4 Developers List

Andreas Dilger <adilger@dilger.ca> writes:

> On Mar 27, 2024, at 9:43 AM, Luis Henriques (SUSE) <luis.henriques@linux.dev> wrote:
>> 
>> 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 patch simply updates the quota data accordingly, after the directory is
>> written and it's size has been updated.
>
> Hi Luis,
> thanks for the patch.  It looks reasonable, and might (partially) explain
> why quota accounting occasionally reports inconsistencies by a few blocks
> after running e2fsck.  You can add my Reviewed-by: to the patch.

Awesome, thank you for your review.  In the meantime, I have already a
similar fix for another case where quota inconsistencies occur in e2fsck.
I'm still running a few more tests, but I should be sending another patch
soon.

> Could you please include an e2fsck regression test for this, to confirm
> that it is working and continues to work in the future?  It should be
> possible to use something like the following to create a test case:
>
>     # cd tests
>     # make testnew
>     # tune2fs -O quota,project f_testnew/image
>     # mkdir /mnt/tmp
>     # mount -t ext4 -o loop f_testnew/image /mnt/tmp
>     # mkdir /mnt/tmp/subdir
>     # chattr -p 1000 -P /mnt/tmp/subdir
>     # touch /mnt/tmp/subdir/long-filenames-for-test-{1..1024}
>     # rm /mnt/tmp/subdir/long-filenames-for-test-{1..1024..2}
>     # umount /mnt/tmp
>     # echo "directory optimization updates quota" > f_testnew/name
>     # make testend
>     # mv f_testnew f_quota_shrinkdir
>
> and confirm in expect.[12] that the quota did not need to be repaired
> afterward by e2fsck (i.e. there shouldn't be any error messages about
> inconsistent quota).  Running this test with an unpatched e2fsck should
> report that the quotas had to be fixed and the test should fail.

Right, it makes sense to add a regression test, of course.  I'll need to
get familiar with the framework used but I'll have a look and see what I
can come up with.

> On a related note, it would be convenient if "make testnew" passed an
> environment variable (e.g. $TESTNEW_OPTS) to mke2fs so more options
> could be set at format time instead of using tune2fs afterward:
>
>     # TESTNEW_OPTS="-O quota,project" make testnew
>
> It isn't a big deal in this case, but might be useful in the future.

Hmm... Okay, as I said I'll need to get familiar with this but that seems
to make sense.

Cheers,
-- 
Luis

> Cheers, Andreas
>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218626
>> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
>> ---
>> e2fsck/rehash.c | 27 +++++++++++++++++++++------
>> 1 file changed, 21 insertions(+), 6 deletions(-)
>> 
>> diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c
>> index c1da7d52724e..4847d172e5fe 100644
>> --- a/e2fsck/rehash.c
>> +++ b/e2fsck/rehash.c
>> @@ -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);
>> 
>
>
> Cheers, Andreas

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-03-27 21:17 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-27 15:43 [PATCH e2fsprogs] e2fsck: update quota accounting after directory optimization Luis Henriques (SUSE)
2024-03-27 19:25 ` Andreas Dilger
2024-03-27 21:16   ` Luis Henriques

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.