Re: [PATCH 12/15] btrfs: add new read repair infrastructure

From: Christoph Hellwig <hch@lst.de>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: Christoph Hellwig <hch@lst.de>, Chris Mason <clm@fb.com>,
	Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>, Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 12/15] btrfs: add new read repair infrastructure
Date: Fri, 20 May 2022 17:25:38 +0200	[thread overview]
Message-ID: <20220520152538.GA21215@lst.de> (raw)
In-Reply-To: <e2c4e54c-548b-2221-3bf7-31f6c14a03ee@gmx.com>

On Wed, May 18, 2022 at 07:04:22AM +0800, Qu Wenruo wrote:
> A big NONO here.
>
> Function btrfs_repair_read_bio() will only return true if all of its
> data matches csum.
>
> Consider the following case:
>
> Profile RAID1C3, 2 sectors to read, the initial mirror is 1.
>
> Mirror 1:	|X|X|
> Mirror 2:	|X| |
> Mirror 3:	| |X|
>
> Now we will got -EIO, but in reality, we can repair the read by using
> the first sector from mirror 3 and the 2nd sector from mirror 2.
>
> This is a behavior regression.

Now that I've written tests and code to treat this properly I have to
say that at least on x86 I can't actually trigger this behavior
regression and had to add instrumentation to see a low-level change
in behavior.

Why?

For some reason (and I'd love to see an explanation for it!), btrfs
limits direct I/O reads to a single sector, so for direct I/O is is
impossible to hit this case as all bios (and thus repair bios) are
limited to a single sector.

For buffered I/O we can hit this case, but all ->readahead error are
ignored and the pages actually needed are eventually retried using
->readpage.  And ->reapage is limit to a single sector.   But given
that btrfs does not seem to support sub-4K sector sizes I can't
actually test the sub-sector code here - I assume this might be
an issue on larger page size systems.