Re: ext4_fiemap gives 0 extents for files smaller than a block (patch included)
Leonard Michlmayr
> >
> > @@ -3700,7 +3701,8 @@
> > start_blk = start >> inode->i_sb->s_blocksize_bits;
> > - len_blks = len >> inode->i_sb->s_blocksize_bits;
> > + end_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
> > + len_blks = end_blk - start_blk + 1;
>
> I don't think this is quite correct either. For example, if blocksize
> is 1024
> and start is 1023 (start_blk = 0) and len is 2 (end = 1024, end_blk =
> 1) then
> len_blks = 2 which is too much.
I think that len_blks = 2 is the correct value, because the requested
region extends into 2 blocks (namely 0 and 1). If both blocks are in two
separate extents, then ext4_ext_walk_space should report 2 extents. (If
it's the same extent, only 1 will be reported anyways)
> I think the right calculation here is:
>
> end_blk = (start + len + inode->i_sb->s_blocksize - 1) >>
> inode->i_sb->s_blocksize_bits;
> len_blks = end_blk - start_blk;
>
This is exactly the same (provided that len > 0). You can convince
yourself easily that ((blocksize + x) >> blocksize_bits == x >>
blocksize_bits + 1) for any positive x, because the lower bits of
blocksize are all 0. (Your calculation would handle the case len == 0
right, if that was allowed.)
Regards
Leonard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Leonard Michlmayr
(fm_start=0, fm_length=filesize)
File affected: fs/ext4/extents.c
I found the reason of the bug: wrong rounding. It will not only affect
small files, but any request that overlaps an extent boundary by less
that blocksize.
The attached patch is against 2.6.32-rc5.
Leonard Michlmayr
Andreas Dilger
> Fiemap (ioctl) does not return any extents for small files on ext4.
> (fm_start=0, fm_length=filesize)
>
> File affected: fs/ext4/extents.c
>
> I found the reason of the bug: wrong rounding. It will not only affect
> small files, but any request that overlaps an extent boundary by less
> that blocksize.
>
> @@ -3700,7 +3701,8 @@
> start_blk = start >> inode->i_sb->s_blocksize_bits;
> - len_blks = len >> inode->i_sb->s_blocksize_bits;
> + end_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
> + len_blks = end_blk - start_blk + 1;
I don't think this is quite correct either. For example, if blocksize
is 1024
and start is 1023 (start_blk = 0) and len is 2 (end = 1024, end_blk =
1) then
len_blks = 2 which is too much.
I think the right calculation here is:
end_blk = (start + len + inode->i_sb->s_blocksize -
1) >>
inode->i_sb->s_blocksize_bits;
len_blks = end_blk - start_blk;
I'm also wondering (unrelated to this bug) why inode->i_sb-
>s_blocksize_bits
is used instead of inode->i_blkbits? That is probably worth a
separate cleanup
patch.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
Leonard Michlmayr
Will you apply the patch? Do you not plan to apply the patch?
Do you need an update of the patch?
Regards
Leonard
...
Surbhi Palande
I was wondering if there is any update on the status of this patch.
Thanks !
Warm Regards,
Surbhi.
Leonard Michlmayr
fs/ext4/extents.c:ext4_fiemap rounds the length of the requested range
down to blocksize. This is not the true number of blocks that cover the
requested region. This problem is especially impressive if the user
requests only the first byte of a file: not a single extent will be
reported.
Solution:
Calculate the last byte of the region and round to blocksize. Then get
the number of blocks by subtracting last_blk - start_blk and adding 1
for the first block. (The variable last_blk is introduced just for
easier reading.) This patch will fix this.
I already suggested this patch some time ago, this is the same patch for
a more recent kernel version.
Signed-off-by: Leonard Michlmayr <leonard....@gmail.com>
diff -rup linux-2.6.32.7/fs/ext4/extents.c linux-2.6.32.7-lm/fs/ext4/extents.c
--- linux-2.6.32.7/fs/ext4/extents.c 2010-01-29 00:06:20.000000000 +0100
+++ linux-2.6.32.7-lm/fs/ext4/extents.c 2010-02-11 21:26:40.000000000 +0100
@@ -3711,6 +3711,7 @@ int ext4_fiemap(struct inode *inode, str
__u64 start, __u64 len)
{
ext4_lblk_t start_blk;
+ ext4_lblk_t last_blk;
ext4_lblk_t len_blks;
int error = 0;
@@ -3726,7 +3727,9 @@ int ext4_fiemap(struct inode *inode, str
error = ext4_xattr_fiemap(inode, fieinfo);
} else {
start_blk = start >> inode->i_sb->s_blocksize_bits;
- len_blks = len >> inode->i_sb->s_blocksize_bits;
+ /* the last byte in the range is (start + len - 1) */
+ last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
+ len_blks = last_blk - start_blk + 1;
/*
* Walk the extent tree gathering extent information.
Andreas Dilger
If "last_blk" is only used in this one place, please put the
declaration inside the scope of the "else" clause. Looks fine
otherwise.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
--
Leonard Michlmayr
> If "last_blk" is only used in this one place, please put the
> declaration inside the scope of the "else" clause. Looks fine
> otherwise.
Done. Thanks.
---
Problem:
fs/ext4/extents.c:ext4_fiemap rounds the length of the requested range
down to blocksize. This is not the true number of blocks that cover the
requested region. This problem is especially impressive if the user
requests only the first byte of a file: not a single extent will be
reported.
Solution:
Calculate the last byte of the region and round to blocksize. Then get
the number of blocks by subtracting last_blk - start_blk and adding 1
for the first block. (The variable last_blk is introduced just for
easier reading.) This patch will fix this.
Signed-off-by: Leonard Michlmayr <leonard....@gmail.com>
diff -rup linux-2.6.32.7/fs/ext4/extents.c linux-2.6.32.7-lm/fs/ext4/extents.c
--- linux-2.6.32.7/fs/ext4/extents.c 2010-01-29 00:06:20.000000000 +0100
+++ linux-2.6.32.7-lm/fs/ext4/extents.c 2010-02-13 00:01:56.000000000 +0100
@@ -3725,8 +3725,11 @@ int ext4_fiemap(struct inode *inode, str
if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
error = ext4_xattr_fiemap(inode, fieinfo);
} else {
+ ext4_lblk_t last_blk;
start_blk = start >> inode->i_sb->s_blocksize_bits;
- len_blks = len >> inode->i_sb->s_blocksize_bits;
+ /* the last byte in the range is (start + len - 1) */
+ last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
+ len_blks = last_blk - start_blk + 1;
/*
* Walk the extent tree gathering extent information.
ty...@mit.edu
added to the ext4 patch queue.
- Ted
commit 7f9aeb212e41ea20f61061d51ad11ee2449853f1
Author: Theodore Ts'o <ty...@mit.edu>
Date: Mon Feb 15 14:48:32 2010 -0500
ext4: correctly calculate number of blocks for fiemap
ext4_fiemap() rounds the length of the requested range down to
blocksize, which is is not the true number of blocks that cover the
requested region. This problem is especially impressive if the user
requests only the first byte of a file: not a single extent will be
reported.
We fix this by calculating the last block of the region and then
subtract to find the number of blocks in the extents.
Signed-off-by: Leonard Michlmayr <leonard....@gmail.com>
Signed-off-by: "Theodore Ts'o" <ty...@mit.edu>
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index bd80891..bc9860f 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3768,7 +3768,6 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
__u64 start, __u64 len)
{
ext4_lblk_t start_blk;
- ext4_lblk_t len_blks;
int error = 0;
/* fallback to generic here if not in extents fmt */
@@ -3782,8 +3781,11 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
error = ext4_xattr_fiemap(inode, fieinfo);
} else {
+ ext4_lblk_t last_blk, len_blks;
+
start_blk = start >> inode->i_sb->s_blocksize_bits;
- len_blks = len >> inode->i_sb->s_blocksize_bits;
Eric Sandeen
> Sorry for the delay in attending to this patch. This is what I've
> added to the ext4 patch queue.
>
> - Ted
>
> commit 7f9aeb212e41ea20f61061d51ad11ee2449853f1
> Author: Theodore Ts'o <ty...@mit.edu>
> Date: Mon Feb 15 14:48:32 2010 -0500
Ted, it looks like perhaps you accidentally reassigned authorship for
this patch?
-Eric
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
ty...@mit.edu
> ty...@mit.edu wrote:
> > Sorry for the delay in attending to this patch. This is what I've
> > added to the ext4 patch queue.
> >
> > - Ted
> >
> > commit 7f9aeb212e41ea20f61061d51ad11ee2449853f1
> > Author: Theodore Ts'o <ty...@mit.edu>
> > Date: Mon Feb 15 14:48:32 2010 -0500
>
> Ted, it looks like perhaps you accidentally reassigned authorship for
> this patch?
Oops, I forgot the fix the ownership when I applied the patch. Fixed.
- Ted