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

[Leonard Michlmayr-3]

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


[ext4_fiemap_fix.patch]

---

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.

---

[Andreas Dilger-4]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Leonard Michlmayr-3]

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


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html