On Wed, Feb 19, 2020 at 02:29:00PM +1100, Dave Chinner wrote:
> On Mon, Feb 17, 2020 at 10:46:11AM -0800, Matthew Wilcox wrote:
> > @@ -418,6 +412,15 @@ iomap_readpages_actor(struct inode *inode, loff_t pos, loff_t length,
> > }
> > ret = iomap_readpage_actor(inode, pos + done, length - done,
> > ctx, iomap, srcmap);
> > + if (WARN_ON(ret == 0))
> > + break;
>
> This error case now leaks ctx->cur_page....
Yes ... and I see the consequence. I mean, this is a "shouldn't happen",
so do we want to put effort into cleanup here ...
> > @@ -451,11 +454,7 @@ iomap_readpages(struct address_space *mapping, struct list_head *pages,
> > done:
> > if (ctx.bio)
> > submit_bio(ctx.bio);
> > - if (ctx.cur_page) {
> > - if (!ctx.cur_page_in_bio)
> > - unlock_page(ctx.cur_page);
> > - put_page(ctx.cur_page);
> > - }
> > + BUG_ON(ctx.cur_page);
>
> And so will now trigger both a warn and a bug....
... or do we just want to run slap bang into this bug?
Option 1: Remove the check for 'ret == 0' altogether, as we had it before.
That puts us into endless loop territory for a failure mode, and it's not
parallel with iomap_readpage().
Option 2: Remove the WARN_ON from the check. Then we just hit the BUG_ON,
but we don't know why we did it.
Option 3: Set cur_page to NULL. We'll hit the WARN_ON, avoid the BUG_ON,
might end up with a page in the page cache which is never unlocked.
Option 4: Do the unlock/put page dance before setting the cur_page to NULL.
We might double-unlock the page.
There are probably other options here too.
