Re: [RFC PATCH 3/4] nfs/blocklayout: Fix premature PR key unregistration

[Chuck Lever <chuck.lever@oracle.com>]

On Thu, Jun 20, 2024 at 09:52:59AM -0400, Benjamin Coddington wrote:
> On 20 Jun 2024, at 1:06, Christoph Hellwig wrote:
> 
> > On Wed, Jun 19, 2024 at 01:39:33PM -0400, cel@kernel.org wrote:
> >> -	if (test_bit(NFS_DEVICEID_UNAVAILABLE, &node->flags) == 0)
> >> +	if (test_bit(NFS_DEVICEID_UNAVAILABLE, &node->flags) == 0) {
> >
> > It might be worth to invert this and keep the unavailable handling in
> > the branch as that's the exceptional case.   That code is also woefully
> > under-documented and could have really used a comment.
> 
> The transient device handling in general, or just this bit of it?

I read Christoph's comment as referring specifically to the logic
in bl_find_get_deviceid(). 


> >> +		struct pnfs_block_dev *d =
> >> +			container_of(node, struct pnfs_block_dev, node);
> >> +		if (d->pr_reg)
> >> +			if (d->pr_reg(d) < 0)
> >> +				goto out_put;
> >
> > Empty line after variable declarations.  Also is there anything that
> > synchronizes the lookups here so that we don't do multiple registrations
> > in parallel?
> 
> I don't think there is.  Do we get an error if we register twice?

pr_register() does not throw an error in that case, so I didn't
protect against it. However, I could add atomic bit flags to
pnfs_block_dev to ensure registration is done only once, if we
believe that is necessary.


> Ben
> 
> >> +
> >> +	if (d->pr_registered)
> >> +		return 0;
> >> +
> >> +	error = ops->pr_register(bdev, 0, d->pr_key, true);
> >> +	if (error) {
> >> +		trace_bl_pr_key_reg_err(bdev->bd_disk->disk_name, d->pr_key, error);
> >> +		return -error;
> >
> > ->pr_register returns either negative errnos or positive PR_STS_* values,
> > simply inverting the error here isn't doing the right thing.
> 

-- 
Chuck Lever