On 20 Jun 2024, at 1:06, 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.
The transient device handling in general, or just this bit of it?
>> + 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?
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.
