[PATCH v2 0/2] iomap: small block problems

[PATCH v2 0/2] iomap: small block problems

[Andreas Gruenbacher]

Here are the two fixes that make sure that iop objects get attached to
pages eventually (in iomap_writepage_map if not earlier), but not too
early (before inline inodes are read).  These are the fixes required for
making gfs2 filesystems with a block size smaller than the page size
work again.

As Christoph has pointed out [*], there are several more cases in which
we can avoid iop creation.  Those improvements are still left to be done.

[*] https://lore.kernel.org/linux-fsdevel/YNqy0E4xFwHDhK32@infradead.org/

Thanks,
Andreas

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

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

-- 
2.26.3

[Cluster-devel] [PATCH v2 0/2] iomap: small block problems

[Andreas Gruenbacher]

Here are the two fixes that make sure that iop objects get attached to
pages eventually (in iomap_writepage_map if not earlier), but not too
early (before inline inodes are read).  These are the fixes required for
making gfs2 filesystems with a block size smaller than the page size
work again.

As Christoph has pointed out [*], there are several more cases in which
we can avoid iop creation.  Those improvements are still left to be done.

[*] https://lore.kernel.org/linux-fsdevel/YNqy0E4xFwHDhK32 at infradead.org/

Thanks,
Andreas

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

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

-- 
2.26.3

[PATCH v2 1/2] 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>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 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 9023717c5188..03537ecb2a94 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

[Cluster-devel] [PATCH v2 1/2] 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>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 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 9023717c5188..03537ecb2a94 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

[PATCH v2 2/2] 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.  The only current case we
do that for is 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>
---
 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 03537ecb2a94..6330dabc451e 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1336,14 +1336,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

[Cluster-devel] [PATCH v2 2/2] 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.  The only current case we
do that for is 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>
---
 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 03537ecb2a94..6330dabc451e 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1336,14 +1336,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 v2 1/2] iomap: Don't create iomap_page objects for inline files

[Christoph Hellwig]

On Mon, Jul 05, 2021 at 08:18: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>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

As mentioned last round I'd prefer to simply not create the iomap_page
at all in the readpage/readpages path.

Also this patch needs to go after the current patch 2 to be bisection
clean.

[Cluster-devel] [PATCH v2 1/2] iomap: Don't create iomap_page objects for inline files

[Christoph Hellwig]

On Mon, Jul 05, 2021 at 08:18: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>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

As mentioned last round I'd prefer to simply not create the iomap_page
at all in the readpage/readpages path.

Also this patch needs to go after the current patch 2 to be bisection
clean.

Re: [PATCH v2 2/2] iomap: Permit pages without an iop to enter writeback

[Christoph Hellwig]

On Mon, Jul 05, 2021 at 08:18:24PM +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.  The only current case we
> do that for is 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>

Looks good,

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

[Cluster-devel] [PATCH v2 2/2] iomap: Permit pages without an iop to enter writeback

[Christoph Hellwig]

On Mon, Jul 05, 2021 at 08:18:24PM +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.  The only current case we
> do that for is 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>

Looks good,

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

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

[Andreas Gruenbacher]

On Tue, Jul 6, 2021 at 7:07 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jul 05, 2021 at 08:18: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>
> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>
> As mentioned last round I'd prefer to simply not create the iomap_page
> at all in the readpage/readpages path.

I've tried that by replacing the iomap_page_create with to_iomap_page
in iomap_readpage_actor and with that, I'm getting a
VM_BUG_ON_PAGE(!PageLocked(page)) in iomap_read_end_io -> unlock_page
with generic/029. So there's obviously more to it than just not
creating the iomap_page in iomap_readpage_actor.

Getting rid of the iomap_page_create in iomap_readpage_actor
completely isn't a necessary part of the bug fix. So can we focus on
the bug fix for now, and worry about the improvement later?

> Also this patch needs to go after the current patch 2 to be bisection clean.

Yes, makes sense.

Thanks,
Andreas

[Cluster-devel] [PATCH v2 1/2] iomap: Don't create iomap_page objects for inline files

[Andreas Gruenbacher]

On Tue, Jul 6, 2021 at 7:07 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jul 05, 2021 at 08:18: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>
> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>
> As mentioned last round I'd prefer to simply not create the iomap_page
> at all in the readpage/readpages path.

I've tried that by replacing the iomap_page_create with to_iomap_page
in iomap_readpage_actor and with that, I'm getting a
VM_BUG_ON_PAGE(!PageLocked(page)) in iomap_read_end_io -> unlock_page
with generic/029. So there's obviously more to it than just not
creating the iomap_page in iomap_readpage_actor.

Getting rid of the iomap_page_create in iomap_readpage_actor
completely isn't a necessary part of the bug fix. So can we focus on
the bug fix for now, and worry about the improvement later?

> Also this patch needs to go after the current patch 2 to be bisection clean.

Yes, makes sense.

Thanks,
Andreas