On 9.03.20 г. 23:32 ч., Omar Sandoval wrote:
> From: Omar Sandoval <osandov@xxxxxx>
>
> 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@xxxxxx>
> ---
> 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?
> - 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.
> + 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.
> + 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) {
> + need_validation = true;
> + break;
> + }
> }
> }
> }
> @@ -2674,32 +2646,41 @@ static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset,
> if (!btrfs_check_repairable(inode, need_validation, failrec,
> failed_mirror)) {
> free_io_failure(failure_tree, tree, failrec);
> - return -EIO;
> + return BLK_STS_IOERR;
> }
>
> + bio = btrfs_io_bio_alloc(1);
> + io_bio = btrfs_io_bio(bio);
> + bio->bi_opf = REQ_OP_READ;
> if (need_validation)
> - read_mode |= REQ_FAILFAST_DEV;
> + bio->bi_opf |= REQ_FAILFAST_DEV;
> + bio->bi_end_io = failed_bio->bi_end_io;
> + bio->bi_iter.bi_sector = failrec->logical >> 9;
> + bio->bi_private = failed_bio->bi_private;
nit: I'd rather have this named as repair_bio, right now the function has:
failed_bio, failed_io_bio and simply bio. But in fact the latter is a
repair bio derived from the io_failured_record.
>
> - phy_offset >>= inode->i_sb->s_blocksize_bits;
> - bio = btrfs_create_repair_bio(inode, failed_bio, failrec, page,
> - start - page_offset(page),
> - (int)phy_offset, failed_bio->bi_end_io,
> - NULL);
> - bio->bi_opf = REQ_OP_READ | read_mode;
> + if (failed_io_bio->csum) {
> + u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
> +
> + io_bio->csum = io_bio->csum_inline;
> + memcpy(io_bio->csum, failed_io_bio->csum + csum_size * icsum,
> + csum_size);
> + }
> +
> + bio_add_page(bio, page, failrec->len, pgoff);
> + io_bio->logical = failrec->start;
> + io_bio->iter = bio->bi_iter;
>
> btrfs_debug(btrfs_sb(inode->i_sb),
> - "Repair Read Error: submitting new read[%#x] to this_mirror=%d, in_validation=%d",
> - read_mode, failrec->this_mirror, failrec->in_validation);
> +"Repair Read Error: submitting new read to this_mirror=%d, in_validation=%d",
> + failrec->this_mirror, failrec->in_validation);
>
> - status = tree->ops->submit_bio_hook(tree->private_data, bio, failrec->this_mirror,
> - failrec->bio_flags);
> + status = submit_bio_hook(inode, bio, failrec->this_mirror,
> + failrec->bio_flags);
> if (status) {
> free_io_failure(failure_tree, tree, failrec);
> bio_put(bio);
> - ret = blk_status_to_errno(status);
> }
> -
> - return ret;
> + return status;
> }
>
> /* lots and lots of room for performance fixes in the end_bio funcs */
<snip>
>
