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@xxxxxxxxxxxxx >
