the FS_IOC_FIEMAP ioctl does not report extents of small files

Bug #474597 reported by Leonard Michlmayr
18
This bug affects 1 person
Affects Status Importance Assigned to Milestone
linux (Ubuntu)
Fix Released
Medium
Surbhi Palande
Lucid
Fix Released
Undecided
Unassigned
Maverick
Fix Released
Medium
Surbhi Palande

Bug Description

Thi bugfix has been included in the update of Lucid to 2.6.32.13 upstream stable (see Bug #583414).

----

I have already reported this upstream. on ext4, fiemap will not report extents if the requested logical region of a file overlaps the extent less than blocksize. So, if you request star=0, len=100, you will get nothing.

I can provide example code, if you need.

There is a patch attached to this bug report.

SourcePackage: linux
Uname: Linux 2.6.31-14-generic x86_64

Revision history for this message
Leonard Michlmayr (leonard-michlmayr) wrote :
Changed in linux (Ubuntu):
assignee: nobody → Canonical Kernel Team (canonical-kernel-team)
Changed in linux (Ubuntu):
importance: Undecided → Medium
status: New → Triaged
Surbhi Palande (csurbhi)
Changed in linux (Ubuntu):
assignee: Canonical Kernel Team (canonical-kernel-team) → Surbhi Palande (csurbhi)
Surbhi Palande (csurbhi)
Changed in linux (Ubuntu):
status: Triaged → In Progress
description: updated
Revision history for this message
Surbhi Palande (csurbhi) wrote :

@Leonard, I can see that you have submitted the same patch to ext4 mailing list. Can you please request for an update or status of the patch ?

Revision history for this message
Leonard Michlmayr (leonard-michlmayr) wrote : Re: ext4_fiemap gives 0 extents for files smaller than a block (patch included)

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

Revision history for this message
Surbhi Palande (csurbhi) wrote :

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.

Revision history for this message
Leonard Michlmayr (leonard-michlmayr) wrote : [PATCH 2.6.32.7] ext4: number of blocks for fiemap

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 address hidden>

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.

Revision history for this message
Leonard Michlmayr (leonard-michlmayr) wrote :

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 address hidden>

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.

Revision history for this message
Theodore Ts'o (tytso) 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 address hidden>
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 address hidden>
    Signed-off-by: "Theodore Ts'o" <email address hidden>

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.

Revision history for this message
sandeen (sandeen) wrote :

<email address hidden> 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 address hidden>
> 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 address hidden>
> Signed-off-by: "Theodore Ts'o" <email address hidden>
>
> 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 address hidden>
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Revision history for this message
Theodore Ts'o (tytso) wrote :

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

Revision history for this message
Surbhi Palande (csurbhi) wrote :

Ted, thanks for the notification.
The patch posted above is different than the one in the linux kernel.

commit aca92ff6f57c000d1b4523e383c8bd6b8269b8b1
Author: Leonard Michlmayr <email address hidden>
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 <email address hidden>
    Signed-off-by: "Theodore Ts'o" <email address hidden>

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.

The difference being the following two lines are added:
+ if (last_blk >= EXT_MAX_BLOCK)
+ last_blk = EXT_MAX_BLOCK-1;

* This is just a note to point the difference.

Andy Whitcroft (apw)
Changed in linux (Ubuntu):
status: In Progress → Fix Committed
Stefan Bader (smb)
description: updated
Revision history for this message
Martin Pitt (pitti) wrote : Please test proposed package

Accepted linux into lucid-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Changed in linux (Ubuntu Lucid):
status: New → Fix Committed
tags: added: verification-needed
Revision history for this message
Launchpad Janitor (janitor) wrote :
Download full text (25.2 KiB)

This bug was fixed in the package linux - 2.6.32-23.37

---------------
linux (2.6.32-23.37) lucid-proposed; urgency=low

  [ Alex Deucher ]

  * SAUCE: drm/radeon/kms/atom: fix dual-link DVI on DCE3.2/4.0
    - LP: #564559

  [ Andy Whitcroft ]

  * [Config] ports -- build in dm-mod to enable LVM boot
    - LP: #560717
  * tools -- fix perf version extraction for multi-part flavours
    - LP: #555130
  * SAUCE: ACPI: EC: Allow multibyte access to EC (v3)
    - LP: #526354
  * [Config] enforce -- ensure dm_mod is built-in for LVM
    - LP: #560717
  * update to ubuntu-debian:7e708d33054c373faf41da23b73e8b48c342d958
    - LP: #570500, #576274

  [ Chase Douglas ]

  * Revert "(pre-stable): input: ALPS - Add signature for HP Pavilion dm3
    laptops"
    - LP: #550625
  * Enable ftrace function profiler
    - LP: #570389
  * enforce CONFIG_TMPFS_POSIX_ACL=y
    - LP: #575940

  [ Leann Ogasawara ]

  * Revert "staging/comdi -- disable"
    - LP: #563436
  * [Config] Enable multicast routing for sparc
    - LP: #416266
  * [Config] Add ahci.ko to virtual sub-flavour
    - LP: #570542

  [ Stefan Bader ]

  * Revert "SAUCE: drm/i915: Disable FBC on 915GM and 945GM"
    - LP: #588832

  [ Tim Gardner ]

  * ubuntu: rtl8192se -- update to version 0015.0127.2010
    - LP: #567016
  * [Config] Add atl1c to nic-modules udeb
    - LP: #557130

  [ Upstream Kernel Changes ]

  * Revert "(pre-stable) iwlwifi: fix nfreed--"
    - LP: #575853
  * Revert "backlight: mbp_nvidia_bl - add five more MacBook variants"
    - LP: #575853
  * Revert "(pre-stable) pata_via: Add VIA VX900 support"
    - LP: #575853
  * Revert "(pre-stable) x86-32, resume: do a global tlb flush in S4
    resume"
    - LP: #575853
  * Revert "x86: disable IOMMUs on kernel crash"
    - LP: #575853
  * Revert "sunrpc: fix peername failed on closed listener"
    - LP: #575853
  * Revert "sunrpc: move the close processing after do recvfrom method"
    - LP: #575853
  * Revert "(pre-stable) drm/edid: allow certain bogus edids to hit a fixup
    path rather than fail"
    - LP: #575853
  * Revert "drm/radeon/kms: don't print error on -ERESTARTSYS."
    - LP: #575853
  * Revert "ath9k: fix lockdep warning when unloading module" on stable
    kernels
    - LP: #588832
  * Staging: comedi: removed "depricated" from COMEDI_CB_BLOCK
    - LP: #483343
  * fat: fix buffer overflow in vfat_create_shortname()
    - LP: #575853
  * xfs: simplify inode teardown
    - LP: #575853
  * xfs: fix mmap_sem/iolock inversion in xfs_free_eofblocks
    - LP: #575853
  * xfs: I/O completion handlers must use NOFS allocations
    - LP: #575853
  * xfs: Wrapped journal record corruption on read at recovery
    - LP: #575853
  * xfs: Fix error return for fallocate() on XFS
    - LP: #575853
  * xfs: check for not fully initialized inodes in xfs_ireclaim
    - LP: #575853
  * xfs: fix timestamp handling in xfs_setattr
    - LP: #575853
  * xfs: Don't flush stale inodes
    - LP: #575853
  * xfs: Ensure we force all busy extents in range to disk
    - LP: #575853
  * xfs: reclaim inodes under a write lock
    - LP: #575853
  * xfs: Avoid inodes in reclaim when flushing from inode cache
    - LP: #575853
  * xfs: recla...

Changed in linux (Ubuntu Lucid):
status: Fix Committed → Fix Released
Changed in linux (Ubuntu Maverick):
status: Fix Committed → Fix Released
Revision history for this message
Herton R. Krzesinski (herton) wrote :

This is just a notification (FYI).

A patch for this bug is now being included in the package linux-fsl-imx51, in the update which is being tracked in bug 873059. Besides of being included in a stable update, it is also a CVE fix, which is not subject to verification, thus tagging as verification-done-lucid (this is part of Ubuntu SRU policies for kernel updates, just ignore).

tags: added: verification-done-lucid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.