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

From: Andreas Gruenbacher <agruenba@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Andreas Gruenbacher <agruenba@redhat.com>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	linux-xfs@vger.kernel.org,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	cluster-devel <cluster-devel@redhat.com>
Subject: Re: [PATCH 15/15] gfs2: use iomap for buffered I/O in ordered and writeback mode
Date: Mon, 30 Sep 2019 22:49:45 +0200	[thread overview]
Message-ID: <20190930204945.30780-1-agruenba@redhat.com> (raw)
In-Reply-To: <20190806053031.GD13409@lst.de>

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

