On Fri, Mar 08, 2024 at 12:59:56PM +1100, Dave Chinner wrote:
> On Thu, Mar 07, 2024 at 02:46:54PM -0800, Darrick J. Wong wrote:
> > On Mon, Mar 04, 2024 at 08:10:34PM +0100, Andrey Albershteyn wrote:
> > > One of essential ideas of fs-verity is that pages which are already
> > > verified won't need to be re-verified if they still in page cache.
> > >
> > > XFS will store Merkle tree blocks in extended file attributes. When
> > > read extended attribute data is put into xfs_buf.
> > >
> > > fs-verity uses PG_checked flag to track status of the blocks in the
> > > page. This flag can has two meanings - page was re-instantiated and
> > > the only block in the page is verified.
> > >
> > > However, in XFS, the data in the buffer is not aligned with xfs_buf
> > > pages and we don't have a reference to these pages. Moreover, these
> > > pages are released when value is copied out in xfs_attr code. In
> > > other words, we can not directly mark underlying xfs_buf's pages as
> > > verified as it's done by fs-verity for other filesystems.
> > >
> > > One way to track that these pages were processed by fs-verity is to
> > > mark buffer as verified instead. If buffer is evicted the incore
> > > XBF_VERITY_SEEN flag is lost. When the xattr is read again
> > > xfs_attr_get() returns new buffer without the flag. The xfs_buf's
> > > flag is then used to tell fs-verity this buffer was cached or not.
> > >
> > > The second state indicated by PG_checked is if the only block in the
> > > PAGE is verified. This is not the case for XFS as there could be
> > > multiple blocks in single buffer (page size 64k block size 4k). This
> > > is handled by fs-verity bitmap. fs-verity is always uses bitmap for
> > > XFS despite of Merkle tree block size.
> > >
> > > The meaning of the flag is that value of the extended attribute in
> > > the buffer is processed by fs-verity.
> > >
> > > Signed-off-by: Andrey Albershteyn <aalbersh@xxxxxxxxxx>
> > > ---
> > > fs/xfs/xfs_buf.h | 18 ++++++++++--------
> > > 1 file changed, 10 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > > index 73249abca968..2a73918193ba 100644
> > > --- a/fs/xfs/xfs_buf.h
> > > +++ b/fs/xfs/xfs_buf.h
> > > @@ -24,14 +24,15 @@ struct xfs_buf;
> > >
> > > #define XFS_BUF_DADDR_NULL ((xfs_daddr_t) (-1LL))
> > >
> > > -#define XBF_READ (1u << 0) /* buffer intended for reading from device */
> > > -#define XBF_WRITE (1u << 1) /* buffer intended for writing to device */
> > > -#define XBF_READ_AHEAD (1u << 2) /* asynchronous read-ahead */
> > > -#define XBF_NO_IOACCT (1u << 3) /* bypass I/O accounting (non-LRU bufs) */
> > > -#define XBF_ASYNC (1u << 4) /* initiator will not wait for completion */
> > > -#define XBF_DONE (1u << 5) /* all pages in the buffer uptodate */
> > > -#define XBF_STALE (1u << 6) /* buffer has been staled, do not find it */
> > > -#define XBF_WRITE_FAIL (1u << 7) /* async writes have failed on this buffer */
> > > +#define XBF_READ (1u << 0) /* buffer intended for reading from device */
> > > +#define XBF_WRITE (1u << 1) /* buffer intended for writing to device */
> > > +#define XBF_READ_AHEAD (1u << 2) /* asynchronous read-ahead */
> > > +#define XBF_NO_IOACCT (1u << 3) /* bypass I/O accounting (non-LRU bufs) */
> > > +#define XBF_ASYNC (1u << 4) /* initiator will not wait for completion */
> > > +#define XBF_DONE (1u << 5) /* all pages in the buffer uptodate */
> > > +#define XBF_STALE (1u << 6) /* buffer has been staled, do not find it */
> > > +#define XBF_WRITE_FAIL (1u << 7) /* async writes have failed on this buffer */
> > > +#define XBF_VERITY_SEEN (1u << 8) /* buffer was processed by fs-verity */
> >
> > Yuck. I still dislike this entire approach.
> >
> > XBF_DOUBLE_ALLOC doubles the memory consumption of any xattr block that
> > gets loaded on behalf of a merkle tree request, then uses the extra
> > space to shadow the contents of the ondisk block. AFAICT the shadow
> > doesn't get updated even if the cached data does, which sounds like a
> > landmine for coherency issues.
> >
> > XFS_DA_OP_BUFFER is a little gross, since I don't like the idea of
> > exposing the low level buffering details of the xattr code to
> > xfs_attr_get callers.
> >
> > XBF_VERITY_SEEN is a layering violation because now the overall buffer
> > cache can track file metadata state. I think the reason why you need
> > this state flag is because the datadev buffer target indexes based on
> > physical xfs_daddr_t, whereas merkle tree blocks have their own internal
> > block numbers. You can't directly go from the merkle block number to an
> > xfs_daddr_t, so you can't use xfs_buf_incore to figure out if the block
> > fell out of memory.
> >
> > ISTR asking for a separation of these indices when I reviewed some
> > previous version of this patchset. At the time it seems to me that a
> > much more efficient way to cache the merkle tree blocks would be to set
> > up a direct (merkle tree block number) -> (blob of data) lookup table.
> > That I don't see here.
> >
> > In the spirit of the recent collaboration style that I've taken with
> > Christoph, I pulled your branch and started appending patches to it to
> > see if the design that I'm asking for is reasonable. As it so happens,
> > I was working on a simplified version of the xfs buffer cache ("fsbuf")
> > that could be used by simple filesystems to get them off of buffer
> > heads.
> >
> > (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?
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.
--D
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
