| Message ID | 20200217184613.19668-31-willy@infradead.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Change readahead API | expand |
On 2/17/20 10:46 AM, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" <willy@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. Also added a new WARN_ON() and BUG(), although I'm wondering about the BUG below... > > Signed-off-by: Matthew Wilcox (Oracle) <willy@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) { nit: this "for" loop was perfect just the way it was. :) I'd vote here for reverting the change to a "while" loop. Because with this change, now the code has to separately initialize "done", separately increment "done", and the beauty of a for loop is that the loop init and control is all clearly in one place. For things that follow that model (as in this case!), that's a Good Thing. And I don't see any technical reason (even in the following patch) that requires this change. > - 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; > + 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); Is a full BUG_ON() definitely called for here? Seems like a WARN might suffice... > > /* > * Check that we didn't lose a page due to the arcance calling > thanks,
On Mon, Feb 17, 2020 at 10:46:11AM -0800, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" <willy@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@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.
On Tue, Feb 18, 2020 at 07:17:18PM -0800, John Hubbard wrote: > > - for (done = 0; done < length; done += ret) { > > nit: this "for" loop was perfect just the way it was. :) I'd vote here for reverting > the change to a "while" loop. Because with this change, now the code has to > separately initialize "done", separately increment "done", and the beauty of a > for loop is that the loop init and control is all clearly in one place. For things > that follow that model (as in this case!), that's a Good Thing. > > And I don't see any technical reason (even in the following patch) that requires > this change. It's doing the increment in the wrong place. We want the increment done in the middle of the loop, before we check whether we've got to the end of the page. Not at the end of the loop. > > + BUG_ON(ctx.cur_page); > > Is a full BUG_ON() definitely called for here? Seems like a WARN might suffice... Dave made a similar comment; I'll pick this up there.
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.
On Tue, Feb 18, 2020 at 10:04:15PM -0800, Matthew Wilcox wrote: > 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 ... Well, the normal thing for XFS is that a production kernel cleans up and handles the error gracefully with a WARN_ON_ONCE, while a debug kernel build will chuck a tanty and burn the house down so to make the developers aware that there is a "should not happen" situation occurring.... > > > @@ -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. None of these are appealing. > Option 4: Do the unlock/put page dance before setting the cur_page to NULL. > We might double-unlock the page. why would we double unlock the page? Oh, the readahead cursor doesn't handle the case of partial page submission, which would result in IO completion unlocking the page. Ok, that's what the ctx.cur_page_in_bio check is used to detect i.e. if we've got a page that the readahead cursor points at, and we haven't actually added it to a bio, then we can leave it to the read_pages() to unlock and clean up. If it's in a bio, then IO completion will unlock it and so we only have to drop the submission reference and move the readahead cursor forwards so read_pages() doesn't try to unlock this page. i.e: /* clean up partial page submission failures */ if (ctx.cur_page && ctx.cur_page_in_bio) { put_page(ctx.cur_page); readahead_next(rac); } looks to me like it will handle the case of "ret == 0" in the actor function just fine. Cheers, Dave.
On Wed, Feb 19, 2020 at 05:40:05PM +1100, Dave Chinner wrote: > Ok, that's what the ctx.cur_page_in_bio check is used to detect i.e. > if we've got a page that the readahead cursor points at, and we > haven't actually added it to a bio, then we can leave it to the > read_pages() to unlock and clean up. If it's in a bio, then IO > completion will unlock it and so we only have to drop the submission > reference and move the readahead cursor forwards so read_pages() > doesn't try to unlock this page. i.e: > > /* clean up partial page submission failures */ > if (ctx.cur_page && ctx.cur_page_in_bio) { > put_page(ctx.cur_page); > readahead_next(rac); > } > > looks to me like it will handle the case of "ret == 0" in the actor > function just fine. Here's what I ended up with: @@ -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; - - 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; - } + loff_t ret, done = 0; + + while (done < length) { if (!ctx->cur_page) { ctx->cur_page = iomap_next_page(inode, ctx->pages, pos, length, &done); @@ -418,6 +412,20 @@ iomap_readpages_actor(struct inode *inode, loff_t pos, loff_t length, } ret = iomap_readpage_actor(inode, pos + done, length - done, ctx, iomap, srcmap); + done += ret; + + /* Keep working on a partial page */ + if (ret && offset_in_page(pos + done)) + continue; + + if (!ctx->cur_page_in_bio) + unlock_page(ctx->cur_page); + put_page(ctx->cur_page); + ctx->cur_page = NULL; + + /* Don't loop forever if we made no progress */ + if (WARN_ON(!ret)) + break; } return done; @@ -451,11 +459,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); /* * Check that we didn't lose a page due to the arcance calling so we'll WARN if we get a ret == 0 (matching ->readpage), and we'll BUG if we ever see a page being leaked out of readpages_actor, which is a thing that should never happen and we definitely want to be noisy about if it does.
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; + 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); /* * Check that we didn't lose a page due to the arcance calling