| Message ID | 85514d999fd01d20cbabed1b346f58f0fb6f7063.1686017100.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: scrub: respect the read-only flag during repair | expand |
On Tue, Jun 06, 2023 at 10:05:04AM +0800, Qu Wenruo wrote: > [BUG] > With recent scrub rework, the scrub operation no longer respects the > read-only flag passed by "-r" option of "btrfs scrub start" command. > > # mkfs.btrfs -f -d raid1 $dev1 $dev2 > # mount $dev1 $mnt > # xfs_io -f -d -c "pwrite -b 128K -S 0xaa 0 128k" $mnt/file > # sync > # xfs_io -c "pwrite -S 0xff $phy1 64k" $dev1 > # xfs_io -c "pwrite -S 0xff $((phy2 + 65536)) 64k" $dev2 > # mount $dev1 $mnt -o ro > # btrfs scrub start -BrRd $mnt > Scrub device $dev1 (id 1) done > Scrub started: Tue Jun 6 09:59:14 2023 > Status: finished > Duration: 0:00:00 > [...] > corrected_errors: 16 <<< Still has corrupted sectors > last_physical: 1372585984 > > Scrub device $dev2 (id 2) done > Scrub started: Tue Jun 6 09:59:14 2023 > Status: finished > Duration: 0:00:00 > [...] > corrected_errors: 16 <<< Still has corrupted sectors > last_physical: 1351614464 > > # btrfs scrub start -BrRd $mnt > Scrub device $dev1 (id 1) done > Scrub started: Tue Jun 6 10:00:17 2023 > Status: finished > Duration: 0:00:00 > [...] > corrected_errors: 0 <<< No more errors > last_physical: 1372585984 > > Scrub device $dev2 (id 2) done > [...] > corrected_errors: 0 <<< No more errors > last_physical: 1372585984 > > [CAUSE] > In the newly reworked scrub code, repair is always submitted no matter > if we're doing a read-only scrub. > > [FIX] > Fix it by skipping the write submission if the scrub is a read-only one. > > Unfortunately for the report part, even for a read-only scrub we will > still report it as corrected errors, as we know it's repairable, even we > won't really submit the write. We can maybe present the results in scrub status in an different way when it's started as read-only, e.g. not to say "repaired" but "repairable (read-only)". > Fixes: e02ee89baa66 ("btrfs: scrub: switch scrub_simple_mirror() to scrub_stripe infrastructure") > Signed-off-by: Qu Wenruo <wqu@suse.com> Added to misc-next, thanks.
On Thu, Jun 08, 2023 at 08:08:25PM +0800, Qu Wenruo wrote: > > > On 2023/6/8 19:56, David Sterba wrote: > > On Tue, Jun 06, 2023 at 10:05:04AM +0800, Qu Wenruo wrote: > >> [BUG] > >> With recent scrub rework, the scrub operation no longer respects the > >> read-only flag passed by "-r" option of "btrfs scrub start" command. > >> > >> # mkfs.btrfs -f -d raid1 $dev1 $dev2 > >> # mount $dev1 $mnt > >> # xfs_io -f -d -c "pwrite -b 128K -S 0xaa 0 128k" $mnt/file > >> # sync > >> # xfs_io -c "pwrite -S 0xff $phy1 64k" $dev1 > >> # xfs_io -c "pwrite -S 0xff $((phy2 + 65536)) 64k" $dev2 > >> # mount $dev1 $mnt -o ro > >> # btrfs scrub start -BrRd $mnt > >> Scrub device $dev1 (id 1) done > >> Scrub started: Tue Jun 6 09:59:14 2023 > >> Status: finished > >> Duration: 0:00:00 > >> [...] > >> corrected_errors: 16 <<< Still has corrupted sectors > >> last_physical: 1372585984 > >> > >> Scrub device $dev2 (id 2) done > >> Scrub started: Tue Jun 6 09:59:14 2023 > >> Status: finished > >> Duration: 0:00:00 > >> [...] > >> corrected_errors: 16 <<< Still has corrupted sectors > >> last_physical: 1351614464 > >> > >> # btrfs scrub start -BrRd $mnt > >> Scrub device $dev1 (id 1) done > >> Scrub started: Tue Jun 6 10:00:17 2023 > >> Status: finished > >> Duration: 0:00:00 > >> [...] > >> corrected_errors: 0 <<< No more errors > >> last_physical: 1372585984 > >> > >> Scrub device $dev2 (id 2) done > >> [...] > >> corrected_errors: 0 <<< No more errors > >> last_physical: 1372585984 > >> > >> [CAUSE] > >> In the newly reworked scrub code, repair is always submitted no matter > >> if we're doing a read-only scrub. > >> > >> [FIX] > >> Fix it by skipping the write submission if the scrub is a read-only one. > >> > >> Unfortunately for the report part, even for a read-only scrub we will > >> still report it as corrected errors, as we know it's repairable, even we > >> won't really submit the write. > > > > We can maybe present the results in scrub status in an different way > > when it's started as read-only, e.g. not to say "repaired" but > > "repairable (read-only)". > > Yes, this is much better for end users. > > But unfortunately we're still introducing a behavior change, even it's > just the report part. > > Before the rework, read-only scrub would only report errors, no > repairable/correctable checks, thus it would report csum_errors=32 and > corrected_errors=0 for example. > > Now we will report csum_errors=32 and corrected_errors=32. > > With progs changes, it would be an improvement, but still a behavior change. Sometimes we need to do such changes, I don't treat report as an ABI but at least the negative impact should be minimized. If the before/after can be recognized it's still better than printing the same information with a different meaning.
On 2023/6/8 19:56, David Sterba wrote: > On Tue, Jun 06, 2023 at 10:05:04AM +0800, Qu Wenruo wrote: >> [BUG] >> With recent scrub rework, the scrub operation no longer respects the >> read-only flag passed by "-r" option of "btrfs scrub start" command. >> >> # mkfs.btrfs -f -d raid1 $dev1 $dev2 >> # mount $dev1 $mnt >> # xfs_io -f -d -c "pwrite -b 128K -S 0xaa 0 128k" $mnt/file >> # sync >> # xfs_io -c "pwrite -S 0xff $phy1 64k" $dev1 >> # xfs_io -c "pwrite -S 0xff $((phy2 + 65536)) 64k" $dev2 >> # mount $dev1 $mnt -o ro >> # btrfs scrub start -BrRd $mnt >> Scrub device $dev1 (id 1) done >> Scrub started: Tue Jun 6 09:59:14 2023 >> Status: finished >> Duration: 0:00:00 >> [...] >> corrected_errors: 16 <<< Still has corrupted sectors >> last_physical: 1372585984 >> >> Scrub device $dev2 (id 2) done >> Scrub started: Tue Jun 6 09:59:14 2023 >> Status: finished >> Duration: 0:00:00 >> [...] >> corrected_errors: 16 <<< Still has corrupted sectors >> last_physical: 1351614464 >> >> # btrfs scrub start -BrRd $mnt >> Scrub device $dev1 (id 1) done >> Scrub started: Tue Jun 6 10:00:17 2023 >> Status: finished >> Duration: 0:00:00 >> [...] >> corrected_errors: 0 <<< No more errors >> last_physical: 1372585984 >> >> Scrub device $dev2 (id 2) done >> [...] >> corrected_errors: 0 <<< No more errors >> last_physical: 1372585984 >> >> [CAUSE] >> In the newly reworked scrub code, repair is always submitted no matter >> if we're doing a read-only scrub. >> >> [FIX] >> Fix it by skipping the write submission if the scrub is a read-only one. >> >> Unfortunately for the report part, even for a read-only scrub we will >> still report it as corrected errors, as we know it's repairable, even we >> won't really submit the write. > > We can maybe present the results in scrub status in an different way > when it's started as read-only, e.g. not to say "repaired" but > "repairable (read-only)". Yes, this is much better for end users. But unfortunately we're still introducing a behavior change, even it's just the report part. Before the rework, read-only scrub would only report errors, no repairable/correctable checks, thus it would report csum_errors=32 and corrected_errors=0 for example. Now we will report csum_errors=32 and corrected_errors=32. With progs changes, it would be an improvement, but still a behavior change. BTW, the rework also fixed a report bug (I didn't even notice), that csum_discards is incorrectly reported before the rework. But I guess no one even noticed anyway... Thanks, Qu > >> Fixes: e02ee89baa66 ("btrfs: scrub: switch scrub_simple_mirror() to scrub_stripe infrastructure") >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > Added to misc-next, thanks.
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index 28caad17ccc7..375c1f8fef4d 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -1695,7 +1695,7 @@ static int flush_scrub_stripes(struct scrub_ctx *sctx) break; } } - } else { + } else if (!sctx->readonly) { for (int i = 0; i < nr_stripes; i++) { unsigned long repaired;
[BUG] With recent scrub rework, the scrub operation no longer respects the read-only flag passed by "-r" option of "btrfs scrub start" command. # mkfs.btrfs -f -d raid1 $dev1 $dev2 # mount $dev1 $mnt # xfs_io -f -d -c "pwrite -b 128K -S 0xaa 0 128k" $mnt/file # sync # xfs_io -c "pwrite -S 0xff $phy1 64k" $dev1 # xfs_io -c "pwrite -S 0xff $((phy2 + 65536)) 64k" $dev2 # mount $dev1 $mnt -o ro # btrfs scrub start -BrRd $mnt Scrub device $dev1 (id 1) done Scrub started: Tue Jun 6 09:59:14 2023 Status: finished Duration: 0:00:00 [...] corrected_errors: 16 <<< Still has corrupted sectors last_physical: 1372585984 Scrub device $dev2 (id 2) done Scrub started: Tue Jun 6 09:59:14 2023 Status: finished Duration: 0:00:00 [...] corrected_errors: 16 <<< Still has corrupted sectors last_physical: 1351614464 # btrfs scrub start -BrRd $mnt Scrub device $dev1 (id 1) done Scrub started: Tue Jun 6 10:00:17 2023 Status: finished Duration: 0:00:00 [...] corrected_errors: 0 <<< No more errors last_physical: 1372585984 Scrub device $dev2 (id 2) done [...] corrected_errors: 0 <<< No more errors last_physical: 1372585984 [CAUSE] In the newly reworked scrub code, repair is always submitted no matter if we're doing a read-only scrub. [FIX] Fix it by skipping the write submission if the scrub is a read-only one. Unfortunately for the report part, even for a read-only scrub we will still report it as corrected errors, as we know it's repairable, even we won't really submit the write. Fixes: e02ee89baa66 ("btrfs: scrub: switch scrub_simple_mirror() to scrub_stripe infrastructure") Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/scrub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)