From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B9CD6C76188 for ; Tue, 4 Apr 2023 23:32:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236560AbjDDXc3 (ORCPT ); Tue, 4 Apr 2023 19:32:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51848 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236366AbjDDXc2 (ORCPT ); Tue, 4 Apr 2023 19:32:28 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 54301171A; Tue, 4 Apr 2023 16:32:27 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E0AFA63317; Tue, 4 Apr 2023 23:32:26 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id BF8AEC433EF; Tue, 4 Apr 2023 23:32:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1680651146; bh=mzC9yxS3pU0gQe8btlzdUEVy3UG1HFMbmPEVa9F5ySY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=IQ5deiEXGx1gFAZISmhBLeCx8NzxQoUgL9nmQTkP3esfYaFFQQ3b7ESc/W//rQnfl 3sC6jG70uboYZiTYrN8C1P21fsc+gnsV48CY7KPCfaJ/OQnp8UfTYsmdcdkTEW4buW hVn+2CEI8t2/lO/2BdLWwWucOOyJ253E2xRfYlRB0Mh95jIzG88ZgJaCkbFY/S4ke/ DQzAT3I9w8UZewW6kTUEN8IG68kmycB6WIeZaSCrRu4RRTDEL/ofiUNKTs5kJkkHKg x1+XwPQ6O+zGp2wSQJhkj/co7Ojg7M32UskAXMdTeQ7xaEOcp4FpffpkAMVswDrSRa iX8vXvvL6hmJg== Date: Tue, 4 Apr 2023 16:32:24 -0700 From: Eric Biggers To: Andrey Albershteyn Cc: djwong@kernel.org, dchinner@redhat.com, hch@infradead.org, linux-xfs@vger.kernel.org, fsverity@lists.linux.dev, rpeterso@redhat.com, agruenba@redhat.com, xiang@kernel.org, chao@kernel.org, damien.lemoal@opensource.wdc.com, jth@kernel.org, linux-erofs@lists.ozlabs.org, linux-btrfs@vger.kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com Subject: Re: [PATCH v2 21/23] xfs: handle merkle tree block size != fs blocksize != PAGE_SIZE Message-ID: <20230404233224.GE1893@sol.localdomain> References: <20230404145319.2057051-1-aalbersh@redhat.com> <20230404145319.2057051-22-aalbersh@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230404145319.2057051-22-aalbersh@redhat.com> Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org Hi Andrey, On Tue, Apr 04, 2023 at 04:53:17PM +0200, Andrey Albershteyn wrote: > In case of different Merkle tree block size fs-verity expects > ->read_merkle_tree_page() to return Merkle tree page filled with > Merkle tree blocks. The XFS stores each merkle tree block under > extended attribute. Those attributes are addressed by block offset > into Merkle tree. > > This patch make ->read_merkle_tree_page() to fetch multiple merkle > tree blocks based on size ratio. Also the reference to each xfs_buf > is passed with page->private to ->drop_page(). > > Signed-off-by: Andrey Albershteyn > --- > fs/xfs/xfs_verity.c | 74 +++++++++++++++++++++++++++++++++++---------- > fs/xfs/xfs_verity.h | 8 +++++ > 2 files changed, 66 insertions(+), 16 deletions(-) > > diff --git a/fs/xfs/xfs_verity.c b/fs/xfs/xfs_verity.c > index a9874ff4efcd..ef0aff216f06 100644 > --- a/fs/xfs/xfs_verity.c > +++ b/fs/xfs/xfs_verity.c > @@ -134,6 +134,10 @@ xfs_read_merkle_tree_page( > struct page *page = NULL; > __be64 name = cpu_to_be64(index << PAGE_SHIFT); > uint32_t bs = 1 << log_blocksize; > + int blocks_per_page = > + (1 << (PAGE_SHIFT - log_blocksize)); > + int n = 0; > + int offset = 0; > struct xfs_da_args args = { > .dp = ip, > .attr_filter = XFS_ATTR_VERITY, > @@ -143,26 +147,59 @@ xfs_read_merkle_tree_page( > .valuelen = bs, > }; > int error = 0; > + bool is_checked = true; > + struct xfs_verity_buf_list *buf_list; > > page = alloc_page(GFP_KERNEL); > if (!page) > return ERR_PTR(-ENOMEM); > > - error = xfs_attr_get(&args); > - if (error) { > - kmem_free(args.value); > - xfs_buf_rele(args.bp); > + buf_list = kzalloc(sizeof(struct xfs_verity_buf_list), GFP_KERNEL); > + if (!buf_list) { > put_page(page); > - return ERR_PTR(-EFAULT); > + return ERR_PTR(-ENOMEM); > } > > - if (args.bp->b_flags & XBF_VERITY_CHECKED) > + /* > + * Fill the page with Merkle tree blocks. The blcoks_per_page is higher > + * than 1 when fs block size != PAGE_SIZE or Merkle tree block size != > + * PAGE SIZE > + */ > + for (n = 0; n < blocks_per_page; n++) { > + offset = bs * n; > + name = cpu_to_be64(((index << PAGE_SHIFT) + offset)); > + args.name = (const uint8_t *)&name; > + > + error = xfs_attr_get(&args); > + if (error) { > + kmem_free(args.value); > + /* > + * No more Merkle tree blocks (e.g. this was the last > + * block of the tree) > + */ > + if (error == -ENOATTR) > + break; > + xfs_buf_rele(args.bp); > + put_page(page); > + kmem_free(buf_list); > + return ERR_PTR(-EFAULT); > + } > + > + buf_list->bufs[buf_list->buf_count++] = args.bp; > + > + /* One of the buffers was dropped */ > + if (!(args.bp->b_flags & XBF_VERITY_CHECKED)) > + is_checked = false; > + > + memcpy(page_address(page) + offset, args.value, args.valuelen); > + kmem_free(args.value); > + args.value = NULL; > + } I was really hoping for a solution where the cached data can be used directly, instead allocating a temporary page and copying the cached data into it every time the cache is accessed. The problem with what you have now is that every time a single 32-byte hash is accessed, a full page (potentially 64KB!) will be allocated and filled. That's not very efficient. The need to allocate the temporary page can also cause ENOMEM (which will get reported as EIO). Did you consider alternatives that would work more efficiently? I think it would be worth designing something that works properly with how XFS is planned to cache the Merkle tree, instead of designing a workaround. ->read_merkle_tree_page was not really designed for what you are doing here. How about replacing ->read_merkle_tree_page with a function that takes in a Merkle tree block index (not a page index!) and hands back a (page, offset) pair that identifies where the Merkle tree block's data is located? Or (folio, offset), I suppose. With that, would it be possible to directly return the XFS cache? - Eric