Re: [PATCH v5 03/13] btrfs: introduce a new helper to submit read bio for scrub

From: Christoph Hellwig <hch@infradead.org>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: Christoph Hellwig <hch@infradead.org>, Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org, David Sterba <dsterba@suse.com>
Subject: Re: [PATCH v5 03/13] btrfs: introduce a new helper to submit read bio for scrub
Date: Mon, 27 Mar 2023 17:53:24 -0700	[thread overview]
Message-ID: <ZCI6hOjU+yrQ9SCE@infradead.org> (raw)
In-Reply-To: <7ec722e8-d685-004c-6c24-6bdac7982e0b@gmx.com>

On Tue, Mar 28, 2023 at 08:48:29AM +0800, Qu Wenruo wrote:
> The new behavior itself reduces IOPS, thus it should be an overall win.
> And for regular devices or non-RST zoned devices, we just read out some
> garbage, and scrub verification code would skip them.
> 
> But for RST cases, there would be no mapping for the garbage range, thus we
> need to skip those garbage for RST cases.
> 
> The dedicated read helper would provide the location for us to handle this
> RST scrub specific case.

Well, if did just call btrfs_submit_chunk, the RST lookup would ensure
you only get the length of the RST mapping, and you get the behavior
you want without duplication.  We'd need to make it non-static (and
document it), but I'd still be much happier about that than yet another
I/O submission interface.

> 
> The core shares the same idea, only the support code is different.
> 
> I'm fine merging the core logic, although I believe we still need different
> entrance functions.
> (e.g repair is only done for one sector, requires inode/file offset, and is
> done synchronously.)

Well, the inode number and start is only used for a debug printk,
so we can easily move that into the caller.  As for the single sectors
vs not, what we really should do is to move the bio allocation
to the caller, which means the caller cna decided how large the bio
is.  Together with moving the printk that also makes the interface
much simpler as we'll need a lot less arguments.