Re: [PATCH V8 10/11] fs/xfs: Change xfs_ioctl_setattr_dax_invalidate()

[Dave Chinner]

On Mon, Apr 20, 2020 at 11:36:17AM -0700, Ira Weiny wrote:
> On Mon, Apr 20, 2020 at 12:31:31PM +1000, Dave Chinner wrote:
> > On Tue, Apr 14, 2020 at 11:45:22PM -0700, ira.weiny@xxxxxxxxx wrote:
> > > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> > > -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 (mp->m_flags & (XFS_MOUNT_DAX_ALWAYS | 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)))
> > > +		flag_inode_dontcache(inode);
> > 
> > This doesn't set the XFS inode's "don't cache" flag, despite it
> > having one that serves exactly the same purpose.  IOWs, if the XFS_IDONTCACHE
> > flag is now redundant, please replace it's current usage with this new flag
> > and get rid of the XFS inode flag. i.e.  the only place we set XFS_IDONTCACHE
> > can be replaced with a call to this mark_inode_dontcache() call...
> 
> I agree, and I would have removed XFS_IDONTCACHE, except I was not convinced
> that XFS_IDONTCACHE was redundant.
> 
> Currently XFS_IDONTCACHE can be cleared if the inode is found in the cache and
> I was unable to convince myself that it would be ok to remove it.  I mentioned
> this to Darrick in V7.
> 
> https://lore.kernel.org/lkml/20200413194432.GD1649878@xxxxxxxxxxxxxxxxxxxxxxxxx/
> 
> What am I missing with this code?
> 
> xfs_iget_cache_hit():
> ...
>         if (!(flags & XFS_IGET_INCORE))
> 		xfs_iflags_clear(ip, XFS_ISTALE | XFS_IDONTCACHE);
> ...
> 
> Why is XFS_IDONTCACHE not 'sticky'?
> And why does xfs_iget_cache_hit() clear it

Because it was designed to do exactly what bulkstat required, and
nothing else.  xfs_iget() is an internal filesystem interface, not a
VFS level interface. Hence we can make up whatever semantics we
want. And if we get a cache hit, we have multiple references to the
inode so we probably should cache it regardless of whether the
original lookup said "I'm a one-shot wonder, so don't cache me".

IOWs, it's a classic "don't cache unless a second reference comes
along during the current life cycle" algorithm.

This isn't actually a frequently travelled path - bulkstat is a
pretty rare thing to be doing - so the behaviour is "be nice to the
cache because we can do it easily", not a hard requirement.

> rather than fail when XFS_IDONTCACHE is set?

Because then it would be impossible to access an inode that has
IDONTCACHE set on it. e.g. bulkstat an inode, now you can't open()
it because it has XFS_IDONTCACHE set and VFS pathwalk lookups fail
trying to resolve the inode number to a struct inode....

Same goes for I_DONTCACHE - this does not prevent new lookups from
taking references to the inode while it is still referenced. i.e.
the reference count can still go up after the flag is set. The flag
only takes effect when the reference count goes to zero.

Hence the only difference between XFS_IDONTCACHE and I_DONTCACHE is
the behaviour when cache hits on existing XFS_IDONTCACHE inodes
occur. It's not going to make a significant difference to cache
residency if we leave the I_DONTCACHE flag in place, because the
vast majority of inodes with that flag (from bulkstat) are still
one-shot wonders and hence the reclaim decision is still the
overwhelmingly correct decision to be making...

And, realistically, we have a second layer of inode caching in XFS
(the cluster buffers) and so it's likely if we evict and reclaim an
inode just before it gets re-used, then we'll hit the buffer cache
anyway. i.e. we still avoid the IO to read the inode back into
memory, we just burn a little more CPU re-instantiating it from the
buffer....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx