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.
The attached patch is against 2.6.32-rc5.
Leonard Michlmayr
On 2009-11-04, at 11:42, Leonard Michlmayr wrote:
> 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.
Thank you for your reply.
> >
> > @@ -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
Do you have any news on this bug?
Will you apply the patch? Do you not plan to apply the patch?
Do you need an update of the patch?
Regards
Leonard
Am Mittwoch, den 04.11.2009, 12:44 -0700 schrieb Andreas Dilger:
> On 2009-11-04, at 11:42, Leonard Michlmayr wrote:
> > 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.
...
Leonard Michlmayr wrote:
> Thank you for your reply.
>
>>> @@ -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
I was wondering if there is any update on the status of this patch.
Thanks !
Warm Regards,
Surbhi.
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.
I already suggested this patch some time ago, this is the same patch for
a more recent kernel version.
Signed-off-by: Leonard Michlmayr <[email protected]>
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.
On 2010-02-12, at 11:42, Leonard Michlmayr wrote:
> 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 <[email protected]>
>
> 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;
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.
On 2010-02-12, at 14:49 -0700 Andreas Dilger wrote:
> 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 <[email protected]>
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.
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 <[email protected]>
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 <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
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;
+ 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.
[email protected] 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 <[email protected]>
> Date: Mon Feb 15 14:48:32 2010 -0500
Ted, it looks like perhaps you accidentally reassigned authorship for
this patch?
-Eric
> 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 <[email protected]>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
>
> 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;
> + 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.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 15, 2010 at 02:33:03PM -0600, Eric Sandeen wrote:
> [email protected] 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 <[email protected]>
> > 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