Re: [PATCH v5 11/24] xfs: add XBF_VERITY_SEEN xfs_buf flag

Darrick J. Wong
2024-03-12T20:04:24Z

On Tue, Mar 12, 2024 at 06:01:01PM +1100, Dave Chinner wrote:
> On Mon, Mar 11, 2024 at 07:45:07PM -0700, Darrick J. Wong wrote:
> > On Mon, Mar 11, 2024 at 08:25:05AM -0700, Darrick J. Wong wrote:
> > > > But, if a generic blob cache is what it takes to move this forwards,
> > > > so be it.
> > >
> > > Not necessarily. ;)
> >
> > And here's today's branch, with xfs_blobcache.[ch] removed and a few
> > more cleanups:
> > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/tag/?h=fsverity-cleanups-6.9_2024-03-11
>
> Walking all the inodes counting all the verity blobs in the shrinker
> is going to be -expensive-. Shrinkers are run very frequently and
> with high concurrency under memory pressure by direct reclaim, and
> every single shrinker execution is going to run that traversal even
> if it is decided there is nothing that can be shrunk.
>
> IMO, it would be better to keep a count of reclaimable objects
> either on the inode itself (protected by the xa_lock when
> adding/removing) to avoid needing to walk the xarray to count the
> blocks on the inode. Even better would be a counter in the perag or
> a global percpu counter in the mount of all caches objects. Both of
> those pretty much remove all the shrinker side counting overhead.

I went with a global percpu counter, let's see if lockdep/kasan have
anything to say about my new design. :P

> Couple of other small things.
>
> - verity shrinker belongs in xfs_verity.c, not xfs_icache.c. It
> really has nothing to do with the icache other than calling
> xfs_icwalk(). That gets rid of some of the config ifdefs.

Done.

> - SHRINK_STOP is what should be returned by the scan when
> xfs_verity_shrinker_scan() wants the shrinker to immediately stop,
> not LONG_MAX.

Aha. Ok, thanks for the tipoff. ;)

> - In xfs_verity_cache_shrink_scan(), the nr_to_scan is a count of
> how many object to try to free, not how many we must free. i.e.
> even if we can't free objects, they are still objects that got
> scanned and so should decement nr_to_scan...

<nod>

Is there any way for ->scan_objects to tell its caller how much memory
it actually freed? Or does it only know about objects? I suppose
"number of bytes freed" wouldn't be that helpful since someone else
could allocate all the freed memory immediately anyway.

--D

> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>