Re: Missing unread page handling in readpages

[Filipe Manana <fdmanana@gmail.com>]

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.”