[PATCH v3 0/3] iomap: small block problems

[PATCH v3 0/3] iomap: small block problems

[Andreas Gruenbacher]

Here's another update of this patch queue.  Changes from v2:

* Reverse the order of the first two patches to make the queue bisect
  clean.  Adjust the patch descriptions accordingly.

* With the second patch, iomap_readpage_actor currently still creates
  iops.  Christoph has indicated that this should now be unnecessary
  as well, but testing has proven that we're not quite at that point,
  yet.

* Don't create iomap_page objects in iomap_page_mkwrite_actor anymore;
  this clearly has become obsolete with the first patch.

Thanks,
Andreas

Andreas Gruenbacher (3):
  iomap: Permit pages without an iop to enter writeback
  iomap: Don't create iomap_page objects for inline files
  iomap: Don't create iomap_page objects in iomap_page_mkwrite_actor

 fs/iomap/buffered-io.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.26.3

[PATCH v3 1/3] iomap: Permit pages without an iop to enter writeback

[Andreas Gruenbacher]

Create an iop in the writeback path if one doesn't exist.  This allows us
to avoid creating the iop in some cases.  We'll initially do that for pages
with inline data, but it can be extended to pages which are entirely within
an extent.  It also allows for an iop to be removed from pages in the
future (eg page split).

Co-developed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/buffered-io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 9023717c5188..598fcfabc337 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1334,14 +1334,13 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 		struct writeback_control *wbc, struct inode *inode,
 		struct page *page, u64 end_offset)
 {
-	struct iomap_page *iop = to_iomap_page(page);
+	struct iomap_page *iop = iomap_page_create(inode, page);
 	struct iomap_ioend *ioend, *next;
 	unsigned len = i_blocksize(inode);
 	u64 file_offset; /* file offset of page */
 	int error = 0, count = 0, i;
 	LIST_HEAD(submit_list);
 
-	WARN_ON_ONCE(i_blocks_per_page(inode, page) > 1 && !iop);
 	WARN_ON_ONCE(iop && atomic_read(&iop->write_bytes_pending) != 0);
 
 	/*
-- 
2.26.3

Re: [PATCH v3 1/3] iomap: Permit pages without an iop to enter writeback

[Darrick J. Wong]

On Wed, Jul 07, 2021 at 01:55:22PM +0200, Andreas Gruenbacher wrote:
> Create an iop in the writeback path if one doesn't exist.  This allows us
> to avoid creating the iop in some cases.  We'll initially do that for pages
> with inline data, but it can be extended to pages which are entirely within
> an extent.  It also allows for an iop to be removed from pages in the
> future (eg page split).
> 
> Co-developed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Seems simple enough...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/iomap/buffered-io.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 9023717c5188..598fcfabc337 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1334,14 +1334,13 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
>  		struct writeback_control *wbc, struct inode *inode,
>  		struct page *page, u64 end_offset)
>  {
> -	struct iomap_page *iop = to_iomap_page(page);
> +	struct iomap_page *iop = iomap_page_create(inode, page);
>  	struct iomap_ioend *ioend, *next;
>  	unsigned len = i_blocksize(inode);
>  	u64 file_offset; /* file offset of page */
>  	int error = 0, count = 0, i;
>  	LIST_HEAD(submit_list);
>  
> -	WARN_ON_ONCE(i_blocks_per_page(inode, page) > 1 && !iop);
>  	WARN_ON_ONCE(iop && atomic_read(&iop->write_bytes_pending) != 0);
>  
>  	/*
> -- 
> 2.26.3
> 

[PATCH v3 2/3] iomap: Don't create iomap_page objects for inline files

[Andreas Gruenbacher]

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(-)

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

Re: [PATCH v3 2/3] iomap: Don't create iomap_page objects for inline files

[Matthew Wilcox]

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.

Re: [PATCH v3 2/3] iomap: Don't create iomap_page objects for inline files

[Darrick J. Wong]

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

Re: [PATCH v3 2/3] iomap: Don't create iomap_page objects for inline files

[Christoph Hellwig]

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.

Re: [PATCH v3 2/3] iomap: Don't create iomap_page objects for inline files

[Matthew Wilcox]

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

Re: [PATCH v3 2/3] iomap: Don't create iomap_page objects for inline files

[Darrick J. Wong]

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
> 

Re: [PATCH v3 2/3] iomap: Don't create iomap_page objects for inline files

[Christoph Hellwig]

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>

[PATCH v3 3/3] iomap: Don't create iomap_page objects in iomap_page_mkwrite_actor

[Andreas Gruenbacher]

Now that we create those objects in iomap_writepage_map when needed,
there's no need to pre-create them in iomap_page_mkwrite_actor anymore.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/iomap/buffered-io.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 6330dabc451e..9f45050b61dd 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -999,7 +999,6 @@ iomap_page_mkwrite_actor(struct inode *inode, loff_t pos, loff_t length,
 		block_commit_write(page, 0, length);
 	} else {
 		WARN_ON_ONCE(!PageUptodate(page));
-		iomap_page_create(inode, page);
 		set_page_dirty(page);
 	}
 
-- 
2.26.3

Re: [PATCH v3 3/3] iomap: Don't create iomap_page objects in iomap_page_mkwrite_actor

[Matthew Wilcox]

On Wed, Jul 07, 2021 at 01:55:24PM +0200, Andreas Gruenbacher wrote:
> Now that we create those objects in iomap_writepage_map when needed,
> there's no need to pre-create them in iomap_page_mkwrite_actor anymore.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Thanks for sticking with this.  It looks like a nice cleanup now
rather than "argh, a bug, burn it with fire".

Re: [PATCH v3 3/3] iomap: Don't create iomap_page objects in iomap_page_mkwrite_actor

[Darrick J. Wong]

On Wed, Jul 07, 2021 at 01:55:24PM +0200, Andreas Gruenbacher wrote:
> Now that we create those objects in iomap_writepage_map when needed,
> there's no need to pre-create them in iomap_page_mkwrite_actor anymore.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

I'd like to stage this series as a bugfix branch against -rc1 next week,
if there are no other objections?

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/iomap/buffered-io.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 6330dabc451e..9f45050b61dd 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -999,7 +999,6 @@ iomap_page_mkwrite_actor(struct inode *inode, loff_t pos, loff_t length,
>  		block_commit_write(page, 0, length);
>  	} else {
>  		WARN_ON_ONCE(!PageUptodate(page));
> -		iomap_page_create(inode, page);
>  		set_page_dirty(page);
>  	}
>  
> -- 
> 2.26.3
> 

Re: [PATCH v3 3/3] iomap: Don't create iomap_page objects in iomap_page_mkwrite_actor

[Andreas Gruenbacher]

On Fri, Jul 9, 2021 at 6:29 AM Darrick J. Wong <djwong@kernel.org> wrote:
> On Wed, Jul 07, 2021 at 01:55:24PM +0200, Andreas Gruenbacher wrote:
> > Now that we create those objects in iomap_writepage_map when needed,
> > there's no need to pre-create them in iomap_page_mkwrite_actor anymore.
> >
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>
> I'd like to stage this series as a bugfix branch against -rc1 next week,
> if there are no other objections?

Yes, that would help a lot, thanks.

Andreas

Re: [PATCH v3 3/3] iomap: Don't create iomap_page objects in iomap_page_mkwrite_actor

[Christoph Hellwig]

On Wed, Jul 07, 2021 at 01:55:24PM +0200, Andreas Gruenbacher wrote:
> Now that we create those objects in iomap_writepage_map when needed,
> there's no need to pre-create them in iomap_page_mkwrite_actor anymore.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>