[PATCH 0/4] More GFS2 folio conversions

[PATCH 0/4] More GFS2 folio conversions

[Matthew Wilcox (Oracle)]

Yet more gfs2 folio conversions.  As usual, compile tested only.
The third patch is a bit more "interesting" than most.

Matthew Wilcox (Oracle) (4):
  gfs2: Convert gfs2_page_mkwrite() to use a folio
  gfs2: Add a migrate_folio operation for journalled files
  gfs2: Simplify gfs2_read_super
  gfs2: Convert gfs2_aspace_writepage() to use a folio

 fs/gfs2/aops.c       | 34 ++-----------------------
 fs/gfs2/file.c       | 59 ++++++++++++++++++++++----------------------
 fs/gfs2/meta_io.c    | 16 ++++++------
 fs/gfs2/ops_fstype.c | 46 ++++++++++------------------------
 4 files changed, 53 insertions(+), 102 deletions(-)

-- 
2.43.0

[PATCH 1/4] gfs2: Convert gfs2_page_mkwrite() to use a folio

[Matthew Wilcox (Oracle)]

Convert the incoming page to a folio and use it throughout saving several
calls to compound_head().  Also use 'pos' for file position rather than
the ambiguou 'offset' and increase 'length' to size_t in cae we get some
truly ridiculous sized folios in future.  This function should now be
large-folio safe, but I may have missed something.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/gfs2/file.c | 59 +++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 4c42ada60ae7..08982937b5df 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -376,23 +376,23 @@ static void gfs2_size_hint(struct file *filep, loff_t offset, size_t size)
 }
 
 /**
- * gfs2_allocate_page_backing - Allocate blocks for a write fault
- * @page: The (locked) page to allocate backing for
+ * gfs2_allocate_folio_backing - Allocate blocks for a write fault
+ * @folio: The (locked) folio to allocate backing for
  * @length: Size of the allocation
  *
- * We try to allocate all the blocks required for the page in one go.  This
+ * We try to allocate all the blocks required for the folio in one go.  This
  * might fail for various reasons, so we keep trying until all the blocks to
- * back this page are allocated.  If some of the blocks are already allocated,
+ * back this folio are allocated.  If some of the blocks are already allocated,
  * that is ok too.
  */
-static int gfs2_allocate_page_backing(struct page *page, unsigned int length)
+static int gfs2_allocate_folio_backing(struct folio *folio, size_t length)
 {
-	u64 pos = page_offset(page);
+	u64 pos = folio_pos(folio);
 
 	do {
 		struct iomap iomap = { };
 
-		if (gfs2_iomap_alloc(page->mapping->host, pos, length, &iomap))
+		if (gfs2_iomap_alloc(folio->mapping->host, pos, length, &iomap))
 			return -EIO;
 
 		if (length < iomap.length)
@@ -414,16 +414,16 @@ static int gfs2_allocate_page_backing(struct page *page, unsigned int length)
 
 static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 {
-	struct page *page = vmf->page;
+	struct folio *folio = page_folio(vmf->page);
 	struct inode *inode = file_inode(vmf->vma->vm_file);
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	struct gfs2_alloc_parms ap = {};
-	u64 offset = page_offset(page);
+	u64 pos = folio_pos(folio);
 	unsigned int data_blocks, ind_blocks, rblocks;
 	vm_fault_t ret = VM_FAULT_LOCKED;
 	struct gfs2_holder gh;
-	unsigned int length;
+	size_t length;
 	loff_t size;
 	int err;
 
@@ -436,23 +436,23 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 		goto out_uninit;
 	}
 
-	/* Check page index against inode size */
+	/* Check folio index against inode size */
 	size = i_size_read(inode);
-	if (offset >= size) {
+	if (pos >= size) {
 		ret = VM_FAULT_SIGBUS;
 		goto out_unlock;
 	}
 
-	/* Update file times before taking page lock */
+	/* Update file times before taking folio lock */
 	file_update_time(vmf->vma->vm_file);
 
-	/* page is wholly or partially inside EOF */
-	if (size - offset < PAGE_SIZE)
-		length = size - offset;
+	/* folio is wholly or partially inside EOF */
+	if (size - pos < folio_size(folio))
+		length = size - pos;
 	else
-		length = PAGE_SIZE;
+		length = folio_size(folio);
 
-	gfs2_size_hint(vmf->vma->vm_file, offset, length);
+	gfs2_size_hint(vmf->vma->vm_file, pos, length);
 
 	set_bit(GLF_DIRTY, &ip->i_gl->gl_flags);
 	set_bit(GIF_SW_PAGED, &ip->i_flags);
@@ -463,11 +463,12 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 	 */
 
 	if (!gfs2_is_stuffed(ip) &&
-	    !gfs2_write_alloc_required(ip, offset, length)) {
-		lock_page(page);
-		if (!PageUptodate(page) || page->mapping != inode->i_mapping) {
+	    !gfs2_write_alloc_required(ip, pos, length)) {
+		folio_lock(folio);
+		if (!folio_test_uptodate(folio) ||
+		    folio->mapping != inode->i_mapping) {
 			ret = VM_FAULT_NOPAGE;
-			unlock_page(page);
+			folio_unlock(folio);
 		}
 		goto out_unlock;
 	}
@@ -504,7 +505,7 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 		goto out_trans_fail;
 	}
 
-	/* Unstuff, if required, and allocate backing blocks for page */
+	/* Unstuff, if required, and allocate backing blocks for folio */
 	if (gfs2_is_stuffed(ip)) {
 		err = gfs2_unstuff_dinode(ip);
 		if (err) {
@@ -513,22 +514,22 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 		}
 	}
 
-	lock_page(page);
+	folio_lock(folio);
 	/* If truncated, we must retry the operation, we may have raced
 	 * with the glock demotion code.
 	 */
-	if (!PageUptodate(page) || page->mapping != inode->i_mapping) {
+	if (!folio_test_uptodate(folio) || folio->mapping != inode->i_mapping) {
 		ret = VM_FAULT_NOPAGE;
 		goto out_page_locked;
 	}
 
-	err = gfs2_allocate_page_backing(page, length);
+	err = gfs2_allocate_folio_backing(folio, length);
 	if (err)
 		ret = vmf_fs_error(err);
 
 out_page_locked:
 	if (ret != VM_FAULT_LOCKED)
-		unlock_page(page);
+		folio_unlock(folio);
 out_trans_end:
 	gfs2_trans_end(sdp);
 out_trans_fail:
@@ -540,8 +541,8 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 out_uninit:
 	gfs2_holder_uninit(&gh);
 	if (ret == VM_FAULT_LOCKED) {
-		set_page_dirty(page);
-		wait_for_stable_page(page);
+		folio_mark_dirty(folio);
+		folio_wait_stable(folio);
 	}
 	sb_end_pagefault(inode->i_sb);
 	return ret;
-- 
2.43.0

[PATCH 2/4] gfs2: Add a migrate_folio operation for journalled files

[Matthew Wilcox (Oracle)]

For journalled data, folio migration currently works by writing the folio
back, freeing the folio and faulting the new folio back in.  We can
bypass that by telling the migration code to migrate the buffer_heads
attached to our folios.  That lets us delete gfs2_jdata_writepage()
as it has no more callers.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/gfs2/aops.c | 34 ++--------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 974aca9c8ea8..68fc8af14700 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -116,8 +116,7 @@ static int gfs2_write_jdata_folio(struct folio *folio,
  * @folio: The folio to write
  * @wbc: The writeback control
  *
- * This is shared between writepage and writepages and implements the
- * core of the writepage operation. If a transaction is required then
+ * Implements the core of write back. If a transaction is required then
  * the checked flag will have been set and the transaction will have
  * already been started before this is called.
  */
@@ -139,35 +138,6 @@ static int __gfs2_jdata_write_folio(struct folio *folio,
 	return gfs2_write_jdata_folio(folio, wbc);
 }
 
-/**
- * gfs2_jdata_writepage - Write complete page
- * @page: Page to write
- * @wbc: The writeback control
- *
- * Returns: errno
- *
- */
-
-static int gfs2_jdata_writepage(struct page *page, struct writeback_control *wbc)
-{
-	struct folio *folio = page_folio(page);
-	struct inode *inode = page->mapping->host;
-	struct gfs2_inode *ip = GFS2_I(inode);
-	struct gfs2_sbd *sdp = GFS2_SB(inode);
-
-	if (gfs2_assert_withdraw(sdp, ip->i_gl->gl_state == LM_ST_EXCLUSIVE))
-		goto out;
-	if (folio_test_checked(folio) || current->journal_info)
-		goto out_ignore;
-	return __gfs2_jdata_write_folio(folio, wbc);
-
-out_ignore:
-	folio_redirty_for_writepage(wbc, folio);
-out:
-	folio_unlock(folio);
-	return 0;
-}
-
 /**
  * gfs2_writepages - Write a bunch of dirty pages back to disk
  * @mapping: The mapping to write
@@ -749,12 +719,12 @@ static const struct address_space_operations gfs2_aops = {
 };
 
 static const struct address_space_operations gfs2_jdata_aops = {
-	.writepage = gfs2_jdata_writepage,
 	.writepages = gfs2_jdata_writepages,
 	.read_folio = gfs2_read_folio,
 	.readahead = gfs2_readahead,
 	.dirty_folio = jdata_dirty_folio,
 	.bmap = gfs2_bmap,
+	.migrate_folio = buffer_migrate_folio,
 	.invalidate_folio = gfs2_invalidate_folio,
 	.release_folio = gfs2_release_folio,
 	.is_partially_uptodate = block_is_partially_uptodate,
-- 
2.43.0

Re: [PATCH 2/4] gfs2: Add a migrate_folio operation for journalled files

[Andreas Gruenbacher]

On Wed, Apr 3, 2024 at 7:24 PM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
> For journalled data, folio migration currently works by writing the folio
> back, freeing the folio and faulting the new folio back in.  We can
> bypass that by telling the migration code to migrate the buffer_heads
> attached to our folios.

This part sounds reasonable, but I disagree with the following assertion:

> That lets us delete gfs2_jdata_writepage() as it has no more callers.

The reason is that the log flush code calls gfs2_jdata_writepage()
indirectly via mapping->a_ops->writepage. So with this patch, we end
up with a bunch of Oopses.

Do you want to resend, or should I back out the gfs2_jdata_writepage
removal and add the remaining one-liner?

Thanks,
Andreas


> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/gfs2/aops.c | 34 ++--------------------------------
>  1 file changed, 2 insertions(+), 32 deletions(-)
>
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 974aca9c8ea8..68fc8af14700 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -116,8 +116,7 @@ static int gfs2_write_jdata_folio(struct folio *folio,
>   * @folio: The folio to write
>   * @wbc: The writeback control
>   *
> - * This is shared between writepage and writepages and implements the
> - * core of the writepage operation. If a transaction is required then
> + * Implements the core of write back. If a transaction is required then
>   * the checked flag will have been set and the transaction will have
>   * already been started before this is called.
>   */
> @@ -139,35 +138,6 @@ static int __gfs2_jdata_write_folio(struct folio *folio,
>         return gfs2_write_jdata_folio(folio, wbc);
>  }
>
> -/**
> - * gfs2_jdata_writepage - Write complete page
> - * @page: Page to write
> - * @wbc: The writeback control
> - *
> - * Returns: errno
> - *
> - */
> -
> -static int gfs2_jdata_writepage(struct page *page, struct writeback_control *wbc)
> -{
> -       struct folio *folio = page_folio(page);
> -       struct inode *inode = page->mapping->host;
> -       struct gfs2_inode *ip = GFS2_I(inode);
> -       struct gfs2_sbd *sdp = GFS2_SB(inode);
> -
> -       if (gfs2_assert_withdraw(sdp, ip->i_gl->gl_state == LM_ST_EXCLUSIVE))
> -               goto out;
> -       if (folio_test_checked(folio) || current->journal_info)
> -               goto out_ignore;
> -       return __gfs2_jdata_write_folio(folio, wbc);
> -
> -out_ignore:
> -       folio_redirty_for_writepage(wbc, folio);
> -out:
> -       folio_unlock(folio);
> -       return 0;
> -}
> -
>  /**
>   * gfs2_writepages - Write a bunch of dirty pages back to disk
>   * @mapping: The mapping to write
> @@ -749,12 +719,12 @@ static const struct address_space_operations gfs2_aops = {
>  };
>
>  static const struct address_space_operations gfs2_jdata_aops = {
> -       .writepage = gfs2_jdata_writepage,
>         .writepages = gfs2_jdata_writepages,
>         .read_folio = gfs2_read_folio,
>         .readahead = gfs2_readahead,
>         .dirty_folio = jdata_dirty_folio,
>         .bmap = gfs2_bmap,
> +       .migrate_folio = buffer_migrate_folio,
>         .invalidate_folio = gfs2_invalidate_folio,
>         .release_folio = gfs2_release_folio,
>         .is_partially_uptodate = block_is_partially_uptodate,
> --
> 2.43.0
>

Re: [PATCH 2/4] gfs2: Add a migrate_folio operation for journalled files

[Matthew Wilcox]

On Thu, May 02, 2024 at 10:23:41PM +0200, Andreas Gruenbacher wrote:
> On Wed, Apr 3, 2024 at 7:24 PM Matthew Wilcox (Oracle)
> <willy@infradead.org> wrote:
> > For journalled data, folio migration currently works by writing the folio
> > back, freeing the folio and faulting the new folio back in.  We can
> > bypass that by telling the migration code to migrate the buffer_heads
> > attached to our folios.
> 
> This part sounds reasonable, but I disagree with the following assertion:
> 
> > That lets us delete gfs2_jdata_writepage() as it has no more callers.
> 
> The reason is that the log flush code calls gfs2_jdata_writepage()
> indirectly via mapping->a_ops->writepage. So with this patch, we end
> up with a bunch of Oopses.
> 
> Do you want to resend, or should I back out the gfs2_jdata_writepage
> removal and add the remaining one-liner?

Ugh, I see.  If you could just add the one-liner for now, and I'll
come back with a better proposal next for merge window?

Re: [PATCH 2/4] gfs2: Add a migrate_folio operation for journalled files

[Andreas Gruenbacher]

On Fri, May 3, 2024 at 8:30 PM Matthew Wilcox <willy@infradead.org> wrote:
> On Thu, May 02, 2024 at 10:23:41PM +0200, Andreas Gruenbacher wrote:
> > On Wed, Apr 3, 2024 at 7:24 PM Matthew Wilcox (Oracle)
> > <willy@infradead.org> wrote:
> > > For journalled data, folio migration currently works by writing the folio
> > > back, freeing the folio and faulting the new folio back in.  We can
> > > bypass that by telling the migration code to migrate the buffer_heads
> > > attached to our folios.
> >
> > This part sounds reasonable, but I disagree with the following assertion:
> >
> > > That lets us delete gfs2_jdata_writepage() as it has no more callers.
> >
> > The reason is that the log flush code calls gfs2_jdata_writepage()
> > indirectly via mapping->a_ops->writepage. So with this patch, we end
> > up with a bunch of Oopses.
> >
> > Do you want to resend, or should I back out the gfs2_jdata_writepage
> > removal and add the remaining one-liner?
>
> Ugh, I see.  If you could just add the one-liner for now, and I'll
> come back with a better proposal next for merge window?

Sure, pushed to for-next.

Thanks again,
Andreas

[PATCH 3/4] gfs2: Simplify gfs2_read_super

[Matthew Wilcox (Oracle)]

Use submit_bio_wait() instead of hand-rolling our own synchronous
wait.  Also allocate the BIO on the stack since we're not deep in
the call stack at this point.

There's no need to kmap the page, since it isn't allocated from HIGHMEM.
Turn the GFP_NOFS allocation into GFP_KERNEL; if the page allocator
enters reclaim, we cannot be called as the filesystem has not yet been
initialised and so has no pages to reclaim.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/gfs2/ops_fstype.c | 46 +++++++++++++-------------------------------
 1 file changed, 13 insertions(+), 33 deletions(-)

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 572d58e86296..f98651229c8f 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -184,22 +184,10 @@ static int gfs2_check_sb(struct gfs2_sbd *sdp, int silent)
 	return 0;
 }
 
-static void end_bio_io_page(struct bio *bio)
-{
-	struct page *page = bio->bi_private;
-
-	if (!bio->bi_status)
-		SetPageUptodate(page);
-	else
-		pr_warn("error %d reading superblock\n", bio->bi_status);
-	unlock_page(page);
-}
-
-static void gfs2_sb_in(struct gfs2_sbd *sdp, const void *buf)
+static void gfs2_sb_in(struct gfs2_sbd *sdp, const struct gfs2_sb *str)
 {
 	struct gfs2_sb_host *sb = &sdp->sd_sb;
 	struct super_block *s = sdp->sd_vfs;
-	const struct gfs2_sb *str = buf;
 
 	sb->sb_magic = be32_to_cpu(str->sb_header.mh_magic);
 	sb->sb_type = be32_to_cpu(str->sb_header.mh_type);
@@ -239,34 +227,26 @@ static void gfs2_sb_in(struct gfs2_sbd *sdp, const void *buf)
 static int gfs2_read_super(struct gfs2_sbd *sdp, sector_t sector, int silent)
 {
 	struct super_block *sb = sdp->sd_vfs;
-	struct gfs2_sb *p;
 	struct page *page;
-	struct bio *bio;
+	struct bio_vec bvec;
+	struct bio bio;
+	int err;
 
-	page = alloc_page(GFP_NOFS);
+	page = alloc_page(GFP_KERNEL);
 	if (unlikely(!page))
 		return -ENOMEM;
 
-	ClearPageUptodate(page);
-	ClearPageDirty(page);
-	lock_page(page);
-
-	bio = bio_alloc(sb->s_bdev, 1, REQ_OP_READ | REQ_META, GFP_NOFS);
-	bio->bi_iter.bi_sector = sector * (sb->s_blocksize >> 9);
-	__bio_add_page(bio, page, PAGE_SIZE, 0);
+	bio_init(&bio, sb->s_bdev, &bvec, 1, REQ_OP_READ | REQ_META);
+	bio.bi_iter.bi_sector = sector * (sb->s_blocksize >> 9);
+	__bio_add_page(&bio, page, PAGE_SIZE, 0);
 
-	bio->bi_end_io = end_bio_io_page;
-	bio->bi_private = page;
-	submit_bio(bio);
-	wait_on_page_locked(page);
-	bio_put(bio);
-	if (!PageUptodate(page)) {
+	err = submit_bio_wait(&bio);
+	if (err) {
+		pr_warn("error %d reading superblock\n", err);
 		__free_page(page);
-		return -EIO;
+		return err;
 	}
-	p = kmap(page);
-	gfs2_sb_in(sdp, p);
-	kunmap(page);
+	gfs2_sb_in(sdp, page_address(page));
 	__free_page(page);
 	return gfs2_check_sb(sdp, silent);
 }
-- 
2.43.0

[PATCH 4/4] gfs2: Convert gfs2_aspace_writepage() to use a folio

[Matthew Wilcox (Oracle)]

Convert the incoming struct page to a folio and use it throughout.
Saves six calls to compound_head().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/gfs2/meta_io.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
index f814054c8cd0..2b26e8d529aa 100644
--- a/fs/gfs2/meta_io.c
+++ b/fs/gfs2/meta_io.c
@@ -32,14 +32,14 @@
 
 static int gfs2_aspace_writepage(struct page *page, struct writeback_control *wbc)
 {
+	struct folio *folio = page_folio(page);
 	struct buffer_head *bh, *head;
 	int nr_underway = 0;
 	blk_opf_t write_flags = REQ_META | REQ_PRIO | wbc_to_write_flags(wbc);
 
-	BUG_ON(!PageLocked(page));
-	BUG_ON(!page_has_buffers(page));
+	BUG_ON(!folio_test_locked(folio));
 
-	head = page_buffers(page);
+	head = folio_buffers(folio);
 	bh = head;
 
 	do {
@@ -55,7 +55,7 @@ static int gfs2_aspace_writepage(struct page *page, struct writeback_control *wb
 		if (wbc->sync_mode != WB_SYNC_NONE) {
 			lock_buffer(bh);
 		} else if (!trylock_buffer(bh)) {
-			redirty_page_for_writepage(wbc, page);
+			folio_redirty_for_writepage(wbc, folio);
 			continue;
 		}
 		if (test_clear_buffer_dirty(bh)) {
@@ -69,8 +69,8 @@ static int gfs2_aspace_writepage(struct page *page, struct writeback_control *wb
 	 * The page and its buffers are protected by PageWriteback(), so we can
 	 * drop the bh refcounts early.
 	 */
-	BUG_ON(PageWriteback(page));
-	set_page_writeback(page);
+	BUG_ON(folio_test_writeback(folio));
+	folio_start_writeback(folio);
 
 	do {
 		struct buffer_head *next = bh->b_this_page;
@@ -80,10 +80,10 @@ static int gfs2_aspace_writepage(struct page *page, struct writeback_control *wb
 		}
 		bh = next;
 	} while (bh != head);
-	unlock_page(page);
+	folio_unlock(folio);
 
 	if (nr_underway == 0)
-		end_page_writeback(page);
+		folio_end_writeback(folio);
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH 0/4] More GFS2 folio conversions

[Andreas Gruenbacher]

Hi Willy,

thanks a lot for yet another set of folio conversion patches.

On Wed, Apr 3, 2024 at 7:24 PM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
> Yet more gfs2 folio conversions.  As usual, compile tested only.
> The third patch is a bit more "interesting" than most.

The third patch looks fine to me, as does the first and fourth. Those
are the ones I've added so far.

The second one is problematic; I'll respond to that separately.

Thanks,
Andreas

> Matthew Wilcox (Oracle) (4):
>   gfs2: Convert gfs2_page_mkwrite() to use a folio
>   gfs2: Add a migrate_folio operation for journalled files
>   gfs2: Simplify gfs2_read_super
>   gfs2: Convert gfs2_aspace_writepage() to use a folio
>
>  fs/gfs2/aops.c       | 34 ++-----------------------
>  fs/gfs2/file.c       | 59 ++++++++++++++++++++++----------------------
>  fs/gfs2/meta_io.c    | 16 ++++++------
>  fs/gfs2/ops_fstype.c | 46 ++++++++++------------------------
>  4 files changed, 53 insertions(+), 102 deletions(-)
>
> --
> 2.43.0
>