Re: [PATCH] btrfs-progs: check: Also compare data between mirrors to detect corruption for NODATASUM extents

[james harvey <jamespharvey20@gmail.com>]

On Tue, May 29, 2018 at 3:45 AM, Qu Wenruo <wqu@suse.com> wrote:
> As the lzo corruption reported by James Harvey, for data extents without
> checksum, neither btrfs check nor kernel scrub could detect anything
> wrong.
>
> However if our profile supports duplication, we still have a chance to
> detect such corruption, as in that case the mirrors will mismatch with
> each other.
>
> So enhance --check-data-csum option to do such check, and this could
> help us to detect such corruption in an autonomous test case.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

I think it's good to have check --check-data-csum do such check.

I also think there should be a check option that only checks the
mirrored copies, so it could be ran without taking the time to check
everything that scrub looks at.  (If the feature isn't also added to
scrub as I argue for below, one could run this option every time they
scrub.)

IMO, it is important to also add this to scrub.  First reason is to
allow online scanning, since check is recommended not to run with
--force on a quiescent/read-only fs.  Second, I think users expect
scrub to ensure mirrored data integrity, and this goes along with
that, of course without automatic correction.  I think without doing
so, users who periodically run scrub would be surprised if they had
data corruption, that it hadn't been being checked all along.

It's recommended many places that users setup scrub to run
automatically on a periodic basis, but I'm not sure if I've seen
anywhere recommend setting up check to do so as well, especially as
doing so on a mounted filesystem is discouraged and potentially will
crash.

I haven't looked at this specifically, but I'm thinking this would be
better in the btrfs-progs side of scrub after it runs the kernel side
real scrub.

I have to head out for a while.  I haven't looked over or tried the
code yet, but will later.