iomap preparations for GFS2 v2

iomap preparations for GFS2 v2

[Christoph Hellwig]

Hi all,

this is a slight rework of the patches from Andreas to prepare for gfs2
using the iomap code.

Note: I'd like to start with an immutable branch for iomap patches in either
the XFS tree or a tree of mine at the beginning of the merge window so that
we have a common base for the GFS2 and XFS iomap work.

Changes since v1:
 - move code to a different spot in iomap.c to prefer for readpages
   inline data support
 - add a forward declaration for struct page to iomap.h
 - fix a typo in the dax_dev/bdev unioning
 - fix a comment typo

[PATCH 1/6] fs: factor out a __generic_write_end helper

[Christoph Hellwig]

Bits of the buffer.c based write_end implementations that don't know
about buffer_heads and can be reused by other implementations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/buffer.c   | 67 +++++++++++++++++++++++++++------------------------
 fs/internal.h |  2 ++
 2 files changed, 37 insertions(+), 32 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index cabc045f483d..aba2a948b235 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2076,6 +2076,40 @@ int block_write_begin(struct address_space *mapping, loff_t pos, unsigned len,
 }
 EXPORT_SYMBOL(block_write_begin);
 
+int __generic_write_end(struct inode *inode, loff_t pos, unsigned copied,
+		struct page *page)
+{
+	loff_t old_size = inode->i_size;
+	bool i_size_changed = false;
+
+	/*
+	 * No need to use i_size_read() here, the i_size cannot change under us
+	 * because we hold i_rwsem.
+	 *
+	 * But it's important to update i_size while still holding page lock:
+	 * page writeout could otherwise come in and zero beyond i_size.
+	 */
+	if (pos + copied > inode->i_size) {
+		i_size_write(inode, pos + copied);
+		i_size_changed = true;
+	}
+
+	unlock_page(page);
+	put_page(page);
+
+	if (old_size < pos)
+		pagecache_isize_extended(inode, old_size, pos);
+	/*
+	 * Don't mark the inode dirty under page lock. First, it unnecessarily
+	 * makes the holding time of page lock longer. Second, it forces lock
+	 * ordering of page lock and transaction start for journaling
+	 * filesystems.
+	 */
+	if (i_size_changed)
+		mark_inode_dirty(inode);
+	return copied;
+}
+
 int block_write_end(struct file *file, struct address_space *mapping,
 			loff_t pos, unsigned len, unsigned copied,
 			struct page *page, void *fsdata)
@@ -2116,39 +2150,8 @@ int generic_write_end(struct file *file, struct address_space *mapping,
 			loff_t pos, unsigned len, unsigned copied,
 			struct page *page, void *fsdata)
 {
-	struct inode *inode = mapping->host;
-	loff_t old_size = inode->i_size;
-	int i_size_changed = 0;
-
 	copied = block_write_end(file, mapping, pos, len, copied, page, fsdata);
-
-	/*
-	 * No need to use i_size_read() here, the i_size
-	 * cannot change under us because we hold i_mutex.
-	 *
-	 * But it's important to update i_size while still holding page lock:
-	 * page writeout could otherwise come in and zero beyond i_size.
-	 */
-	if (pos+copied > inode->i_size) {
-		i_size_write(inode, pos+copied);
-		i_size_changed = 1;
-	}
-
-	unlock_page(page);
-	put_page(page);
-
-	if (old_size < pos)
-		pagecache_isize_extended(inode, old_size, pos);
-	/*
-	 * Don't mark the inode dirty under page lock. First, it unnecessarily
-	 * makes the holding time of page lock longer. Second, it forces lock
-	 * ordering of page lock and transaction start for journaling
-	 * filesystems.
-	 */
-	if (i_size_changed)
-		mark_inode_dirty(inode);
-
-	return copied;
+	return __generic_write_end(mapping->host, pos, copied, page);
 }
 EXPORT_SYMBOL(generic_write_end);
 
diff --git a/fs/internal.h b/fs/internal.h
index 980d005b21b4..4a18bdbd2214 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -43,6 +43,8 @@ static inline int __sync_blockdev(struct block_device *bdev, int wait)
 extern void guard_bio_eod(int rw, struct bio *bio);
 extern int __block_write_begin_int(struct page *page, loff_t pos, unsigned len,
 		get_block_t *get_block, struct iomap *iomap);
+int __generic_write_end(struct inode *inode, loff_t pos, unsigned copied,
+		struct page *page);
 
 /*
  * char_dev.c
-- 
2.17.1

[PATCH 2/6] iomap: move bdev and dax_dev in a union

[Christoph Hellwig]

We always either ask for a block device or DAX device mapping, so we can
use the same space for both.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ext2/inode.c       | 6 ++++--
 fs/ext4/inode.c       | 6 ++++--
 fs/xfs/xfs_iomap.c    | 6 ++++--
 include/linux/iomap.h | 6 ++++--
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 71635909df3b..8aead4e9dbc1 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -815,9 +815,11 @@ static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 		return ret;
 
 	iomap->flags = 0;
-	iomap->bdev = inode->i_sb->s_bdev;
 	iomap->offset = (u64)first_block << blkbits;
-	iomap->dax_dev = sbi->s_daxdev;
+	if (IS_DAX(inode))
+		iomap->dax_dev = sbi->s_daxdev;
+	else
+		iomap->bdev = inode->i_sb->s_bdev;
 
 	if (ret == 0) {
 		iomap->type = IOMAP_HOLE;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2ea07efbe016..79027e99118f 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3524,8 +3524,10 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	iomap->flags = 0;
 	if (ext4_inode_datasync_dirty(inode))
 		iomap->flags |= IOMAP_F_DIRTY;
-	iomap->bdev = inode->i_sb->s_bdev;
-	iomap->dax_dev = sbi->s_daxdev;
+	if (IS_DAX(inode))
+		iomap->dax_dev = sbi->s_daxdev;
+	else
+		iomap->bdev = inode->i_sb->s_bdev;
 	iomap->offset = (u64)first_block << blkbits;
 	iomap->length = (u64)map.m_len << blkbits;
 
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index c6ce6f9335b6..c9c3d0df5e4c 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -70,8 +70,10 @@ xfs_bmbt_to_iomap(
 	}
 	iomap->offset = XFS_FSB_TO_B(mp, imap->br_startoff);
 	iomap->length = XFS_FSB_TO_B(mp, imap->br_blockcount);
-	iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip));
-	iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
+	if (IS_DAX(VFS_I(ip)))
+		iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
+	else
+		iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip));
 }
 
 xfs_extlen_t
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index a044a824da85..212f4d59bcbf 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -53,8 +53,10 @@ struct iomap {
 	u64			length;	/* length of mapping, bytes */
 	u16			type;	/* type of mapping */
 	u16			flags;	/* flags for mapping */
-	struct block_device	*bdev;	/* block device for I/O */
-	struct dax_device	*dax_dev; /* dax_dev for dax operations */
+	union {
+		struct block_device *bdev;
+		struct dax_device   *dax_dev;
+	};
 };
 
 /*
-- 
2.17.1

[PATCH 3/6] iomap: mark newly allocated buffer heads as new

[Christoph Hellwig]

From: Andreas Gruenbacher <agruenba@redhat.com>

In iomap_to_bh, not only mark buffer heads in IOMAP_UNWRITTEN maps as
new, but also buffer heads in IOMAP_MAPPED maps with the IOMAP_F_NEW
flag set.  This will be used by filesystems like gfs2, which allocate
blocks in iomap->begin.

Minor corrections to the comment for IOMAP_UNWRITTEN maps.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/buffer.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index aba2a948b235..c8c2b7d8b8d6 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1900,15 +1900,16 @@ iomap_to_bh(struct inode *inode, sector_t block, struct buffer_head *bh,
 		break;
 	case IOMAP_UNWRITTEN:
 		/*
-		 * For unwritten regions, we always need to ensure that
-		 * sub-block writes cause the regions in the block we are not
-		 * writing to are zeroed. Set the buffer as new to ensure this.
+		 * For unwritten regions, we always need to ensure that regions
+		 * in the block we are not writing to are zeroed. Mark the
+		 * buffer as new to ensure this.
 		 */
 		set_buffer_new(bh);
 		set_buffer_unwritten(bh);
 		/* FALLTHRU */
 	case IOMAP_MAPPED:
-		if (offset >= i_size_read(inode))
+		if ((iomap->flags & IOMAP_F_NEW) ||
+		    offset >= i_size_read(inode))
 			set_buffer_new(bh);
 		bh->b_blocknr = (iomap->addr + offset - iomap->offset) >>
 				inode->i_blkbits;
-- 
2.17.1

[PATCH 4/6] iomap: complete partial direct I/O writes synchronously

[Christoph Hellwig]

From: Andreas Gruenbacher <agruenba@redhat.com>

According to xfstest generic/240, applications seem to expect direct I/O
writes to either complete as a whole or to fail; short direct I/O writes
are apparently not appreciated.  This means that when only part of an
asynchronous direct I/O write succeeds, we can either fail the entire
write, or we can wait for the partial write to complete and retry the
remaining write as buffered I/O.  The old __blockdev_direct_IO helper
has code for waiting for partial writes to complete; the new
iomap_dio_rw iomap helper does not.

The above mentioned fallback mode is needed for gfs2, which doesn't
allow block allocations under direct I/O to avoid taking cluster-wide
exclusive locks.  As a consequence, an asynchronous direct I/O write to
a file range that contains a hole will result in a short write.  In that
case, wait for the short write to complete to allow gfs2 to recover.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 7d1e9f45f098..bf1e81b08962 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -811,6 +811,7 @@ struct iomap_dio {
 	atomic_t		ref;
 	unsigned		flags;
 	int			error;
+	bool			wait_for_completion;
 
 	union {
 		/* used during submission and for synchronous completion: */
@@ -914,9 +915,8 @@ static void iomap_dio_bio_end_io(struct bio *bio)
 		iomap_dio_set_error(dio, blk_status_to_errno(bio->bi_status));
 
 	if (atomic_dec_and_test(&dio->ref)) {
-		if (is_sync_kiocb(dio->iocb)) {
+		if (dio->wait_for_completion) {
 			struct task_struct *waiter = dio->submit.waiter;
-
 			WRITE_ONCE(dio->submit.waiter, NULL);
 			wake_up_process(waiter);
 		} else if (dio->flags & IOMAP_DIO_WRITE) {
@@ -1131,13 +1131,12 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	dio->end_io = end_io;
 	dio->error = 0;
 	dio->flags = 0;
+	dio->wait_for_completion = is_sync_kiocb(iocb);
 
 	dio->submit.iter = iter;
-	if (is_sync_kiocb(iocb)) {
-		dio->submit.waiter = current;
-		dio->submit.cookie = BLK_QC_T_NONE;
-		dio->submit.last_queue = NULL;
-	}
+	dio->submit.waiter = current;
+	dio->submit.cookie = BLK_QC_T_NONE;
+	dio->submit.last_queue = NULL;
 
 	if (iov_iter_rw(iter) == READ) {
 		if (pos >= dio->i_size)
@@ -1187,7 +1186,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		dio_warn_stale_pagecache(iocb->ki_filp);
 	ret = 0;
 
-	if (iov_iter_rw(iter) == WRITE && !is_sync_kiocb(iocb) &&
+	if (iov_iter_rw(iter) == WRITE && !dio->wait_for_completion &&
 	    !inode->i_sb->s_dio_done_wq) {
 		ret = sb_init_dio_done_wq(inode->i_sb);
 		if (ret < 0)
@@ -1202,8 +1201,10 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 				iomap_dio_actor);
 		if (ret <= 0) {
 			/* magic error code to fall back to buffered I/O */
-			if (ret == -ENOTBLK)
+			if (ret == -ENOTBLK) {
+				dio->wait_for_completion = true;
 				ret = 0;
+			}
 			break;
 		}
 		pos += ret;
@@ -1224,7 +1225,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		dio->flags &= ~IOMAP_DIO_NEED_SYNC;
 
 	if (!atomic_dec_and_test(&dio->ref)) {
-		if (!is_sync_kiocb(iocb))
+		if (!dio->wait_for_completion)
 			return -EIOCBQUEUED;
 
 		for (;;) {
-- 
2.17.1

[PATCH 5/6] iomap: generic inline data handling

[Christoph Hellwig]

From: Andreas Gruenbacher <agruenba@redhat.com>

Add generic inline data handling by adding a pointer to the inline data
region to struct iomap.  When handling a buffered IOMAP_INLINE write,
iomap_write_begin will copy the current inline data from the inline data
region into the page cache, and iomap_write_end will copy the changes in
the page cache back to the inline data region.

This doesn't cover inline data reads and direct I/O yet because so far,
we have no users.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
[hch: small cleanups to better fit in with other iomap work]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c            | 62 ++++++++++++++++++++++++++++++++++++++-----
 include/linux/iomap.h |  1 +
 2 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index bf1e81b08962..6d5a2ebe5dd8 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -103,6 +103,26 @@ iomap_sector(struct iomap *iomap, loff_t pos)
 	return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT;
 }
 
+static void
+iomap_read_inline_data(struct inode *inode, struct page *page,
+		struct iomap *iomap)
+{
+	size_t size = i_size_read(inode);
+	void *addr;
+
+	if (PageUptodate(page))
+		return;
+
+	BUG_ON(page->index);
+	BUG_ON(size > PAGE_SIZE - offset_in_page(iomap->inline_data));
+
+	addr = kmap_atomic(page);
+	memcpy(addr, iomap->inline_data, size);
+	memset(addr + size, 0, PAGE_SIZE - size);
+	kunmap_atomic(addr);
+	SetPageUptodate(page);
+}
+
 static void
 iomap_write_failed(struct inode *inode, loff_t pos, unsigned len)
 {
@@ -133,7 +153,11 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
 	if (!page)
 		return -ENOMEM;
 
-	status = __block_write_begin_int(page, pos, len, NULL, iomap);
+	if (iomap->type == IOMAP_INLINE)
+		iomap_read_inline_data(inode, page, iomap);
+	else
+		status = __block_write_begin_int(page, pos, len, NULL, iomap);
+
 	if (unlikely(status)) {
 		unlock_page(page);
 		put_page(page);
@@ -146,14 +170,37 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
 	return status;
 }
 
+static int
+iomap_write_end_inline(struct inode *inode, struct page *page,
+		struct iomap *iomap, loff_t pos, unsigned copied)
+{
+	void *addr;
+
+	WARN_ON_ONCE(!PageUptodate(page));
+	BUG_ON(pos + copied > PAGE_SIZE - offset_in_page(iomap->inline_data));
+
+	addr = kmap_atomic(page);
+	memcpy(iomap->inline_data + pos, addr + pos, copied);
+	kunmap_atomic(addr);
+
+	mark_inode_dirty(inode);
+	__generic_write_end(inode, pos, copied, page);
+	return copied;
+}
+
 static int
 iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
-		unsigned copied, struct page *page)
+		unsigned copied, struct page *page, struct iomap *iomap)
 {
 	int ret;
 
-	ret = generic_write_end(NULL, inode->i_mapping, pos, len,
-			copied, page, NULL);
+	if (iomap->type == IOMAP_INLINE) {
+		ret = iomap_write_end_inline(inode, page, iomap, pos, copied);
+	} else {
+		ret = generic_write_end(NULL, inode->i_mapping, pos, len,
+				copied, page, NULL);
+	}
+
 	if (ret < len)
 		iomap_write_failed(inode, pos, len);
 	return ret;
@@ -208,7 +255,8 @@ iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
 
 		flush_dcache_page(page);
 
-		status = iomap_write_end(inode, pos, bytes, copied, page);
+		status = iomap_write_end(inode, pos, bytes, copied, page,
+				iomap);
 		if (unlikely(status < 0))
 			break;
 		copied = status;
@@ -302,7 +350,7 @@ iomap_dirty_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
 
 		WARN_ON_ONCE(!PageUptodate(page));
 
-		status = iomap_write_end(inode, pos, bytes, bytes, page);
+		status = iomap_write_end(inode, pos, bytes, bytes, page, iomap);
 		if (unlikely(status <= 0)) {
 			if (WARN_ON_ONCE(status == 0))
 				return -EIO;
@@ -354,7 +402,7 @@ static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset,
 	zero_user(page, offset, bytes);
 	mark_page_accessed(page);
 
-	return iomap_write_end(inode, pos, bytes, bytes, page);
+	return iomap_write_end(inode, pos, bytes, bytes, page, iomap);
 }
 
 static int iomap_dax_zero(loff_t pos, unsigned offset, unsigned bytes,
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 212f4d59bcbf..69ef622f650e 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -56,6 +56,7 @@ struct iomap {
 	union {
 		struct block_device *bdev;
 		struct dax_device   *dax_dev;
+		void		    *inline_data;
 	};
 };
 
-- 
2.17.1

[PATCH 6/6] iomap: add a page_done callback

[Christoph Hellwig]

This will be used by gfs2 to attach data to transactions for the journaled
data mode.  But the concept is generic enough that we might be able to
use it for other purposes like encryption/integrity post-processing in the
future.

Based on a patch from Andreas Gruenbacher.

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

diff --git a/fs/iomap.c b/fs/iomap.c
index 6d5a2ebe5dd8..9e265118e8b0 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -201,6 +201,9 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
 				copied, page, NULL);
 	}
 
+	if (iomap->page_done)
+		iomap->page_done(inode, pos, copied, page, iomap);
+
 	if (ret < len)
 		iomap_write_failed(inode, pos, len);
 	return ret;
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 69ef622f650e..43cc9208599f 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -9,6 +9,7 @@ struct fiemap_extent_info;
 struct inode;
 struct iov_iter;
 struct kiocb;
+struct page;
 struct vm_area_struct;
 struct vm_fault;
 
@@ -58,6 +59,14 @@ struct iomap {
 		struct dax_device   *dax_dev;
 		void		    *inline_data;
 	};
+
+	/*
+	 * Called when finished processing a page in the mapping returned in
+	 * this iomap.  At least for now this is only supported in the buffered
+	 * write path.
+	 */
+	void (*page_done)(struct inode *inode, loff_t pos, unsigned copied,
+			struct page *page, struct iomap *iomap);
 };
 
 /*
-- 
2.17.1

Re: iomap preparations for GFS2 v2

[Andreas Gruenbacher]

On 14 June 2018 at 14:04, Christoph Hellwig <hch@lst.de> wrote:
> Hi all,
>
> this is a slight rework of the patches from Andreas to prepare for gfs2
> using the iomap code.
>
> Note: I'd like to start with an immutable branch for iomap patches in either
> the XFS tree or a tree of mine at the beginning of the merge window so that
> we have a common base for the GFS2 and XFS iomap work.
>
> Changes since v1:
>  - move code to a different spot in iomap.c to prefer for readpages
>    inline data support
>  - add a forward declaration for struct page to iomap.h
>  - fix a typo in the dax_dev/bdev unioning
>  - fix a comment typo

I saw that you've pushed this onto the gfs2-iomap branch in your xfs
repository. I've rebased the gfs2 iomap-write branch onto that;
there's a trivial patch for adding a private pointer to struct iomap
at the head of that branch that would sense to move to the shared
branch as well now.

The next step would probably be to start using iomap_readpage /
iomap_readpages in gfs2 for block size == page size. This requires
adding inline data support to iomap_readpage which is trivial, but
because of gfs2's reliance on buffer heads, that alone isn't enough.

Thanks,
Andreas

Re: iomap preparations for GFS2 v2

[Christoph Hellwig]

On Thu, Jun 14, 2018 at 03:04:38PM +0200, Andreas Gruenbacher wrote:
> I saw that you've pushed this onto the gfs2-iomap branch in your xfs
> repository. I've rebased the gfs2 iomap-write branch onto that;
> there's a trivial patch for adding a private pointer to struct iomap
> at the head of that branch that would sense to move to the shared
> branch as well now.

Please send that patch out ASAP.

> The next step would probably be to start using iomap_readpage /
> iomap_readpages in gfs2 for block size == page size. This requires
> adding inline data support to iomap_readpage which is trivial, but
> because of gfs2's reliance on buffer heads, that alone isn't enough.

Is it?  At least for block size == page size we will only call
readpage on a pristine, newly allocated page.  So buffer heads won't
be in the game at that point, and the iomap buffered write code will
just allocate them for you once we start a write operation, or take
a page fault that makes the page writable.

Re: [Cluster-devel] iomap preparations for GFS2 v2

[Steven Whitehouse]

Hi,


On 15/06/18 09:03, Christoph Hellwig wrote:
> On Thu, Jun 14, 2018 at 03:04:38PM +0200, Andreas Gruenbacher wrote:
>> I saw that you've pushed this onto the gfs2-iomap branch in your xfs
>> repository. I've rebased the gfs2 iomap-write branch onto that;
>> there's a trivial patch for adding a private pointer to struct iomap
>> at the head of that branch that would sense to move to the shared
>> branch as well now.
> Please send that patch out ASAP.
>
>> The next step would probably be to start using iomap_readpage /
>> iomap_readpages in gfs2 for block size == page size. This requires
>> adding inline data support to iomap_readpage which is trivial, but
>> because of gfs2's reliance on buffer heads, that alone isn't enough.
> Is it?  At least for block size == page size we will only call
> readpage on a pristine, newly allocated page.  So buffer heads won't
> be in the game at that point, and the iomap buffered write code will
> just allocate them for you once we start a write operation, or take
> a page fault that makes the page writable.
>

Yes, for block size == page size, it should not be an issue to drop the 
use of buffer heads on reads in GFS2. I was fairly sure that we already 
did that in ->readpages() anyway, but it is a while since I looked at 
the code and my memory may be playing tricks on me,

Steve.

Re: [PATCH 2/6] iomap: move bdev and dax_dev in a union

[Darrick J. Wong]

On Thu, Jun 14, 2018 at 02:04:53PM +0200, Christoph Hellwig wrote:
> We always either ask for a block device or DAX device mapping, so we can
> use the same space for both.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/ext2/inode.c       | 6 ++++--
>  fs/ext4/inode.c       | 6 ++++--
>  fs/xfs/xfs_iomap.c    | 6 ++++--
>  include/linux/iomap.h | 6 ++++--
>  4 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 71635909df3b..8aead4e9dbc1 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -815,9 +815,11 @@ static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>  		return ret;
>  
>  	iomap->flags = 0;
> -	iomap->bdev = inode->i_sb->s_bdev;
>  	iomap->offset = (u64)first_block << blkbits;
> -	iomap->dax_dev = sbi->s_daxdev;
> +	if (IS_DAX(inode))
> +		iomap->dax_dev = sbi->s_daxdev;
> +	else
> +		iomap->bdev = inode->i_sb->s_bdev;
>  
>  	if (ret == 0) {
>  		iomap->type = IOMAP_HOLE;
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 2ea07efbe016..79027e99118f 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3524,8 +3524,10 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>  	iomap->flags = 0;
>  	if (ext4_inode_datasync_dirty(inode))
>  		iomap->flags |= IOMAP_F_DIRTY;
> -	iomap->bdev = inode->i_sb->s_bdev;
> -	iomap->dax_dev = sbi->s_daxdev;
> +	if (IS_DAX(inode))
> +		iomap->dax_dev = sbi->s_daxdev;
> +	else
> +		iomap->bdev = inode->i_sb->s_bdev;
>  	iomap->offset = (u64)first_block << blkbits;
>  	iomap->length = (u64)map.m_len << blkbits;
>  
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index c6ce6f9335b6..c9c3d0df5e4c 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -70,8 +70,10 @@ xfs_bmbt_to_iomap(
>  	}
>  	iomap->offset = XFS_FSB_TO_B(mp, imap->br_startoff);
>  	iomap->length = XFS_FSB_TO_B(mp, imap->br_blockcount);
> -	iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip));
> -	iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
> +	if (IS_DAX(VFS_I(ip)))
> +		iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
> +	else
> +		iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip));
>  }
>  
>  xfs_extlen_t
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index a044a824da85..212f4d59bcbf 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -53,8 +53,10 @@ struct iomap {
>  	u64			length;	/* length of mapping, bytes */
>  	u16			type;	/* type of mapping */
>  	u16			flags;	/* flags for mapping */
> -	struct block_device	*bdev;	/* block device for I/O */
> -	struct dax_device	*dax_dev; /* dax_dev for dax operations */
> +	union {
> +		struct block_device *bdev;
> +		struct dax_device   *dax_dev;

Is this going to blow up iomap_dax_zero?  It seems to use both bdev and
dax_dev on __dax_zero_page_range, which definitely uses both.

(Or did all that get rearranged when I wasn't looking?)

Also, I guess this will break iomap swapfiles since it checks
iomap->bdev which we stop supplying with this patch...
though I have no idea if DAX swapfiles are even supported.

What's the harm in supplying both pointers?

--D

> +	};
>  };
>  
>  /*
> -- 
> 2.17.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] iomap: move bdev and dax_dev in a union

[Christoph Hellwig]

On Mon, Jun 18, 2018 at 11:25:57PM -0700, Darrick J. Wong wrote:
> Is this going to blow up iomap_dax_zero?  It seems to use both bdev and
> dax_dev on __dax_zero_page_range, which definitely uses both.
> 
> (Or did all that get rearranged when I wasn't looking?)

Ouch, it does.  And that looks pretty broken.

> Also, I guess this will break iomap swapfiles since it checks
> iomap->bdev which we stop supplying with this patch...
> though I have no idea if DAX swapfiles are even supported.

Not sure if we support it.  We didn't use to support it when
swap used ->bmap, so until someone volunteers to test it we
should disable it with the iomap swapfile code as well.  But
even then doing a detour through the block layer and thus
the bdev makes very little sense.

> 
> What's the harm in supplying both pointers?

Just blowing up the size of the iomap.  Especially once we add
the inline data as the third option.

Re: [PATCH 2/6] iomap: move bdev and dax_dev in a union

[Darrick J. Wong]

On Tue, Jun 19, 2018 at 08:44:51AM +0200, Christoph Hellwig wrote:
> On Mon, Jun 18, 2018 at 11:25:57PM -0700, Darrick J. Wong wrote:
> > Is this going to blow up iomap_dax_zero?  It seems to use both bdev and
> > dax_dev on __dax_zero_page_range, which definitely uses both.
> > 
> > (Or did all that get rearranged when I wasn't looking?)
> 
> Ouch, it does.  And that looks pretty broken.

Indeed.

> > Also, I guess this will break iomap swapfiles since it checks
> > iomap->bdev which we stop supplying with this patch...
> > though I have no idea if DAX swapfiles are even supported.
> 
> Not sure if we support it.  We didn't use to support it when
> swap used ->bmap, so until someone volunteers to test it we
> should disable it with the iomap swapfile code as well.  But
> even then doing a detour through the block layer and thus
> the bdev makes very little sense.

For now all we do with it is compare iomap->bdev to the swapfile bdev so
it'll effectively disable DAX swapfiles in a weird and scary way...

...but maybe we should ask around if anyone cares about DAX swapfiles.
Everyone may just run away, but at least we will have tried. :)

> > 
> > What's the harm in supplying both pointers?
> 
> Just blowing up the size of the iomap.  Especially once we add
> the inline data as the third option.

<nod>

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Cluster-devel] iomap preparations for GFS2 v2

[Andreas Gruenbacher]

On 15 June 2018 at 10:31, Steven Whitehouse <swhiteho@redhat.com> wrote:
> Hi,
>
> On 15/06/18 09:03, Christoph Hellwig wrote:
>>
>> On Thu, Jun 14, 2018 at 03:04:38PM +0200, Andreas Gruenbacher wrote:
>>>
>>> I saw that you've pushed this onto the gfs2-iomap branch in your xfs
>>> repository. I've rebased the gfs2 iomap-write branch onto that;
>>> there's a trivial patch for adding a private pointer to struct iomap
>>> at the head of that branch that would sense to move to the shared
>>> branch as well now.
>>
>> Please send that patch out ASAP.
>>
>>> The next step would probably be to start using iomap_readpage /
>>> iomap_readpages in gfs2 for block size == page size. This requires
>>> adding inline data support to iomap_readpage which is trivial, but
>>> because of gfs2's reliance on buffer heads, that alone isn't enough.
>>
>> Is it?  At least for block size == page size we will only call
>> readpage on a pristine, newly allocated page.  So buffer heads won't
>> be in the game at that point, and the iomap buffered write code will
>> just allocate them for you once we start a write operation, or take
>> a page fault that makes the page writable.

What I'm seeing in the readpage address space operation is pages which
are not PageUptodate(), with a page-size buffer head that is
buffer_uptodate(). The filesystem doesn't bother keeping the page
flags in sync with the buffer head flags, nothing unusual. When
iomap_readpage is called on such a page, it will replace the current
contents with what's on disk, losing the changes in memory. So we
cannot just call iomap_readpages, we need to check the buffer head
flags as well. Or, since the old code is still needed for page size !=
block size anyway, we can fall back to that for pages that have
buffers for now.

> Yes, for block size == page size, it should not be an issue to drop the use
> of buffer heads on reads in GFS2. I was fairly sure that we already did that
> in ->readpages() anyway, but it is a while since I looked at the code and my
> memory may be playing tricks on me,

I've pushed what I have so far here in case you want to have a look:

  https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=iomap-readpage

There's one remaining failure in xfstest generic/299 that I'm still
looking into.

Thanks,
Andreas

Re: [Cluster-devel] iomap preparations for GFS2 v2

[Christoph Hellwig]

On Tue, Jun 19, 2018 at 01:08:12PM +0200, Andreas Gruenbacher wrote:
> What I'm seeing in the readpage address space operation is pages which
> are not PageUptodate(), with a page-size buffer head that is
> buffer_uptodate(). The filesystem doesn't bother keeping the page
> flags in sync with the buffer head flags, nothing unusual.

It is in fact highly unusual, as all the generic routines do set
the page uptodate once all buffers are uptodate.

> When
> iomap_readpage is called on such a page, it will replace the current
> contents with what's on disk, losing the changes in memory. So we
> cannot just call iomap_readpages, we need to check the buffer head
> flags as well. Or, since the old code is still needed for page size !=
> block size anyway, we can fall back to that for pages that have
> buffers for now.

I'd like to understand where that buffer_head comes from, something
seems fishy here.

Re: [Cluster-devel] iomap preparations for GFS2 v2

[Andreas Gruenbacher]

On 19 June 2018 at 16:35, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, Jun 19, 2018 at 01:08:12PM +0200, Andreas Gruenbacher wrote:
>> What I'm seeing in the readpage address space operation is pages which
>> are not PageUptodate(), with a page-size buffer head that is
>> buffer_uptodate(). The filesystem doesn't bother keeping the page
>> flags in sync with the buffer head flags, nothing unusual.
>
> It is in fact highly unusual, as all the generic routines do set
> the page uptodate once all buffers are uptodate.
>
>> When
>> iomap_readpage is called on such a page, it will replace the current
>> contents with what's on disk, losing the changes in memory. So we
>> cannot just call iomap_readpages, we need to check the buffer head
>> flags as well. Or, since the old code is still needed for page size !=
>> block size anyway, we can fall back to that for pages that have
>> buffers for now.
>
> I'd like to understand where that buffer_head comes from, something
> seems fishy here.

Ok, here is one test case that triggered the problem for me.

Starting from commit bd926eb58b13 on the iomap-readpage branch,

  https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=iomap-readpage

with this patch on top which causes iomap_readpage to be called even
for pages with buffers:

--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -511,8 +511,7 @@ static int __gfs2_readpage(void *file, struct page *page)

        int error;

-       if (i_blocksize(page->mapping->host) == PAGE_SIZE &&
-           !page_has_buffers(page)) {
+       if (i_blocksize(page->mapping->host) == PAGE_SIZE) {
                error = iomap_readpage(page, &gfs2_iomap_ops);
        } else if (gfs2_is_stuffed(ip)) {
                error = stuffed_readpage(ip, page);

The following fsx operations, stored as junk.fsxops:

  write 0x11400 0x1800 0x6e6d4 *
  punch_hole 0xfa7a 0x2410 0x0 *
  mapread 0xd000 0x78ea 0x34200 *

Can be replayed as:

  # mkfs.gfs2 -O -b 4096 -p lock_nolock $DEV
  # mount $DEV $MNT
  # ltp/fsx -N 10000 -o 32768 -l 500000 -r 4096 -t 512 -w 512 -Z -W
--replay-ops junk.fsxops $MNT/junk

(Most of the above fsx options could probably be removed ...)

The hole in this example is unaligned, so punch_hole will zero the end
of the first as well as the beginning of the last page of the hole.
This will leave at least the last page of the hole as not
PageUptodate(), with a buffer_uptodate() buffer head. The mapread will
call into iomap_readpage. Because the page has buffers, the
WARN_ON_ONCE(page_has_buffers(page)) in iomap_readpage will trigger.
And iomap_readpage will reread the page from disk, overwriting the
zeroes written by punch_hole. This will cause fsx to complain because
it doesn't see the zeroes it expects.

This could be a bug in __gfs2_punch_hole => gfs2_block_zero_range as
well, but it's not clear to me how.

Andreas