ext4_fiemap gives 0 extents for files smaller than a block (patch included)

ext4_fiemap gives 0 extents for files smaller than a block (patch included)

[Leonard Michlmayr]

[-- Attachment #1: Type: text/plain, Size: 356 bytes --]

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


[-- Attachment #2: ext4_fiemap_fix.patch --]
[-- Type: text/x-patch, Size: 745 bytes --]

diff -Naur linux-2.6.32-rc5/fs/ext4/extents.c linux-2.6.32-rc5.patched/fs/ext4/extents.c
--- linux-2.6.32-rc5/fs/ext4/extents.c	2009-10-16 02:41:50.000000000 +0200
+++ linux-2.6.32-rc5.patched/fs/ext4/extents.c	2009-11-04 19:35:44.000000000 +0100
@@ -3685,6 +3685,7 @@
 		__u64 start, __u64 len)
 {
 	ext4_lblk_t start_blk;
+	ext4_lblk_t end_blk;
 	ext4_lblk_t len_blks;
 	int error = 0;
 
@@ -3700,7 +3701,8 @@
 		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;
+		end_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
+		len_blks = end_blk - start_blk + 1;
 
 		/*
 		 * Walk the extent tree gathering extent information.

Re: ext4_fiemap gives 0 extents for files smaller than a block (patch included)

[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.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: ext4_fiemap gives 0 extents for files smaller than a block (patch included)

[Leonard Michlmayr]

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

Re: ext4_fiemap gives 0 extents for files smaller than a block (patch included)

[Surbhi Palande]

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.

[PATCH 2.6.32.7] ext4: number of blocks for fiemap

[Leonard Michlmayr]

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 <leonard.michlmayr@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.

Re: [PATCH 2.6.32.7] ext4: number of blocks for fiemap

[Andreas Dilger]

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 <leonard.michlmayr@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;


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.

[PATCH 2.6.32.7] ext4: number of blocks for fiemap

[Leonard Michlmayr]

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 <leonard.michlmayr@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.

Re: [PATCH 2.6.32.7] ext4: number of blocks for fiemap

[tytso]

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 <tytso@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.michlmayr@gmail.com>
    Signed-off-by: "Theodore Ts'o" <tytso@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;
+		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.

Re: [PATCH 2.6.32.7] ext4: number of blocks for fiemap

[Eric Sandeen]

tytso@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 <tytso@mit.edu>
> 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 <leonard.michlmayr@gmail.com>
>     Signed-off-by: "Theodore Ts'o" <tytso@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;
> +		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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2.6.32.7] ext4: number of blocks for fiemap

[tytso]

On Mon, Feb 15, 2010 at 02:33:03PM -0600, Eric Sandeen wrote:
> tytso@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 <tytso@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

Re: ext4_fiemap gives 0 extents for files smaller than a block (patch included)

[Leonard Michlmayr]

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.
...