Dave Chinner
2020-Feb-19 03:29 UTC
[Ocfs2-devel] [PATCH v6 17/19] iomap: Restructure iomap_readpages_actor
On Mon, Feb 17, 2020 at 10:46:11AM -0800, Matthew Wilcox wrote:> From: "Matthew Wilcox (Oracle)" <willy at infradead.org> > > By putting the 'have we reached the end of the page' condition at the end > of the loop instead of the beginning, we can remove the 'submit the last > page' code from iomap_readpages(). Also check that iomap_readpage_actor() > didn't return 0, which would lead to an endless loop. > > Signed-off-by: Matthew Wilcox (Oracle) <willy at infradead.org> > --- > fs/iomap/buffered-io.c | 25 ++++++++++++------------- > 1 file changed, 12 insertions(+), 13 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index cb3511eb152a..44303f370b2d 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -400,15 +400,9 @@ iomap_readpages_actor(struct inode *inode, loff_t pos, loff_t length, > void *data, struct iomap *iomap, struct iomap *srcmap) > { > struct iomap_readpage_ctx *ctx = data; > - loff_t done, ret; > + loff_t ret, done = 0; > > - for (done = 0; done < length; done += ret) { > - if (ctx->cur_page && offset_in_page(pos + done) == 0) { > - if (!ctx->cur_page_in_bio) > - unlock_page(ctx->cur_page); > - put_page(ctx->cur_page); > - ctx->cur_page = NULL; > - } > + while (done < length) { > if (!ctx->cur_page) { > ctx->cur_page = iomap_next_page(inode, ctx->pages, > pos, length, &done); > @@ -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....> + done += ret; > + if (offset_in_page(pos + done) == 0) { > + if (!ctx->cur_page_in_bio) > + unlock_page(ctx->cur_page); > + put_page(ctx->cur_page); > + ctx->cur_page = NULL; > + } > } > > return done; > @@ -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.... Cheers, Dave. -- Dave Chinner david at fromorbit.com
Matthew Wilcox
2020-Feb-19 06:04 UTC
[Ocfs2-devel] [PATCH v6 17/19] iomap: Restructure iomap_readpages_actor
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.