From: Eric Biggers <ebiggers@kernel.org>
To: Andrey Albershteyn <aalbersh@redhat.com>
Cc: fsverity@lists.linux.dev
Subject: Re: [PATCH] fsverity: remove hash page spin lock
Date: Wed, 31 Jan 2024 21:37:38 -0800 [thread overview]
Message-ID: <20240201053738.GC1526@sol.localdomain> (raw)
In-Reply-To: <20240131182021.2730678-1-aalbersh@redhat.com>
On Wed, Jan 31, 2024 at 07:20:23PM +0100, Andrey Albershteyn wrote:
> diff --git a/fs/verity/verify.c b/fs/verity/verify.c
> index 904ccd7e8e16..7759f6dbd16f 100644
> --- a/fs/verity/verify.c
> +++ b/fs/verity/verify.c
> @@ -46,20 +46,25 @@ static bool is_hash_block_verified(struct fsverity_info *vi, struct page *hpage,
> * the hash page is newly instantiated or not.
> *
> * The first thread that sees PG_checked=0 must clear the corresponding
> - * bitmap bits, then set PG_checked=1. This requires a spinlock. To
> - * avoid having to take this spinlock in the common case of
> - * PG_checked=1, we start with an opportunistic lockless read.
> + * bitmap bits, then set PG_checked=1. The requirement is that
> + * PG_checked=1 is set after bits are cleared. Other threads could see
> + * the case of bit set to 1 and PG_checked=1 (block is verified and
> + * wasn't evicted from memory). This will lead to one of the thread
> + * skipping verification of a block. To restrict order of operations
> + * memory barrier is used.
> + *
> + * However, it can happen that two threads get into PG_checked=0
> + * section. This is fine as both of them will reset bits to 0 and do
> + * same verification job. This is not expected to happen often.
> */
> if (PageChecked(hpage)) {
> /*
> * A read memory barrier is needed here to give ACQUIRE
> * semantics to the above PageChecked() test.
> + *
> + * Matching to the smp_wmb() in PG_checked=0
> */
> smp_rmb();
> - return test_bit(hblock_idx, vi->hash_block_verified);
> - }
> - spin_lock(&vi->hash_page_init_lock);
> - if (PageChecked(hpage)) {
> verified = test_bit(hblock_idx, vi->hash_block_verified);
> } else {
> blocks_per_page = vi->tree_params.blocks_per_page;
> @@ -74,7 +79,7 @@ static bool is_hash_block_verified(struct fsverity_info *vi, struct page *hpage,
> SetPageChecked(hpage);
> verified = false;
> }
> - spin_unlock(&vi->hash_page_init_lock);
> +
> return verified;
> }
Thanks! I made some changes to the comment to make the explanation more
complete and hopefully easier to understand, and removed the 'verified' variable
which is no longer necessary. Sent out v2 at
https://lore.kernel.org/fsverity/20240201052813.68380-1-ebiggers@kernel.org.
Let me know if it looks good.
- Eric
prev parent reply other threads:[~2024-02-01 5:37 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-31 18:20 [PATCH] fsverity: remove hash page spin lock Andrey Albershteyn
2024-02-01 5:37 ` Eric Biggers [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240201053738.GC1526@sol.localdomain \
--to=ebiggers@kernel.org \
--cc=aalbersh@redhat.com \
--cc=fsverity@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).