[PATCH 0/2] iomap: fixes for fiemap interface

[PATCH 0/2] iomap: fixes for fiemap interface

[Dave Chinner]

Hi folks,

A couple of fixes things that Darrick and I noticed when using the
iomap fiemap implementation. Darrick noticed that the attribute fork
couldn't be mapped when updating his XFS scrubbing utility, and I
noticed that the FIEMAP_FLAG_SYNC was being ignored by the code
while looking at the attribute mapping issue.  Both are regressions
agains the old XFS fiemap implementation.

The attribute mapping patch is the one that needs the most attention
as it adds an iomap control flag and a new iomap internal error. The
issue is that we don't know if there are attributes to map until
we call down into the filesystem specific code, and at that point we
need to tell iomap_apply() that there is nothing to map and so break
out. This is done by returning -ENOENT, and then capturing that
in iomap_fiemap() and treating it like we've reached the end of
file. This causes fiemap to report the attribute map as having no
extents.

We also need to protect against write mappings being made against
the attribute fork in XFS as it's not valid to write into the
attribute extent map via this method. Hence we return -EINVAL if we
detect an attempt to do so through the iomap control flags.

I'm not sure it's the best way to solve the problem, but it does
work. Better ideas are welcome...

-Dave.

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

[PATCH 1/2] iomap: fiemap should honor the FIEMAP_FLAG_SYNC flag

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The flag is checked as supported, but then we do an unconditional
sync of the file, regardless of whether the flag is set or not. Make
the sync conditional on having the FIEMAP_FLAG_SYNC flag set.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/iomap.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 48141b8..189742b 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -470,9 +470,11 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
 	if (ret)
 		return ret;
 
-	ret = filemap_write_and_wait(inode->i_mapping);
-	if (ret)
-		return ret;
+	if (fi->fi_flags & FIEMAP_FLAG_SYNC) {
+		ret = filemap_write_and_wait(inode->i_mapping);
+		if (ret)
+			return ret;
+	}
 
 	while (len > 0) {
 		ret = iomap_apply(inode, start, len, 0, ops, &ctx,
-- 
2.8.0.rc3

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

Re: [PATCH 1/2] iomap: fiemap should honor the FIEMAP_FLAG_SYNC flag

[Christoph Hellwig]

On Mon, Aug 08, 2016 at 04:22:30PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The flag is checked as supported, but then we do an unconditional
> sync of the file, regardless of whether the flag is set or not. Make
> the sync conditional on having the FIEMAP_FLAG_SYNC flag set.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks fine,

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

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

[PATCH 2/2] iomap: add fiemap support for attribute mappings

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Prior to the iomap conversion, XFS supported the FIEMAP_FLAG_XATTR
flag for mapping the attribute fork block map. This flag was not
added to the iomap fiemap support so we have regressed fiemap
functionality with this change.

Add an iomap control flag to indicate that we should be operating
on the attribute map rather than the file data map and pass it from
iomap_fiemap() as appropriate. Add the appropriate flags to the XFS
code to switch to the attribute fork lookup, and ensure we return
EINVAL if anyone attempts a write mapping of the attribute fork.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/iomap.c            | 11 +++++++++--
 fs/xfs/xfs_iomap.c    | 14 +++++++++++++-
 include/linux/iomap.h |  1 +
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 189742b..2a04e5e 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -461,12 +461,13 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
 {
 	struct fiemap_ctx ctx;
 	loff_t ret;
+	int flags = 0;
 
 	memset(&ctx, 0, sizeof(ctx));
 	ctx.fi = fi;
 	ctx.prev.type = IOMAP_HOLE;
 
-	ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC);
+	ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);
 	if (ret)
 		return ret;
 
@@ -476,9 +477,15 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
 			return ret;
 	}
 
+	if (fi->fi_flags & FIEMAP_FLAG_XATTR)
+		flags |= IOMAP_ATTR;
+
 	while (len > 0) {
-		ret = iomap_apply(inode, start, len, 0, ops, &ctx,
+		ret = iomap_apply(inode, start, len, flags, ops, &ctx,
 				iomap_fiemap_actor);
+		/* inode with no attribute mapping will give ENOENT */
+		if (ret == -ENOENT)
+			break;
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 4398932..17b5b82 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -993,10 +993,22 @@ xfs_file_iomap_begin(
 	struct xfs_bmbt_irec	imap;
 	xfs_fileoff_t		offset_fsb, end_fsb;
 	int			nimaps = 1, error = 0;
+	int			bmflags = XFS_BMAPI_ENTIRE;
 
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
+	/* Attribute fork can only be read via this interface */
+	if (flags & IOMAP_ATTR) {
+		if (flags & ~IOMAP_ATTR)
+			return -EINVAL;
+		/* if there are no attribute fork or extents, return ENOENT */
+		if (!XFS_IFORK_Q(ip) || !ip->i_d.di_anextents)
+			return -ENOENT;
+		ASSERT(ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL);
+		bmflags |= XFS_BMAPI_ATTRFORK;
+	}
+
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 
 	ASSERT(offset <= mp->m_super->s_maxbytes);
@@ -1006,7 +1018,7 @@ xfs_file_iomap_begin(
 	end_fsb = XFS_B_TO_FSB(mp, offset + length);
 
 	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
-			       &nimaps, XFS_BMAPI_ENTIRE);
+			       &nimaps, bmflags);
 	if (error) {
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		return error;
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 3267df4..00a5477 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -36,6 +36,7 @@ struct iomap {
  */
 #define IOMAP_WRITE		(1 << 0)
 #define IOMAP_ZERO		(1 << 1)
+#define IOMAP_ATTR		(1 << 2)	/* operate on attributes */
 
 struct iomap_ops {
 	/*
-- 
2.8.0.rc3

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

Re: [PATCH 2/2] iomap: add fiemap support for attribute mappings

[Christoph Hellwig]

On Mon, Aug 08, 2016 at 04:22:31PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Prior to the iomap conversion, XFS supported the FIEMAP_FLAG_XATTR
> flag for mapping the attribute fork block map. This flag was not
> added to the iomap fiemap support so we have regressed fiemap
> functionality with this change.
> 
> Add an iomap control flag to indicate that we should be operating
> on the attribute map rather than the file data map and pass it from
> iomap_fiemap() as appropriate. Add the appropriate flags to the XFS
> code to switch to the attribute fork lookup, and ensure we return
> EINVAL if anyone attempts a write mapping of the attribute fork.

I'm a little worried about this for a few reasons:

> -	ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC);
> +	ret = fiemap_check_flags(fi, FIEMAP_FLAG_SYNC | FIEMAP_FLAG_XATTR);

FIEMAP_FLAG_XATTR is a magic thing only supported by ext4 and
historically XFS.  By claiming general support here we make iomap_fiemap
unsuitable for general use.  At least we should pass in a flag if it's
supported or not into the generic helper.  Or see below for another
idea.

> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 4398932..17b5b82 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -993,10 +993,22 @@ xfs_file_iomap_begin(
>  	struct xfs_bmbt_irec	imap;
>  	xfs_fileoff_t		offset_fsb, end_fsb;
>  	int			nimaps = 1, error = 0;
> +	int			bmflags = XFS_BMAPI_ENTIRE;
>  
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return -EIO;
>  
> +	/* Attribute fork can only be read via this interface */
> +	if (flags & IOMAP_ATTR) {
> +		if (flags & ~IOMAP_ATTR)
> +			return -EINVAL;
> +		/* if there are no attribute fork or extents, return ENOENT */
> +		if (!XFS_IFORK_Q(ip) || !ip->i_d.di_anextents)
> +			return -ENOENT;
> +		ASSERT(ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL);
> +		bmflags |= XFS_BMAPI_ATTRFORK;
> +	}

And this adds a special case for totally rare attr fork operation to
the fast path that we'll soon be using for all file I/O.

I'd suggest to just have a separate stub set of iomap_ops for the xattr
case.  xfs_vn_fiemap would then look something like:


	xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
	if (fi->fi_flags & FIEMAP_FLAG_XATTR) {
		fi->fi_flags &= ~FIEMAP_FLAG_XATTR;
		error = iomap_fiemap(inode, fieinfo, start, length,
				&xfs_attr_iomap_ops);
	} else {
		error = iomap_fiemap(inode, fieinfo, start, length,
				&xfs_iomap_ops);
	}
	xfs_iunlock(XFS_I(inode), XFS_IOLOCK_SHARED);

	return error;

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