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

[Dave Chinner]

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, ira.weiny@xxxxxxxxx wrote:
> > > > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> > > > 
> > > > 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 <ira.weiny@xxxxxxxxx>
> > > > 
> > > > ---
> > > > 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.
-- 
Dave Chinner
david@xxxxxxxxxxxxx