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
