On Sat, Feb 15, 2020 at 11:16:46AM +0000, Filipe Manana wrote:
> On Sat, Feb 15, 2020 at 5:22 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> >
> >
> > As part of my rewrite of the readahead code, I think I spotted a hole
> > in btrfs' handling of errors in the current readpages code.
> >
> > btrfs_readpages() calls extent_readpages() calls (a number of things)
> > then finishes up by calling submit_one_bio(). If submit_one_bio()
> > returns an error, I believe btrfs never unlocks the pages which were in
> > that bio.
>
> So, the pages are unlocked at end_bio_extent_readpage().
>
> The bio created by extent_readpages() (more specifically at
> __do_readpage()) has its ->bi_end_io set to end_bio_extent_readpage().
> Then submit_one_bio() calls btrfs_submit_bio_hook() or
> btree_submit_bio_hook(). When these functions return an error, they
> set the bio's ->bi_status to an error and then call bio_endio(), which
> results in end_bio_extent_readpage() being called, and that will
> unlock the pages, call SetPageError(), and do all necessary error
> handling.
Thanks! I missed that call to bio_endio(). Now, the reason I started
down this track is that submit_one_bio() is marked as __must_check.
In the readahead code, there's really nothing to be done with an error.
So I can shut up the check by doing something like:
if (bio) {
if (submit_one_bio(bio, 0, bio_flags))
return;
}
but that's kind of crappy. Can we acknowledge there is legitimately
nothing to do on an error for this user and remove the __must_check
from submit_one_bio()?
