Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Wu Bo <bo.wu@vivo.com>
To:     chao@kernel.org
Cc:     bo.wu@vivo.com, daehojeong@google.com, jaegeuk@kernel.org,
        linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, wubo.oduw@gmail.com
Subject: Re: [f2fs-dev] [PATCH 1/1] f2fs: move fiemap to use iomap framework
Date:   Mon,  7 Aug 2023 10:38:23 +0800
Message-Id: <20230807023823.49564-1-bo.wu@vivo.com>
In-Reply-To: <791fbf71-093e-3ef5-b02b-6d6bba72b57f@kernel.org>
References: <791fbf71-093e-3ef5-b02b-6d6bba72b57f@kernel.org>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?xKLLU8XAfRmNMJxvAZj6m6oje5wHrHK7S3W+K7jm3s+XDb3j4wQcm2JJ9HYO?=
 =?us-ascii?Q?XlsNZu888Opv77riwzQe7/2GuLf4vmwjAYs12IiVmom87stKUJMSEW3xoBqK?=
 =?us-ascii?Q?j1qWSrWnwvPbFL634yy4m+gcmGesM8uAXMGjaPjn4JDPJjNq+B/Yu5rk+mMA?=
 =?us-ascii?Q?6jyTh2cuXWTxLRA2PWc3oSr2MsqIownwkNaq6db5fhLo+B5Cdff9I0qAS+w6?=
 =?us-ascii?Q?dPFYjc9seh6M/Otz0sA0YjX0G/sY684uEo1DrRs2nlS2NYkXqCiBYfLIMAy3?=
 =?us-ascii?Q?MJ1kAYG4xG9p22H4bynRZd+zQwQeZ2kdgv5QvEYR9x5/dUFKNZhYPMb4iEXb?=
 =?us-ascii?Q?VyVtmf0JfviQgioFrHqFDJ+DS7Ysof2+kFGFjDpi1OFfJxDfzs2jP7WMbGVG?=
 =?us-ascii?Q?Zi/tcaXkiNQOh3Ck1pEGHWq5ojr2NkLQS7RjDSbN7of9Tv6rV3NkOq6K7krk?=
 =?us-ascii?Q?QIJXg0tMf+fTYDFT2sErlXvouHTDt2LnkiE6HbMlPjZOlO4BFNvFcFcSqqOb?=
 =?us-ascii?Q?3iXygJotMkFdIdZL2hrVnxy+PmGC94NI49mY/t1ZkcJUL2c1k6nDJCLCfBSo?=
 =?us-ascii?Q?snoVPRWngcuTAyogGUR9nFwBY4digDFJ0UA4wjM31VCb6ohjaWN1NGNt0vRg?=
 =?us-ascii?Q?AwM/6wRjI3jjPUpt2X1U5UOdC5mI05BIVtETcz+w+6TcphDbFDjCRkxJexI9?=
 =?us-ascii?Q?BslzlUeiYBkDPVJAgIUOdnPL5XAfT2Szon4+rEVkt5TusTxbXnPVE/9w0LZw?=
 =?us-ascii?Q?0Yc8hdgPh9TPRIRM8kZZSfMizG9rk92O+OoOsvtIryX00lzwOLCbdBiJUdPD?=
 =?us-ascii?Q?yvbO8uf3CsO8WzYtv8p7WGljPJrtR4L4HJKKIX1w38hcBNPtDfiTp2HSt5yZ?=
 =?us-ascii?Q?ZBNlf4f/2SrMPB23rjB6LUrcQSvP1cr8MSDLPprZVr+oL4J3z8GYjQ/Qada6?=
 =?us-ascii?Q?3mC5NjYkUfIIkOmsANgiBoeSzpuhCOsyh4aUc7oIYJlbjHgprHeL/gvJvyjQ?=
 =?us-ascii?Q?tSOHLmdvfxTS8rYzqQqNT84zFpMOfmKW0mPS5x8ppBozLTadZwsaEwrA1hXQ?=
 =?us-ascii?Q?+wLH1UL/R+G4Inz5Wq+ze6maJ0htiEQwV7IdLuFW/wgCUO6MG2TvUbT4Msbj?=
 =?us-ascii?Q?cqRLdA6cXZxFaVqrmhMjU20XFh/eG98z5sMidXR6AEMi/VQZmsdc0wtygRtc?=
 =?us-ascii?Q?nQbB8TWJ/wUI5h4TKbQ3As9AWI+Rozv02L9WfdB13zHVpfM4bfeXxty9qYMC?=
 =?us-ascii?Q?7wPzGKRdr646JliceiEaHeNmGZDjlB83WUBvoSXlSmwtuHNBpMLX+ECQw6iT?=
 =?us-ascii?Q?Ia9zbXLx+YQQZJ/JIIE1/NYwHLtL+HJjL1ksAgIR5xnL0IWLiUSzT/98Z5Tm?=
 =?us-ascii?Q?4g667Noi4ndf/nPPjR/CJjs5TrOK3/H0mAvI7azqcXeS5IvpR0bYgv8gKGvY?=
 =?us-ascii?Q?DNfun4v90Nyc8lmZHrj4tVVA2BpjwmnTgiU/KkOphd0qftyuCYY5mSjIBuVs?=
 =?us-ascii?Q?phzpp74YAf57ekG6DKE3hyl3RKxbjkYlQ37sod1g9wbMssSekCwna7AOUasd?=
 =?us-ascii?Q?TAbjfBbjTllyf1Jm0T03ShYkntsAVu00PAUptpQ/?=
X-OriginatorOrg: vivo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a921fc4-8c16-42db-87c1-08db96ef6378
X-MS-Exchange-CrossTenant-AuthSource: SL2PR06MB3017.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Aug 2023 02:38:31.9501
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 923e42dc-48d5-4cbe-b582-1a797a6412ed
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2/D6yI6WeWrJWDLNdy+j1t01mYwAUVexQr7U8327As5V2i4r9b+XrHCOJBqnn7N668oyrHGbUsoYEnAtPl54xw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: TYZPR06MB5321
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2023/8/6 10:05, Chao Yu wrote:

> On 2023/7/31 9:26, Wu Bo wrote:
>> This patch has been tested with xfstests by running 'kvm-xfstests -c
>> f2fs -g auto' with and without this patch; no regressions were seen.
>>
>> Some tests fail both before and after, and the test results are:
>> f2fs/default: 683 tests, 9 failures, 226 skipped, 30297 seconds
>>    Failures: generic/050 generic/064 generic/250 generic/252 generic/459
>>        generic/506 generic/563 generic/634 generic/635
>
> Can you please take a look at gerneic/473 ?

This generic/473 case is failed on xfs too. It's an issue of iomap.

>
> generic/473 1s ... - output mismatch (see
> /media/fstests/results//generic/473.out.bad)
>     --- tests/generic/473.out    2022-11-10 08:42:19.231395230 +0000
>     +++ /media/fstests/results//generic/473.out.bad    2023-08-04
> 02:02:01.000000000 +0000
>     @@ -6,7 +6,7 @@
>      1: [256..287]: hole
>      Hole + Data
>      0: [0..127]: hole
>     -1: [128..255]: data
>     +1: [128..135]: data
>      Hole + Data + Hole
>      0: [0..127]: hole
>     ...
>     (Run 'diff -u /media/fstests/tests/generic/473.out
> /media/fstests/results//generic/473.out.bad'  to see the entire diff)

The layout of the test file is:
fiemap.473:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..127]:        hole               128
   1: [128..255]:      5283840..5283967   128 0x1000
   2: [256..383]:      hole               128
   3: [384..511]:      5283968..5284095   128 0x1000

And the test command is:
xfs_io -c "fiemap -v 0 65k" fiemap.473

So the difference is about when to stop traversal the extents.
The iomap stop when the length beyond it is requested from fiemap command:
...
xfs_io-7399    [001] .....  1385.656328: f2fs_map_blocks: dev = (254,48), ino = 5, file offset = 15, start blkaddr = 0x0, len = 0x0, flags = 0, seg_type = 8, may_create = 0, multidevice = 0, flag = 1, err = 0                                                                                       
xfs_io-7399    [001] .....  1385.656328: f2fs_map_blocks: dev = (254,48), ino = 5, file offset = 16, start blkaddr = 0x3400, len = 0x1, flags = 2, seg_type = 8, may_create = 0, multidevice = 0, flag = 1, err = 0            

While previous logic is that stop traversal until next data extent is found:
...
xfs_io-2194    [000] .....   116.046690: f2fs_map_blocks: dev = (254,48), ino = 5, file offset = 15, start blkaddr = 0x0, len = 0x0, flags = 0, seg_type = 8, may_create = 0, multidevice = 0, flag = 1, err = 0
xfs_io-2194    [000] .....   116.046690: f2fs_map_blocks: dev = (254,48), ino = 5, file offset = 16, start blkaddr = 0xa1400, len = 0x10, flags = 2, seg_type = 8, may_create = 0, multidevice = 0, flag = 1, err = 0
xfs_io-2194    [000] .....   116.046691: f2fs_map_blocks: dev = (254,48), ino = 5, file offset = 32, start blkaddr = 0x0, len = 0x0, flags = 0, seg_type = 8, may_create = 0, multidevice = 0, flag = 1, err = 0
...
xfs_io-2194    [000] .....   116.046706: f2fs_map_blocks: dev = (254,48), ino = 5, file offset = 48, start blkaddr = 0xa1410, len = 0x10, flags = 2, seg_type = 8, may_create = 0, multidevice = 0, flag = 1, err = 0

>
> Other concern is, it needs to test this implementation on compressed
> file,
> since the logic is a little bit complicated.

To be honest, all the complex logic is try to handle compressed file situation.

I used enwiki8 dataset to test compressed file:
    mkfs.f2fs -f -O extra_attr,compression f2fs.img
    mount f2fs.img f2fs -o compress_algorithm=lz4,compress_log_size=3,compress_mode=user
    touch compressed_file
    f2fs_io setflags compression compressed_file
    cat enwiki8 > compressed_file
    f2fs_io compress compressed_file
    f2fs_io release_cblocks compressed_file
    xfs_io -c fiemap compressed_file | awk '{print $2 $3}'

enwiki8 download url: http://mattmahoney.net/dc/enwik8.zip

And the result is:
--- a/orig
+++ b/new
@@ -1750,8 +1750,8 @@
 [111872..111935]:323448..323511
 [111936..111999]:323488..323551
 [112000..112063]:323520..323583
-[112064..112087]:323560..323583
-[112088..112127]:53248..53287
+[112064..112095]:323560..323591
+[112096..112127]:53248..53279
 [112128..112191]:53256..53319
 [112192..112255]:53288..53351
 [112256..112319]:53328..53391
@@ -2078,10 +2078,8 @@
 [132800..132863]:65408..65471
 [132864..132927]:65448..65511
 [132928..132991]:65488..65551
-[132992..132999]:65528..65535
-[133000..133007]:65528..65535
-[133008..133039]:69632..69663
-[133040..133055]:hole
+[132992..133007]:65528..65543
+[133008..133055]:69632..69679
 [133056..133119]:69664..69727
 [133120..133183]:69704..69767
 [133184..133247]:69744..69807

The first diff is I count the space of COMPRESS_ADDR belong to the head of one
compressed cluster while previous count at the rear of cluster.
The secound diff show the previous print a 'hole' in one cluster. I think a
compressed cluster should not include a 'hole', so there may have a bug before.

Also, as discussed in this thread:
https://lore.kernel.org/linux-f2fs-devel/ZJmBmt3WmUpWR3+2@casper.infradead.org/T/#t
If f2fs can support async buffer write, the performance can be greatly improved
when using io_uring. 

I think it's time to move f2fs to iomap framework. And really looking forward
to hearing your opinion on this.

Thanks