Message-Id: <4CCBB8C1-30AD-411F-AED2-781677E89FB7@dilger.ca>
Date: Mon, 1 Apr 2024 14:52:23 -0600
From: Andreas Dilger <adilger@...ger.ca>
To: "Luis Henriques (SUSE)" <luis.henriques@...ux.dev>
Cc: Theodore Ts'o <tytso@....edu>,
 Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH 2/4] e2fsck: update quota when deallocating a bad inode

On Mar 28, 2024, at 11:29 AM, Luis Henriques (SUSE) <luis.henriques@...ux.dev> wrote:
> 
> 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.

I was wondering about the safety of this patch, since "bad inode"
usually means corrupted inode with random garbage, and continuing to
process it is as likely to introduce problems as it is to fix them.

The only caller of deallocate_inode() is e2fsck_process_bad_inode()
when the file type is bad or not matching the inode content.  This
appears it would mostly affect only the inode quota, though in some
rare cases it might affect the block quota (xattr or long symlink).

Looking at the history of deallocate_inode(), it appears risky that
it processes blocks from a corrupt inode at all.  The saving grace
is that it should only "undo" the blocks marked in-use by pass1 so
that a second e2fsck run is not needed to fix up the bitmaps and
counters.  It looks like the same is true with the quota accounting,
that deallocate_inode() is only updating the in-memory inode/block
counters for the UID/GID/PRJID but not messing with any on-disk data
until on-disk quotas are updated in pass5.

It wouldn't be terrible to update the comment before deallocate_inode()
to explain this more clearly, since "This function deallocates an inode"
doesn't really give the reader much more insight than the function name
"deallocate_inode()".  Maybe something more useful like:

/*
 * This function reverts various counters and bitmaps incremented in
 * pass1 for the inode, blocks, and quotas before it was decided the
 * inode was corrupt and needed to be cleared.  This avoids the need
 * to run e2fsck a second time (or have it restart itself) to repair
 * these counters.
 *
 * It does not modify any on-disk state, so even if the inode is bad
 * it _should_ reset in-memory state to before the inode was first
 * processed.
 */

... would be helpful to readers in the future.  In either case, you
can add my:

Reviewed-by: Andreas Dilger <adilger@...ger.ca>

Cheers, Andreas

> 
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@...ux.dev>
> ---
> e2fsck/pass2.c | 33 +++++++++++++++++++++++----------
> 1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
> index b91628567a7f..e16b488af643 100644
> --- a/e2fsck/pass2.c
> +++ b/e2fsck/pass2.c
> @@ -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_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");
> }
> 
> /*


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists