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. 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. - SHRINK_STOP is what should be returned by the scan when xfs_verity_shrinker_scan() wants the shrinker to immediately stop, not LONG_MAX. - 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... -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
