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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 20, 2024 at 07:06:14AM +0200, Christoph Hellwig wrote:
> On Wed, Jun 19, 2024 at 01:39:33PM -0400, cel@xxxxxxxxxx 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.
> 
> > +		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?

Is there something (more than d->pr_registered) that prevents that
today?


> > +
> > +	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.

I don't see pr_register() returning a negative errno, but I will
remove the "-" and document the expected return values.


-- 
Chuck Lever




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux