RFC: use the iomap writepage path in gfs2

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 01/15] FOLD: iomap: make the discard_page method optional

[Christoph Hellwig]

For file system that do not support delayed allocations there is
nothing to discard here, so don't require them to implement the
method.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 72ba3962acf3..ebfff663b2a9 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -2545,7 +2545,8 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 	 */
 	if (unlikely(error)) {
 		if (!count) {
-			wpc->ops->discard_page(page);
+			if (wpc->ops->discard_page)
+				wpc->ops->discard_page(page);
 			ClearPageUptodate(page);
 			unlock_page(page);
 			goto done;
-- 
2.20.1

[PATCH 02/15] FOLD: iomap: make ->submit_ioend optional

[Christoph Hellwig]

Provide a default end_io handler that comple file systems can override
if they need deferred action.  With that we don't need an submit_ioend
method for simple file systems.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c        | 26 +++++++++++++++++++++-----
 fs/xfs/xfs_aops.c | 23 ++++++++++-------------
 2 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index ebfff663b2a9..7574f63939cc 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -2350,6 +2350,13 @@ iomap_sort_ioends(struct list_head *ioend_list)
 }
 EXPORT_SYMBOL_GPL(iomap_sort_ioends);
 
+static void iomap_writepage_end_bio(struct bio *bio)
+{
+	struct iomap_ioend *ioend = bio->bi_private;
+
+	iomap_finish_ioend(ioend, blk_status_to_errno(bio->bi_status));
+}
+
 /*
  * Submit the bio for an ioend. We are passed an ioend with a bio attached to
  * it, and we submit that bio. The ioend may be used for multiple bio
@@ -2368,14 +2375,23 @@ static int
 iomap_submit_ioend(struct iomap_writepage_ctx *wpc, struct iomap_ioend *ioend,
 		int error)
 {
+	ioend->io_bio->bi_private = ioend;
+	ioend->io_bio->bi_end_io = iomap_writepage_end_bio;
+
 	/*
-	 * If we are failing the IO now, just mark the ioend with an error and
-	 * finish it.  This will run IO completion immediately as there is only
-	 * one reference to the ioend at this point in time.
+	 * File systems can perform actions at submit time and/or override
+	 * the end_io handler here for complex operations like copy on write
+	 * extent manipulation or unwritten extent conversions.
 	 */
-	ioend->io_bio->bi_private = ioend;
-	error = wpc->ops->submit_ioend(ioend, error);
+	if (wpc->ops->submit_ioend)
+		error = wpc->ops->submit_ioend(ioend, error);
 	if (error) {
+		/*
+		 * If we are failing the IO now, just mark the ioend with an
+		 * error and finish it.  This will run IO completion immediately
+		 * as there is only one reference to the ioend at this point in
+		 * time.
+		 */
 		ioend->io_bio->bi_status = errno_to_blk_status(error);
 		bio_endio(ioend->io_bio);
 		return error;
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 0821312a1d11..ac1404bc583c 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -265,20 +265,14 @@ xfs_end_bio(
 {
 	struct iomap_ioend	*ioend = bio->bi_private;
 	struct xfs_inode	*ip = XFS_I(ioend->io_inode);
-	struct xfs_mount	*mp = ip->i_mount;
 	unsigned long		flags;
 
-	if ((ioend->io_flags & IOMAP_F_SHARED) ||
-	    ioend->io_type == IOMAP_UNWRITTEN ||
-	    ioend->io_private) {
-		spin_lock_irqsave(&ip->i_ioend_lock, flags);
-		if (list_empty(&ip->i_ioend_list))
-			WARN_ON_ONCE(!queue_work(mp->m_unwritten_workqueue,
-						 &ip->i_ioend_work));
-		list_add_tail(&ioend->io_list, &ip->i_ioend_list);
-		spin_unlock_irqrestore(&ip->i_ioend_lock, flags);
-	} else
-		iomap_finish_ioend(ioend, blk_status_to_errno(bio->bi_status));
+	spin_lock_irqsave(&ip->i_ioend_lock, flags);
+	if (list_empty(&ip->i_ioend_list))
+		WARN_ON_ONCE(!queue_work(ip->i_mount->m_unwritten_workqueue,
+					 &ip->i_ioend_work));
+	list_add_tail(&ioend->io_list, &ip->i_ioend_list);
+	spin_unlock_irqrestore(&ip->i_ioend_lock, flags);
 }
 
 /*
@@ -531,7 +525,10 @@ xfs_submit_ioend(
 
 	memalloc_nofs_restore(nofs_flag);
 
-	ioend->io_bio->bi_end_io = xfs_end_bio;
+	if ((ioend->io_flags & IOMAP_F_SHARED) ||
+	    ioend->io_type == IOMAP_UNWRITTEN ||
+	    ioend->io_private)
+		ioend->io_bio->bi_end_io = xfs_end_bio;
 	return status;
 }
 
-- 
2.20.1

[PATCH 03/15] iomap: zero newly allocated mapped blocks

[Christoph Hellwig]

File systems like gfs2 don't support delayed allocations or unwritten
extents and thus allocate normal mapped blocks to fill holes.  To
cover the case of such file systems allocating new blocks to fill holes
also zero out mapped blocks with the new flag.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 7574f63939cc..0a86aaee961f 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -284,6 +284,13 @@ iomap_read_inline_data(struct inode *inode, struct page *page,
 	SetPageUptodate(page);
 }
 
+static inline bool iomap_block_needs_zeroing(struct inode *inode,
+		struct iomap *iomap, loff_t pos)
+{
+	return iomap->type != IOMAP_MAPPED || (iomap->flags & IOMAP_F_NEW) ||
+			pos >= i_size_read(inode);
+}
+
 static loff_t
 iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
 		struct iomap *iomap)
@@ -307,7 +314,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
 	if (plen == 0)
 		goto done;
 
-	if (iomap->type != IOMAP_MAPPED || pos >= i_size_read(inode)) {
+	if (iomap_block_needs_zeroing(inode, iomap, pos)) {
 		zero_user(page, poff, plen);
 		iomap_set_range_uptodate(page, poff, plen);
 		goto done;
@@ -617,7 +624,7 @@ iomap_read_page_sync(struct inode *inode, loff_t block_start, struct page *page,
 	struct bio_vec bvec;
 	struct bio bio;
 
-	if (iomap->type != IOMAP_MAPPED || block_start >= i_size_read(inode)) {
+	if (iomap_block_needs_zeroing(inode, iomap, block_start)) {
 		zero_user_segments(page, poff, from, to, poff + plen);
 		iomap_set_range_uptodate(page, poff, plen);
 		return 0;
-- 
2.20.1

[PATCH 04/15] iomap: warn on inline maps iomap_writepage_map

[Christoph Hellwig]

And inline mapping should never mark the page dirty and thus never end up
in writepages.  Add a check for that condition and warn if it happens.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/iomap.c b/fs/iomap.c
index 0a86aaee961f..ea5b8e7c8903 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -2541,6 +2541,8 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 		error = wpc->ops->map_blocks(wpc, inode, file_offset);
 		if (error)
 			break;
+		if (WARN_ON_ONCE(wpc->iomap.type == IOMAP_INLINE))
+			continue;
 		if (wpc->iomap.type == IOMAP_HOLE)
 			continue;
 		iomap_add_to_ioend(inode, file_offset, page, iop, wpc, wbc,
-- 
2.20.1

[PATCH 05/15] iomap: move struct iomap_page to iomap.c

[Christoph Hellwig]

Now that all the writepage code is in the iomap code there is no
need to keep this structure public.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c            | 17 +++++++++++++++++
 include/linux/iomap.h | 17 -----------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index ea5b8e7c8903..63952a7b1c05 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -100,6 +100,23 @@ iomap_sector(struct iomap *iomap, loff_t pos)
 	return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT;
 }
 
+/*
+ * Structure allocate for each page when block size < PAGE_SIZE to track
+ * sub-page uptodate status and I/O completions.
+ */
+struct iomap_page {
+	atomic_t		read_count;
+	atomic_t		write_count;
+	DECLARE_BITMAP(uptodate, PAGE_SIZE / 512);
+};
+
+static inline struct iomap_page *to_iomap_page(struct page *page)
+{
+	if (page_has_private(page))
+		return (struct iomap_page *)page_private(page);
+	return NULL;
+}
+
 static struct iomap_page *
 iomap_page_create(struct inode *inode, struct page *page)
 {
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index f9fb716e60ab..a5f0565210a0 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -117,23 +117,6 @@ struct iomap_ops {
 			ssize_t written, unsigned flags, struct iomap *iomap);
 };
 
-/*
- * Structure allocate for each page when block size < PAGE_SIZE to track
- * sub-page uptodate status and I/O completions.
- */
-struct iomap_page {
-	atomic_t		read_count;
-	atomic_t		write_count;
-	DECLARE_BITMAP(uptodate, PAGE_SIZE / 512);
-};
-
-static inline struct iomap_page *to_iomap_page(struct page *page)
-{
-	if (page_has_private(page))
-		return (struct iomap_page *)page_private(page);
-	return NULL;
-}
-
 ssize_t iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *from,
 		const struct iomap_ops *ops);
 int iomap_readpage(struct page *page, const struct iomap_ops *ops);
-- 
2.20.1

[PATCH 06/15] HACK: disable lockdep annotation in iomap_dio_rw

[Christoph Hellwig]

gfs2 seems to never calls this with i_rwsem held, disable for testing.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 63952a7b1c05..264cfb2e796f 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -1885,7 +1885,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	struct blk_plug plug;
 	struct iomap_dio *dio;
 
-	lockdep_assert_held(&inode->i_rwsem);
+//	lockdep_assert_held(&inode->i_rwsem);
 
 	if (!count)
 		return 0;
-- 
2.20.1

[PATCH 07/15] gfs2: use page_offset in gfs2_page_mkwrite

[Christoph Hellwig]

Without casting page->index to a guaranteed 64-bit type the value
might be treated as 32-bit on 32-bit platforms and thus get truncated.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 1cb0c3afd3dc..282a4aaab900 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -424,7 +424,7 @@ static vm_fault_t gfs2_page_mkwrite(struct vm_fault *vmf)
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	struct gfs2_alloc_parms ap = { .aflags = 0, };
 	unsigned long last_index;
-	u64 pos = page->index << PAGE_SHIFT;
+	u64 pos = page_offset(page);
 	unsigned int data_blocks, ind_blocks, rblocks;
 	struct gfs2_holder gh;
 	loff_t size;
-- 
2.20.1

[PATCH 08/15] gfs2: remove the unused gfs2_stuffed_write_end function

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/aops.c | 41 -----------------------------------------
 fs/gfs2/aops.h |  3 ---
 2 files changed, 44 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index abeac61cfed3..3c58b40c93eb 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -685,47 +685,6 @@ void adjust_fs_space(struct inode *inode)
 	gfs2_trans_end(sdp);
 }
 
-/**
- * gfs2_stuffed_write_end - Write end for stuffed files
- * @inode: The inode
- * @dibh: The buffer_head containing the on-disk inode
- * @pos: The file position
- * @copied: How much was actually copied by the VFS
- * @page: The page
- *
- * This copies the data from the page into the inode block after
- * the inode data structure itself.
- *
- * Returns: copied bytes or errno
- */
-int gfs2_stuffed_write_end(struct inode *inode, struct buffer_head *dibh,
-			   loff_t pos, unsigned copied,
-			   struct page *page)
-{
-	struct gfs2_inode *ip = GFS2_I(inode);
-	u64 to = pos + copied;
-	void *kaddr;
-	unsigned char *buf = dibh->b_data + sizeof(struct gfs2_dinode);
-
-	BUG_ON(pos + copied > gfs2_max_stuffed_size(ip));
-
-	kaddr = kmap_atomic(page);
-	memcpy(buf + pos, kaddr + pos, copied);
-	flush_dcache_page(page);
-	kunmap_atomic(kaddr);
-
-	WARN_ON(!PageUptodate(page));
-	unlock_page(page);
-	put_page(page);
-
-	if (copied) {
-		if (inode->i_size < to)
-			i_size_write(inode, to);
-		mark_inode_dirty(inode);
-	}
-	return copied;
-}
-
 /**
  * jdata_set_page_dirty - Page dirtying function
  * @page: The page to dirty
diff --git a/fs/gfs2/aops.h b/fs/gfs2/aops.h
index fa8e5d0144dd..3a6d8a90d99e 100644
--- a/fs/gfs2/aops.h
+++ b/fs/gfs2/aops.h
@@ -9,9 +9,6 @@
 #include "incore.h"
 
 extern int stuffed_readpage(struct gfs2_inode *ip, struct page *page);
-extern int gfs2_stuffed_write_end(struct inode *inode, struct buffer_head *dibh,
-				  loff_t pos, unsigned copied,
-				  struct page *page);
 extern void adjust_fs_space(struct inode *inode);
 extern void gfs2_page_add_databufs(struct gfs2_inode *ip, struct page *page,
 				   unsigned int from, unsigned int len);
-- 
2.20.1

[PATCH 09/15] gfs2: merge gfs2_writeback_aops and gfs2_ordered_aops

[Christoph Hellwig]

The only difference between the two is that gfs2_ordered_aops sets the
set_page_dirty method to __set_page_dirty_buffers, but given that
__set_page_dirty_buffers is the default if no method is set there is
no need to to do that.  Merge the two sets of operations into one.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/aops.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 3c58b40c93eb..3b3043332e5a 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -847,7 +847,7 @@ int gfs2_releasepage(struct page *page, gfp_t gfp_mask)
 	return 0;
 }
 
-static const struct address_space_operations gfs2_writeback_aops = {
+static const struct address_space_operations gfs2_aops = {
 	.writepage = gfs2_writepage,
 	.writepages = gfs2_writepages,
 	.readpage = gfs2_readpage,
@@ -861,21 +861,6 @@ static const struct address_space_operations gfs2_writeback_aops = {
 	.error_remove_page = generic_error_remove_page,
 };
 
-static const struct address_space_operations gfs2_ordered_aops = {
-	.writepage = gfs2_writepage,
-	.writepages = gfs2_writepages,
-	.readpage = gfs2_readpage,
-	.readpages = gfs2_readpages,
-	.set_page_dirty = __set_page_dirty_buffers,
-	.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,
-};
-
 static const struct address_space_operations gfs2_jdata_aops = {
 	.writepage = gfs2_jdata_writepage,
 	.writepages = gfs2_jdata_writepages,
@@ -891,15 +876,8 @@ static const struct address_space_operations gfs2_jdata_aops = {
 
 void gfs2_set_aops(struct inode *inode)
 {
-	struct gfs2_inode *ip = GFS2_I(inode);
-	struct gfs2_sbd *sdp = GFS2_SB(inode);
-
-	if (gfs2_is_jdata(ip))
+	if (gfs2_is_jdata(GFS2_I(inode)))
 		inode->i_mapping->a_ops = &gfs2_jdata_aops;
-	else if (gfs2_is_writeback(sdp))
-		inode->i_mapping->a_ops = &gfs2_writeback_aops;
-	else if (gfs2_is_ordered(sdp))
-		inode->i_mapping->a_ops = &gfs2_ordered_aops;
 	else
-		BUG();
+		inode->i_mapping->a_ops = &gfs2_aops;
 }
-- 
2.20.1

[PATCH 10/15] gfs2: merge gfs2_writepage_common into gfs2_writepage

[Christoph Hellwig]

No need to keep these two functions separate.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/aops.c | 32 ++++++--------------------------
 1 file changed, 6 insertions(+), 26 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 3b3043332e5a..d78b5778fca7 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -82,15 +82,11 @@ static int gfs2_get_block_noalloc(struct inode *inode, sector_t lblock,
 }
 
 /**
- * gfs2_writepage_common - Common bits of writepage
- * @page: The page to be written
+ * gfs2_writepage - Write page for writeback mappings
+ * @page: The page
  * @wbc: The writeback control
- *
- * Returns: 1 if writepage is ok, otherwise an error code or zero if no error.
  */
-
-static int gfs2_writepage_common(struct page *page,
-				 struct writeback_control *wbc)
+static int gfs2_writepage(struct page *page, struct writeback_control *wbc)
 {
 	struct inode *inode = page->mapping->host;
 	struct gfs2_inode *ip = GFS2_I(inode);
@@ -109,7 +105,9 @@ static int gfs2_writepage_common(struct page *page,
 		page->mapping->a_ops->invalidatepage(page, 0, PAGE_SIZE);
 		goto out;
 	}
-	return 1;
+
+	return nobh_writepage(page, gfs2_get_block_noalloc, wbc);
+
 redirty:
 	redirty_page_for_writepage(wbc, page);
 out:
@@ -117,24 +115,6 @@ static int gfs2_writepage_common(struct page *page,
 	return 0;
 }
 
-/**
- * gfs2_writepage - Write page for writeback mappings
- * @page: The page
- * @wbc: The writeback control
- *
- */
-
-static int gfs2_writepage(struct page *page, struct writeback_control *wbc)
-{
-	int ret;
-
-	ret = gfs2_writepage_common(page, wbc);
-	if (ret <= 0)
-		return ret;
-
-	return nobh_writepage(page, gfs2_get_block_noalloc, wbc);
-}
-
 /* This is the same as calling block_write_full_page, but it also
  * writes pages outside of i_size
  */
-- 
2.20.1

[PATCH 11/15] gfs2: mark stuffed_readpage static

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/aops.c | 3 +--
 fs/gfs2/aops.h | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index d78b5778fca7..030210f1430b 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -434,8 +434,7 @@ static int gfs2_jdata_writepages(struct address_space *mapping,
  *
  * Returns: errno
  */
-
-int stuffed_readpage(struct gfs2_inode *ip, struct page *page)
+static int stuffed_readpage(struct gfs2_inode *ip, struct page *page)
 {
 	struct buffer_head *dibh;
 	u64 dsize = i_size_read(&ip->i_inode);
diff --git a/fs/gfs2/aops.h b/fs/gfs2/aops.h
index 3a6d8a90d99e..ff9877a68780 100644
--- a/fs/gfs2/aops.h
+++ b/fs/gfs2/aops.h
@@ -8,7 +8,6 @@
 
 #include "incore.h"
 
-extern int stuffed_readpage(struct gfs2_inode *ip, struct page *page);
 extern void adjust_fs_space(struct inode *inode);
 extern void gfs2_page_add_databufs(struct gfs2_inode *ip, struct page *page,
 				   unsigned int from, unsigned int len);
-- 
2.20.1

[PATCH 12/15] gfs2: use iomap_bmap instead of generic_block_bmap

[Christoph Hellwig]

No need to indirect through get_blocks and buffer_heads when we can
just use the iomap version.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/aops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 030210f1430b..15a234fb8f88 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -697,7 +697,7 @@ static sector_t gfs2_bmap(struct address_space *mapping, sector_t lblock)
 		return 0;
 
 	if (!gfs2_is_stuffed(ip))
-		dblock = generic_block_bmap(mapping, lblock, gfs2_block_map);
+		dblock = iomap_bmap(mapping, lblock, &gfs2_iomap_ops);
 
 	gfs2_glock_dq_uninit(&i_gh);
 
-- 
2.20.1

[PATCH 13/15] gfs2: implement gfs2_block_zero_range using iomap_zero_range

[Christoph Hellwig]

iomap handles all the nitty-gritty details of zeroing a file
range for us, so use the proper helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/bmap.c | 68 +-------------------------------------------------
 1 file changed, 1 insertion(+), 67 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 8e8768685264..b7bd811872cb 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1281,76 +1281,10 @@ int gfs2_extent_map(struct inode *inode, u64 lblock, int *new, u64 *dblock, unsi
 	return ret;
 }
 
-/**
- * gfs2_block_zero_range - Deal with zeroing out data
- *
- * This is partly borrowed from ext3.
- */
 static int gfs2_block_zero_range(struct inode *inode, loff_t from,
 				 unsigned int length)
 {
-	struct address_space *mapping = inode->i_mapping;
-	struct gfs2_inode *ip = GFS2_I(inode);
-	unsigned long index = from >> PAGE_SHIFT;
-	unsigned offset = from & (PAGE_SIZE-1);
-	unsigned blocksize, iblock, pos;
-	struct buffer_head *bh;
-	struct page *page;
-	int err;
-
-	page = find_or_create_page(mapping, index, GFP_NOFS);
-	if (!page)
-		return 0;
-
-	blocksize = inode->i_sb->s_blocksize;
-	iblock = index << (PAGE_SHIFT - inode->i_sb->s_blocksize_bits);
-
-	if (!page_has_buffers(page))
-		create_empty_buffers(page, blocksize, 0);
-
-	/* Find the buffer that contains "offset" */
-	bh = page_buffers(page);
-	pos = blocksize;
-	while (offset >= pos) {
-		bh = bh->b_this_page;
-		iblock++;
-		pos += blocksize;
-	}
-
-	err = 0;
-
-	if (!buffer_mapped(bh)) {
-		gfs2_block_map(inode, iblock, bh, 0);
-		/* unmapped? It's a hole - nothing to do */
-		if (!buffer_mapped(bh))
-			goto unlock;
-	}
-
-	/* Ok, it's mapped. Make sure it's up-to-date */
-	if (PageUptodate(page))
-		set_buffer_uptodate(bh);
-
-	if (!buffer_uptodate(bh)) {
-		err = -EIO;
-		ll_rw_block(REQ_OP_READ, 0, 1, &bh);
-		wait_on_buffer(bh);
-		/* Uhhuh. Read error. Complain and punt. */
-		if (!buffer_uptodate(bh))
-			goto unlock;
-		err = 0;
-	}
-
-	if (gfs2_is_jdata(ip))
-		gfs2_trans_add_data(ip->i_gl, bh);
-	else
-		gfs2_ordered_add_inode(ip);
-
-	zero_user(page, offset, length);
-	mark_buffer_dirty(bh);
-unlock:
-	unlock_page(page);
-	put_page(page);
-	return err;
+	return iomap_zero_range(inode, from, length, NULL, &gfs2_iomap_ops);
 }
 
 #define GFS2_JTRUNC_REVOKES 8192
-- 
2.20.1

[PATCH 14/15] gfs2: don't use buffer_heads in gfs2_allocate_page_backing

[Christoph Hellwig]

Rewrite gfs2_allocate_page_backing to call gfs2_iomap_get_alloc and
operate on struct iomap directly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/gfs2/file.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 282a4aaab900..8c72e4cecd89 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -379,31 +379,30 @@ static void gfs2_size_hint(struct file *filep, loff_t offset, size_t size)
 }
 
 /**
- * gfs2_allocate_page_backing - Use bmap to allocate blocks
+ * gfs2_allocate_page_backing - Allocate blocks for a write fault
  * @page: The (locked) page to allocate backing for
  *
- * We try to allocate all the blocks required for the page 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, thats ok too.
+ * We try to allocate all the blocks required for the page 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,
+ * that is ok too.
  */
-
 static int gfs2_allocate_page_backing(struct page *page)
 {
-	struct inode *inode = page->mapping->host;
-	struct buffer_head bh;
-	unsigned long size = PAGE_SIZE;
-	u64 lblock = page->index << (PAGE_SHIFT - inode->i_blkbits);
+	u64 pos = page_offset(page);
+	u64 size = PAGE_SIZE;
 
 	do {
-		bh.b_state = 0;
-		bh.b_size = size;
-		gfs2_block_map(inode, lblock, &bh, 1);
-		if (!buffer_mapped(&bh))
+		struct iomap iomap = { };
+
+		if (gfs2_iomap_get_alloc(page->mapping->host, pos, 1, &iomap))
 			return -EIO;
-		size -= bh.b_size;
-		lblock += (bh.b_size >> inode->i_blkbits);
-	} while(size > 0);
+
+		iomap.length = min(iomap.length, size);
+		size -= iomap.length;
+		pos += iomap.length;
+	} while (size > 0);
+
 	return 0;
 }
 
-- 
2.20.1

[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: RFC: use the iomap writepage path in gfs2

[Andreas Gruenbacher]

Hi Christoph,

On Mon, 1 Jul 2019 at 23:54, Christoph Hellwig <hch@lst.de> wrote:
> 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.

thank you very much, this is looking very good. I've done some testing
with your cleanups applied so that those can go in in this merge
window. The result can be found here:

  https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=for-next.cleanups

Patch "gfs2: implement gfs2_block_zero_range using iomap_zero_range"
isn't quite ready: the gfs2 iomap operations don't handle IOMAP_ZERO
correctly so far, and that needs to be fixed first.

The actual buffer head removal will obviously have to wait a little
longer because of the required infrastructure changes, but also
because that still needs a lot more review and testing work.

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

Some of the tests assume that the filesystem supports unwritten
extents, trusted xattrs, the usrquota / grpquota / prjquota mount
options. There shouldn't be a huge number of failing tests beyond
that, but I know things aren't perfect.

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

Again, thanks a lot for the patches!

Andreas

Re: RFC: use the iomap writepage path in gfs2

[Dave Chinner]

On Mon, Jul 01, 2019 at 11:54:24PM +0200, Christoph Hellwig wrote:
> 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.

Ok, this doesn't look too bad from the iomap perspective, though it
does raise more questions. :)

gfs2 now has two iopaths, right? One that uses bufferheads for
journalled data, and the other that uses iomap? That seems like it's
only a partial conversion - what needs to be done to iomap and gfs2
to support the journalled data path so there's a single data IO
path?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: RFC: use the iomap writepage path in gfs2

[Christoph Hellwig]

On Thu, Jul 04, 2019 at 12:35:41AM +0200, Andreas Gruenbacher wrote:
> Patch "gfs2: implement gfs2_block_zero_range using iomap_zero_range"
> isn't quite ready: the gfs2 iomap operations don't handle IOMAP_ZERO
> correctly so far, and that needs to be fixed first.

What is the issue with IOMAP_ZERO on gfs2?  Zeroing never does block
allocations except when on COW extents, which gfs2 doesn't support,
so there shouldn't really be any need for additional handling.

> Some of the tests assume that the filesystem supports unwritten
> extents, trusted xattrs, the usrquota / grpquota / prjquota mount
> options. There shouldn't be a huge number of failing tests beyond
> that, but I know things aren't perfect.

In general xfstests is supposed to have tests for that and not run
the tests if not supported.  In most cases this is automatic, but
in case a feature can't be autodetect we have a few manual overrides.

Re: RFC: use the iomap writepage path in gfs2

[Christoph Hellwig]

On Mon, Jul 08, 2019 at 10:01:03AM +1000, Dave Chinner wrote:
> Ok, this doesn't look too bad from the iomap perspective, though it
> does raise more questions. :)
> 
> gfs2 now has two iopaths, right? One that uses bufferheads for
> journalled data, and the other that uses iomap? That seems like it's
> only a partial conversion - what needs to be done to iomap and gfs2
> to support the journalled data path so there's a single data IO
> path?

gfs2 always had to very different writeback I/O paths, including a copy
and pasted versiom of write_cache_pages for journaled data, they just
diverge a little bit more now. In the longer run I'd also like to add
journaled data support to iomap for use with XFS, and then also switch
gfs2 to it. 

Re: RFC: use the iomap writepage path in gfs2

[Andreas Gruenbacher]

On Mon, 8 Jul 2019 at 18:04, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, Jul 04, 2019 at 12:35:41AM +0200, Andreas Gruenbacher wrote:
> > Patch "gfs2: implement gfs2_block_zero_range using iomap_zero_range"
> > isn't quite ready: the gfs2 iomap operations don't handle IOMAP_ZERO
> > correctly so far, and that needs to be fixed first.
>
> What is the issue with IOMAP_ZERO on gfs2?  Zeroing never does block
> allocations except when on COW extents, which gfs2 doesn't support,
> so there shouldn't really be any need for additional handling.

We still want to set iomap->page_ops for journalled data files on gfs2.

Also, if we go through the existing gfs2_iomap_begin_write /
__gfs2_iomap_begin logic for iomap_zero_range, it will work for
stuffed files as well, and so we can replace stuffed_zero_range with
iomap_zero_range.

> > Some of the tests assume that the filesystem supports unwritten
> > extents, trusted xattrs, the usrquota / grpquota / prjquota mount
> > options. There shouldn't be a huge number of failing tests beyond
> > that, but I know things aren't perfect.
>
> In general xfstests is supposed to have tests for that and not run
> the tests if not supported.  In most cases this is automatic, but
> in case a feature can't be autodetect we have a few manual overrides.

Yes, that needs a bit of work. Let's see.

Thanks,
Andreas

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

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?

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