All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
@ 2010-03-05  5:57 Akira Fujita
  0 siblings, 0 replies; 11+ messages in thread
From: Akira Fujita @ 2010-03-05  5:57 UTC (permalink / raw)
  Cc: ext4 development

Hi Ted,

> commit aca92ff6f57c000d1b4523e383c8bd6b8269b8b1
> Author: Leonard Michlmayr<leonard.michlmayr@gmail.com>
> Date:   Thu Mar 4 17:07:28 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..7d54850 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,14 @@ 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 len_blks;
> +        __u64 last_blk;
> +
>           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;
> +        if (last_blk>= EXT_MAX_BLOCK)
> +            last_blk = EXT_MAX_BLOCK-1;

               last_blk = EXT_MAX_BLOCK - 1;

You seem to have already pushed this patch to Linus, though.
There is a coding style thing.
Anyway, the patch works fine for me.
Thanks for your work.  :-)

Tested-by: Akira Fujita <a-fujita@rs.jp.nec.com>

Regards,
Akira Fujita

> +        len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1;
>
>           /*
>            * Walk the extent tree gathering extent information.
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
  2010-03-04 23:38           ` Eric Sandeen
@ 2010-03-05 16:46             ` Leonard Michlmayr
  0 siblings, 0 replies; 11+ messages in thread
From: Leonard Michlmayr @ 2010-03-05 16:46 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Ext4 Developers List

2010/3/5 Eric Sandeen <sandeen@redhat.com>
>
> > Thank you for pointing this out. I had not checked s_maxbytes.
> > Appearently s_maxbytes can be 1<<(32 + s_blocksize_bits) - 1. Therefore
> > the number of blocks in a file cannot be stored in a 32bit integer.
>
> For extent-based files it had better be....
>
> struct ext4_extent {
>        __le32  ee_block;       /* first logical block extent covers */
>        __le16  ee_len;         /* number of blocks covered by extent */
>
> The start block can't be more than 32 bits; this essentially limits
> the file size / maximum logical block to 2^32 blocks right?
>
> s_maxbytes comes out to 17592186044415
>
> 2^32 4k blocks is 17592186044416 bytes, or max byte offset 17592186044415
>
> What am I missing?  (confusion between max byte count and max
> byte offset, perhaps?)
>

Yes. The block offset has the maximum value 1<<32 - 1.
However the number of blocks may be 1<<32.
--
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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
  2010-03-04 22:08         ` tytso
@ 2010-03-04 23:47           ` Eric Sandeen
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Sandeen @ 2010-03-04 23:47 UTC (permalink / raw)
  To: tytso; +Cc: Akira Fujita, Leonard Michlmayr, Ext4 Developers List

tytso@mit.edu wrote:
> On Thu, Mar 04, 2010 at 02:40:22PM +0900, Akira Fujita wrote:
>> <user-space>
>> filefrag:
>> 	fiemap->fm_start(0) fiemap->fm_length(~0ULL)
>>
>> <kernel-space>
>> fs/ioctl.c ioctl_fimap():
>>
>> filemap_check_ranges():
>> 	len(~0ULL)
>> 	new_len(4398046511103 = s_maxbytes)  <--- Because 'len > s_maxbytes'
>>
>> fs/ext4/extents.c ext4_fiemap():
>> 	last_blk = start(0) + len(4398046511103) - 1 >> s_blocksize_bits(11)
>> 	         = 4294967295 (0xFFFFFFFF)
>> 	len_blks = 4294967295 + 1 (0xFFFFFFFF + 0x00000001)
>> 		 = 4294967296 (0x100000000)  <--- _OVERFLOW!!_
>> 	
>> ext4_ext_walk_space():
>> 	num = 0
>>
>> This overflow leads to incorrect output like the below,
>> even though 2 extents exist.
> 
> Akira-san,
> 
> Thank you for your clear explanation; you're absolutely correct.  I've
> replaced the patch with the following, which I think is a bit clearer.
> I've tested to make sure it does the right thing, including a number
> of corner cases.

Ted, I think those testcases you used would be great xfstests candidates,
hint hint.  :)

-Eric

> Regards,
> 
> 					- Ted
> 
> commit aca92ff6f57c000d1b4523e383c8bd6b8269b8b1
> Author: Leonard Michlmayr <leonard.michlmayr@gmail.com>
> Date:   Thu Mar 4 17:07:28 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..7d54850 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,14 @@ 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 len_blks;
> +		__u64 last_blk;
> +
>  		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;
> +		if (last_blk >= EXT_MAX_BLOCK)
> +			last_blk = EXT_MAX_BLOCK-1;
> +		len_blks = ((ext4_lblk_t) 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
  2010-03-04 21:44         ` Leonard Michlmayr
@ 2010-03-04 23:38           ` Eric Sandeen
  2010-03-05 16:46             ` Leonard Michlmayr
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Sandeen @ 2010-03-04 23:38 UTC (permalink / raw)
  To: Leonard Michlmayr; +Cc: Akira Fujita, tytso, Ext4 Developers List

Leonard Michlmayr wrote:
> Akira Fujita:
>> <kernel-space>
>> fs/ioctl.c ioctl_fimap():
>>
>> filemap_check_ranges():
>> 	len(~0ULL)
>> 	new_len(4398046511103 = s_maxbytes)  <--- Because 'len > s_maxbytes'
>>
>> fs/ext4/extents.c ext4_fiemap():
>> 	last_blk = start(0) + len(4398046511103) - 1 >> s_blocksize_bits(11)
>> 	         = 4294967295 (0xFFFFFFFF)
>> 	len_blks = 4294967295 + 1 (0xFFFFFFFF + 0x00000001)
>> 		 = 4294967296 (0x100000000)  <--- _OVERFLOW!!_
>> 	
>> ext4_ext_walk_space():
>> 	num = 0
>>
>> This overflow leads to incorrect output like the below,
>> even though 2 extents exist.
>>
> 
> Thank you for pointing this out. I had not checked s_maxbytes.
> Appearently s_maxbytes can be 1<<(32 + s_blocksize_bits) - 1. Therefore
> the number of blocks in a file cannot be stored in a 32bit integer.

For extent-based files it had better be....

struct ext4_extent {
        __le32  ee_block;       /* first logical block extent covers */
        __le16  ee_len;         /* number of blocks covered by extent */

The start block can't be more than 32 bits; this essentially limits
the file size / maximum logical block to 2^32 blocks right?

s_maxbytes comes out to 17592186044415

2^32 4k blocks is 17592186044416 bytes, or max byte offset 17592186044415

What am I missing?  (confusion between max byte count and max
byte offset, perhaps?)

-Eric

> I have a patch that should fix it for fiemap. I have just compiled it
> and I will do some testing and double checking tomorrow. I will send a
> separate email with the patch.
> 
> regards
> Leonard
> 
> 
> --
> 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
  2010-03-04  5:40       ` Akira Fujita
  2010-03-04 21:44         ` Leonard Michlmayr
@ 2010-03-04 22:08         ` tytso
  2010-03-04 23:47           ` Eric Sandeen
  1 sibling, 1 reply; 11+ messages in thread
From: tytso @ 2010-03-04 22:08 UTC (permalink / raw)
  To: Akira Fujita; +Cc: Leonard Michlmayr, Ext4 Developers List

On Thu, Mar 04, 2010 at 02:40:22PM +0900, Akira Fujita wrote:
> <user-space>
> filefrag:
> 	fiemap->fm_start(0) fiemap->fm_length(~0ULL)
> 
> <kernel-space>
> fs/ioctl.c ioctl_fimap():
> 
> filemap_check_ranges():
> 	len(~0ULL)
> 	new_len(4398046511103 = s_maxbytes)  <--- Because 'len > s_maxbytes'
> 
> fs/ext4/extents.c ext4_fiemap():
> 	last_blk = start(0) + len(4398046511103) - 1 >> s_blocksize_bits(11)
> 	         = 4294967295 (0xFFFFFFFF)
> 	len_blks = 4294967295 + 1 (0xFFFFFFFF + 0x00000001)
> 		 = 4294967296 (0x100000000)  <--- _OVERFLOW!!_
> 	
> ext4_ext_walk_space():
> 	num = 0
> 
> This overflow leads to incorrect output like the below,
> even though 2 extents exist.

Akira-san,

Thank you for your clear explanation; you're absolutely correct.  I've
replaced the patch with the following, which I think is a bit clearer.
I've tested to make sure it does the right thing, including a number
of corner cases.

Regards,

					- Ted

commit aca92ff6f57c000d1b4523e383c8bd6b8269b8b1
Author: Leonard Michlmayr <leonard.michlmayr@gmail.com>
Date:   Thu Mar 4 17:07:28 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..7d54850 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,14 @@ 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 len_blks;
+		__u64 last_blk;
+
 		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;
+		if (last_blk >= EXT_MAX_BLOCK)
+			last_blk = EXT_MAX_BLOCK-1;
+		len_blks = ((ext4_lblk_t) last_blk) - start_blk + 1;
 
 		/*
 		 * Walk the extent tree gathering extent information.

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
  2010-03-04  5:40       ` Akira Fujita
@ 2010-03-04 21:44         ` Leonard Michlmayr
  2010-03-04 23:38           ` Eric Sandeen
  2010-03-04 22:08         ` tytso
  1 sibling, 1 reply; 11+ messages in thread
From: Leonard Michlmayr @ 2010-03-04 21:44 UTC (permalink / raw)
  To: Akira Fujita, tytso; +Cc: Ext4 Developers List

Akira Fujita:
> <kernel-space>
> fs/ioctl.c ioctl_fimap():
> 
> filemap_check_ranges():
> 	len(~0ULL)
> 	new_len(4398046511103 = s_maxbytes)  <--- Because 'len > s_maxbytes'
> 
> fs/ext4/extents.c ext4_fiemap():
> 	last_blk = start(0) + len(4398046511103) - 1 >> s_blocksize_bits(11)
> 	         = 4294967295 (0xFFFFFFFF)
> 	len_blks = 4294967295 + 1 (0xFFFFFFFF + 0x00000001)
> 		 = 4294967296 (0x100000000)  <--- _OVERFLOW!!_
> 	
> ext4_ext_walk_space():
> 	num = 0
> 
> This overflow leads to incorrect output like the below,
> even though 2 extents exist.
> 

Thank you for pointing this out. I had not checked s_maxbytes.
Appearently s_maxbytes can be 1<<(32 + s_blocksize_bits) - 1. Therefore
the number of blocks in a file cannot be stored in a 32bit integer.

I have a patch that should fix it for fiemap. I have just compiled it
and I will do some testing and double checking tomorrow. I will send a
separate email with the patch.

regards
Leonard



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
  2010-03-03 17:52     ` tytso
@ 2010-03-04  5:40       ` Akira Fujita
  2010-03-04 21:44         ` Leonard Michlmayr
  2010-03-04 22:08         ` tytso
  0 siblings, 2 replies; 11+ messages in thread
From: Akira Fujita @ 2010-03-04  5:40 UTC (permalink / raw)
  To: tytso, Leonard Michlmayr; +Cc: Ext4 Developers List


(2010/03/04 2:52), tytso@mit.edu wrote:
> On Wed, Mar 03, 2010 at 04:47:28PM +0900, Akira Fujita wrote:
>>
>> In 1KB block size, the overflow occurs at above line.
>> Since last_blk is set 0xffffffff when len is equal to s_maxbytes.
>> Therefore ext4_fiemap() can not get correct extent information
>> with 0 length.  How about adding this change?
> 
> What do you think _is_ the correct thing to do when length is 0?  As
> Leonard has already pointed out, fiemap_check_ranges() already filters
> out the length=0 case.

/mnt/mp1/file1
 ext logical physical expected length flags
   0       0    49153            8192
   1    8192    77825    57344   2048 eof

For example, we do filefrag command for a above file (file1).
FS_IOC_FIEMAP tries to get whole extents information of file1,
so the output has to be 2 extents.
In this case, fm_length (requested block length) is passed
from the user-space to the kernel-space, as follows:

<user-space>
filefrag:
	fiemap->fm_start(0) fiemap->fm_length(~0ULL)

<kernel-space>
fs/ioctl.c ioctl_fimap():

filemap_check_ranges():
	len(~0ULL)
	new_len(4398046511103 = s_maxbytes)  <--- Because 'len > s_maxbytes'

fs/ext4/extents.c ext4_fiemap():
	last_blk = start(0) + len(4398046511103) - 1 >> s_blocksize_bits(11)
	         = 4294967295 (0xFFFFFFFF)
	len_blks = 4294967295 + 1 (0xFFFFFFFF + 0x00000001)
		 = 4294967296 (0x100000000)  <--- _OVERFLOW!!_
	
ext4_ext_walk_space():
	num = 0

This overflow leads to incorrect output like the below,
even though 2 extents exist.

[root@bsd086 akira]# filefrag -v  /mnt/mp1/file1
Filesystem type is: ef53
File size of /mnt/mp1/file1 is 10485760 (10240 blocks, blocksize 1024)
 ext logical physical expected length flags
/mnt/mp1/file6: 1 extent found

Regards,
Akira Fujita

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
  2010-03-03  7:47   ` Akira Fujita
  2010-03-03  8:34     ` Leonard Michlmayr
@ 2010-03-03 17:52     ` tytso
  2010-03-04  5:40       ` Akira Fujita
  1 sibling, 1 reply; 11+ messages in thread
From: tytso @ 2010-03-03 17:52 UTC (permalink / raw)
  To: Akira Fujita; +Cc: Leonard Michlmayr, Ext4 Developers List

On Wed, Mar 03, 2010 at 04:47:28PM +0900, Akira Fujita wrote:
> 
> In 1KB block size, the overflow occurs at above line.
> Since last_blk is set 0xffffffff when len is equal to s_maxbytes.
> Therefore ext4_fiemap() can not get correct extent information
> with 0 length.  How about adding this change?

What do you think _is_ the correct thing to do when length is 0?  As
Leonard has already pointed out, fiemap_check_ranges() already filters
out the length=0 case.

					- Ted

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
  2010-03-03  7:47   ` Akira Fujita
@ 2010-03-03  8:34     ` Leonard Michlmayr
  2010-03-03 17:52     ` tytso
  1 sibling, 0 replies; 11+ messages in thread
From: Leonard Michlmayr @ 2010-03-03  8:34 UTC (permalink / raw)
  To: Akira Fujita, Theodore Ts'o; +Cc: Ext4 Developers List

Hi

I think that the patch is valid as it is.

<snip>
>  >   		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;
> 
> In 1KB block size, the overflow occurs at above line.
> Since last_blk is set 0xffffffff when len is equal to s_maxbytes.
> Therefore ext4_fiemap() can not get correct extent information
> with 0 length.  How about adding this change?
> 

I have considered this possibility, but len == 0 is an invalid request
and would be sorted out by fs/ioctl.c:fiemap_check_ranges. If you want
to make sure that everything is correct even if len == 0 slips through
consider Andreas Dilger's solution which does not introduce a new branch
to the code:

(Here end_blk is one more than the last block)

end_blk = (start + len + inode->i_sb->s_blocksize - 1) >> inode->i_sb->s_blocksize_bits;
len_blks = end_blk - start_blk;

I think we don't need to copy the functionality of fiemap_check_ranges.
At least if there a no danger that len == 0 will be allowed in the
specification in future?

> Signed-off-by: Akira Fujita <a-fujita@rs.jp.nec.com>
> ---
>   extents.c |    3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> --- linux-2.6.33-rc8-a/fs/ext4/extents.c        2010-03-03 14:53:49.000000000 +0900
> +++ linux-2.6.33-rc8-b/fs/ext4/extents.c        2010-03-03 15:31:57.000000000 +0900
> @@ -3912,7 +3912,8 @@ int ext4_fiemap(struct inode *inode, str
> 
>                  start_blk = start >> inode->i_sb->s_blocksize_bits;
>                  last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
> -               len_blks = last_blk - start_blk + 1;
> +               len_blks = (loff_t)last_blk - start_blk + 1 < EXT_MAX_BLOCK ?
> +                               last_blk - start_blk + 1 : EXT_MAX_BLOCK;
> 
>                  /*
>                   * Walk the extent tree gathering extent information.
> 
> 
> (2010/03/03 3:18), Theodore Ts'o wrote:
> > From: Leonard Michlmayr<leonard.michlmayr@gmail.com>
> >
> > 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>
> > ---
> >   fs/ext4/extents.c |    6 ++++--
> >   1 files changed, 4 insertions(+), 2 deletions(-)
> >
> > 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.
> 





^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
  2010-03-02 18:18 ` [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap Theodore Ts'o
@ 2010-03-03  7:47   ` Akira Fujita
  2010-03-03  8:34     ` Leonard Michlmayr
  2010-03-03 17:52     ` tytso
  0 siblings, 2 replies; 11+ messages in thread
From: Akira Fujita @ 2010-03-03  7:47 UTC (permalink / raw)
  To: Theodore Ts'o, Leonard Michlmayr; +Cc: Ext4 Developers List

Hi,

 > From: Leonard Michlmayr<leonard.michlmayr@gmail.com>
 >
 > 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.

<snip>

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

In 1KB block size, the overflow occurs at above line.
Since last_blk is set 0xffffffff when len is equal to s_maxbytes.
Therefore ext4_fiemap() can not get correct extent information
with 0 length.  How about adding this change?

Signed-off-by: Akira Fujita <a-fujita@rs.jp.nec.com>
---
  extents.c |    3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-2.6.33-rc8-a/fs/ext4/extents.c        2010-03-03 14:53:49.000000000 +0900
+++ linux-2.6.33-rc8-b/fs/ext4/extents.c        2010-03-03 15:31:57.000000000 +0900
@@ -3912,7 +3912,8 @@ int ext4_fiemap(struct inode *inode, str

                 start_blk = start >> inode->i_sb->s_blocksize_bits;
                 last_blk = (start + len - 1) >> inode->i_sb->s_blocksize_bits;
-               len_blks = last_blk - start_blk + 1;
+               len_blks = (loff_t)last_blk - start_blk + 1 < EXT_MAX_BLOCK ?
+                               last_blk - start_blk + 1 : EXT_MAX_BLOCK;

                 /*
                  * Walk the extent tree gathering extent information.


(2010/03/03 3:18), Theodore Ts'o wrote:
> From: Leonard Michlmayr<leonard.michlmayr@gmail.com>
>
> 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>
> ---
>   fs/ext4/extents.c |    6 ++++--
>   1 files changed, 4 insertions(+), 2 deletions(-)
>
> 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.

-- 
Akira Fujita <a-fujita@rs.jp.nec.com>

The First Fundamental Software Development Group,
Software Development Division,
NEC Software Tohoku, Ltd.

--Separator@a-fujita@rs.jp.nec.com:

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap
  2010-03-02 18:18 [PATCH 00/28] Ext4 patch queue for the 2.6.34 merge window Theodore Ts'o
@ 2010-03-02 18:18 ` Theodore Ts'o
  2010-03-03  7:47   ` Akira Fujita
  0 siblings, 1 reply; 11+ messages in thread
From: Theodore Ts'o @ 2010-03-02 18:18 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: Leonard Michlmayr, Theodore Ts'o

From: Leonard Michlmayr <leonard.michlmayr@gmail.com>

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>
---
 fs/ext4/extents.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

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.
-- 
1.6.6.1.1.g974db.dirty


^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2010-03-05 16:47 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-05  5:57 [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap Akira Fujita
  -- strict thread matches above, loose matches on Subject: below --
2010-03-02 18:18 [PATCH 00/28] Ext4 patch queue for the 2.6.34 merge window Theodore Ts'o
2010-03-02 18:18 ` [PATCH 11/28] ext4: correctly calculate number of blocks for fiemap Theodore Ts'o
2010-03-03  7:47   ` Akira Fujita
2010-03-03  8:34     ` Leonard Michlmayr
2010-03-03 17:52     ` tytso
2010-03-04  5:40       ` Akira Fujita
2010-03-04 21:44         ` Leonard Michlmayr
2010-03-04 23:38           ` Eric Sandeen
2010-03-05 16:46             ` Leonard Michlmayr
2010-03-04 22:08         ` tytso
2010-03-04 23:47           ` Eric Sandeen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.