Message ID | 20170421200600.zvgjbblbrftvlfb4@thunk.org |
---|---|
State | Accepted, archived |
Headers | show |
On Apr 21, 2017, at 2:06 PM, Theodore Ts'o <tytso@mit.edu> wrote: > > From bc177d425d4fe33b0ae774b218b055465a840ca1 Mon Sep 17 00:00:00 2001 > From: Theodore Ts'o <tytso@mit.edu> > 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@mit.edu> > --- > 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
On Fri, Apr 21, 2017 at 03:15:07PM -0600, Andreas Dilger wrote: > > 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? The blocks are already freed at this point. > 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). We're actually not modifying the quota file at this point; we're just modifying the in-memory "correct" quota data numbers. Also, if we fail in the middle, given that the extent tree blocks have been freed and potentially partially reused, the fact that the quota file has been modified is the least of our problems. In theory we could save the list of old extent tree index blocks, and try to use new blocks for the new extent tree index blocks as much as possible. But that's not true today. - Ted
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"); /* 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); + 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)); 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);