Missing unread page handling in readpages

Missing unread page handling in readpages

[Matthew Wilcox]

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.  Certainly the VFS does not; the filesystem is presumed to
unlock those pages when IO finishes.  But AFAICT, returning an error
there means btrfs will never start IO on those pages submit_one_bio()
is a btrfs function.  It calls tree->ops->submit_bio_hook() so that's
either btree_submit_bio_hook() or btrfs_submit_bio_hook(), which certainly
seem like they might be able to return errors.

So do we need to do something to complete the bio with an error in
order to unlock those pages?  Or have I failed to notice that already
happening somewhere?

Re: Missing unread page handling in readpages

[Filipe Manana]

On Sat, Feb 15, 2020 at 5:22 AM Matthew Wilcox <willy@infradead.org> 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

>  Certainly the VFS does not; the filesystem is presumed to
> unlock those pages when IO finishes.  But AFAICT, returning an error
> there means btrfs will never start IO on those pages submit_one_bio()
> is a btrfs function.  It calls tree->ops->submit_bio_hook() so that's
> either btree_submit_bio_hook() or btrfs_submit_bio_hook(), which certainly
> seem like they might be able to return errors.
>
> So do we need to do something to complete the bio with an error in
> order to unlock those pages?  Or have I failed to notice that already
> happening somewhere?
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: Missing unread page handling in readpages

[Matthew Wilcox]

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@infradead.org> 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()?