| Message ID | 1577705766-20736-1-git-send-email-wangshilong1991@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | e2fsprogs: fix to use inode i_blocks correctly | expand |
On Mon, Dec 30, 2019 at 08:36:06PM +0900, Wang Shilong wrote: > blocks_from_inode() did not return wrong inode blocks, and > ext2fs_inode_i_blocks() is not taking EXT4_HUGE_FILE_FL into account > at all, while the some callers deal it correctly, some not. This patch > try to unify to handle it in ext2fs_inode_i_blocks() to return. > blocks(based on 512 bytes) I don't really want to change the functionality of ext2fs_inode_i_blocks(). First of all, it's in a shared library, so if there are any binaries which were expecting the old behavior, that could get surprising. Secondly, its name is confusing and so we're better off creating a new function ext2fs_get_stat_i_blocks() which makes it clear that we function is using units of 512 byte sectors, instead of either the file system block size, or the raw i_blocks value from the inode. Two other things about your patch. First of all, the filefrag command in debugfs was intended to print the number of file system blocks, so it was correct as written. Secondly, please note the blkcnt_t is a signed type (because the block iterator functions use negative values to indicate various kinds of metadata blocks), while blk64_t is an unsigned type. So using blkcnt_t as a temporary value and returning it in a function which has a return type of blk64_t will (righly) trigger compiler warnings. Here's the patch I've checked into the maint branch of e2fsprogs to address the issue you've identified. - Ted
On Dec 30, 2019, at 8:19 AM, Theodore Y. Ts'o <tytso@mit.edu> wrote: > > On Mon, Dec 30, 2019 at 08:36:06PM +0900, Wang Shilong wrote: >> blocks_from_inode() did not return wrong inode blocks, and >> ext2fs_inode_i_blocks() is not taking EXT4_HUGE_FILE_FL into account >> at all, while the some callers deal it correctly, some not. This patch >> try to unify to handle it in ext2fs_inode_i_blocks() to return. >> blocks(based on 512 bytes) > > I don't really want to change the functionality of > ext2fs_inode_i_blocks(). First of all, it's in a shared library, so > if there are any binaries which were expecting the old behavior, that > could get surprising. Secondly, its name is confusing and so we're > better off creating a new function ext2fs_get_stat_i_blocks() which > makes it clear that we function is using units of 512 byte sectors, > instead of either the file system block size, or the raw i_blocks > value from the inode. > > Two other things about your patch. First of all, the filefrag command > in debugfs was intended to print the number of file system blocks, so > it was correct as written. Secondly, please note the blkcnt_t is a > signed type (because the block iterator functions use negative values > to indicate various kinds of metadata blocks), while blk64_t is an > unsigned type. So using blkcnt_t as a temporary value and returning > it in a function which has a return type of blk64_t will (righly) > trigger compiler warnings. > > Here's the patch I've checked into the maint branch of e2fsprogs to > address the issue you've identified. No patch is attached? Cheers, Andreas
On Mon, Dec 30, 2019 at 11:37:40AM -0700, Andreas Dilger wrote: > > No patch is attached? Oops, here you go. - Ted commit c90cea86eeef89f29f7bd5535fbaa5809a812cc7 Author: Theodore Ts'o <tytso@mit.edu> Date: Mon Dec 30 10:12:58 2019 -0500 ext2fs: add ext2fs_get_stat_i_blocks() function The function ext2fs_inode_i_blocks() is a bit confusing whether it is returning the inode's i_blocks value, or whether it is returning the value ala the stat(2) system call, which returns i_blocks in units of 512 byte sectors. This caused ext2fs_inode_i_blocks() to be incorrectly used in fuse2fs and the function quota_compute_usage(). To address this, we add a new function, ext2fs_get_stat_i_blocks() which is clearly labelled what it is returning, and use it in fuse2fs and quota_compute_usage(). It's also a bit more convenient to use it in e2fsck, so use it there too. Reported-by: Wang Shilong <wangshilong1991@gmail.com> Signed-off-by: Theodore Ts'o <tytso@mit.edu> diff --git a/e2fsck/extents.c b/e2fsck/extents.c index 3073725a..e9af1bbe 100644 --- a/e2fsck/extents.c +++ b/e2fsck/extents.c @@ -264,7 +264,7 @@ extents_loaded: goto err; ext_written = 0; - start_val = ext2fs_inode_i_blocks(ctx->fs, EXT2_INODE(&inode)); + start_val = ext2fs_get_stat_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; @@ -302,15 +302,10 @@ 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); - } + delta = ext2fs_get_stat_i_blocks(ctx->fs, EXT2_INODE(&inode)) - + start_val; + if (delta) + quota_data_add(ctx->qctx, &inode, ino, delta << 9); #if defined(DEBUG) || defined(DEBUG_SUMMARY) printf("rebuild: ino=%d extents=%d->%d\n", ino, list->ext_read, diff --git a/lib/ext2fs/blknum.c b/lib/ext2fs/blknum.c index 9ee5c66e..31055c34 100644 --- a/lib/ext2fs/blknum.c +++ b/lib/ext2fs/blknum.c @@ -85,6 +85,22 @@ blk64_t ext2fs_inode_i_blocks(ext2_filsys fs, (__u64)inode->osd2.linux2.l_i_blocks_hi << 32 : 0)); } +/* + * Return the inode i_blocks in stat (512 byte) units + */ +blk64_t ext2fs_get_stat_i_blocks(ext2_filsys fs, + struct ext2_inode *inode) +{ + blk64_t ret = inode->i_blocks; + + if (ext2fs_has_feature_huge_file(fs->super)) { + ret += ((long long) inode->osd2.linux2.l_i_blocks_hi) << 32; + if (inode->i_flags & EXT4_HUGE_FILE_FL) + ret *= (fs->blocksize / 512); + } + return ret; +} + /* * Return the fs block count */ diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h index 59fd9742..ca5e3321 100644 --- a/lib/ext2fs/ext2fs.h +++ b/lib/ext2fs/ext2fs.h @@ -908,7 +908,9 @@ extern int ext2fs_group_blocks_count(ext2_filsys fs, dgrp_t group); extern blk64_t ext2fs_inode_data_blocks2(ext2_filsys fs, struct ext2_inode *inode); extern blk64_t ext2fs_inode_i_blocks(ext2_filsys fs, - struct ext2_inode *inode); + struct ext2_inode *inode); +extern blk64_t ext2fs_get_stat_i_blocks(ext2_filsys fs, + struct ext2_inode *inode); extern blk64_t ext2fs_blocks_count(struct ext2_super_block *super); extern void ext2fs_blocks_count_set(struct ext2_super_block *super, blk64_t blk); diff --git a/lib/support/mkquota.c b/lib/support/mkquota.c index ddb53124..6f7ae6d6 100644 --- a/lib/support/mkquota.c +++ b/lib/support/mkquota.c @@ -503,8 +503,8 @@ errcode_t quota_compute_usage(quota_ctx_t qctx) if (inode->i_links_count && (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(fs->super))) { - space = ext2fs_inode_i_blocks(fs, - EXT2_INODE(inode)) << 9; + space = ext2fs_get_stat_i_blocks(fs, + EXT2_INODE(inode)) << 9; quota_data_add(qctx, inode, ino, space); quota_data_inodes(qctx, inode, ino, +1); } diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c index 94cd5f67..2cfc6af3 100644 --- a/misc/fuse2fs.c +++ b/misc/fuse2fs.c @@ -754,23 +754,6 @@ static void *op_init(struct fuse_conn_info *conn) return ff; } -static blkcnt_t blocks_from_inode(ext2_filsys fs, - struct ext2_inode_large *inode) -{ - blkcnt_t b; - - b = inode->i_blocks; - if (ext2fs_has_feature_huge_file(fs->super)) - b += ((long long) inode->osd2.linux2.l_i_blocks_hi) << 32; - - if (!ext2fs_has_feature_huge_file(fs->super) || - !(inode->i_flags & EXT4_HUGE_FILE_FL)) - b *= fs->blocksize / 512; - b *= EXT2FS_CLUSTER_RATIO(fs); - - return b; -} - static int stat_inode(ext2_filsys fs, ext2_ino_t ino, struct stat *statbuf) { struct ext2_inode_large inode; @@ -794,7 +777,7 @@ static int stat_inode(ext2_filsys fs, ext2_ino_t ino, struct stat *statbuf) statbuf->st_gid = inode_gid(inode); statbuf->st_size = EXT2_I_SIZE(&inode); statbuf->st_blksize = fs->blocksize; - statbuf->st_blocks = blocks_from_inode(fs, &inode); + statbuf->st_blocks = ext2fs_get_stat_i_blocks(fs, &inode); EXT4_INODE_GET_XTIME(i_atime, &tv, &inode); statbuf->st_atime = tv.tv_sec; EXT4_INODE_GET_XTIME(i_mtime, &tv, &inode);
On Tue, Dec 31, 2019 at 3:52 AM Theodore Y. Ts'o <tytso@mit.edu> wrote: > > On Mon, Dec 30, 2019 at 11:37:40AM -0700, Andreas Dilger wrote: > > > > No patch is attached? > > Oops, here you go. > > - Ted > > commit c90cea86eeef89f29f7bd5535fbaa5809a812cc7 > Author: Theodore Ts'o <tytso@mit.edu> > Date: Mon Dec 30 10:12:58 2019 -0500 > > ext2fs: add ext2fs_get_stat_i_blocks() function > > The function ext2fs_inode_i_blocks() is a bit confusing whether it is > returning the inode's i_blocks value, or whether it is returning the > value ala the stat(2) system call, which returns i_blocks in units of > 512 byte sectors. This caused ext2fs_inode_i_blocks() to be > incorrectly used in fuse2fs and the function quota_compute_usage(). > > To address this, we add a new function, ext2fs_get_stat_i_blocks() > which is clearly labelled what it is returning, and use it in fuse2fs > and quota_compute_usage(). It's also a bit more convenient to use it > in e2fsck, so use it there too. > > Reported-by: Wang Shilong <wangshilong1991@gmail.com> > Signed-off-by: Theodore Ts'o <tytso@mit.edu> Reviewed-by: Wang Shilong <wangshilong1991@gmail.com> Thanks! Shilong > > diff --git a/e2fsck/extents.c b/e2fsck/extents.c > index 3073725a..e9af1bbe 100644 > --- a/e2fsck/extents.c > +++ b/e2fsck/extents.c > @@ -264,7 +264,7 @@ extents_loaded: > goto err; > > ext_written = 0; > - start_val = ext2fs_inode_i_blocks(ctx->fs, EXT2_INODE(&inode)); > + start_val = ext2fs_get_stat_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; > @@ -302,15 +302,10 @@ 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); > - } > + delta = ext2fs_get_stat_i_blocks(ctx->fs, EXT2_INODE(&inode)) - > + start_val; > + if (delta) > + quota_data_add(ctx->qctx, &inode, ino, delta << 9); > > #if defined(DEBUG) || defined(DEBUG_SUMMARY) > printf("rebuild: ino=%d extents=%d->%d\n", ino, list->ext_read, > diff --git a/lib/ext2fs/blknum.c b/lib/ext2fs/blknum.c > index 9ee5c66e..31055c34 100644 > --- a/lib/ext2fs/blknum.c > +++ b/lib/ext2fs/blknum.c > @@ -85,6 +85,22 @@ blk64_t ext2fs_inode_i_blocks(ext2_filsys fs, > (__u64)inode->osd2.linux2.l_i_blocks_hi << 32 : 0)); > } > > +/* > + * Return the inode i_blocks in stat (512 byte) units > + */ > +blk64_t ext2fs_get_stat_i_blocks(ext2_filsys fs, > + struct ext2_inode *inode) > +{ > + blk64_t ret = inode->i_blocks; > + > + if (ext2fs_has_feature_huge_file(fs->super)) { > + ret += ((long long) inode->osd2.linux2.l_i_blocks_hi) << 32; > + if (inode->i_flags & EXT4_HUGE_FILE_FL) > + ret *= (fs->blocksize / 512); > + } > + return ret; > +} > + > /* > * Return the fs block count > */ > diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h > index 59fd9742..ca5e3321 100644 > --- a/lib/ext2fs/ext2fs.h > +++ b/lib/ext2fs/ext2fs.h > @@ -908,7 +908,9 @@ extern int ext2fs_group_blocks_count(ext2_filsys fs, dgrp_t group); > extern blk64_t ext2fs_inode_data_blocks2(ext2_filsys fs, > struct ext2_inode *inode); > extern blk64_t ext2fs_inode_i_blocks(ext2_filsys fs, > - struct ext2_inode *inode); > + struct ext2_inode *inode); > +extern blk64_t ext2fs_get_stat_i_blocks(ext2_filsys fs, > + struct ext2_inode *inode); > extern blk64_t ext2fs_blocks_count(struct ext2_super_block *super); > extern void ext2fs_blocks_count_set(struct ext2_super_block *super, > blk64_t blk); > diff --git a/lib/support/mkquota.c b/lib/support/mkquota.c > index ddb53124..6f7ae6d6 100644 > --- a/lib/support/mkquota.c > +++ b/lib/support/mkquota.c > @@ -503,8 +503,8 @@ errcode_t quota_compute_usage(quota_ctx_t qctx) > if (inode->i_links_count && > (ino == EXT2_ROOT_INO || > ino >= EXT2_FIRST_INODE(fs->super))) { > - space = ext2fs_inode_i_blocks(fs, > - EXT2_INODE(inode)) << 9; > + space = ext2fs_get_stat_i_blocks(fs, > + EXT2_INODE(inode)) << 9; > quota_data_add(qctx, inode, ino, space); > quota_data_inodes(qctx, inode, ino, +1); > } > diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c > index 94cd5f67..2cfc6af3 100644 > --- a/misc/fuse2fs.c > +++ b/misc/fuse2fs.c > @@ -754,23 +754,6 @@ static void *op_init(struct fuse_conn_info *conn) > return ff; > } > > -static blkcnt_t blocks_from_inode(ext2_filsys fs, > - struct ext2_inode_large *inode) > -{ > - blkcnt_t b; > - > - b = inode->i_blocks; > - if (ext2fs_has_feature_huge_file(fs->super)) > - b += ((long long) inode->osd2.linux2.l_i_blocks_hi) << 32; > - > - if (!ext2fs_has_feature_huge_file(fs->super) || > - !(inode->i_flags & EXT4_HUGE_FILE_FL)) > - b *= fs->blocksize / 512; > - b *= EXT2FS_CLUSTER_RATIO(fs); > - > - return b; > -} > - > static int stat_inode(ext2_filsys fs, ext2_ino_t ino, struct stat *statbuf) > { > struct ext2_inode_large inode; > @@ -794,7 +777,7 @@ static int stat_inode(ext2_filsys fs, ext2_ino_t ino, struct stat *statbuf) > statbuf->st_gid = inode_gid(inode); > statbuf->st_size = EXT2_I_SIZE(&inode); > statbuf->st_blksize = fs->blocksize; > - statbuf->st_blocks = blocks_from_inode(fs, &inode); > + statbuf->st_blocks = ext2fs_get_stat_i_blocks(fs, &inode); > EXT4_INODE_GET_XTIME(i_atime, &tv, &inode); > statbuf->st_atime = tv.tv_sec; > EXT4_INODE_GET_XTIME(i_mtime, &tv, &inode);
diff --git a/debugfs/filefrag.c b/debugfs/filefrag.c index 961b6962..3e818b19 100644 --- a/debugfs/filefrag.c +++ b/debugfs/filefrag.c @@ -145,10 +145,6 @@ static void filefrag(ext2_ino_t ino, struct ext2_inode *inode, if (fs->options & VERBOSE_OPT) { blk64_t num_blocks = ext2fs_inode_i_blocks(current_fs, inode); - if (!ext2fs_has_feature_huge_file(current_fs->super) || - !(inode->i_flags & EXT4_HUGE_FILE_FL)) - num_blocks /= current_fs->blocksize / 512; - fprintf(fs->f, "\n%s has %llu block(s), i_size is %llu\n", fs->name, num_blocks, EXT2_I_SIZE(inode)); } diff --git a/e2fsck/extents.c b/e2fsck/extents.c index 3073725a..d9509297 100644 --- a/e2fsck/extents.c +++ b/e2fsck/extents.c @@ -304,11 +304,7 @@ extents_loaded: 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; + delta *= 512; quota_data_add(ctx->qctx, &inode, ino, delta); } diff --git a/lib/ext2fs/blknum.c b/lib/ext2fs/blknum.c index 18af3408..6ab843c4 100644 --- a/lib/ext2fs/blknum.c +++ b/lib/ext2fs/blknum.c @@ -80,9 +80,16 @@ blk64_t ext2fs_inode_data_blocks2(ext2_filsys fs, blk64_t ext2fs_inode_i_blocks(ext2_filsys fs, struct ext2_inode *inode) { - return (inode->i_blocks | - (ext2fs_has_feature_huge_file(fs->super) ? - (__u64)inode->osd2.linux2.l_i_blocks_hi << 32 : 0)); + blkcnt_t i_blocks = inode->i_blocks; + + if (ext2fs_has_feature_huge_file(fs->super)) { + i_blocks += ((long long) inode->osd2.linux2.l_i_blocks_hi) << 32; + if (inode->i_flags & EXT4_HUGE_FILE_FL) + i_blocks *= (fs->blocksize / 512); + + } + + return i_blocks; } /* diff --git a/lib/support/mkquota.c b/lib/support/mkquota.c index ddb53124..2b0c6fb5 100644 --- a/lib/support/mkquota.c +++ b/lib/support/mkquota.c @@ -504,7 +504,8 @@ errcode_t quota_compute_usage(quota_ctx_t qctx) (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(fs->super))) { space = ext2fs_inode_i_blocks(fs, - EXT2_INODE(inode)) << 9; + EXT2_INODE(inode)); + space *= 512;; quota_data_add(qctx, inode, ino, space); quota_data_inodes(qctx, inode, ino, +1); } diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c index dc7a0392..748e3f7c 100644 --- a/misc/fuse2fs.c +++ b/misc/fuse2fs.c @@ -755,23 +755,6 @@ static void *op_init(struct fuse_conn_info *conn) return ff; } -static blkcnt_t blocks_from_inode(ext2_filsys fs, - struct ext2_inode_large *inode) -{ - blkcnt_t b; - - b = inode->i_blocks; - if (ext2fs_has_feature_huge_file(fs->super)) - b += ((long long) inode->osd2.linux2.l_i_blocks_hi) << 32; - - if (!ext2fs_has_feature_huge_file(fs->super) || - !(inode->i_flags & EXT4_HUGE_FILE_FL)) - b *= fs->blocksize / 512; - b *= EXT2FS_CLUSTER_RATIO(fs); - - return b; -} - static int stat_inode(ext2_filsys fs, ext2_ino_t ino, struct stat *statbuf) { struct ext2_inode_large inode; @@ -795,7 +778,7 @@ static int stat_inode(ext2_filsys fs, ext2_ino_t ino, struct stat *statbuf) statbuf->st_gid = inode.i_gid; statbuf->st_size = EXT2_I_SIZE(&inode); statbuf->st_blksize = fs->blocksize; - statbuf->st_blocks = blocks_from_inode(fs, &inode); + statbuf->st_blocks = ext2fs_inode_i_blocks(fs, &inode); EXT4_INODE_GET_XTIME(i_atime, &tv, &inode); statbuf->st_atime = tv.tv_sec; EXT4_INODE_GET_XTIME(i_mtime, &tv, &inode);