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

From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: Andrey Albershteyn <aalbersh@redhat.com>,
	fsverity@lists.linux.dev, linux-xfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, chandan.babu@oracle.com,
	ebiggers@kernel.org
Subject: Re: [PATCH v5 11/24] xfs: add XBF_VERITY_SEEN xfs_buf flag
Date: Mon, 11 Mar 2024 19:45:07 -0700	[thread overview]
Message-ID: <20240312024507.GY1927156@frogsfrogsfrogs> (raw)
In-Reply-To: <20240311152505.GR1927156@frogsfrogsfrogs>

On Mon, Mar 11, 2024 at 08:25:05AM -0700, Darrick J. Wong wrote:

<snip>

> <nod> The latest version of this tries to avoid letting reclaim take the
> top of the tree.  Logically this makes sense to me to reduce read verify
> latency, but I was hoping Eric or Andrey or someone with more
> familiarity with fsverity would chime in on whether or not that made
> sense.
> 
> > > > 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".
> 
> Overly focused on unfamiliar data structures -- I've never built
> anything with an xarray before.
> 
> > 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.
> 
> Oho, that's a good idea.  I didn't like the separate allocation anyway.
> Friday was mostly chatting with willy and trying to make sure I got the
> xarray access patterns correct.
> 
> >                                                    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...
> 
> This too is a wonderful suggestion -- use the third radix tree tag to
> mark inodes with extra incore caches that can be reclaimed, then teach
> xfs_reclaim_inodes_{nr,count} to scan them.  Allocating a per-inode
> shrinker was likely to cause problems with flooding debugfs with too
> many knobs anyway.
> 
> > 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

--D

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