On Wed, Apr 10, 2024 at 06:15:02PM -0700, Darrick J. Wong wrote:
> > This looks a little weird to me. Can't we simply use XFS_IGET_DONTCACHE
> > at iget time and then clear I_DONTCACHE here if we want to keep the
> > inode around?
>
> Not anymore, because other threads can mess around with the dontcache
> state (yay fsdax access path changes!!) while we are scrubbing the
> inode.
You mean xfs_ioctl_setattr_prepare_dax? Oh lovely, a completely
undocumented d_mark_dontcache in a completely non-obvious place.
It sems to have appeared in
commit e4f9ba20d3b8c2b86ec71f326882e1a3c4e47953
Author: Ira Weiny <ira.weiny@xxxxxxxxx>
Date: Thu Apr 30 07:41:38 2020 -0700
fs/xfs: Update xfs_ioctl_setattr_dax_invalidate()
without any explanation either. And I can't see any reason why
we'd prevent inodes and dentries to be cached after DAX mode
switches to start with. I can only guess, maybe the commit thinks
d_mark_dontcache is about data caching?
>
> > Given that we only set the uncached flag from
> > XFS_IGET_DONTCACHE on a cache miss, we won't have set
> > DCACHE_DONTCACHE anywhere (and don't really care about the dentries to
> > start with).
> >
> > But why do we care about keeping the inodes with errors in memory
> > here, but not elsewhere?
>
> We actually, do, but it's not obvious...
>
> > Maybe this can be explained in an expanded comment.
>
> ...because this bit here is basically the same as xchk_irele, but we
> don't have a xfs_scrub object to pass in, so it's opencoded. I could
> pull this logic out into:
Eww, I hadn't seen xchk_irele before. To me it looks like
I_DONTCACHE/d_mark_dontcache is really the wrong vehicle here.
I'd instead have a XFS_IGET_SCRUB, which will set an XFS_ISCRUB or
whatever flag on a cache miss. Any cache hit without XFS_IGET_SCRUB
will clear it.
->drop_inode then always returns true for XFS_ISCRUB inodes unless
in a transaction. Talking about the in transaction part - why do
we drop inodes in the transaction in scrub, but not elsewhere?
