Re: [PATCH 15/15] btrfs: unify buffered and direct I/O read repair

[Omar Sandoval <osandov@osandov.com>]

On Thu, Mar 19, 2020 at 10:53:22AM +0200, Nikolay Borisov wrote:
> 
> 
> On 9.03.20 г. 23:32 ч., Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > Currently, direct I/O has its own versions of bio_readpage_error() and
> > btrfs_check_repairable() (dio_read_error() and
> > btrfs_check_dio_repairable(), respectively). The main difference is that
> > the direct I/O version doesn't do read validation. The rework of direct
> > I/O repair makes it possible to do validation, so we can get rid of
> > btrfs_check_dio_repairable() and combine bio_readpage_error() and
> > dio_read_error() into a new helper, btrfs_submit_read_repair().
> > 
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >  fs/btrfs/extent_io.c | 126 +++++++++++++++++++------------------------
> >  fs/btrfs/extent_io.h |  17 +++---
> >  fs/btrfs/inode.c     | 103 ++++-------------------------------
> >  3 files changed, 76 insertions(+), 170 deletions(-)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index fad86ef4d09d..a5cbe04da803 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> 
> <snip>
> 
> > -/*
> > - * This is a generic handler for readpage errors. If other copies exist, read
> > - * those and write back good data to the failed position. Does not investigate
> > - * in remapping the failed extent elsewhere, hoping the device will be smart
> > - * enough to do this as needed
> > - */
> > -static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset,
> > -			      struct page *page, u64 start, u64 end,
> > -			      int failed_mirror)
> > +blk_status_t btrfs_submit_read_repair(struct inode *inode,
> > +				      struct bio *failed_bio, u64 phy_offset,
> > +				      struct page *page, unsigned int pgoff,
> > +				      u64 start, u64 end, int failed_mirror,
> > +				      submit_bio_hook_t *submit_bio_hook)
> >  {
> >  	struct io_failure_record *failrec;
> > -	struct inode *inode = page->mapping->host;
> > +	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> >  	struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree;
> >  	struct extent_io_tree *failure_tree = &BTRFS_I(inode)->io_failure_tree;
> > +	struct btrfs_io_bio *failed_io_bio = btrfs_io_bio(failed_bio);
> > +	struct btrfs_io_bio *io_bio;
> > +	int icsum = phy_offset >> inode->i_sb->s_blocksize_bits;
> >  	bool need_validation = false;
> >  	struct bio *bio;
> > -	int read_mode = 0;
> >  	blk_status_t status;
> >  	int ret;
> >  
> > +	btrfs_info(btrfs_sb(inode->i_sb),
> > +		   "Repair Read Error: read error at %llu", start);
> > +
> >  	BUG_ON(bio_op(failed_bio) == REQ_OP_WRITE);
> >  
> >  	ret = btrfs_get_io_failure_record(inode, start, end, &failrec);
> >  	if (ret)
> > -		return ret;
> > +		return errno_to_blk_status(ret);
> >  
> >  	/*
> >  	 * If there was an I/O error and the I/O was for multiple sectors, we
> >  	 * need to validate each sector individually.
> >  	 */
> >  	if (failed_bio->bi_status != BLK_STS_OK) {
> 
> Is this correct though, in case of buffered reads we are always called
> with bi_status != BLK_STS_OK (we are called from end_bio_extent_readpage
> in case uptodate is false,  which happens if failed_bio->bi_status is
> non-zero. Additionally the bio is guaranteed to not be cloned because
> there is : ASSERT(!bio_flagged(bio, BIO_CLONED));
> 
> The end effect of all of this is in case of buffered bios we never set
> need_revalidate, is this intentional?

For buffered I/O, this is called when bi_status != BLK_STS_OK OR
readpage_end_io_hook (i.e., check_data_csum()) failed. This check
distinguishes between those two cases: if we didn't hit an I/O error
(bi_status == BLK_STS_OK), then we don't need validation, otherwise, we
need validation if the bio is more than one sector.

> > -		u64 len = 0;
> > -		int i;
> > -
> > -		for (i = 0; i < failed_bio->bi_vcnt; i++) {
> > -			len += failed_bio->bi_io_vec[i].bv_len;
> > -			if (len > inode->i_sb->s_blocksize) {
> > +		if (bio_flagged(failed_bio, BIO_CLONED)) {
> 
> If I understand this correctly this is the "this is a DIO " branch. IMO
> it'd be clearer if you had bool is_dio = bio_flagged(failed_bio,
> BIO_CLONED) at the top of the function and you used that.

Repair bios for direct I/O aren't cloned, so is_dio isn't accurate. IMO
it shouldn't matter whether it came from direct I/O or not. If it's a
cloned bio, you get the size out of io_bio->iter, and if it's not, you
get it out of bi_io_vec.

> > +			if (failed_io_bio->iter.bi_size >
> > +			    inode->i_sb->s_blocksize)
> >  				need_validation = true;
> > -				break;
> > +		} else {
> 
> This branch will only ever be executed in case of DIO with csum failure.
> So either add a comment to demarcate when various leaves of the 2 'if'
> should be called or, and I think this would be the better solution,
> rewrite it.

As commented above, this outer branch is for I/O errors (not checksum
errors), and this specific branch is for non-cloned bios, which happens
to be buffered read bios and buffered or direct I/O repair bios. Would
it be clearer as:

static u64 btrfs_bio_size(struct bio *bio)
{
	if (bio_flagged(bio, BIO_CLONED))
		return bio->iter.bi_size;
	else
		return bio_size_all(bio);
}

blk_status_t btrfs_submit_read_repair(...)
{
	...
	/*
	 * If there was an I/O error and the I/O was for multiple sectors, we
	 * need to validate each sector individually.
	 */
	need_validation = (failed_bio->bi_status != BLK_STS_OK &&
			   btrfs_bio_size() > inode->i_sb->s_blocksize);
	...
}