Re: [PATCH 13/15] btrfs: simplify direct I/O read repair

[David Sterba <dsterba@suse.cz>]

On Mon, Mar 09, 2020 at 02:32:39PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Direct I/O read repair is an over-complicated mess. 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.
> 
> 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 replace
> the atrocity that was probably the inspiration for "Go To Statement
> Considered Harmful" with normal loops. We also no longer have to wait
> for each repair I/O to complete one by one.

This patch looks like a revert of 8b110e393c5a ("Btrfs: implement repair
function when direct read fails"), that probably added the extra layer
you're removing.

So instead of the funny remarks, I'd rather see some analysis that the
issues in the original patch are not coming back.  Quoting from the
changelog:

- When we find the data is not right, we try to read the data from the other
  mirror.
- When the io on the mirror ends, we will insert the endio work into the
  dedicated btrfs workqueue, not common read endio workqueue, because the
  original endio work is still blocked in the btrfs endio workqueue, if we
  insert the endio work of the io on the mirror into that workqueue, deadlock
  would happen.
- After we get right data, we write it back to the corrupted mirror.
- And if the data on the new mirror is still corrupted, we will try next
  mirror until we read right data or all the mirrors are traversed.
- After the above work, we set the uptodate flag according to the result.

It's not too detailed either, but what immediatelly looks suspicious is
the extra workqueue that was added to avoid deadlocks. That is now going
to be removed. This seems like a high level change even for such an old
code (2014) so that its effects are not affected by some other changes
in the dio code.