Re: [PATCH v2] btrfs: do more strict compressed read merge check

在 2025/8/25 20:12, Filipe Manana 写道:

On Sun, Aug 24, 2025 at 12:27 AM Qu Wenruo wqu@suse.com wrote:

...

[BUG] When running test case generic/457, there is a chance to hit the following error, with 64K page size and 4K btrfs block size, and "compress=zstd" mount option:

FSTYP -- btrfs PLATFORM -- Linux/aarch64 btrfs-aarch64 6.17.0-rc2-custom+ #129 SMP PREEMPT_DYNAMIC Wed Aug 20 18:52:51 ACST 2025 MKFS_OPTIONS -- -s 4k /dev/mapper/test-scratch1 MOUNT_OPTIONS -- -o compress=zstd /dev/mapper/test-scratch1 /mnt/scratch

generic/457 2s ... [failed, exit status 1]- output mismatch (see /home/adam/xfstests-dev/results//generic/457.out.bad) --- tests/generic/457.out 2024-04-25 18:13:45.160550980 +0930 +++ /home/adam/xfstests-dev/results//generic/457.out.bad 2025-08-22 16:09:41.039352391 +0930 @@ -1,2 +1,3 @@ QA output created by 457 -Silence is golden +testfile6 end md5sum mismatched +(see /home/adam/xfstests-dev/results//generic/457.full for details) ... (Run 'diff -u /home/adam/xfstests-dev/tests/generic/457.out /home/adam/xfstests-dev/results//generic/457.out.bad' to see the entire diff)

The root problem is, after certain fsx operations the file contents change just after a mount cycle.

There is a much smaller reproducer based on that test case, which I mainly used to debug the bug:

workload() { mkfs.btrfs -f $dev > /dev/null dmesg -C trace-cmd clear mount -o compress=zstd $dev $mnt xfs_io -f -c "pwrite -S 0xff 0 256K" -c "sync" $mnt/base > /dev/null cp --reflink=always -p -f $mnt/base $mnt/file $fsx -N 4 -d -k -S 3746842 $mnt/file if [ $? -ne 0 ]; then echo "!!! FSX FAILURE !!!" fail fi csum_before=$(_md5_checksum $mnt/file) stop_trace umount $mnt mount $dev $mnt csum_after=$(_md5_checksum $mnt/file) umount $mnt if [ "$csum_before" != "$csum_after" ]; then echo "!!! CSUM MISMATCH !!!" fail fi }

This seed value will cause 100% reproducible csum mismatch after a mount cycle.

The seed value results only 2 real operations:

Seed set to 3746842 main: filesystem does not support fallocate mode FALLOC_FL_UNSHARE_RANGE, disabling! main: filesystem does not support fallocate mode FALLOC_FL_COLLAPSE_RANGE, disabling! main: filesystem does not support fallocate mode FALLOC_FL_INSERT_RANGE, disabling! main: filesystem does not support exchange range, disabling! main: filesystem does not support dontcache IO, disabling! 2 clone from 0x3b000 to 0x3f000, (0x4000 bytes) at 0x1f000 3 write 0x2975b thru 0x2ba20 (0x22c6 bytes) dontcache=0 All 4 operations completed A-OK!

Can you please make a test case for fstests? Without fsx of course, since when new operations are added or fsx is changed in other ways, the same seed is likely to not exercise the bug anymore.

Just using xfs_io (writes, cloning, etc).

Sure.

Although by the nature of page reads, on x86_64 it will mostly not reproduce and only trigger on bs < ps cases.

[...]

...

This means, we reads of range [124K, 140K) and [140K, 165K) should not

"we reads of range..." -> the reads of ranges...

...

be merged.

But read merge check function, btrfs_bio_is_contig(), is only checking the disk_bytenr of two compressed reads, as there are not enough info like the involved extent maps to do more comprehensive checks, resulting the incorrect compressed read.

So this is a variant of similar problems solved in the past where for compressed extents we merged when we shouldn't have:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... http://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...

Exactly.

And we have test cases btrfs/103, btrfs/106 and btrfs/183 that exercise those past pugs.

...

Unfortunately this is a long existing bug, way before subpage block size support.

But subpage block size support (and experimental large folio support) makes it much easier to detect.

If block size equals page size, regular page read will only read one block each time, thus no extent map sharing nor merge.

(This means for bs == ps cases, it's still possible to hit the bug with readahead, just we don't have test coverage with content verification for readahead)

[FIX] Save the last hit compressed extent map start/len into btrfs_bio_ctrl, and check if the current extent map is the same as the saved one.

Here we only save em::start/len to save memory for btrfs_bio_ctrl, as it's using the stack memory, which is a very limited resource inside the kernel.

Since the compressed extent maps are never merged, their start/len are unique inside the same inode, thus just checking start/len will be enough to make sure they are the same extent map.

If the extent maps do not match, force submitting the current bio, so that the read will never be merged.

CC: stable@vger.kernel.org Signed-off-by: Qu Wenruo wqu@suse.com

---

v2:

• Only save extent_map::start/len to save memory for btrfs_bio_ctrl It's using on-stack memory which is very limited inside the kernel.

• Remove the commit message mentioning of clearing last saved em Since we're using em::start/len, there is no need to clear them. Either we hit the same em::start/len, meaning hitting the same extent map, or we hit a different em, which will have a different start/len.

---

fs/btrfs/extent_io.c | 52 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 0c12fd64a1f3..418e3bf40f94 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -131,6 +131,22 @@ struct btrfs_bio_ctrl { */ unsigned long submit_bitmap; struct readahead_control *ractl;

•   /*
  

•    * The start/len of the last hit compressed extent map.
  

•    *
  

•    * The current btrfs_bio_is_contig() only uses disk_bytenr as
  

•    * the condition to check if the read can be merged with previous
  

•    * bio, which is not correct. E.g. two file extents pointing to the
  

•    * same extent.
  

•    *
  

•    * So here we need to do extra check to merge reads that are
  

•    * covered by the same extent map.
  

•    * Just extent_map::start/len will be enough, as they are unique
  

•    * inside the same inode.
  

•    */
  

•   u64 last_compress_em_start;
  

•   u64 last_compress_em_len;
  

  };

  /*

@@ -957,6 +973,32 @@ static void btrfs_readahead_expand(struct readahead_control *ractl, readahead_expand(ractl, ra_pos, em_end - ra_pos); }

+static void save_compressed_em(struct btrfs_bio_ctrl *bio_ctrl,

•                          const struct extent_map *em)
  

+{

•   if (btrfs_extent_map_compression(em) == BTRFS_COMPRESS_NONE)
  

•           return;
  

•   bio_ctrl->last_compress_em_start = em->start;
  

•   bio_ctrl->last_compress_em_len = em->len;
  

+}

Something so simple can be open coded instead in the single caller...

...

+static bool is_same_compressed_em(struct btrfs_bio_ctrl *bio_ctrl,

•                             const struct extent_map *em)
  

+{

•   /*
  

•    * Only if the em is completely the same as the previous one we can merge
  

•    * the current folio in the read bio.
  

•    *
  

•    * Here we only need to compare the em->start/len against saved
  

•    * last_compress_em_start/len, as start/len inside an inode are unique,
  

•    * and compressed extent maps are never merged.
  

•    */
  

•   if (em->start != bio_ctrl->last_compress_em_start ||
  

•       em->len != bio_ctrl->last_compress_em_len)
  

•           return false;
  

Same here. In fact we already have part of this logic in the caller, see below.

...

•   return true;
  

+}

• /*
  • basic readpage implementation. Locked extent state structs are inserted
  • into the tree that are removed when the IO is done (by the end_io

@@ -1080,9 +1122,19 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached, *prev_em_start != em->start) force_bio_submit = true;

•           /*
  

•            * We must ensure we only merge compressed read when the current
  

•            * extent map matches the previous one exactly.
  

•            */
  

•           if (compress_type != BTRFS_COMPRESS_NONE) {
  

•                   if (!is_same_compressed_em(bio_ctrl, em))
  

•                           force_bio_submit = true;
  

•           }
  

We already do almost all of this above - we only miss the extent map length check. We have the prev_em_start which already stores the start offset of the last found compressed extent map, so we're duplicating code and logic here unnecessarily.

Further with this new logic, since last_compress_em_start and last_compress_em_len are initialized to zero, we always do an unnecessary split for the first readahead call that spans more than page/folio. We need to distinguish the first call and ignore it - that's why *prev_em_start is initialized to (u64)-1 and we check that special value above.

...

•            if (prev_em_start)
                     *prev_em_start = em->start;
  
  

•           save_compressed_em(bio_ctrl, em);
  

This could have been placed under the check for compress_type != BTRFS_COMPRESS_NONE...

In other words, this could be simplified to this:

(also at https://pastebin.com/raw/tZHq7icd)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 0c12fd64a1f3..a982277f852b 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -131,6 +131,22 @@ struct btrfs_bio_ctrl { */ unsigned long submit_bitmap; struct readahead_control *ractl;

• /*

• • The start/len of the last hit compressed extent map.
• • The current btrfs_bio_is_contig() only uses disk_bytenr as
• • the condition to check if the read can be merged with previous
• • bio, which is not correct. E.g. two file extents pointing to the
• • same extent.
• • So here we need to do extra check to merge reads that are
• • covered by the same extent map.
• • Just extent_map::start/len will be enough, as they are unique
• • inside the same inode.

• */

• u64 last_compress_em_start;

• u64 last_compress_em_len; };

  /*

@@ -965,7 +981,7 @@ static void btrfs_readahead_expand(struct readahead_control *ractl,

• return 0 on success, otherwise return error

*/ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,

•  struct btrfs_bio_ctrl *bio_ctrl, u64 *prev_em_start)
  

• struct btrfs_bio_ctrl *bio_ctrl)
  

  { struct inode *inode = folio->mapping->host; struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);

@@ -1075,13 +1091,15 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,

• is a corner case so we prioritize correctness over
• non-optimal behavior (submitting 2 bios for the same extent).

*/

• if (compress_type != BTRFS_COMPRESS_NONE &&
• prev_em_start && *prev_em_start != (u64)-1 &&
• *prev_em_start != em->start)
• force_bio_submit = true;
• if (prev_em_start)
• *prev_em_start = em->start;

• if (compress_type != BTRFS_COMPRESS_NONE) {
• if (bio_ctrl->last_compress_em_start != U64_MAX &&

Since we have last_compress_em_len and it's initialized to 0, we can use that em_len instead of using U64_MAX for em_start.

Since we should never hit an em with 0 length.

• (em->start != bio_ctrl->last_compress_em_start ||

• em->len != bio_ctrl->last_compress_em_len))
  

• force_bio_submit = true;

• bio_ctrl->last_compress_em_start = em->start;

• bio_ctrl->last_compress_em_len = em->len;

• }

  em_gen = em->generation; btrfs_free_extent_map(em);

@@ -1296,12 +1314,15 @@ int btrfs_read_folio(struct file *file, struct folio *folio) const u64 start = folio_pos(folio); const u64 end = start + folio_size(folio) - 1; struct extent_state *cached_state = NULL;

• struct btrfs_bio_ctrl bio_ctrl = { .opf = REQ_OP_READ };

• struct btrfs_bio_ctrl bio_ctrl = {

• .opf = REQ_OP_READ,

• .last_compress_em_start = U64_MAX,

• }; struct extent_map *em_cached = NULL; int ret;

  lock_extents_for_read(inode, start, end, &cached_state);

• ret = btrfs_do_readpage(folio, &em_cached, &bio_ctrl, NULL);

• ret = btrfs_do_readpage(folio, &em_cached, &bio_ctrl); btrfs_unlock_extent(&inode->io_tree, start, end, &cached_state);

  btrfs_free_extent_map(em_cached);

@@ -2641,7 +2662,8 @@ void btrfs_readahead(struct readahead_control *rac) { struct btrfs_bio_ctrl bio_ctrl = { .opf = REQ_OP_READ | REQ_RAHEAD,

• .ractl = rac

• .ractl = rac,
• .last_compress_em_start = U64_MAX, }; struct folio *folio; struct btrfs_inode *inode = BTRFS_I(rac->mapping->host);

@@ -2649,12 +2671,11 @@ void btrfs_readahead(struct readahead_control *rac) const u64 end = start + readahead_length(rac) - 1; struct extent_state *cached_state = NULL; struct extent_map *em_cached = NULL;

• u64 prev_em_start = (u64)-1;

  lock_extents_for_read(inode, start, end, &cached_state);

  while ((folio = readahead_folio(rac)) != NULL)

• btrfs_do_readpage(folio, &em_cached, &bio_ctrl, &prev_em_start);

• btrfs_do_readpage(folio, &em_cached, &bio_ctrl);

  btrfs_unlock_extent(&inode->io_tree, start, end, &cached_state);

Thanks a lot for the simplified version, will update the patch with this change and a more simplified reproducer.

Thanks, Qu

Thanks.

...

             em_gen = em->generation;
             btrfs_free_extent_map(em);
             em = NULL;

-- 2.50.1