[V9,10/11] fs/xfs: Update xfs_ioctl_setattr_dax_invalidate()

Commit Message

Ira Weiny April 21, 2020, 7:17 p.m. UTC

From: Ira Weiny <[e-mail]>

Because of the separation of FS_XFLAG_DAX from S_DAX and the delayed
setting of S_DAX, data invalidation no longer needs to happen when
FS_XFLAG_DAX is changed.

Change xfs_ioctl_setattr_dax_invalidate() to be
xfs_ioctl_dax_check_set_cache() and alter the code to reflect the new
functionality.

Furthermore, we no longer need the locking so we remove the join_flags
logic.

Signed-off-by: Ira Weiny <[e-mail]>

---
Changes from V8:
	Change name of function to xfs_ioctl_dax_check_set_cache()
	Update commit message
	Fix bit manipulations

Changes from V7:
	Use new flag_inode_dontcache()
	Skip don't cache if mount over ride is active.

Changes from v6:
	Fix completely broken implementation and update commit message.
	Use the new VFS layer I_DONTCACHE to facilitate inode eviction
	and S_DAX changing on drop_caches

Changes from v5:
	New patch
---
 fs/xfs/xfs_ioctl.c | 108 +++++++++------------------------------------
 1 file changed, 20 insertions(+), 88 deletions(-)

Comments

Darrick J. Wong April 21, 2020, 8:31 p.m. UTC | #1

On Tue, Apr 21, 2020 at 12:17:52PM -0700, [e-mail] wrote:
> From: Ira Weiny <[e-mail]>
> 
> Because of the separation of FS_XFLAG_DAX from S_DAX and the delayed
> setting of S_DAX, data invalidation no longer needs to happen when
> FS_XFLAG_DAX is changed.
> 
> Change xfs_ioctl_setattr_dax_invalidate() to be
> xfs_ioctl_dax_check_set_cache() and alter the code to reflect the new
> functionality.
> 
> Furthermore, we no longer need the locking so we remove the join_flags
> logic.
> 
> Signed-off-by: Ira Weiny <[e-mail]>
> 
> ---
> Changes from V8:
> 	Change name of function to xfs_ioctl_dax_check_set_cache()
> 	Update commit message
> 	Fix bit manipulations
> 
> Changes from V7:
> 	Use new flag_inode_dontcache()
> 	Skip don't cache if mount over ride is active.
> 
> Changes from v6:
> 	Fix completely broken implementation and update commit message.
> 	Use the new VFS layer I_DONTCACHE to facilitate inode eviction
> 	and S_DAX changing on drop_caches
> 
> Changes from v5:
> 	New patch
> ---
>  fs/xfs/xfs_ioctl.c | 108 +++++++++------------------------------------
>  1 file changed, 20 insertions(+), 88 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 104495ac187c..b87b571a6748 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1245,64 +1245,26 @@ xfs_ioctl_setattr_xflags(
>  	return 0;
>  }
>  
> -/*
> - * 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.
> - */
> -static int
> -xfs_ioctl_setattr_dax_invalidate(
> +static void
> +xfs_ioctl_dax_check_set_cache(

That's a ... strange name.  Set cache on what?

Oh, this is the function that sets I_DONTCACHE if an FS_XFLAG_DAX change
could have an immediate effect on S_DAX (assuming no other users).  What
do you think of the following?

	/*
	 * If a change to FS_XFLAG_DAX will result in an change to S_DAX
	 * the next time the incore inode is initialized, set the VFS
	 * I_DONTCACHE flag to try to hurry that along.
	 */
	static void
	xfs_ioctl_try_change_vfs_dax(...)

--D

>  	struct xfs_inode	*ip,
> -	struct fsxattr		*fa,
> -	int			*join_flags)
> +	struct fsxattr		*fa)
>  {
> -	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;
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct inode            *inode = VFS_I(ip);
>  
>  	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;
> -
> -	*join_flags = XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL;
> -	return 0;
> +		return;
>  
> -out_unlock:
> -	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
> -	return error;
> +	if ((mp->m_flags & XFS_MOUNT_DAX_ALWAYS) ||
> +	    (mp->m_flags & XFS_MOUNT_DAX_NEVER))
> +		return;
>  
> +	if (((fa->fsx_xflags & FS_XFLAG_DAX) &&
> +	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_DAX)) ||
> +	    (!(fa->fsx_xflags & FS_XFLAG_DAX) &&
> +	     (ip->i_d.di_flags2 & XFS_DIFLAG2_DAX)))
> +		mark_inode_dontcache(inode);
>  }
>  
>  /*
> @@ -1310,17 +1272,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;
> @@ -1337,8 +1292,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:
> @@ -1359,8 +1313,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);
>  }
>  
> @@ -1486,7 +1438,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);
>  
> @@ -1510,18 +1461,9 @@ 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);
> -	if (code)
> -		goto error_free_dquots;
> +	xfs_ioctl_dax_check_set_cache(ip, fa);
>  
> -	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;
> @@ -1651,7 +1593,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)))
> @@ -1668,18 +1609,9 @@ 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);
> -	if (error)
> -		goto out_drop_write;
> +	xfs_ioctl_dax_check_set_cache(ip, &fa);
>  
> -	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
>

Dave Chinner April 21, 2020, 9:30 p.m. UTC | #2

On Tue, Apr 21, 2020 at 01:31:40PM -0700, Darrick J. Wong wrote:
> On Tue, Apr 21, 2020 at 12:17:52PM -0700, [e-mail] wrote:
> > From: Ira Weiny <[e-mail]>
> > 
> > Because of the separation of FS_XFLAG_DAX from S_DAX and the delayed
> > setting of S_DAX, data invalidation no longer needs to happen when
> > FS_XFLAG_DAX is changed.
> > 
> > Change xfs_ioctl_setattr_dax_invalidate() to be
> > xfs_ioctl_dax_check_set_cache() and alter the code to reflect the new
> > functionality.
> > 
> > Furthermore, we no longer need the locking so we remove the join_flags
> > logic.
> > 
> > Signed-off-by: Ira Weiny <[e-mail]>
> > 
> > ---
> > Changes from V8:
> > 	Change name of function to xfs_ioctl_dax_check_set_cache()
> > 	Update commit message
> > 	Fix bit manipulations
> > 
> > Changes from V7:
> > 	Use new flag_inode_dontcache()
> > 	Skip don't cache if mount over ride is active.
> > 
> > Changes from v6:
> > 	Fix completely broken implementation and update commit message.
> > 	Use the new VFS layer I_DONTCACHE to facilitate inode eviction
> > 	and S_DAX changing on drop_caches
> > 
> > Changes from v5:
> > 	New patch
> > ---
> >  fs/xfs/xfs_ioctl.c | 108 +++++++++------------------------------------
> >  1 file changed, 20 insertions(+), 88 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 104495ac187c..b87b571a6748 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1245,64 +1245,26 @@ xfs_ioctl_setattr_xflags(
> >  	return 0;
> >  }
> >  
> > -/*
> > - * 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.
> > - */
> > -static int
> > -xfs_ioctl_setattr_dax_invalidate(
> > +static void
> > +xfs_ioctl_dax_check_set_cache(
> 
> That's a ... strange name.  Set cache on what?
> 
> Oh, this is the function that sets I_DONTCACHE if an FS_XFLAG_DAX change
> could have an immediate effect on S_DAX (assuming no other users).  What
> do you think of the following?
> 
> 	/*
> 	 * If a change to FS_XFLAG_DAX will result in an change to S_DAX
> 	 * the next time the incore inode is initialized, set the VFS
> 	 * I_DONTCACHE flag to try to hurry that along.
> 	 */
> 	static void
> 	xfs_ioctl_try_change_vfs_dax(...)

That doesn't seem any better. This is a preparation function now, in
that it can't fail and doesn't change the outcome of the operation
being performed. So, IMO, calling it something like
xfs_ioctl_setattr_prepare_dax() would be a better name for it.

Cheers,

Dave.

Ira Weiny April 21, 2020, 9:43 p.m. UTC | #3

On Wed, Apr 22, 2020 at 07:30:49AM +1000, Dave Chinner wrote:
> On Tue, Apr 21, 2020 at 01:31:40PM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 21, 2020 at 12:17:52PM -0700, [e-mail] wrote:
> > > From: Ira Weiny <[e-mail]>
> > > 
> > > Because of the separation of FS_XFLAG_DAX from S_DAX and the delayed
> > > setting of S_DAX, data invalidation no longer needs to happen when
> > > FS_XFLAG_DAX is changed.
> > > 
> > > Change xfs_ioctl_setattr_dax_invalidate() to be
> > > xfs_ioctl_dax_check_set_cache() and alter the code to reflect the new
> > > functionality.
> > > 
> > > Furthermore, we no longer need the locking so we remove the join_flags
> > > logic.
> > > 
> > > Signed-off-by: Ira Weiny <[e-mail]>
> > > 
> > > ---
> > > Changes from V8:
> > > 	Change name of function to xfs_ioctl_dax_check_set_cache()
> > > 	Update commit message
> > > 	Fix bit manipulations
> > > 
> > > Changes from V7:
> > > 	Use new flag_inode_dontcache()
> > > 	Skip don't cache if mount over ride is active.
> > > 
> > > Changes from v6:
> > > 	Fix completely broken implementation and update commit message.
> > > 	Use the new VFS layer I_DONTCACHE to facilitate inode eviction
> > > 	and S_DAX changing on drop_caches
> > > 
> > > Changes from v5:
> > > 	New patch
> > > ---
> > >  fs/xfs/xfs_ioctl.c | 108 +++++++++------------------------------------
> > >  1 file changed, 20 insertions(+), 88 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > > index 104495ac187c..b87b571a6748 100644
> > > --- a/fs/xfs/xfs_ioctl.c
> > > +++ b/fs/xfs/xfs_ioctl.c
> > > @@ -1245,64 +1245,26 @@ xfs_ioctl_setattr_xflags(
> > >  	return 0;
> > >  }
> > >  
> > > -/*
> > > - * 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.
> > > - */
> > > -static int
> > > -xfs_ioctl_setattr_dax_invalidate(
> > > +static void
> > > +xfs_ioctl_dax_check_set_cache(
> > 
> > That's a ... strange name.  Set cache on what?
> > 
> > Oh, this is the function that sets I_DONTCACHE if an FS_XFLAG_DAX change
> > could have an immediate effect on S_DAX (assuming no other users).  What
> > do you think of the following?
> > 
> > 	/*
> > 	 * If a change to FS_XFLAG_DAX will result in an change to S_DAX
> > 	 * the next time the incore inode is initialized, set the VFS
> > 	 * I_DONTCACHE flag to try to hurry that along.
> > 	 */
> > 	static void
> > 	xfs_ioctl_try_change_vfs_dax(...)
> 
> That doesn't seem any better. This is a preparation function now, in
> that it can't fail and doesn't change the outcome of the operation
> being performed. So, IMO, calling it something like
> xfs_ioctl_setattr_prepare_dax() would be a better name for it.

But it does potentially (after a check) set I_DONTCACHE.

What about?

xfs_ioctl_dax_check_set_dontcache()?

Ira

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> [e-mail]

Dave Chinner April 21, 2020, 11:13 p.m. UTC | #4

On Tue, Apr 21, 2020 at 02:43:29PM -0700, Ira Weiny wrote:
> On Wed, Apr 22, 2020 at 07:30:49AM +1000, Dave Chinner wrote:
> > On Tue, Apr 21, 2020 at 01:31:40PM -0700, Darrick J. Wong wrote:
> > > On Tue, Apr 21, 2020 at 12:17:52PM -0700, [e-mail] wrote:
> > > > From: Ira Weiny <[e-mail]>
> > > > 
> > > > Because of the separation of FS_XFLAG_DAX from S_DAX and the delayed
> > > > setting of S_DAX, data invalidation no longer needs to happen when
> > > > FS_XFLAG_DAX is changed.
> > > > 
> > > > Change xfs_ioctl_setattr_dax_invalidate() to be
> > > > xfs_ioctl_dax_check_set_cache() and alter the code to reflect the new
> > > > functionality.
> > > > 
> > > > Furthermore, we no longer need the locking so we remove the join_flags
> > > > logic.
> > > > 
> > > > Signed-off-by: Ira Weiny <[e-mail]>
> > > > 
> > > > ---
> > > > Changes from V8:
> > > > 	Change name of function to xfs_ioctl_dax_check_set_cache()
> > > > 	Update commit message
> > > > 	Fix bit manipulations
> > > > 
> > > > Changes from V7:
> > > > 	Use new flag_inode_dontcache()
> > > > 	Skip don't cache if mount over ride is active.
> > > > 
> > > > Changes from v6:
> > > > 	Fix completely broken implementation and update commit message.
> > > > 	Use the new VFS layer I_DONTCACHE to facilitate inode eviction
> > > > 	and S_DAX changing on drop_caches
> > > > 
> > > > Changes from v5:
> > > > 	New patch
> > > > ---
> > > >  fs/xfs/xfs_ioctl.c | 108 +++++++++------------------------------------
> > > >  1 file changed, 20 insertions(+), 88 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > > > index 104495ac187c..b87b571a6748 100644
> > > > --- a/fs/xfs/xfs_ioctl.c
> > > > +++ b/fs/xfs/xfs_ioctl.c
> > > > @@ -1245,64 +1245,26 @@ xfs_ioctl_setattr_xflags(
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -/*
> > > > - * 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.
> > > > - */
> > > > -static int
> > > > -xfs_ioctl_setattr_dax_invalidate(
> > > > +static void
> > > > +xfs_ioctl_dax_check_set_cache(
> > > 
> > > That's a ... strange name.  Set cache on what?
> > > 
> > > Oh, this is the function that sets I_DONTCACHE if an FS_XFLAG_DAX change
> > > could have an immediate effect on S_DAX (assuming no other users).  What
> > > do you think of the following?
> > > 
> > > 	/*
> > > 	 * If a change to FS_XFLAG_DAX will result in an change to S_DAX
> > > 	 * the next time the incore inode is initialized, set the VFS
> > > 	 * I_DONTCACHE flag to try to hurry that along.
> > > 	 */
> > > 	static void
> > > 	xfs_ioctl_try_change_vfs_dax(...)
> > 
> > That doesn't seem any better. This is a preparation function now, in
> > that it can't fail and doesn't change the outcome of the operation
> > being performed. So, IMO, calling it something like
> > xfs_ioctl_setattr_prepare_dax() would be a better name for it.
> 
> But it does potentially (after a check) set I_DONTCACHE.

That is an implementation detail - it doesn't change the outcome of
the function, the current behaviour of the inode, or the result of
the ioctl....

> What about?
> 
> xfs_ioctl_dax_check_set_dontcache()?

Then we have to rename it again the moment we change it's
functionality. i.e. we have exactly the same problem we have now
where the function name describes the implementation, not the
operational reason the function is being called...

Cheers,

Dave.

Ira Weiny April 21, 2020, 11:24 p.m. UTC | #5

On Wed, Apr 22, 2020 at 09:13:43AM +1000, Dave Chinner wrote:
> On Tue, Apr 21, 2020 at 02:43:29PM -0700, Ira Weiny wrote:
> > On Wed, Apr 22, 2020 at 07:30:49AM +1000, Dave Chinner wrote:
> > > On Tue, Apr 21, 2020 at 01:31:40PM -0700, Darrick J. Wong wrote:
> > > > On Tue, Apr 21, 2020 at 12:17:52PM -0700, [e-mail] wrote:
> > > > > From: Ira Weiny <[e-mail]>

[snip]

> > > > 
> > > > That's a ... strange name.  Set cache on what?
> > > > 
> > > > Oh, this is the function that sets I_DONTCACHE if an FS_XFLAG_DAX change
> > > > could have an immediate effect on S_DAX (assuming no other users).  What
> > > > do you think of the following?
> > > > 
> > > > 	/*
> > > > 	 * If a change to FS_XFLAG_DAX will result in an change to S_DAX
> > > > 	 * the next time the incore inode is initialized, set the VFS
> > > > 	 * I_DONTCACHE flag to try to hurry that along.
> > > > 	 */
> > > > 	static void
> > > > 	xfs_ioctl_try_change_vfs_dax(...)
> > > 
> > > That doesn't seem any better. This is a preparation function now, in
> > > that it can't fail and doesn't change the outcome of the operation
> > > being performed. So, IMO, calling it something like
> > > xfs_ioctl_setattr_prepare_dax() would be a better name for it.
> > 
> > But it does potentially (after a check) set I_DONTCACHE.
> 
> That is an implementation detail - it doesn't change the outcome of
> the function, the current behaviour of the inode, or the result of
> the ioctl....

I'm confused.  This function does potentially flag the inode to not be cached.
How is that not changing the behavior of the inode?

> 
> > What about?
> > 
> > xfs_ioctl_dax_check_set_dontcache()?
> 
> Then we have to rename it again the moment we change it's
> functionality. i.e. we have exactly the same problem we have now
> where the function name describes the implementation, not the
> operational reason the function is being called...

Ok, I see what you are driving at.  You want this function to potentially do
other things and it should 'prepare' the inode for 'dax stuff'.  Is that about
it?

I'm ok with xfs_ioctl_setattr_prepare_dax() then.

Ira

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> [e-mail]

Patch

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 104495ac187c..b87b571a6748 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1245,64 +1245,26 @@  xfs_ioctl_setattr_xflags(
 	return 0;
 }
 
-/*
- * 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.
- */
-static int
-xfs_ioctl_setattr_dax_invalidate(
+static void
+xfs_ioctl_dax_check_set_cache(
 	struct xfs_inode	*ip,
-	struct fsxattr		*fa,
-	int			*join_flags)
+	struct fsxattr		*fa)
 {
-	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;
+	struct xfs_mount	*mp = ip->i_mount;
+	struct inode            *inode = VFS_I(ip);
 
 	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;
-
-	*join_flags = XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL;
-	return 0;
+		return;
 
-out_unlock:
-	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
-	return error;
+	if ((mp->m_flags & XFS_MOUNT_DAX_ALWAYS) ||
+	    (mp->m_flags & XFS_MOUNT_DAX_NEVER))
+		return;
 
+	if (((fa->fsx_xflags & FS_XFLAG_DAX) &&
+	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_DAX)) ||
+	    (!(fa->fsx_xflags & FS_XFLAG_DAX) &&
+	     (ip->i_d.di_flags2 & XFS_DIFLAG2_DAX)))
+		mark_inode_dontcache(inode);
 }
 
 /*
@@ -1310,17 +1272,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;
@@ -1337,8 +1292,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:
@@ -1359,8 +1313,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);
 }
 
@@ -1486,7 +1438,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);
 
@@ -1510,18 +1461,9 @@  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);
-	if (code)
-		goto error_free_dquots;
+	xfs_ioctl_dax_check_set_cache(ip, fa);
 
-	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;
@@ -1651,7 +1593,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)))
@@ -1668,18 +1609,9 @@  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);
-	if (error)
-		goto out_drop_write;
+	xfs_ioctl_dax_check_set_cache(ip, &fa);
 
-	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;