lkml.org 
[lkml]   [2020]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V6 7/8] fs/xfs: Change xfs_ioctl_setattr_dax_invalidate() to xfs_ioctl_dax_check()
    On Wed, Apr 08, 2020 at 08:37:17AM -0700, Darrick J. Wong wrote:
    > On Tue, Apr 07, 2020 at 11:29:57AM -0700, ira.weiny@intel.com wrote:
    > > From: Ira Weiny <ira.weiny@intel.com>
    > >
    > > We only support changing FS_XFLAG_DAX on directories. Files get their
    > > flag from the parent directory on creation only. So no data
    > > invalidation needs to happen.
    > >
    > > Alter the xfs_ioctl_setattr_dax_invalidate() to be
    > > xfs_ioctl_dax_check().
    > >
    > > This also allows use to remove the join_flags logic.
    > >
    > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
    > >
    > > ---
    > > Changes from v5:
    > > New patch
    > > ---
    > > fs/xfs/xfs_ioctl.c | 91 +++++-----------------------------------------
    > > 1 file changed, 10 insertions(+), 81 deletions(-)
    > >
    > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
    > > index c6cd92ef4a05..5472faab7c4f 100644
    > > --- a/fs/xfs/xfs_ioctl.c
    > > +++ b/fs/xfs/xfs_ioctl.c
    > > @@ -1145,63 +1145,18 @@ xfs_ioctl_setattr_xflags(
    > > }
    > >
    > > /*
    > > - * If we are changing DAX flags, we have to ensure the file is clean and any
    > > - * cached objects in the address space are invalidated and removed. This
    > > - * requires us to lock out other IO and page faults similar to a truncate
    > > - * operation. The locks need to be held until the transaction has been committed
    > > - * so that the cache invalidation is atomic with respect to the DAX flag
    > > - * manipulation.
    > > + * Only directories are allowed to change dax flags
    > > */
    > > static int
    > > xfs_ioctl_setattr_dax_invalidate(
    > > - struct xfs_inode *ip,
    > > - struct fsxattr *fa,
    > > - int *join_flags)
    > > + struct xfs_inode *ip)
    > > {
    > > struct inode *inode = VFS_I(ip);
    > > - struct super_block *sb = inode->i_sb;
    > > - int error;
    > > -
    > > - *join_flags = 0;
    > > -
    > > - /*
    > > - * It is only valid to set the DAX flag on regular files and
    > > - * directories on filesystems where the block size is equal to the page
    > > - * size. On directories it serves as an inherited hint so we don't
    > > - * have to check the device for dax support or flush pagecache.
    > > - */
    > > - if (fa->fsx_xflags & FS_XFLAG_DAX) {
    > > - struct xfs_buftarg *target = xfs_inode_buftarg(ip);
    > > -
    > > - if (!bdev_dax_supported(target->bt_bdev, sb->s_blocksize))
    > > - return -EINVAL;
    > > - }
    > > -
    > > - /* If the DAX state is not changing, we have nothing to do here. */
    > > - if ((fa->fsx_xflags & FS_XFLAG_DAX) && IS_DAX(inode))
    > > - return 0;
    > > - if (!(fa->fsx_xflags & FS_XFLAG_DAX) && !IS_DAX(inode))
    > > - return 0;
    >
    > Does the !S_ISDIR check below apply unconditionally even if we weren't
    > trying to change the DAX flag?

    :-/

    shoot... I really screwed this up...

    >
    > > - if (S_ISDIR(inode->i_mode))
    > > - return 0;
    > >
    > > - /* lock, flush and invalidate mapping in preparation for flag change */
    > > - xfs_ilock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
    > > - error = filemap_write_and_wait(inode->i_mapping);
    > > - if (error)
    > > - goto out_unlock;
    > > - error = invalidate_inode_pages2(inode->i_mapping);
    > > - if (error)
    > > - goto out_unlock;
    > > + if (!S_ISDIR(inode->i_mode))
    > > + return -EINVAL;
    >
    > If this entire function collapses to an S_ISDIR check then you might
    > as well just hoist this one piece to the caller.

    Oops... yea this is broken... All broken.

    > Also, where is
    > xfs_ioctl_dax_check?

    I forgot to change the name.

    >
    > <confused>

    Much less so than me... :-/

    I'll clean it up.

    Thanks, this was really bad on my part...
    Ira

    >
    > --D
    >
    > >
    > > - *join_flags = XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL;
    > > return 0;
    > > -
    > > -out_unlock:
    > > - xfs_iunlock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
    > > - return error;
    > > -
    > > }
    > >
    > > /*
    > > @@ -1209,17 +1164,10 @@ xfs_ioctl_setattr_dax_invalidate(
    > > * have permission to do so. On success, return a clean transaction and the
    > > * inode locked exclusively ready for further operation specific checks. On
    > > * failure, return an error without modifying or locking the inode.
    > > - *
    > > - * The inode might already be IO locked on call. If this is the case, it is
    > > - * indicated in @join_flags and we take full responsibility for ensuring they
    > > - * are unlocked from now on. Hence if we have an error here, we still have to
    > > - * unlock them. Otherwise, once they are joined to the transaction, they will
    > > - * be unlocked on commit/cancel.
    > > */
    > > static struct xfs_trans *
    > > xfs_ioctl_setattr_get_trans(
    > > - struct xfs_inode *ip,
    > > - int join_flags)
    > > + struct xfs_inode *ip)
    > > {
    > > struct xfs_mount *mp = ip->i_mount;
    > > struct xfs_trans *tp;
    > > @@ -1236,8 +1184,7 @@ xfs_ioctl_setattr_get_trans(
    > > goto out_unlock;
    > >
    > > xfs_ilock(ip, XFS_ILOCK_EXCL);
    > > - xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL | join_flags);
    > > - join_flags = 0;
    > > + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
    > >
    > > /*
    > > * CAP_FOWNER overrides the following restrictions:
    > > @@ -1258,8 +1205,6 @@ xfs_ioctl_setattr_get_trans(
    > > out_cancel:
    > > xfs_trans_cancel(tp);
    > > out_unlock:
    > > - if (join_flags)
    > > - xfs_iunlock(ip, join_flags);
    > > return ERR_PTR(error);
    > > }
    > >
    > > @@ -1386,7 +1331,6 @@ xfs_ioctl_setattr(
    > > struct xfs_dquot *pdqp = NULL;
    > > struct xfs_dquot *olddquot = NULL;
    > > int code;
    > > - int join_flags = 0;
    > >
    > > trace_xfs_ioctl_setattr(ip);
    > >
    > > @@ -1410,18 +1354,11 @@ xfs_ioctl_setattr(
    > > return code;
    > > }
    > >
    > > - /*
    > > - * Changing DAX config may require inode locking for mapping
    > > - * invalidation. These need to be held all the way to transaction commit
    > > - * or cancel time, so need to be passed through to
    > > - * xfs_ioctl_setattr_get_trans() so it can apply them to the join call
    > > - * appropriately.
    > > - */
    > > - code = xfs_ioctl_setattr_dax_invalidate(ip, fa, &join_flags);
    > > + code = xfs_ioctl_setattr_dax_invalidate(ip);
    > > if (code)
    > > goto error_free_dquots;
    > >
    > > - tp = xfs_ioctl_setattr_get_trans(ip, join_flags);
    > > + tp = xfs_ioctl_setattr_get_trans(ip);
    > > if (IS_ERR(tp)) {
    > > code = PTR_ERR(tp);
    > > goto error_free_dquots;
    > > @@ -1552,7 +1489,6 @@ xfs_ioc_setxflags(
    > > struct fsxattr fa;
    > > struct fsxattr old_fa;
    > > unsigned int flags;
    > > - int join_flags = 0;
    > > int error;
    > >
    > > if (copy_from_user(&flags, arg, sizeof(flags)))
    > > @@ -1569,18 +1505,11 @@ xfs_ioc_setxflags(
    > > if (error)
    > > return error;
    > >
    > > - /*
    > > - * Changing DAX config may require inode locking for mapping
    > > - * invalidation. These need to be held all the way to transaction commit
    > > - * or cancel time, so need to be passed through to
    > > - * xfs_ioctl_setattr_get_trans() so it can apply them to the join call
    > > - * appropriately.
    > > - */
    > > - error = xfs_ioctl_setattr_dax_invalidate(ip, &fa, &join_flags);
    > > + error = xfs_ioctl_setattr_dax_invalidate(ip);
    > > if (error)
    > > goto out_drop_write;
    > >
    > > - tp = xfs_ioctl_setattr_get_trans(ip, join_flags);
    > > + tp = xfs_ioctl_setattr_get_trans(ip);
    > > if (IS_ERR(tp)) {
    > > error = PTR_ERR(tp);
    > > goto out_drop_write;
    > > --
    > > 2.25.1
    > >

    \
     
     \ /
      Last update: 2020-04-08 20:14    [W:4.580 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site