On Sat, Mar 09, 2024 at 08:28:28AM -0800, Darrick J. Wong wrote: > On Thu, Mar 07, 2024 at 07:31:38PM -0800, Darrick J. Wong wrote: > > On Fri, Mar 08, 2024 at 12:59:56PM +1100, Dave Chinner wrote: > > > > (Ab)using the fsbuf code did indeed work (and passed all the fstests -g > > > > verity tests), so now I know the idea is reasonable. Patches 11, 12, > > > > 14, and 15 become unnecessary. However, this solution is itself grossly > > > > overengineered, since all we want are the following operations: > > > > > > > > peek(key): returns an fsbuf if there's any data cached for key > > > > > > > > get(key): returns an fsbuf for key, regardless of state > > > > > > > > store(fsbuf, p): attach a memory buffer p to fsbuf > > > > > > > > Then the xfs ->read_merkle_tree_block function becomes: > > > > > > > > bp = peek(key) > > > > if (bp) > > > > /* return bp data up to verity */ > > > > > > > > p = xfs_attr_get(key) > > > > if (!p) > > > > /* error */ > > > > > > > > bp = get(key) > > > > store(bp, p) > > > > > > Ok, that looks good - it definitely gets rid of a lot of the > > > nastiness, but I have to ask: why does it need to be based on > > > xfs_bufs? > > > > (copying from IRC) It was still warm in my brain L2 after all the xfile > > buftarg cleaning and merging that just got done a few weeks ago. So I > > went with the simplest thing I could rig up to test my ideas, and now > > we're at the madly iterate until exhaustion stage. ;) > > > > > That's just wasting 300 bytes of memory on a handle to > > > store a key and a opaque blob in a rhashtable. > > > > Yep. The fsbufs implementation was a lot more slender, but a bunch more > > code. I agree that I ought to go look at xarrays or something that's > > more of a direct mapping as a next step. However, i wanted to get > > Andrey's feedback on this general approach first. > > > > > IIUC, the key here is a sequential index, so an xarray would be a > > > much better choice as it doesn't require internal storage of the > > > key. > > > > I wonder, what are the access patterns for merkle blobs? Is it actually > > sequential, or is more like 0 -> N -> N*N as we walk towards leaves? I think the leaf level (i.e. individual record) access patterns largely match data access patterns, so I'd just treat it like as if it's a normal file being accessed.... > > Also -- the fsverity block interfaces pass in a "u64 pos" argument. Was > > that done because merkle trees may some day have more than 2^32 blocks > > in them? That won't play well with things like xarrays on 32-bit > > machines. > > > > (Granted we've been talking about deprecating XFS on 32-bit for a while > > now but we're not the whole world) > > > > > i.e. > > > > > > p = xa_load(key); > > > if (p) > > > return p; > > > > > > xfs_attr_get(key); > > > if (!args->value) > > > /* error */ > > > > > > /* > > > * store the current value, freeing any old value that we > > > * replaced at this key. Don't care about failure to store, > > > * this is optimistic caching. > > > */ > > > p = xa_store(key, args->value, GFP_NOFS); > > > if (p) > > > kvfree(p); > > > return args->value; > > > > Attractive. Will have to take a look at that tomorrow. > > Done. I think. Not sure that I actually got all the interactions > between the shrinker and the xarray correct though. KASAN and lockdep > don't have any complaints running fstests, so that's a start. > > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=fsverity-cleanups-6.9_2024-03-09 My initial impression is "over-engineered". I personally would have just allocated the xattr value buffer with a little extra size and added all the external cache information (a reference counter is all we need as these are fixed sized blocks) to the tail of the blob we actually pass to fsverity. If we tag the inode in the radix tree as having verity blobs that can be freed, we can then just extend the existing fs sueprblock shrinker callout to also walk all the verity inodes with cached data to try to reclaim some objects... But, if a generic blob cache is what it takes to move this forwards, so be it. -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
