[PATCH v8 0/6] vfs: help support reflink for XFS

Hi all,

This is the eighth revision of a patchset that adds to XFS kernel
support for mapping multiple file logical blocks to the same physical
block (reflink/deduplication), implements the beginnings of online
metadata scrubbing and preening, and implements reverse mapping for
the realtime device.  There shouldn't be any incompatible on-disk
format changes, pending a thorough review of the patches within.

The patches in this series implement generic changes that will be
necessary for XFS' implementation of reflink and dedupe.  There are
some small fixes for the new VFS hoists of the dedupe ioctl, as well
as a patch adding the reflink and CoW extent size hints to the file
attr ioctl that ext4 and XFS now share.

The second half of the series are amendments to the iomap
infrastructure that Christoph Hellwig refactored in 4.8.  These
changes make it easier to implement CoW and to pass along various
flags through struct iomap.

If you're going to start using this mess, you probably ought to just
pull from my github trees for kernel[1], xfsprogs[2], xfstests[3],
xfs-docs[4], and man-pages[5].  The kernel patches in the git trees
should apply to 4.8-rc3; xfsprogs patches to for-next; and xfstest to
master.

The patches have been xfstested with x64, ppc64, and armhf; all tests
in the clone and rmap groups pass.  AFAICT they don't cause any new
failures for the 'auto' group.

This is an extraordinary way to eat your data.  Enjoy! 
Comments and questions are, as always, welcome.

--D

[1] https://github.com/djwong/linux/tree/djwong-devel
[2] https://github.com/djwong/xfsprogs/tree/djwong-devel
[3] https://github.com/djwong/xfstests/tree/djwong-devel
[4] https://github.com/djwong/xfs-documentation/tree/djwong-devel
[5] https://github.com/djwong/man-pages/tree/djwong-devel

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

All the VFS functions in the dedupe ioctl path return int status, so
the ioctl handler ought to as well.

Found by Coverity, CID 1350952.

Signed-off-by: Darrick J. Wong <redacted>
---
 fs/ioctl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 0f56deb..26aba09 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c

@@ -568,7 +568,7 @@ static int ioctl_fsthaw(struct file *filp)
 	return thaw_super(sb);
 }
 
-static long ioctl_file_dedupe_range(struct file *file, void __user *arg)
+static int ioctl_file_dedupe_range(struct file *file, void __user *arg)
 {
 	struct file_dedupe_range __user *argp = arg;
 	struct file_dedupe_range *same = NULL;

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Kirill A. Shutemov reports that the kernel doesn't try to cap dest_count
in any way, and uses the number to allocate kernel memory.  This causes
high order allocation warnings in the kernel log if someone passes in a
big enough value.  We should clamp the allocation at PAGE_SIZE to avoid
stressing the VM.

The two existing users of the dedupe ioctl never send more than 120
requests, so we can safely clamp dest_range at PAGE_SIZE, because with
4k pages we can handle up to 127 dedupe candidates.  Given the max
extent length of 16MB, we can end up doing 2GB of IO which is plenty.

Reported-by: "Kirill A. Shutemov" <redacted>
Signed-off-by: Darrick J. Wong <redacted>
---
 fs/ioctl.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 26aba09..c415668 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c

@@ -582,6 +582,10 @@ static int ioctl_file_dedupe_range(struct file *file, void __user *arg)
 	}
 
 	size = offsetof(struct file_dedupe_range __user, info[count]);
+	if (size > PAGE_SIZE) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	same = memdup_user(argp, size);
 	if (IS_ERR(same)) {

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Introduce XFLAGs for the new XFS reflink inode flag and the CoW extent
size hint, and actually plumb the CoW extent size hint into the fsxattr
structure.

Signed-off-by: Darrick J. Wong <redacted>
---
 include/uapi/linux/fs.h |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 3b00f7c..fb371a5 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h

@@ -157,7 +157,8 @@ struct fsxattr {
 	__u32		fsx_extsize;	/* extsize field value (get/set)*/
 	__u32		fsx_nextents;	/* nextents field value (get)	*/
 	__u32		fsx_projid;	/* project identifier (get/set) */
-	unsigned char	fsx_pad[12];
+	__u32		fsx_cowextsize;	/* CoW extsize field value (get/set)*/
+	unsigned char	fsx_pad[8];
 };
 
 /*

@@ -178,6 +179,8 @@ struct fsxattr {
 #define FS_XFLAG_NODEFRAG	0x00002000	/* do not defragment */
 #define FS_XFLAG_FILESTREAM	0x00004000	/* use filestream allocator */
 #define FS_XFLAG_DAX		0x00008000	/* use DAX for IO */
+#define FS_XFLAG_REFLINK	0x00010000	/* file is reflinked */
+#define FS_XFLAG_COWEXTSIZE	0x00020000	/* CoW extent size allocator hint */
 #define FS_XFLAG_HASATTR	0x80000000	/* no DIFLAG for this	*/
 
 /* the read-only stuff doesn't really belong here, but any other place is

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Originally-from: Christoph Hellwig [off-list ref]

This function uses the iomap infrastructure to re-write all pages
in a given range.  This is useful for doing a copy-up of COW ranges,
and might be useful for scrubbing in the future.

XXX: might want a bigger name, and possible a better implementation
that doesn't require two lookups in the radix tree.

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

diff --git a/fs/iomap.c b/fs/iomap.c
index 0342254..7b295d5 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c

@@ -252,6 +252,88 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *iter,
 }
 EXPORT_SYMBOL_GPL(iomap_file_buffered_write);
 
+static struct page *
+__iomap_read_page(struct inode *inode, loff_t offset)
+{
+	struct address_space *mapping = inode->i_mapping;
+	struct page *page;
+
+	page = read_mapping_page(mapping, offset >> PAGE_SHIFT, NULL);
+	if (IS_ERR(page))
+		return page;
+	if (!PageUptodate(page)) {
+		put_page(page);
+		return ERR_PTR(-EIO);
+	}
+	return page;
+}
+
+static loff_t
+iomap_dirty_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
+		struct iomap *iomap)
+{
+	long status = 0;
+	ssize_t written = 0;
+
+	do {
+		struct page *page, *rpage;
+		unsigned long offset;	/* Offset into pagecache page */
+		unsigned long bytes;	/* Bytes to write to page */
+
+		offset = (pos & (PAGE_SIZE - 1));
+		bytes = min_t(unsigned long, PAGE_SIZE - offset, length);
+
+		rpage = __iomap_read_page(inode, pos);
+		if (IS_ERR(rpage))
+			return PTR_ERR(rpage);
+
+		status = iomap_write_begin(inode, pos, bytes,
+				AOP_FLAG_NOFS | AOP_FLAG_UNINTERRUPTIBLE,
+				&page, iomap);
+		put_page(rpage);
+		if (unlikely(status))
+			return status;
+
+		WARN_ON_ONCE(!PageUptodate(page));
+
+		status = iomap_write_end(inode, pos, bytes, bytes, page);
+		if (unlikely(status <= 0)) {
+			if (WARN_ON_ONCE(status == 0))
+				return -EIO;
+			return status;
+		}
+
+		cond_resched();
+
+		pos += status;
+		written += status;
+		length -= status;
+
+		balance_dirty_pages_ratelimited(inode->i_mapping);
+	} while (length);
+
+	return written;
+}
+
+int
+iomap_file_dirty(struct inode *inode, loff_t pos, loff_t len,
+		struct iomap_ops *ops)
+{
+	loff_t ret;
+
+	while (len) {
+		ret = iomap_apply(inode, pos, len, IOMAP_WRITE, ops, NULL,
+				iomap_dirty_actor);
+		if (ret <= 0)
+			return ret;
+		pos += ret;
+		len -= ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iomap_file_dirty);
+
 static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset,
 		unsigned bytes, struct iomap *iomap)
 {

diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 3267df4..b2e30e5 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h

@@ -58,6 +58,8 @@ struct iomap_ops {
 
 ssize_t iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *from,
 		struct iomap_ops *ops);
+int iomap_file_dirty(struct inode *inode, loff_t pos, loff_t len,
+		struct iomap_ops *ops);
 int iomap_zero_range(struct inode *inode, loff_t pos, loff_t len,
 		bool *did_zero, struct iomap_ops *ops);
 int iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Signed-off-by: Darrick J. Wong <redacted>
---
 fs/iomap.c            |    2 ++
 include/linux/iomap.h |    1 +
 2 files changed, 3 insertions(+)

diff --git a/fs/iomap.c b/fs/iomap.c
index e9b3f99..ec411a6 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c

@@ -512,6 +512,8 @@ static int iomap_to_fiemap(struct fiemap_extent_info *fi,
 
 	if (iomap->flags & IOMAP_F_MERGED)
 		flags |= FIEMAP_EXTENT_MERGED;
+	if (iomap->flags & IOMAP_F_SHARED)
+		flags |= FIEMAP_EXTENT_SHARED;
 
 	return fiemap_fill_next_extent(fi, iomap->offset,
 			iomap->blkno != IOMAP_NULL_BLOCK ? iomap->blkno << 9: 0,

diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 3a56212..c74226a 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h

@@ -22,6 +22,7 @@ struct vm_fault;
  * Flags for iomap mappings:
  */
 #define IOMAP_F_MERGED	0x01	/* contains multiple blocks/extents */
+#define IOMAP_F_SHARED	0x02	/* block shared with another file */
 
 /*
  * Magic value for blkno:

From: Christoph Hellwig <hch@lst.de>

Filesystems like XFS that use extents should not set the
FIEMAP_EXTENT_MERGED flag in the fiemap extent structures.  To allow
for both behaviors for the upcoming gfs2 usage split the iomap
type field into type and flags, and only set FIEMAP_EXTENT_MERGED if
the IOMAP_F_MERGED flag is set.  The flags field will also come in
handy for future features such as shared extents on reflink-enabled
file systems.

Reported-by: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c            |    5 ++++-
 include/linux/iomap.h |    8 +++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 7b295d5..e9b3f99 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c

@@ -510,9 +510,12 @@ static int iomap_to_fiemap(struct fiemap_extent_info *fi,
 		break;
 	}
 
+	if (iomap->flags & IOMAP_F_MERGED)
+		flags |= FIEMAP_EXTENT_MERGED;
+
 	return fiemap_fill_next_extent(fi, iomap->offset,
 			iomap->blkno != IOMAP_NULL_BLOCK ? iomap->blkno << 9: 0,
-			iomap->length, flags | FIEMAP_EXTENT_MERGED);
+			iomap->length, flags);
 
 }
 

diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index b2e30e5..3a56212 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h

@@ -19,6 +19,11 @@ struct vm_fault;
 #define IOMAP_UNWRITTEN	0x04	/* blocks allocated @blkno in unwritten state */
 
 /*
+ * Flags for iomap mappings:
+ */
+#define IOMAP_F_MERGED	0x01	/* contains multiple blocks/extents */
+
+/*
  * Magic value for blkno:
  */
 #define IOMAP_NULL_BLOCK -1LL	/* blkno is not valid */

@@ -27,7 +32,8 @@ struct iomap {
 	sector_t		blkno;	/* 1st sector of mapping, 512b units */
 	loff_t			offset;	/* file offset of mapping, bytes */
 	u64			length;	/* length of mapping, bytes */
-	int			type;	/* type of mapping */
+	u16			type;	/* type of mapping */
+	u16			flags;	/* flags for mapping */
 	struct block_device	*bdev;	/* block device for I/O */
 };
 

This really should go into 4.8 (and the previous patch probably as
well), and I've said that a couple of times, and you tried a few times
to send it to Al at least.

Al, do you want to pick it up or should Darrick send it straight to
Linus?

On Thu, Aug 25, 2016 at 04:30:54PM -0700, Darrick J. Wong wrote:

---end quoted text---

On Thu, Aug 25, 2016 at 04:31:00PM -0700, Darrick J. Wong wrote:

Introduce XFLAGs for the new XFS reflink inode flag and the CoW extent
size hint, and actually plumb the CoW extent size hint into the fsxattr
structure.

Just curious, but why would we even bother to expose the reflink flag
to userspace?

On Thu, Aug 25, 2016 at 04:31:07PM -0700, Christoph Hellwig wrote:

Originally-from: Christoph Hellwig [off-list ref]

This should be a

From: Christoph Hellwig <hch@lst.de>

so that git picks up authorship information correctly.

XXX: might want a bigger name, and possible a better implementation
that doesn't require two lookups in the radix tree.

And these need to be looked into.  I can take a stab at it, but I need
to get a few other things off my plate first.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

On Mon, Sep 05, 2016 at 07:57:51AM -0700, Christoph Hellwig wrote:

On Thu, Aug 25, 2016 at 04:31:07PM -0700, Christoph Hellwig wrote:

quoted

Originally-from: Christoph Hellwig [off-list ref]

This should be a

From: Christoph Hellwig <hch@lst.de>

so that git picks up authorship information correctly.

quoted

XXX: might want a bigger name, and possible a better implementation
that doesn't require two lookups in the radix tree.

And these need to be looked into.  I can take a stab at it, but I need
to get a few other things off my plate first.

Yeah.  It works well enough for unsharing blocks, if inefficiently.

Not sure what "a bigger name" means, though.  I tried feeding the
function prototype through figlet but gcc didn't like that. ;)

--D

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

On Mon, Sep 05, 2016 at 07:56:22AM -0700, Christoph Hellwig wrote:

On Thu, Aug 25, 2016 at 04:31:00PM -0700, Darrick J. Wong wrote:

quoted

Introduce XFLAGs for the new XFS reflink inode flag and the CoW extent
size hint, and actually plumb the CoW extent size hint into the fsxattr
structure.

Just curious, but why would we even bother to expose the reflink flag
to userspace?

So far I've put the reflink flag to use in xfs_scrub to look for
obvious signs of brokenness such as extents that overlap or have the
shared flag set but the inode flag is off; and to skip various kinds
of checks that don't have to happen when blocks don't overlap.

I doubt there's much of a use for the flag outside of the XFS utilities.
For a while I pondered only exposing the fsxattr flag if the caller had
CAP_SYS_ADMIN (the level of priviledge required to run scrub) but
decided that I wouldn't change the existing interface like that unless
I had a really good reason.

--D

On Tue, Sep 06, 2016 at 12:15:15PM -0700, Darrick J. Wong wrote:

So far I've put the reflink flag to use in xfs_scrub to look for
obvious signs of brokenness such as extents that overlap or have the
shared flag set but the inode flag is off; and to skip various kinds
of checks that don't have to happen when blocks don't overlap.

I doubt there's much of a use for the flag outside of the XFS utilities.
For a while I pondered only exposing the fsxattr flag if the caller had
CAP_SYS_ADMIN (the level of priviledge required to run scrub) but
decided that I wouldn't change the existing interface like that unless
I had a really good reason.

I don't think CAP_SYS_ADMIN is nessecarily the right thing, but it's
still an XFS implementation detail which I don't think we should
pollute a flags API for normal user space applications with.

On Tue, Sep 06, 2016 at 10:34:28AM -0700, Darrick J. Wong wrote:

quoted

quoted

XXX: might want a bigger name, and possible a better implementation
that doesn't require two lookups in the radix tree.

And these need to be looked into.  I can take a stab at it, but I need
to get a few other things off my plate first.

Yeah.  It works well enough for unsharing blocks, if inefficiently.

Not sure what "a bigger name" means, though.  I tried feeding the
function prototype through figlet but gcc didn't like that. ;)

That should have been "better", sorry.

On Sun, Sep 11, 2016 at 05:58:08AM -0700, Christoph Hellwig wrote:

On Tue, Sep 06, 2016 at 12:15:15PM -0700, Darrick J. Wong wrote:

quoted

So far I've put the reflink flag to use in xfs_scrub to look for
obvious signs of brokenness such as extents that overlap or have the
shared flag set but the inode flag is off; and to skip various kinds
of checks that don't have to happen when blocks don't overlap.

I doubt there's much of a use for the flag outside of the XFS utilities.
For a while I pondered only exposing the fsxattr flag if the caller had
CAP_SYS_ADMIN (the level of priviledge required to run scrub) but
decided that I wouldn't change the existing interface like that unless
I had a really good reason.

I don't think CAP_SYS_ADMIN is nessecarily the right thing, but it's
still an XFS implementation detail which I don't think we should
pollute a flags API for normal user space applications with.

I can work around it in xfs_scrub, so I'll give back the xflag bit for
reflink.

--D

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

On Mon, Sep 05, 2016 at 07:55:49AM -0700, Christoph Hellwig wrote:

This really should go into 4.8 (and the previous patch probably as
well), and I've said that a couple of times, and you tried a few times
to send it to Al at least.

Al, do you want to pick it up or should Darrick send it straight to
Linus?

Ping?  Anyone?

--D

On Thu, Aug 25, 2016 at 04:30:54PM -0700, Darrick J. Wong wrote:

quoted

Kirill A. Shutemov reports that the kernel doesn't try to cap dest_count
in any way, and uses the number to allocate kernel memory.  This causes
high order allocation warnings in the kernel log if someone passes in a
big enough value.  We should clamp the allocation at PAGE_SIZE to avoid
stressing the VM.

The two existing users of the dedupe ioctl never send more than 120
requests, so we can safely clamp dest_range at PAGE_SIZE, because with
4k pages we can handle up to 127 dedupe candidates.  Given the max
extent length of 16MB, we can end up doing 2GB of IO which is plenty.

Reported-by: "Kirill A. Shutemov" <redacted>
Signed-off-by: Darrick J. Wong <redacted>
---
 fs/ioctl.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 26aba09..c415668 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c

@@ -582,6 +582,10 @@ static int ioctl_file_dedupe_range(struct file *file, void __user *arg)
 	}
 
 	size = offsetof(struct file_dedupe_range __user, info[count]);
+	if (size > PAGE_SIZE) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	same = memdup_user(argp, size);
 	if (IS_ERR(same)) {

_______________________________________________
xfs mailing list
xfs-VZNHf3L845pBDgjK7y7TUQ@public.gmane.org
http://oss.sgi.com/mailman/listinfo/xfs

---end quoted text---

On Mon, Sep 12, 2016 at 06:16:20PM -0700, Darrick J. Wong wrote:

Ping?  Anyone?

Please resend the patch straight to Linux with a Cc to Al and me.

On Mon, Sep 05, 2016 at 07:57:51AM -0700, Christoph Hellwig wrote:

On Thu, Aug 25, 2016 at 04:31:07PM -0700, Christoph Hellwig wrote:

quoted

Originally-from: Christoph Hellwig [off-list ref]

This should be a

From: Christoph Hellwig <redacted>

so that git picks up authorship information correctly.

quoted

XXX: might want a bigger name, and possible a better implementation
that doesn't require two lookups in the radix tree.

And these need to be looked into.  I can take a stab at it, but I need
to get a few other things off my plate first.

Seeing as it works and isn't too ugly to live, I think I'm going to
merge this as is. When a better implementation comes along we
can replace it with that...

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org