[PATCH] btrfs: scrub: respect the read-only flag during repair

[PATCH] btrfs: scrub: respect the read-only flag during repair

[Qu Wenruo]

[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(-)

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;
 
-- 
2.40.1

Re: [PATCH] btrfs: scrub: respect the read-only flag during repair

[David Sterba]

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.

Re: [PATCH] btrfs: scrub: respect the read-only flag during repair

[David Sterba]

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.

Re: [PATCH] btrfs: scrub: respect the read-only flag during repair

[Qu Wenruo]

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.