All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Dai Ngo <dai.ngo@oracle.com>
Cc: trondmy@kernel.org, anna@kernel.org, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 1/1] pNFS: Serialize SCSI PR registration to avoid reservation conflicts
Date: Tue, 3 Mar 2026 07:33:49 -0800	[thread overview]
Message-ID: <aab_XbwjYoIPk2_a@infradead.org> (raw)
In-Reply-To: <20260302005138.1844156-1-dai.ngo@oracle.com>

On Sun, Mar 01, 2026 at 04:51:23PM -0800, Dai Ngo wrote:
> This problem can be reproduced by running 'fio' test with this
> workload:

I wish we could wire this up somewhere.  Not sure what the right
place for these kinds of nfs tests are, though.

>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/blocklayout/blocklayout.h b/fs/nfs/blocklayout/blocklayout.h
> index 6da40ca19570..535db8b0e89c 100644
> --- a/fs/nfs/blocklayout/blocklayout.h
> +++ b/fs/nfs/blocklayout/blocklayout.h
> @@ -117,6 +117,7 @@ struct pnfs_block_dev {
>  
>  	bool (*map)(struct pnfs_block_dev *dev, u64 offset,
>  			struct pnfs_block_dev_map *map);
> +	struct mutex			pbd_mutex;

Can you keep this up with the non-function pointer fields?  I guess
pbd_registration_lock might be a more descriptive name, and comment
explaining what the lock protects also never hurts.

> diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c
> index cc6327d97a91..45630781f1a8 100644
> --- a/fs/nfs/blocklayout/dev.c
> +++ b/fs/nfs/blocklayout/dev.c
> @@ -33,10 +33,14 @@ static bool bl_register_scsi(struct pnfs_block_dev *dev)
>  	const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
>  	int status;
>  
> -	if (test_and_set_bit(PNFS_BDEV_REGISTERED, &dev->flags))
> +	mutex_lock(&dev->pbd_mutex);
> +	if (test_and_set_bit(PNFS_BDEV_REGISTERED, &dev->flags)) {
> +		mutex_unlock(&dev->pbd_mutex);
>  		return true;
> +	}

This seems to only lock the registration side, and not the
unregistration side, which is a bit odd.  If you fully protect
register/unregister we also don't need atomic bitops for
PNFS_BDEV_REGISTERED and have a more consistent locking scheme.

  reply	other threads:[~2026-03-03 15:33 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-02  0:51 [PATCH 1/1] pNFS: Serialize SCSI PR registration to avoid reservation conflicts Dai Ngo
2026-03-03 15:33 ` Christoph Hellwig [this message]
2026-03-03 16:11   ` Chuck Lever
2026-03-03 18:21   ` Dai Ngo
2026-03-04 13:11     ` Christoph Hellwig

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=aab_XbwjYoIPk2_a@infradead.org \
    --to=hch@infradead.org \
    --cc=anna@kernel.org \
    --cc=dai.ngo@oracle.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=trondmy@kernel.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.