From: Qu Wenruo <wqu@suse.com>
To: linux-btrfs@vger.kernel.org
Cc: Johannes.Thumshirn@wdc.com
Subject: [PATCH v2] btrfs: scrub: handle RST lookup error correctly
Date: Tue, 18 Jun 2024 14:58:20 +0930 [thread overview]
Message-ID: <67e8eaaf47d261c3fb3f26dd1463c06dd3412d5e.1718688466.git.wqu@suse.com> (raw)
[BUG]
When running btrfs/060 with forced RST feature, it would crash the
following ASSERT() inside scrub_read_endio():
ASSERT(sector_nr < stripe->nr_sectors);
Before that, we would have tree dump from
btrfs_get_raid_extent_offset(), as we failed to find the RST entry for
the range.
[CAUSE]
Inside scrub_submit_extent_sector_read() every time we allocated a new
bbio we immediate call btrfs_map_block() to make sure there is some RST
range covering the scrub target.
But if btrfs_map_block() failed, we immediately call endio for the bbio,
meanwhile since the bbio is newly allocated, it's completely empty.
Then inside scrub_read_endio(), we go through the bvecs to find
the sector number (as bi_sector is no longer reliable if the bio is
submitted to lower layers).
And since the bio is empty, such bvecs iteration would not find any
sector matching the sector, and return sector_nr == stripe->nr_sectors,
triggering the ASSERT().
[FIX]
Instead of calling btrfs_map_block() after allocating a new bbio, call
btrfs_map_block() first.
Since our only objective of calling btrfs_map_block() is only to update
stripe_len, there is really no need to do that after btrfs_alloc_bio().
This new timing would avoid the problem of handling empty bbio
completely, and in fact fixes a possible race window for the old code,
where if the submission thread is the only owner of the pending_io, the
scrub would never finish (since we didn't decrease the pending_io
counter).
Although the root cause of RST lookup failure still needs to be
addressed.
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
In fact, with this bug patched, I still hit an RST related problem, and
it's from OE finishing, thus I believe there are still other bugs in the
RST related code.
Changelog:
v2:
- Only mark the current sector as error and continue to the next sector
This would make the error handling of RST error more accurate, other
than marking all the remaining sectors as error.
RFC->v1:
- Fix the bug by altering the btrfs_map_block()
Now we have no chance to create empty bbio, and much easier to handle
the RST error.
---
fs/btrfs/scrub.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 188a9c42c9eb..35cf23a3a432 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -1688,20 +1688,24 @@ static void scrub_submit_extent_sector_read(struct scrub_ctx *sctx,
(i << fs_info->sectorsize_bits);
int err;
+ io_stripe.is_scrub = true;
+ stripe_len = (nr_sectors - i) << fs_info->sectorsize_bits;
+ /*
+ * For RST cases, we need to manually split the bbio to
+ * follow the RST boundary.
+ */
+ err = btrfs_map_block(fs_info, BTRFS_MAP_READ, logical,
+ &stripe_len, &bioc, &io_stripe, &mirror);
+ btrfs_put_bioc(bioc);
+ if (err < 0) {
+ set_bit(i, &stripe->io_error_bitmap);
+ set_bit(i, &stripe->error_bitmap);
+ continue;
+ }
+
bbio = btrfs_bio_alloc(stripe->nr_sectors, REQ_OP_READ,
fs_info, scrub_read_endio, stripe);
bbio->bio.bi_iter.bi_sector = logical >> SECTOR_SHIFT;
-
- io_stripe.is_scrub = true;
- err = btrfs_map_block(fs_info, BTRFS_MAP_READ, logical,
- &stripe_len, &bioc, &io_stripe,
- &mirror);
- btrfs_put_bioc(bioc);
- if (err) {
- btrfs_bio_end_io(bbio,
- errno_to_blk_status(err));
- return;
- }
}
__bio_add_page(&bbio->bio, page, fs_info->sectorsize, pgoff);
--
2.45.2
next reply other threads:[~2024-06-18 5:28 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-18 5:28 Qu Wenruo [this message]
2024-06-18 8:13 ` [PATCH v2] btrfs: scrub: handle RST lookup error correctly Johannes Thumshirn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=67e8eaaf47d261c3fb3f26dd1463c06dd3412d5e.1718688466.git.wqu@suse.com \
--to=wqu@suse.com \
--cc=Johannes.Thumshirn@wdc.com \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox