| Message ID | 20210707115524.2242151-3-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iomap: small block problems | expand |
On Wed, Jul 07, 2021 at 01:55:23PM +0200, Andreas Gruenbacher wrote: > @@ -252,6 +253,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > } > > /* zero post-eof blocks as the page may be mapped */ > + iop = iomap_page_create(inode, page); > iomap_adjust_read_range(inode, iop, &pos, length, &poff, &plen); > if (plen == 0) > goto done; I /think/ a subsequent patch would look like this: + /* No need to create an iop if the page is within an extent */ + loff_t page_pos = page_offset(page); + if (pos > page_pos || pos + length < page_pos + page_size(page)) + iop = iomap_page_create(inode, page); but that might miss some other reasons to create an iop.
On Wed, Jul 07, 2021 at 03:28:47PM +0100, Matthew Wilcox wrote: > On Wed, Jul 07, 2021 at 01:55:23PM +0200, Andreas Gruenbacher wrote: > > @@ -252,6 +253,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > > } > > > > /* zero post-eof blocks as the page may be mapped */ > > + iop = iomap_page_create(inode, page); > > iomap_adjust_read_range(inode, iop, &pos, length, &poff, &plen); > > if (plen == 0) > > goto done; > > I /think/ a subsequent patch would look like this: > > + /* No need to create an iop if the page is within an extent */ > + loff_t page_pos = page_offset(page); > + if (pos > page_pos || pos + length < page_pos + page_size(page)) > + iop = iomap_page_create(inode, page); > > but that might miss some other reasons to create an iop. I was under the impression that for blksize<pagesize filesystems, the page always had to have an iop attached. In principle I think you're right that we don't need one if all i_blocks_per_page blocks have the same uptodate state, but someone would have to perform a close reading of buffered-io.c to make it drop them when unnecessary and re-add them if it becomes necessary. That might be more cycling through kmem_alloc than we like, but as I said, I have never studied this idea. --D
On Wed, Jul 07, 2021 at 01:55:23PM +0200, Andreas Gruenbacher wrote: > In iomap_readpage_actor, don't create iop objects for inline inodes. > Otherwise, iomap_read_inline_data will set PageUptodate without setting > iop->uptodate, and iomap_page_release will eventually complain. > > To prevent this kind of bug from occurring in the future, make sure the > page doesn't have private data attached in iomap_read_inline_data. > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> Looks good to me, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/iomap/buffered-io.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 598fcfabc337..6330dabc451e 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -215,6 +215,7 @@ iomap_read_inline_data(struct inode *inode, struct page *page, > if (PageUptodate(page)) > return; > > + BUG_ON(page_has_private(page)); > BUG_ON(page->index); > BUG_ON(size > PAGE_SIZE - offset_in_page(iomap->inline_data)); > > @@ -239,7 +240,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > { > struct iomap_readpage_ctx *ctx = data; > struct page *page = ctx->cur_page; > - struct iomap_page *iop = iomap_page_create(inode, page); > + struct iomap_page *iop; > bool same_page = false, is_contig = false; > loff_t orig_pos = pos; > unsigned poff, plen; > @@ -252,6 +253,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > } > > /* zero post-eof blocks as the page may be mapped */ > + iop = iomap_page_create(inode, page); > iomap_adjust_read_range(inode, iop, &pos, length, &poff, &plen); > if (plen == 0) > goto done; > -- > 2.26.3 >
On Wed, Jul 07, 2021 at 01:55:23PM +0200, Andreas Gruenbacher wrote: > In iomap_readpage_actor, don't create iop objects for inline inodes. > Otherwise, iomap_read_inline_data will set PageUptodate without setting > iop->uptodate, and iomap_page_release will eventually complain. > > To prevent this kind of bug from occurring in the future, make sure the > page doesn't have private data attached in iomap_read_inline_data. > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> Ok, given that you want a quick fix this looks good for now: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, Jul 08, 2021 at 09:27:37PM -0700, Darrick J. Wong wrote: > I was under the impression that for blksize<pagesize filesystems, the > page always had to have an iop attached. Currently it does. But I've talked since day one of the !bufferhead iomap code that we should eventually lift that.
On Thu, Jul 08, 2021 at 09:27:37PM -0700, Darrick J. Wong wrote: > On Wed, Jul 07, 2021 at 03:28:47PM +0100, Matthew Wilcox wrote: > > On Wed, Jul 07, 2021 at 01:55:23PM +0200, Andreas Gruenbacher wrote: > > > @@ -252,6 +253,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > > > } > > > > > > /* zero post-eof blocks as the page may be mapped */ > > > + iop = iomap_page_create(inode, page); > > > iomap_adjust_read_range(inode, iop, &pos, length, &poff, &plen); > > > if (plen == 0) > > > goto done; > > > > I /think/ a subsequent patch would look like this: > > > > + /* No need to create an iop if the page is within an extent */ > > + loff_t page_pos = page_offset(page); > > + if (pos > page_pos || pos + length < page_pos + page_size(page)) > > + iop = iomap_page_create(inode, page); > > > > but that might miss some other reasons to create an iop. > > I was under the impression that for blksize<pagesize filesystems, the > page always had to have an iop attached. In principle I think you're > right that we don't need one if all i_blocks_per_page blocks have the > same uptodate state, but someone would have to perform a close reading > of buffered-io.c to make it drop them when unnecessary and re-add them > if it becomes necessary. That might be more cycling through kmem_alloc > than we like, but as I said, I have never studied this idea. I wouldn't free them unnecessarily; that is, once we've determined that we need an iop, we should just keep it, even once the entire page is Uptodate (because we'll need it for write-out eventually anyway). I haven't noticed any ill-effects from discarding iops while running xfstests on the THP/multipage folio patches. That will discard iops when splitting a page (the page must be entirely uptodate at that point).
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 598fcfabc337..6330dabc451e 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -215,6 +215,7 @@ iomap_read_inline_data(struct inode *inode, struct page *page, if (PageUptodate(page)) return; + BUG_ON(page_has_private(page)); BUG_ON(page->index); BUG_ON(size > PAGE_SIZE - offset_in_page(iomap->inline_data)); @@ -239,7 +240,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, { struct iomap_readpage_ctx *ctx = data; struct page *page = ctx->cur_page; - struct iomap_page *iop = iomap_page_create(inode, page); + struct iomap_page *iop; bool same_page = false, is_contig = false; loff_t orig_pos = pos; unsigned poff, plen; @@ -252,6 +253,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, } /* zero post-eof blocks as the page may be mapped */ + iop = iomap_page_create(inode, page); iomap_adjust_read_range(inode, iop, &pos, length, &poff, &plen); if (plen == 0) goto done;
In iomap_readpage_actor, don't create iop objects for inline inodes. Otherwise, iomap_read_inline_data will set PageUptodate without setting iop->uptodate, and iomap_page_release will eventually complain. To prevent this kind of bug from occurring in the future, make sure the page doesn't have private data attached in iomap_read_inline_data. Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> --- fs/iomap/buffered-io.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)