Re: [PATCH 15/15] gfs2: use iomap for buffered I/O in ordered and writeback mode

Re: [PATCH 15/15] gfs2: use iomap for buffered I/O in ordered and writeback mode

[Andreas Gruenbacher]

Hi Christoph,

On Mon, Sep 30, 2019 at 10:49 PM Andreas Gruenbacher <agruenba@redhat.com> wrote:
> On Tue, Aug 6, 2019 at 7:30 AM Christoph Hellwig <hch@lst.de> wrote:
> > On Mon, Aug 05, 2019 at 02:27:21PM +0200, Andreas Gruenbacher wrote:
> here are the changes we currently need on top of what you've posted on
> July 1.  [...]

again, thank you for this patch.  After fixing some related bugs around this
change, it seems I've finally got this to work properly.  Below are the minor
changes I needed to make on top of your version.

This requires functions iomap_page_create and iomap_set_range_uptodate to be
exported; i'll post a patch for that sepatately.

The result can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git for-next.iomap

Thanks,
Andreas

---
 fs/gfs2/bmap.c | 6 ++++--
 fs/gfs2/file.c | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 168ac5147dd0..fcd2043fc466 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -75,13 +75,12 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
 		memcpy(kaddr, dibh->b_data + sizeof(struct gfs2_dinode), dsize);
 		memset(kaddr + dsize, 0, PAGE_SIZE - dsize);
 		kunmap(page);
-
-		SetPageUptodate(page);
 	}
 
 	if (gfs2_is_jdata(ip)) {
 		struct buffer_head *bh;
 
+		SetPageUptodate(page);
 		if (!page_has_buffers(page))
 			create_empty_buffers(page, BIT(inode->i_blkbits),
 					     BIT(BH_Uptodate));
@@ -93,6 +92,9 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
 		set_buffer_uptodate(bh);
 		gfs2_trans_add_data(ip->i_gl, bh);
 	} else {
+		iomap_page_create(inode, page);
+		iomap_set_range_uptodate(page, 0, i_blocksize(inode));
+		set_page_dirty(page);
 		gfs2_ordered_add_inode(ip);
 	}
 
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 9d58295ccf7a..9af352ebc904 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -555,6 +555,8 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 out_uninit:
 	gfs2_holder_uninit(&gh);
 	if (ret == 0) {
+		if (!gfs2_is_jdata(ip))
+			iomap_page_create(inode, page);
 		set_page_dirty(page);
 		wait_for_stable_page(page);
 	}
-- 
2.20.1

Re: [PATCH 15/15] gfs2: use iomap for buffered I/O in ordered and writeback mode

[Christoph Hellwig]

On Tue, Dec 10, 2019 at 11:19:38AM +0100, Andreas Gruenbacher wrote:
> @@ -75,13 +75,12 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
>  		memcpy(kaddr, dibh->b_data + sizeof(struct gfs2_dinode), dsize);
>  		memset(kaddr + dsize, 0, PAGE_SIZE - dsize);
>  		kunmap(page);
> -
> -		SetPageUptodate(page);
>  	}
>  
>  	if (gfs2_is_jdata(ip)) {
>  		struct buffer_head *bh;
>  
> +		SetPageUptodate(page);
>  		if (!page_has_buffers(page))
>  			create_empty_buffers(page, BIT(inode->i_blkbits),
>  					     BIT(BH_Uptodate));
> @@ -93,6 +92,9 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
>  		set_buffer_uptodate(bh);
>  		gfs2_trans_add_data(ip->i_gl, bh);
>  	} else {
> +		iomap_page_create(inode, page);
> +		iomap_set_range_uptodate(page, 0, i_blocksize(inode));
> +		set_page_dirty(page);
>  		gfs2_ordered_add_inode(ip);
>  	}

Can you create a helper that copies the data from a passed in kernel
pointer, length pair into the page, then marks it uptodate and dirty,
please?

> @@ -555,6 +555,8 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
>  out_uninit:
>  	gfs2_holder_uninit(&gh);
>  	if (ret == 0) {
> +		if (!gfs2_is_jdata(ip))
> +			iomap_page_create(inode, page);

What is this one for?  The iomap_page is supposed to use lazy
allocation, that is we only allocate it once it is used.  What code
expects the structure but doesn't see it without this hunk?  I
guess it is iomap_writepage_map, which should probably just switch
to call iomap_page_create.

That being said is there any way we can get gfs2 to use
iomap_page_mkwrite for the !jdata case?

RFC: use the iomap writepage path in gfs2

[Christoph Hellwig]

Hi all,

in this straight from the jetplane edition I present the series to
convert gfs2 to full iomap usage for the ordered and writeback mode,
that is we use iomap_page everywhere and entirely get rid of
buffer_heads in the data path.  This has only seen basic testing
which ensured neither 4k or 1k blocksize in ordered mode regressed
vs the xfstests baseline, although that baseline tends to look
pretty bleak.

The series is to be applied on top of my "lift the xfs writepage code
into iomap v2" series.

[PATCH 15/15] gfs2: use iomap for buffered I/O in ordered and writeback mode

[Christoph Hellwig]

Switch to using the iomap readpage and writepage helpers for all I/O in
the ordered and writeback modes, and thus eliminate using buffer_heads
for I/O in these cases.  The journaled data mode is left untouched.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/aops.c | 59 +++++++++++++++++++++++---------------------------
 fs/gfs2/bmap.c | 47 ++++++++++++++++++++++++++++++----------
 fs/gfs2/bmap.h |  1 +
 3 files changed, 63 insertions(+), 44 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 15a234fb8f88..9cdd61a44379 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -91,22 +91,13 @@ static int gfs2_writepage(struct page *page, struct writeback_control *wbc)
 	struct inode *inode = page->mapping->host;
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
-	loff_t i_size = i_size_read(inode);
-	pgoff_t end_index = i_size >> PAGE_SHIFT;
-	unsigned offset;
+	struct iomap_writepage_ctx wpc = { };
 
 	if (gfs2_assert_withdraw(sdp, gfs2_glock_is_held_excl(ip->i_gl)))
 		goto out;
 	if (current->journal_info)
 		goto redirty;
-	/* Is the page fully outside i_size? (truncate in progress) */
-	offset = i_size & (PAGE_SIZE-1);
-	if (page->index > end_index || (page->index == end_index && !offset)) {
-		page->mapping->a_ops->invalidatepage(page, 0, PAGE_SIZE);
-		goto out;
-	}
-
-	return nobh_writepage(page, gfs2_get_block_noalloc, wbc);
+	return iomap_writepage(page, wbc, &wpc, &gfs2_writeback_ops);
 
 redirty:
 	redirty_page_for_writepage(wbc, page);
@@ -210,7 +201,8 @@ static int gfs2_writepages(struct address_space *mapping,
 			   struct writeback_control *wbc)
 {
 	struct gfs2_sbd *sdp = gfs2_mapping2sbd(mapping);
-	int ret = mpage_writepages(mapping, wbc, gfs2_get_block_noalloc);
+	struct iomap_writepage_ctx wpc = { };
+	int ret;
 
 	/*
 	 * Even if we didn't write any pages here, we might still be holding
@@ -218,9 +210,9 @@ static int gfs2_writepages(struct address_space *mapping,
 	 * want balance_dirty_pages() to loop indefinitely trying to write out
 	 * pages held in the ail that it can't find.
 	 */
+	ret = iomap_writepages(mapping, wbc, &wpc, &gfs2_writeback_ops);
 	if (ret == 0)
 		set_bit(SDF_FORCE_AIL_FLUSH, &sdp->sd_flags);
-
 	return ret;
 }
 
@@ -469,7 +461,6 @@ static int stuffed_readpage(struct gfs2_inode *ip, struct page *page)
 	return 0;
 }
 
-
 /**
  * __gfs2_readpage - readpage
  * @file: The file to read a page for
@@ -479,16 +470,15 @@ static int stuffed_readpage(struct gfs2_inode *ip, struct page *page)
  * reading code as in that case we already hold the glock. Also it's
  * called by gfs2_readpage() once the required lock has been granted.
  */
-
 static int __gfs2_readpage(void *file, struct page *page)
 {
-	struct gfs2_inode *ip = GFS2_I(page->mapping->host);
-	struct gfs2_sbd *sdp = GFS2_SB(page->mapping->host);
-
+	struct inode *inode = page->mapping->host;
+	struct gfs2_inode *ip = GFS2_I(inode);
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	int error;
 
-	if (i_blocksize(page->mapping->host) == PAGE_SIZE &&
-	    !page_has_buffers(page)) {
+	if (!gfs2_is_jdata(ip) ||
+	    (i_blocksize(inode) == PAGE_SIZE && !page_has_buffers(page))) {
 		error = iomap_readpage(page, &gfs2_iomap_ops);
 	} else if (gfs2_is_stuffed(ip)) {
 		error = stuffed_readpage(ip, page);
@@ -609,8 +599,12 @@ static int gfs2_readpages(struct file *file, struct address_space *mapping,
 	ret = gfs2_glock_nq(&gh);
 	if (unlikely(ret))
 		goto out_uninit;
-	if (!gfs2_is_stuffed(ip))
+	if (gfs2_is_stuffed(ip))
+		;
+	else if (gfs2_is_jdata(ip))
 		ret = mpage_readpages(mapping, pages, nr_pages, gfs2_block_map);
+	else
+		ret = iomap_readpages(mapping, pages, nr_pages, &gfs2_iomap_ops);
 	gfs2_glock_dq(&gh);
 out_uninit:
 	gfs2_holder_uninit(&gh);
@@ -827,17 +821,18 @@ int gfs2_releasepage(struct page *page, gfp_t gfp_mask)
 }
 
 static const struct address_space_operations gfs2_aops = {
-	.writepage = gfs2_writepage,
-	.writepages = gfs2_writepages,
-	.readpage = gfs2_readpage,
-	.readpages = gfs2_readpages,
-	.bmap = gfs2_bmap,
-	.invalidatepage = gfs2_invalidatepage,
-	.releasepage = gfs2_releasepage,
-	.direct_IO = noop_direct_IO,
-	.migratepage = buffer_migrate_page,
-	.is_partially_uptodate = block_is_partially_uptodate,
-	.error_remove_page = generic_error_remove_page,
+	.writepage		= gfs2_writepage,
+	.writepages		= gfs2_writepages,
+	.readpage		= gfs2_readpage,
+	.readpages		= gfs2_readpages,
+	.set_page_dirty		= iomap_set_page_dirty,
+	.releasepage		= iomap_releasepage,
+	.invalidatepage		= iomap_invalidatepage,
+	.bmap			= gfs2_bmap,
+	.direct_IO		= noop_direct_IO,
+	.migratepage		= iomap_migrate_page,
+	.is_partially_uptodate  = iomap_is_partially_uptodate,
+	.error_remove_page	= generic_error_remove_page,
 };
 
 static const struct address_space_operations gfs2_jdata_aops = {
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index b7bd811872cb..b8d795d277c9 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -56,7 +56,6 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
 			       u64 block, struct page *page)
 {
 	struct inode *inode = &ip->i_inode;
-	struct buffer_head *bh;
 	int release = 0;
 
 	if (!page || page->index) {
@@ -80,20 +79,20 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
 		SetPageUptodate(page);
 	}
 
-	if (!page_has_buffers(page))
-		create_empty_buffers(page, BIT(inode->i_blkbits),
-				     BIT(BH_Uptodate));
+	if (gfs2_is_jdata(ip)) {
+		struct buffer_head *bh;
 
-	bh = page_buffers(page);
+		if (!page_has_buffers(page))
+			create_empty_buffers(page, BIT(inode->i_blkbits),
+					     BIT(BH_Uptodate));
 
-	if (!buffer_mapped(bh))
-		map_bh(bh, inode->i_sb, block);
+		bh = page_buffers(page);
+		if (!buffer_mapped(bh))
+			map_bh(bh, inode->i_sb, block);
 
-	set_buffer_uptodate(bh);
-	if (gfs2_is_jdata(ip))
+		set_buffer_uptodate(bh);
 		gfs2_trans_add_data(ip->i_gl, bh);
-	else {
-		mark_buffer_dirty(bh);
+	} else {
 		gfs2_ordered_add_inode(ip);
 	}
 
@@ -1127,7 +1126,8 @@ static int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 	struct metapath mp = { .mp_aheight = 1, };
 	int ret;
 
-	iomap->flags |= IOMAP_F_BUFFER_HEAD;
+	if (gfs2_is_jdata(ip))
+		iomap->flags |= IOMAP_F_BUFFER_HEAD;
 
 	trace_gfs2_iomap_start(ip, pos, length, flags);
 	if ((flags & IOMAP_WRITE) && !(flags & IOMAP_DIRECT)) {
@@ -2431,3 +2431,26 @@ int __gfs2_punch_hole(struct file *file, loff_t offset, loff_t length)
 		gfs2_trans_end(sdp);
 	return error;
 }
+
+static int gfs2_map_blocks(struct iomap_writepage_ctx *wpc, struct inode *inode,
+		loff_t offset)
+{
+	struct metapath mp = { .mp_aheight = 1, };
+	int ret;
+
+	if (WARN_ON_ONCE(gfs2_is_stuffed(GFS2_I(inode))))
+		return -EIO;
+
+	if (offset >= wpc->iomap.offset &&
+	    offset < wpc->iomap.offset + wpc->iomap.length)
+		return 0;
+
+	memset(&wpc->iomap, 0, sizeof(wpc->iomap));
+	ret = gfs2_iomap_get(inode, offset, INT_MAX, 0, &wpc->iomap, &mp);
+	release_metapath(&mp);
+	return ret;
+}
+
+const struct iomap_writeback_ops gfs2_writeback_ops = {
+	.map_blocks		= gfs2_map_blocks,
+};
diff --git a/fs/gfs2/bmap.h b/fs/gfs2/bmap.h
index b88fd45ab79f..aed4632d47d3 100644
--- a/fs/gfs2/bmap.h
+++ b/fs/gfs2/bmap.h
@@ -44,6 +44,7 @@ static inline void gfs2_write_calc_reserv(const struct gfs2_inode *ip,
 }
 
 extern const struct iomap_ops gfs2_iomap_ops;
+extern const struct iomap_writeback_ops gfs2_writeback_ops;
 
 extern int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page);
 extern int gfs2_block_map(struct inode *inode, sector_t lblock,
-- 
2.20.1

Re: [PATCH 15/15] gfs2: use iomap for buffered I/O in ordered and writeback mode

[Andreas Gruenbacher]

Christoph,

thanks again for this patch and the rest of the patch queue. There's
one minor bug here (see below). With that and the gfs2_walk_metadata
fix I've just posted to cluster-devel, this is now all working nicely.

On Mon, 1 Jul 2019 at 23:56, Christoph Hellwig <hch@lst.de> wrote:
> Switch to using the iomap readpage and writepage helpers for all I/O in
> the ordered and writeback modes, and thus eliminate using buffer_heads
> for I/O in these cases.  The journaled data mode is left untouched.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/gfs2/aops.c | 59 +++++++++++++++++++++++---------------------------
>  fs/gfs2/bmap.c | 47 ++++++++++++++++++++++++++++++----------
>  fs/gfs2/bmap.h |  1 +
>  3 files changed, 63 insertions(+), 44 deletions(-)
>
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 15a234fb8f88..9cdd61a44379 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -91,22 +91,13 @@ static int gfs2_writepage(struct page *page, struct writeback_control *wbc)
>         struct inode *inode = page->mapping->host;
>         struct gfs2_inode *ip = GFS2_I(inode);
>         struct gfs2_sbd *sdp = GFS2_SB(inode);
> -       loff_t i_size = i_size_read(inode);
> -       pgoff_t end_index = i_size >> PAGE_SHIFT;
> -       unsigned offset;
> +       struct iomap_writepage_ctx wpc = { };
>
>         if (gfs2_assert_withdraw(sdp, gfs2_glock_is_held_excl(ip->i_gl)))
>                 goto out;
>         if (current->journal_info)
>                 goto redirty;
> -       /* Is the page fully outside i_size? (truncate in progress) */
> -       offset = i_size & (PAGE_SIZE-1);
> -       if (page->index > end_index || (page->index == end_index && !offset)) {
> -               page->mapping->a_ops->invalidatepage(page, 0, PAGE_SIZE);
> -               goto out;
> -       }
> -
> -       return nobh_writepage(page, gfs2_get_block_noalloc, wbc);
> +       return iomap_writepage(page, wbc, &wpc, &gfs2_writeback_ops);
>
>  redirty:
>         redirty_page_for_writepage(wbc, page);
> @@ -210,7 +201,8 @@ static int gfs2_writepages(struct address_space *mapping,
>                            struct writeback_control *wbc)
>  {
>         struct gfs2_sbd *sdp = gfs2_mapping2sbd(mapping);
> -       int ret = mpage_writepages(mapping, wbc, gfs2_get_block_noalloc);
> +       struct iomap_writepage_ctx wpc = { };
> +       int ret;
>
>         /*
>          * Even if we didn't write any pages here, we might still be holding
> @@ -218,9 +210,9 @@ static int gfs2_writepages(struct address_space *mapping,
>          * want balance_dirty_pages() to loop indefinitely trying to write out
>          * pages held in the ail that it can't find.
>          */
> +       ret = iomap_writepages(mapping, wbc, &wpc, &gfs2_writeback_ops);
>         if (ret == 0)
>                 set_bit(SDF_FORCE_AIL_FLUSH, &sdp->sd_flags);
> -
>         return ret;
>  }
>
> @@ -469,7 +461,6 @@ static int stuffed_readpage(struct gfs2_inode *ip, struct page *page)
>         return 0;
>  }
>
> -
>  /**
>   * __gfs2_readpage - readpage
>   * @file: The file to read a page for
> @@ -479,16 +470,15 @@ static int stuffed_readpage(struct gfs2_inode *ip, struct page *page)
>   * reading code as in that case we already hold the glock. Also it's
>   * called by gfs2_readpage() once the required lock has been granted.
>   */
> -
>  static int __gfs2_readpage(void *file, struct page *page)
>  {
> -       struct gfs2_inode *ip = GFS2_I(page->mapping->host);
> -       struct gfs2_sbd *sdp = GFS2_SB(page->mapping->host);
> -
> +       struct inode *inode = page->mapping->host;
> +       struct gfs2_inode *ip = GFS2_I(inode);
> +       struct gfs2_sbd *sdp = GFS2_SB(inode);
>         int error;
>
> -       if (i_blocksize(page->mapping->host) == PAGE_SIZE &&
> -           !page_has_buffers(page)) {
> +       if (!gfs2_is_jdata(ip) ||
> +           (i_blocksize(inode) == PAGE_SIZE && !page_has_buffers(page))) {
>                 error = iomap_readpage(page, &gfs2_iomap_ops);
>         } else if (gfs2_is_stuffed(ip)) {
>                 error = stuffed_readpage(ip, page);
> @@ -609,8 +599,12 @@ static int gfs2_readpages(struct file *file, struct address_space *mapping,
>         ret = gfs2_glock_nq(&gh);
>         if (unlikely(ret))
>                 goto out_uninit;
> -       if (!gfs2_is_stuffed(ip))
> +       if (gfs2_is_stuffed(ip))
> +               ;
> +       else if (gfs2_is_jdata(ip))
>                 ret = mpage_readpages(mapping, pages, nr_pages, gfs2_block_map);
> +       else
> +               ret = iomap_readpages(mapping, pages, nr_pages, &gfs2_iomap_ops);
>         gfs2_glock_dq(&gh);
>  out_uninit:
>         gfs2_holder_uninit(&gh);
> @@ -827,17 +821,18 @@ int gfs2_releasepage(struct page *page, gfp_t gfp_mask)
>  }
>
>  static const struct address_space_operations gfs2_aops = {
> -       .writepage = gfs2_writepage,
> -       .writepages = gfs2_writepages,
> -       .readpage = gfs2_readpage,
> -       .readpages = gfs2_readpages,
> -       .bmap = gfs2_bmap,
> -       .invalidatepage = gfs2_invalidatepage,
> -       .releasepage = gfs2_releasepage,
> -       .direct_IO = noop_direct_IO,
> -       .migratepage = buffer_migrate_page,
> -       .is_partially_uptodate = block_is_partially_uptodate,
> -       .error_remove_page = generic_error_remove_page,
> +       .writepage              = gfs2_writepage,
> +       .writepages             = gfs2_writepages,
> +       .readpage               = gfs2_readpage,
> +       .readpages              = gfs2_readpages,
> +       .set_page_dirty         = iomap_set_page_dirty,
> +       .releasepage            = iomap_releasepage,
> +       .invalidatepage         = iomap_invalidatepage,
> +       .bmap                   = gfs2_bmap,
> +       .direct_IO              = noop_direct_IO,
> +       .migratepage            = iomap_migrate_page,
> +       .is_partially_uptodate  = iomap_is_partially_uptodate,
> +       .error_remove_page      = generic_error_remove_page,
>  };
>
>  static const struct address_space_operations gfs2_jdata_aops = {
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index b7bd811872cb..b8d795d277c9 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -56,7 +56,6 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
>                                u64 block, struct page *page)
>  {
>         struct inode *inode = &ip->i_inode;
> -       struct buffer_head *bh;
>         int release = 0;
>
>         if (!page || page->index) {
> @@ -80,20 +79,20 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
>                 SetPageUptodate(page);
>         }
>
> -       if (!page_has_buffers(page))
> -               create_empty_buffers(page, BIT(inode->i_blkbits),
> -                                    BIT(BH_Uptodate));
> +       if (gfs2_is_jdata(ip)) {
> +               struct buffer_head *bh;
>
> -       bh = page_buffers(page);
> +               if (!page_has_buffers(page))
> +                       create_empty_buffers(page, BIT(inode->i_blkbits),
> +                                            BIT(BH_Uptodate));
>
> -       if (!buffer_mapped(bh))
> -               map_bh(bh, inode->i_sb, block);
> +               bh = page_buffers(page);
> +               if (!buffer_mapped(bh))
> +                       map_bh(bh, inode->i_sb, block);
>
> -       set_buffer_uptodate(bh);
> -       if (gfs2_is_jdata(ip))
> +               set_buffer_uptodate(bh);
>                 gfs2_trans_add_data(ip->i_gl, bh);
> -       else {
> -               mark_buffer_dirty(bh);

We need to turn mark_buffer_dirty(bh) into set_page_dirty(page) here
instead of just removing it.

> +       } else {
>                 gfs2_ordered_add_inode(ip);
>         }
>
> @@ -1127,7 +1126,8 @@ static int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
>         struct metapath mp = { .mp_aheight = 1, };
>         int ret;
>
> -       iomap->flags |= IOMAP_F_BUFFER_HEAD;
> +       if (gfs2_is_jdata(ip))
> +               iomap->flags |= IOMAP_F_BUFFER_HEAD;
>
>         trace_gfs2_iomap_start(ip, pos, length, flags);
>         if ((flags & IOMAP_WRITE) && !(flags & IOMAP_DIRECT)) {
> @@ -2431,3 +2431,26 @@ int __gfs2_punch_hole(struct file *file, loff_t offset, loff_t length)
>                 gfs2_trans_end(sdp);
>         return error;
>  }
> +
> +static int gfs2_map_blocks(struct iomap_writepage_ctx *wpc, struct inode *inode,
> +               loff_t offset)
> +{
> +       struct metapath mp = { .mp_aheight = 1, };
> +       int ret;
> +
> +       if (WARN_ON_ONCE(gfs2_is_stuffed(GFS2_I(inode))))
> +               return -EIO;
> +
> +       if (offset >= wpc->iomap.offset &&
> +           offset < wpc->iomap.offset + wpc->iomap.length)
> +               return 0;
> +
> +       memset(&wpc->iomap, 0, sizeof(wpc->iomap));
> +       ret = gfs2_iomap_get(inode, offset, INT_MAX, 0, &wpc->iomap, &mp);
> +       release_metapath(&mp);
> +       return ret;
> +}
> +
> +const struct iomap_writeback_ops gfs2_writeback_ops = {
> +       .map_blocks             = gfs2_map_blocks,
> +};
> diff --git a/fs/gfs2/bmap.h b/fs/gfs2/bmap.h
> index b88fd45ab79f..aed4632d47d3 100644
> --- a/fs/gfs2/bmap.h
> +++ b/fs/gfs2/bmap.h
> @@ -44,6 +44,7 @@ static inline void gfs2_write_calc_reserv(const struct gfs2_inode *ip,
>  }
>
>  extern const struct iomap_ops gfs2_iomap_ops;
> +extern const struct iomap_writeback_ops gfs2_writeback_ops;
>
>  extern int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page);
>  extern int gfs2_block_map(struct inode *inode, sector_t lblock,
> --
> 2.20.1
>

Thanks,
Andreas

Re: [PATCH 15/15] gfs2: use iomap for buffered I/O in ordered and writeback mode

[Christoph Hellwig]

On Mon, Aug 05, 2019 at 02:27:21PM +0200, Andreas Gruenbacher wrote:
> Christoph,
> 
> thanks again for this patch and the rest of the patch queue. There's
> one minor bug here (see below). With that and the gfs2_walk_metadata
> fix I've just posted to cluster-devel, this is now all working nicely.

Skipping through the full quote this was a missing set_page_dirty,
right?  Looks fine to me and sorry for messing this up.

Re: [PATCH 15/15] gfs2: use iomap for buffered I/O in ordered and writeback mode

[Andreas Gruenbacher]

Hi Christoph,

On Tue, Aug 6, 2019 at 7:30 AM Christoph Hellwig <hch@lst.de> wrote:
> On Mon, Aug 05, 2019 at 02:27:21PM +0200, Andreas Gruenbacher wrote:
> > Christoph,
> >
> > thanks again for this patch and the rest of the patch queue. There's
> > one minor bug here (see below). With that and the gfs2_walk_metadata
> > fix I've just posted to cluster-devel, this is now all working nicely.
>
> Skipping through the full quote this was a missing set_page_dirty,
> right?  Looks fine to me and sorry for messing this up.

here are the changes we currently need on top of what you've posted on
July 1.  On top of the page dirtying which you patch accidentally
dropped in gfs2_unstuffer_page, there are two places in which we also
need to call iomap_page_create to attach an iomap_page structure to the
pages.

The first place is in gfs2_unstuffer_page, which converts an inline
(stuffed) file into a regular file.  This is implemented in a filesystem
specific way, and I don't think there is any point in trying to make
this more generic.

The second place is in gfs2_page_mkwrite.  This function should
eventually be changed to call iomap_page_mkwrite instead, but we can
just fix it as below just to get this working.

Currently, iomap_page_create is a static function in
fs/iomap/buffered-io.c, so we need to export it before these changes
will work.

I'm still trying to track down consistency problems with a 1k blocksize
in xfstests generic/263 and generic/300, and that is with the mmap
locking issue fixed that Dave Chinner has pointed out [*].  This problem
existed even before your changes, so your changes seem to be working
correctly.

Thanks again,
Andreas

[*] https://lore.kernel.org/linux-fsdevel/20190906205241.2292-1-agruenba@redhat.com/

---
 fs/gfs2/bmap.c | 2 ++
 fs/gfs2/file.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index bf5c494d25ef..48b458c49fa1 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -93,6 +93,8 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
 		set_buffer_uptodate(bh);
 		gfs2_trans_add_data(ip->i_gl, bh);
 	} else {
+		iomap_page_create(inode, page);
+		set_page_dirty(page);
 		gfs2_ordered_add_inode(ip);
 	}
 
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 997b326247e2..30fd180e199d 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -516,6 +516,8 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 out_uninit:
 	gfs2_holder_uninit(&gh);
 	if (ret == 0) {
+		if (!gfs2_is_jdata(ip))
+			iomap_page_create(inode, page);
 		set_page_dirty(page);
 		wait_for_stable_page(page);
 	}
-- 
2.20.1