Re: [PATCH v3] btrfs: fiemap: Cache and merge fiemap extent before submit it to user

[Liu Bo <bo.li.liu@oracle.com>]

On Thu, Apr 20, 2017 at 09:52:00AM +0800, Qu Wenruo wrote:
> 
> 
> At 04/20/2017 09:25 AM, Liu Bo wrote:
> > On Fri, Apr 07, 2017 at 10:43:15AM +0800, Qu Wenruo wrote:
> > > [BUG]
> > > Cycle mount btrfs can cause fiemap to return different result.
> > > Like:
> > >   # mount /dev/vdb5 /mnt/btrfs
> > >   # dd if=/dev/zero bs=16K count=4 oflag=dsync of=/mnt/btrfs/file
> > >   # xfs_io -c "fiemap -v" /mnt/btrfs/file
> > >   /mnt/test/file:
> > >   EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
> > >     0: [0..127]:        25088..25215       128   0x1
> > >   # umount /mnt/btrfs
> > >   # mount /dev/vdb5 /mnt/btrfs
> > >   # xfs_io -c "fiemap -v" /mnt/btrfs/file
> > >   /mnt/test/file:
> > >   EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
> > >     0: [0..31]:         25088..25119        32   0x0
> > >     1: [32..63]:        25120..25151        32   0x0
> > >     2: [64..95]:        25152..25183        32   0x0
> > >     3: [96..127]:       25184..25215        32   0x1
> > > But after above fiemap, we get correct merged result if we call fiemap
> > > again.
> > >   # xfs_io -c "fiemap -v" /mnt/btrfs/file
> > >   /mnt/test/file:
> > >   EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
> > >     0: [0..127]:        25088..25215       128   0x1
> > > 
> > > [REASON]
> > > Btrfs will try to merge extent map when inserting new extent map.
> > > 
> > > btrfs_fiemap(start=0 len=(u64)-1)
> > > |- extent_fiemap(start=0 len=(u64)-1)
> > >     |- get_extent_skip_holes(start=0 len=64k)
> > >     |  |- btrfs_get_extent_fiemap(start=0 len=64k)
> > >     |     |- btrfs_get_extent(start=0 len=64k)
> > >     |        |  Found on-disk (ino, EXTENT_DATA, 0)
> > >     |        |- add_extent_mapping()
> > >     |        |- Return (em->start=0, len=16k)
> > >     |
> > >     |- fiemap_fill_next_extent(logic=0 phys=X len=16k)
> > >     |
> > >     |- get_extent_skip_holes(start=0 len=64k)
> > >     |  |- btrfs_get_extent_fiemap(start=0 len=64k)
> > >     |     |- btrfs_get_extent(start=16k len=48k)
> > >     |        |  Found on-disk (ino, EXTENT_DATA, 16k)
> > >     |        |- add_extent_mapping()
> > >     |        |  |- try_merge_map()
> > >     |        |     Merge with previous em start=0 len=16k
> > >     |        |     resulting em start=0 len=32k
> > >     |        |- Return (em->start=0, len=32K)    << Merged result
> > >     |- Stripe off the unrelated range (0~16K) of return em
> > >     |- fiemap_fill_next_extent(logic=16K phys=X+16K len=16K)
> > >        ^^^ Causing split fiemap extent.
> > > 
> > > And since in add_extent_mapping(), em is already merged, in next
> > > fiemap() call, we will get merged result.
> > > 
> > > [FIX]
> > > Here we introduce a new structure, fiemap_cache, which records previous
> > > fiemap extent.
> > > 
> > > And will always try to merge current fiemap_cache result before calling
> > > fiemap_fill_next_extent().
> > > Only when we failed to merge current fiemap extent with cached one, we
> > > will call fiemap_fill_next_extent() to submit cached one.
> > > 
> > > So by this method, we can merge all fiemap extents.
> > > 
> > 
> > If I understand it correctly, what it's missing currently is 'merge', a
> > straightfoward idea is to make use of the 'merge' ability of btrfs_get_extent. >
> > Since btrfs_get_extent_fiemap is a wrapper of btrfs_get_extent, does it make
> > sense if we make btrfs_get_extent_fiemap iterate all extent maps within the
> > range passing to it or just one more extent map to check if btrfs_get_extent
> > could return a merged extent map before returning?
> 
> So your idea to to do the merging inside btrfs_get_extent(), instead of
> merging it in extent_fiemap()?
> 
> In theory, they have the same effect for fiemap.
> And that's my original idea.
> 
> But the problem is, btrfs_get_extent() is called almost everywhere, more
> frequently than extent_fiemap(), the extra time used to merging extent map
> may cause performance problem.
> 
> For the worst case, if a file is made up by 262144 4K extent (takes up 1G),
> btrfs_get_extent() call on the file will iterate all these extents,
> which will iterate more than 500 16K tree blocks.
> 
> If only fiemap takes such longer time, it's still acceptable. But if
> btrfs_get_extent() takes such longer time, I think it will be a problem
> then.
>

Not related to the patch, but the question is, does fiemap ioctl think
it is important to have unified output?

The filefrag manual only says it's attempting to get extent
information.  And this inconsistent output of filefrag doesn't seem to
confusing users as the reported extent count is correct.

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo