On Mon, Aug 08, 2016 at 04:22:31PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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;
|