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