Received-SPF: pass (google.com: domain of linux-ext4+bounces-1762-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Andreas Dilger <adilger@dilger.ca>
Message-Id: <F7E15565-D316-4F02-93D4-CC081AB421C0@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_4181BD24-3240-484A-858C-8AA278A7662E";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH e2fsprogs] e2fsck: update quota accounting after directory
 optimization
Date: Wed, 27 Mar 2024 13:25:49 -0600
In-Reply-To: <20240327154352.22648-1-luis.henriques@linux.dev>
Cc: Theodore Ts'o <tytso@mit.edu>,
 Ext4 Developers List <linux-ext4@vger.kernel.org>
To: "Luis Henriques (SUSE)" <luis.henriques@linux.dev>
References: <20240327154352.22648-1-luis.henriques@linux.dev>


--Apple-Mail=_4181BD24-3240-484A-858C-8AA278A7662E
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Mar 27, 2024, at 9:43 AM, Luis Henriques (SUSE) =
<luis.henriques@linux.dev> wrote:
>=20
> 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.
>=20
> 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=3D"-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=3D218626
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
> ---
> e2fsck/rehash.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>=20
> 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 =3D ctx->fs;
> 	errcode_t		retval;
> -	struct ext2_inode 	inode;
> +	struct ext2_inode_large	inode;
> 	char			*dir_buf =3D 0;
> 	struct fill_dir_struct	fd =3D { NULL, NULL, 0, 0, 0, NULL,
> 				       0, 0, 0, 0, 0, 0 };
> 	struct out_dir		outdir =3D { 0, 0, 0, 0 };
> -	struct name_cmp_ctx name_cmp_ctx =3D {0, NULL};
> +	struct name_cmp_ctx	name_cmp_ctx =3D {0, NULL};
> +	__u64			osize;
>=20
> -	e2fsck_read_inode(ctx, ino, &inode, "rehash_dir");
> +	e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode),
> +			       sizeof(inode), "rehash_dir");
> +
> +	osize =3D EXT2_I_SIZE(&inode);
>=20
> 	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 =3D ino;
> 	fd.ctx =3D ctx;
> 	fd.buf =3D dir_buf;
> -	fd.inode =3D &inode;
> +	fd.inode =3D EXT2_INODE(&inode);
> 	fd.dir =3D ino;
> 	if (!ext2fs_has_feature_dir_index(fs->super) ||
> 	    (inode.i_size / fs->blocksize) < 2)
> @@ -1092,14 +1096,25 @@ resort:
> 			goto errout;
> 	}
>=20
> -	retval =3D write_directory(ctx, fs, &outdir, ino, &inode, =
fd.compress);
> +	retval =3D write_directory(ctx, fs, &outdir, ino, =
EXT2_INODE(&inode),
> +				 fd.compress);
> 	if (retval)
> 		goto errout;
>=20
> +	if ((osize > EXT2_I_SIZE(&inode)) &&
> +	    (ino !=3D quota_type2inum(PRJQUOTA, fs->super)) &&
> +	    (ino !=3D fs->super->s_orphan_file_inum) &&
> +	    (ino =3D=3D EXT2_ROOT_INO || ino >=3D =
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 =3D e2fsck_rebuild_extents_later(ctx, ino);
> 	else
> -		retval =3D e2fsck_check_rebuild_extents(ctx, ino, =
&inode, pctx);
> +		retval =3D e2fsck_check_rebuild_extents(ctx, ino,
> +						      =
EXT2_INODE(&inode), pctx);
> errout:
> 	ext2fs_free_mem(&dir_buf);
> 	ext2fs_free_mem(&fd.harray);
>=20


Cheers, Andreas


--Apple-Mail=_4181BD24-3240-484A-858C-8AA278A7662E
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iQIzBAEBCAAdFiEEDb73u6ZejP5ZMprvcqXauRfMH+AFAmYEcr0ACgkQcqXauRfM
H+DlRhAAt/nWD+Ez7qF4xJ+8wqJ+lSYS18KwdJ2yza54Fqmt8pGMNqmtaT6yDF6P
EL3iJuMnNNpVxC6staU+v+jojaHgSJUTRQ3B2a1cw4cTQDD3l7cFJ6+5xr+bgJ4D
YmlyTwxjqXIKSCKMBCZ2KPj6hb7c4fL0hBmR7bVSEiSYOPnDSiXerhHBeflpx7+5
eRAijxPhFwogCJt/hHWILh4+rhaNdglB+JXN2ro4oDDTv3dCxr9iXl38tpXwW7LY
fJCOEHzdjGzOVTqEBxWSLQMIu79h8OTOmyb8vqiRYx9pV2eVbOJ8kzdPMOc5184q
FLmNBUHiphRPi2OTesVudrQu0QobA5GjGN4IVw+IQ5NZEZylwqho/eTb1yiBApp8
Zxbv/F3ZtZU1VPZ0XdpCyfcnBNXFJLtIkkApFtdly2fvMbc3Oi3iYeb64NBVFX0n
1t5HqPF7VBbf02a5uRJk02pcmyWOEWH+kPhO1OszVjGM74srm7/AOYtJt0Q/8mgU
xiw9OXo0r+zZb8MbUXC8kNMXJT7Rv8FHVDD5zLoyEgQflW05YKuna9ELLwx7b5pc
4dAQwhJZgiIQz4blPvxfN1zYl62ZDATm2F7zsMWRi/4OCgjnRo7rx5iZE2ZMOnd6
sRhYrAxT+cPCUtkGxaHG4w7dOXztJBn4VDMOrMg2JtVBvgsD3dg=
=P8IU
-----END PGP SIGNATURE-----

--Apple-Mail=_4181BD24-3240-484A-858C-8AA278A7662E--