On Apr 21, 2017, at 2:06 PM, Theodore Ts'o <tytso@xxxxxxx> wrote:
>
> From bc177d425d4fe33b0ae774b218b055465a840ca1 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@xxxxxxx>
> Date: Sat, 15 Apr 2017 00:29:46 -0400
> Subject: [PATCH] e2fsck: update quota when optimizing the extent tree
>
> If quota is enabled, optimizing the extent tree wouldn't update the
> in-memory quota statistics, so that a subsequent e2fsck run would show
> that the quota usage statistics on disk were incorrect.
>
> Google-Bug-Id: 36391645
>
> Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
> ---
> e2fsck/extents.c | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/e2fsck/extents.c b/e2fsck/extents.c
> index c4167e16..7f28e6dd 100644
> --- a/e2fsck/extents.c
> +++ b/e2fsck/extents.c
> @@ -208,17 +208,19 @@ static int find_blocks(ext2_filsys fs, blk64_t *blocknr, e2_blkcnt_t blockcnt,
> static errcode_t rebuild_extent_tree(e2fsck_t ctx, struct extent_list *list,
> ext2_ino_t ino)
> {
> - struct ext2_inode inode;
> + struct ext2_inode_large inode;
> errcode_t retval;
> ext2_extent_handle_t handle;
> unsigned int i, ext_written;
> struct ext2fs_extent *ex, extent;
> + blk64_t start_val, delta;
>
> list->count = 0;
> list->blocks_freed = 0;
> list->ino = ino;
> list->ext_read = 0;
> - e2fsck_read_inode(ctx, ino, &inode, "rebuild_extents");
> + e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode), sizeof(inode),
> + "rebuild_extents");
I'm always a bit puzzled why e2fsck_read_inode_full() takes ext2_inode
as an argument instead of ext2_inode_large, but that isn't the fault of
this patch.
>
> /* Skip deleted inodes and inline data files */
> if (inode.i_links_count == 0 ||
> @@ -248,16 +250,20 @@ extents_loaded:
> memset(inode.i_block, 0, sizeof(inode.i_block));
>
> /* Make a note of freed blocks */
> - retval = ext2fs_iblk_sub_blocks(ctx->fs, &inode, list->blocks_freed);
> + quota_data_sub(ctx->qctx, &inode, ino,
> + list->blocks_freed * ctx->fs->blocksize);
Should this quota_data_sub() call be after ext2fs_iblk_sub_blocks() has
completed successfully, or are the blocks already freed at this point
and only the inode->i_blocks counter would be outdated?
> + retval = ext2fs_iblk_sub_blocks(ctx->fs, EXT2_INODE(&inode),
> + list->blocks_freed);
> if (retval)
> goto err;
>
> /* Now stuff extents into the file */
> - retval = ext2fs_extent_open2(ctx->fs, ino, &inode, &handle);
> + retval = ext2fs_extent_open2(ctx->fs, ino, EXT2_INODE(&inode), &handle);
> if (retval)
> goto err;
>
> ext_written = 0;
> + start_val = ext2fs_inode_i_blocks(ctx->fs, EXT2_INODE(&inode));
In theory this could go at the start before any changes are made to the
inode, and then there wouldn't be the need for the intermediate call to
quota_data_sub() above? That avoids a small amount of overhead, and
also avoids the more complicated issue of the quota file being modified
even if this operation hits an error and is skipped (i.e. any "goto err"
case).
Cheers, Andreas
> for (i = 0, ex = list->extents; i < list->count; i++, ex++) {
> memcpy(&extent, ex, sizeof(struct ext2fs_extent));
> extent.e_flags &= EXT2_EXTENT_FLAGS_UNINIT;
> @@ -295,11 +301,21 @@ extents_loaded:
> ext_written++;
> }
>
> + delta = ext2fs_inode_i_blocks(ctx->fs, EXT2_INODE(&inode)) - start_val;
> + if (delta) {
> + if (!ext2fs_has_feature_huge_file(ctx->fs->super) ||
> + !(inode.i_flags & EXT4_HUGE_FILE_FL))
> + delta <<= 9;
> + else
> + delta *= ctx->fs->blocksize;
> + quota_data_add(ctx->qctx, &inode, ino, delta);
> + }
> +
> #if defined(DEBUG) || defined(DEBUG_SUMMARY)
> printf("rebuild: ino=%d extents=%d->%d\n", ino, list->ext_read,
> ext_written);
> #endif
> - e2fsck_write_inode(ctx, ino, &inode, "rebuild_extents");
> + e2fsck_write_inode(ctx, ino, EXT2_INODE(&inode), "rebuild_extents");
>
> err2:
> ext2fs_extent_free(handle);
> --
> 2.11.0.rc0.7.gbe5a750
>
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
