| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 30 Jan 2020 23:04:56 +0530
From: Ritesh Harjani <riteshh@...ux.ibm.com>
To: "Darrick J. Wong" <darrick.wong@...cle.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 1/30/20 9:30 PM, Darrick J. Wong wrote:
> 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.
Yes, no objection there. Just wanted to get it reviewed.
>
> 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.
Sure, thanks.
>
>> 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.
Thought so too, sure will spend some time to try fixing it.
>
>> 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. :)
Thanks for the review. :)
ritesh
>
> --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