On Mar 28, 2024, at 11:29 AM, Luis Henriques (SUSE) <luis.henriques@xxxxxxxxx> 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@xxxxxxxxx>
Cheers, Andreas
>
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@xxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: Message signed with OpenPGP
