Message-ID: <20200130160018.GC3445353@magnolia>
Date:   Thu, 30 Jan 2020 08:00:18 -0800
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     Ritesh Harjani <riteshh@...ux.ibm.com>
Cc:     jack@...e.cz, tytso@....edu, adilger.kernel@...ger.ca,
        linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        hch@...radead.org, cmaiolino@...hat.com
Subject: Re: [RFCv2 0/4] ext4: bmap & fiemap conversion to use iomap

On Tue, Jan 28, 2020 at 03:48:24PM +0530, Ritesh Harjani wrote:
> Hello All,
> 
> Background
> ==========
> There are RFCv2 patches to move ext4 bmap & fiemap calls to use iomap APIs.
> This reduces the users of ext4_get_block API and thus a step towards getting
> rid of buffer_heads from ext4. Also reduces a lot of code by making use of
> existing iomap_ops (except for xattr implementation).
> 
> Testing (done on ext4 master branch)
> ========
> 'xfstests -g auto' passes with default mkfs/mount configuration
> (v/s which also pass with vanilla kernel without this patch). Except
> generic/473 which also failes on XFS. This seems to be the test case issue
> since it expects the data in slightly different way as compared to what iomap
> returns.
> Point 2.a below describes more about this.
> 
> Observations/Review required
> ============================
> 1. bmap related old v/s new method differences:-
> 	a. In case if addr > INT_MAX, it issues a warning and
> 	   returns 0 as the block no. While earlier it used to return the
> 	   truncated value with no warning.

Good...

> 	b. block no. is only returned in case of iomap->type is IOMAP_MAPPED,
> 	   but not when iomap->type is IOMAP_UNWRITTEN. While with previously
> 	   we used to get block no. for both of above cases.

Assuming the only remaining usecase of bmap is to tell old bootloaders
where to find vmlinuz blocks on disk, I don't see much reason to map
unwritten blocks -- there's no data there, and if your bootloader writes
to the filesystem(!) then you can't read whatever was written there
anyway.

Uh, can we put this ioctl on the deprecation list, please? :)

> 2. Fiemap related old v/s new method differences:-
> 	a. iomap_fiemap returns the disk extent information in exact
> 	   correspondence with start of user requested logical offset till the
> 	   length requested by user. While in previous implementation the
> 	   returned information used to give the complete extent information if
> 	   the range requested by user lies in between the extent mapping.

This is a topic of much disagreement.  The FIEMAP documentation says
that the call must return data for the requested range, but *may* return
a mapping that extends beyond the requested range.

XFS (and now iomap) only return data for the requested range, whereas
ext4 has (had?) the behavior you describe.  generic/473 was an attempt
to enforce the ext4 behavior across all filesystems, but I put it in my
dead list and never run it.

So it's a behavioral change, but the new behavior isn't forbidden.

> 	b. iomap_fiemap adds the FIEMAP_EXTENT_LAST flag also at the last
> 	   fiemap_extent mapping range requested by the user via fm_length (
> 	   if that has a valid mapped extent on the disk).

That sounds like a bug.  _LAST is supposed to be set on the last extent
in the file, not the last record in the queried dataset.

> But if the user
> 	   requested for more fm_length which could not be mapped in the last
> 	   fiemap_extent, then the flag is not set.

Yes... if there were more extents to map than there was space in the map
array, then _LAST should remain unset to encourage userspace to come
back for the rest of the mappings.

(Unless maybe I'm misunderstanding here...)

> e.g. output for above differences 2.a & 2.b
> ===========================================
> create a file with below cmds. 
> 1. fallocate -o 0 -l 8K testfile.txt
> 2. xfs_io -c "pwrite 8K 8K" testfile.txt
> 3. check extent mapping:- xfs_io -c "fiemap -v" testfile.txt
> 4. run this binary on with and without these patches:- ./a.out (test_fiemap_diff.c) [4]
> 
> o/p of xfs_io -c "fiemap -v"
> ============================================
> With this patch on patched kernel:-
> testfile.txt:
>  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS
>    0: [0..15]:         122802736..122802751    16 0x800
>    1: [16..31]:        122687536..122687551    16   0x1
> 
> without patch on vanilla kernel (no difference):-
> testfile.txt:
>  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS
>    0: [0..15]:         332211376..332211391    16 0x800
>    1: [16..31]:        332722392..332722407    16   0x1
> 
> 
> o/p of a.out without patch:-
> ================
> riteshh-> ./a.out 
> logical: [       0..      15] phys: 332211376..332211391 flags: 0x800 tot: 16
> (0) extent flag = 2048
> 
> o/p of a.out with patch (both point 2.a & 2.b could be seen)
> =======================
> riteshh-> ./a.out
> logical: [       0..       7] phys: 122802736..122802743 flags: 0x801 tot: 8
> (0) extent flag = 2049
> 
> FYI - In test_fiemap_diff.c test we had 
> a. fm_extent_count = 1
> b. fm_start = 0
> c. fm_length = 4K
> Whereas when we change fm_extent_count = 32, then we don't see any difference.
> 
> e.g. output for above difference listed in point 1.b
> ====================================================
> 
> o/p without patch (block no returned for unwritten block as well)
> =========Testing IOCTL FIBMAP=========
> File size = 16384, blkcnt = 4, blocksize = 4096
>   0   41526422
>   1   41526423
>   2   41590299
>   3   41590300
> 
> o/p with patch (0 returned for unwritten block)
> =========Testing IOCTL FIBMAP=========
> File size = 16384, blkcnt = 4, blocksize = 4096
>   0          0          0
>   1          0          0
>   2   15335942      29953
>   3   15335943      29953
> 
> 
> Summary:-
> ========
> Due to some of the observational differences to user, listed above,
> requesting to please help with a careful review in moving this to iomap.
> Digging into some older threads, it looks like these differences should be fine,
> since the same tools have been working fine with XFS (which uses iomap based
> implementation) [1]
> Also as Ted suggested in [3]: Fiemap & bmap spec could be made based on the ext4
> implementation. But since all the tools also work with xfs which uses iomap
> based fiemap, so we should be good there.

<nod> Thanks for the worked example output. :)

--D

> 
> References of some previous discussions:
> =======================================
> [1]: https://www.spinics.net/lists/linux-fsdevel/msg128370.html
> [2]: https://www.spinics.net/lists/linux-fsdevel/msg127675.html
> [3]: https://www.spinics.net/lists/linux-fsdevel/msg128368.html
> [4]: https://raw.githubusercontent.com/riteshharjani/LinuxStudy/master/tools/test_fiemap_diff.c
> [RFCv1]: https://www.spinics.net/lists/linux-ext4/msg67077.html 
> 
> 
> Ritesh Harjani (4):
>   ext4: Add IOMAP_F_MERGED for non-extent based mapping
>   ext4: Optimize ext4_ext_precache for 0 depth
>   ext4: Move ext4 bmap to use iomap infrastructure.
>   ext4: Move ext4_fiemap to use iomap infrastructure
> 
>  fs/ext4/extents.c | 288 +++++++---------------------------------------
>  fs/ext4/inline.c  |  41 -------
>  fs/ext4/inode.c   |   6 +-
>  3 files changed, 49 insertions(+), 286 deletions(-)
> 
> -- 
> 2.21.0
> 

Powered by blists - more mailing lists