On 17.04.20 г. 0:46 ч., Omar Sandoval wrote:
> From: Omar Sandoval <osandov@xxxxxx>
>
> Direct I/O read repair was originally implemented in commit 8b110e393c5a
> ("Btrfs: implement repair function when direct read fails"). This
> implementation is unnecessarily complicated. There is major code
> duplication between __btrfs_subio_endio_read() (checks checksums and
> handles I/O errors for files with checksums),
> __btrfs_correct_data_nocsum() (handles I/O errors for files without
> checksums), btrfs_retry_endio() (checks checksums and handles I/O errors
> for retries of files with checksums), and btrfs_retry_endio_nocsum()
> (handles I/O errors for retries of files without checksum). If it sounds
> like these should be one function, that's because they should.
> Additionally, these functions are very hard to follow due to their
> excessive use of goto.
>
> This commit replaces the original implementation. After the previous
> commit getting rid of orig_bio, we can reuse the same endio callback for
> repair I/O and the original I/O, we just need to track the file offset
> and original iterator in the repair bio. We can also unify the handling
> of files with and without checksums and simplify the control flow. We
> also no longer have to wait for each repair I/O to complete one by one.
>
> Reviewed-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> Signed-off-by: Omar Sandoval <osandov@xxxxxx>
> ---
> fs/btrfs/extent_io.c | 2 +
> fs/btrfs/inode.c | 268 +++++++------------------------------------
> 2 files changed, 44 insertions(+), 226 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 85e98ba349a8..6e1d97bb7652 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2631,6 +2631,8 @@ struct bio *btrfs_create_repair_bio(struct inode *inode, struct bio *failed_bio,
> }
>
> bio_add_page(bio, page, failrec->len, pg_offset);
> + btrfs_io_bio(bio)->logical = failrec->start;
> + btrfs_io_bio(bio)->iter = bio->bi_iter;
>
> return bio;
> }
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 79b884d2f3ed..2580f2d251d4 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7435,19 +7435,17 @@ static int btrfs_check_dio_repairable(struct inode *inode,
>
> static blk_status_t dio_read_error(struct inode *inode, struct bio *failed_bio,
> struct page *page, unsigned int pgoff,
> - u64 start, u64 end, int failed_mirror,
> - bio_end_io_t *repair_endio, void *repair_arg)
> + u64 start, u64 end, int failed_mirror)
> {
> + struct btrfs_dio_private *dip = failed_bio->bi_private;
> struct io_failure_record *failrec;
> struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
> struct extent_io_tree *failure_tree = &BTRFS_I(inode)->io_failure_tree;
> struct bio *bio;
> int isector;
> unsigned int read_mode = 0;
> - int segs;
> int ret;
> blk_status_t status;
> - struct bio_vec bvec;
>
> BUG_ON(bio_op(failed_bio) == REQ_OP_WRITE);
>
> @@ -7462,261 +7460,79 @@ static blk_status_t dio_read_error(struct inode *inode, struct bio *failed_bio,
> return BLK_STS_IOERR;
> }
>
> - segs = bio_segments(failed_bio);
> - bio_get_first_bvec(failed_bio, &bvec);
> - if (segs > 1 ||
> - (bvec.bv_len > btrfs_inode_sectorsize(inode)))
> + if (btrfs_io_bio(failed_bio)->iter.bi_size > inode->i_sb->s_blocksize)
> read_mode |= REQ_FAILFAST_DEV;
>
> isector = start - btrfs_io_bio(failed_bio)->logical;
> isector >>= inode->i_sb->s_blocksize_bits;
> - bio = btrfs_create_repair_bio(inode, failed_bio, failrec, page,
> - pgoff, isector, repair_endio, repair_arg);
> + bio = btrfs_create_repair_bio(inode, failed_bio, failrec, page, pgoff,
> + isector, failed_bio->bi_end_io, dip);
> bio->bi_opf = REQ_OP_READ | read_mode;
>
> btrfs_debug(BTRFS_I(inode)->root->fs_info,
> "repair DIO read error: submitting new dio read[%#x] to this_mirror=%d, in_validation=%d",
> read_mode, failrec->this_mirror, failrec->in_validation);
>
> + refcount_inc(&dip->refs);
> status = submit_dio_repair_bio(inode, bio, failrec->this_mirror);
> if (status) {
> free_io_failure(failure_tree, io_tree, failrec);
> bio_put(bio);
> + refcount_dec(&dip->refs);
> }
>
> return status;
> }
>
> -struct btrfs_retry_complete {
> - struct completion done;
> - struct inode *inode;
> - u64 start;
> - int uptodate;
> -};
> -
> -static void btrfs_retry_endio_nocsum(struct bio *bio)
> -{
> - struct btrfs_retry_complete *done = bio->bi_private;
> - struct inode *inode = done->inode;
> - struct bio_vec *bvec;
> - struct extent_io_tree *io_tree, *failure_tree;
> - struct bvec_iter_all iter_all;
> -
> - if (bio->bi_status)
> - goto end;
> -
> - ASSERT(bio->bi_vcnt == 1);
> - io_tree = &BTRFS_I(inode)->io_tree;
> - failure_tree = &BTRFS_I(inode)->io_failure_tree;
> - ASSERT(bio_first_bvec_all(bio)->bv_len == btrfs_inode_sectorsize(inode));
> -
> - done->uptodate = 1;
> - ASSERT(!bio_flagged(bio, BIO_CLONED));
> - bio_for_each_segment_all(bvec, bio, iter_all)
> - clean_io_failure(BTRFS_I(inode)->root->fs_info, failure_tree,
> - io_tree, done->start, bvec->bv_page,
> - btrfs_ino(BTRFS_I(inode)), 0);
> -end:
> - complete(&done->done);
> - bio_put(bio);
> -}
> -
> -static blk_status_t __btrfs_correct_data_nocsum(struct inode *inode,
> - struct btrfs_io_bio *io_bio)
> +static blk_status_t btrfs_check_read_dio_bio(struct inode *inode,
> + struct btrfs_io_bio *io_bio,
> + const bool uptodate)
> {
> - struct btrfs_fs_info *fs_info;
> + struct btrfs_fs_info *fs_info = BTRFS_I(inode)->root->fs_info;
> + u32 sectorsize = fs_info->sectorsize;
> + struct extent_io_tree *failure_tree = &BTRFS_I(inode)->io_failure_tree;
> + struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
> + const bool csum = !(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM);
> struct bio_vec bvec;
> struct bvec_iter iter;
> - struct btrfs_retry_complete done;
> - u64 start;
> - unsigned int pgoff;
> - u32 sectorsize;
> - int nr_sectors;
> - blk_status_t ret;
> + u64 start = io_bio->logical;
> + int icsum = 0;
> blk_status_t err = BLK_STS_OK;
>
> - fs_info = BTRFS_I(inode)->root->fs_info;
> - sectorsize = fs_info->sectorsize;
> -
> - start = io_bio->logical;
> - done.inode = inode;
> - io_bio->bio.bi_iter = io_bio->iter;
> + __bio_for_each_segment(bvec, &io_bio->bio, iter, io_bio->iter) {
> + unsigned int i, nr_sectors, pgoff;
> nr_sectors = BTRFS_BYTES_TO_BLKS(fs_info, bvec.bv_len);
> pgoff = bvec.bv_offset;
<snip>
> + for (i = 0; i < nr_sectors; i++) {
> ASSERT(pgoff < PAGE_SIZE);
<snip>
> + if (uptodate &&
> + (!csum || !check_data_csum(inode, io_bio, icsum,
> + bvec.bv_page, pgoff,
> + start, sectorsize))) {
> + clean_io_failure(fs_info, failure_tree, io_tree,
> + start, bvec.bv_page,
> + btrfs_ino(BTRFS_I(inode)),
> + pgoff);
Why do you have to call clean_io_failure in case there isn't a failure
since that function:
a) Will return because the tree is empty altogether (count_range_bits
returns 0).
b) Or if there was a corruption a different segment - get_state_failrec
would return -ENOENT.
Can't this be reworked to:
if (!uptodate || (csum && check_data_csum)) {
dio_read_error
}
> + } else {
> + blk_status_t status;
> +
> + status = dio_read_error(inode, &io_bio->bio,
> + bvec.bv_page, pgoff,
> + start,
> + start + sectorsize - 1,
> + io_bio->mirror_num);
> + if (status)
> + err = status;
> + }
> + start += sectorsize;
> + icsum++;
> pgoff += sectorsize;
> - ASSERT(pgoff < PAGE_SIZE);
> - goto next_block;
> }
> }
> -
> return err;
> }
<snip>
