From: Wang Shilong <[email protected]>
According to following logic:
"If the huge_file feature flag is not set on the filesystem,
the file consumes i_blocks_lo 512-byte blocks on disk.
If huge_file is set and EXT4_HUGE_FILE_FL is NOT set in
inode.i_flags, then the file consumes i_blocks_lo + (i_blocks_hi << 32)
512-byte blocks on disk. If huge_file is set and EXT4_HUGE_FILE_FL IS set
in inode.i_flags, then this file consumes (i_blocks_lo + i_blocks_hi << 32)
filesystem blocks on disk."
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)
Signed-off-by: Wang Shilong <[email protected]>
---
debugfs/filefrag.c | 4 ----
e2fsck/extents.c | 6 +-----
lib/ext2fs/blknum.c | 13 ++++++++++---
lib/support/mkquota.c | 3 ++-
misc/fuse2fs.c | 19 +------------------
5 files changed, 14 insertions(+), 31 deletions(-)
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);
--
2.21.0
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 <[email protected]> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
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 <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
Reviewed-by: Wang Shilong <[email protected]>
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);