Re: [PATCH v2 1/4] nfs/blocklayout: Fix premature PR key unregistration

Christoph Hellwig
2024-06-23T07:36:36Z

On Sat, Jun 22, 2024 at 01:26:10PM -0400, Chuck Lever wrote:
> This patch currently adds the pr_reg callback to
> bl_find_get_deviceid(), which has no visibility of the volume
> hierarchy. Where should the registration be done instead? I'm
> missing something.

Something like the patch below (untested Sunday morning coding) should
do the trick:

diff --git a/fs/nfs/blocklayout/blocklayout.c b/fs/nfs/blocklayout/blocklayout.c
index 947b2c52344097..6db54b215066e0 100644
--- a/fs/nfs/blocklayout/blocklayout.c
+++ b/fs/nfs/blocklayout/blocklayout.c
@@ -564,34 +564,32 @@ bl_find_get_deviceid(struct nfs_server *server,
gfp_t gfp_mask)
{
struct nfs4_deviceid_node *node;
- struct pnfs_block_dev *d;
- unsigned long start, end;
+ int err = -ENODEV;

retry:
node = nfs4_find_get_deviceid(server, id, cred, gfp_mask);
if (!node)
return ERR_PTR(-ENODEV);

- if (test_bit(NFS_DEVICEID_UNAVAILABLE, &node->flags))
- goto transient;
+ if (test_bit(NFS_DEVICEID_UNAVAILABLE, &node->flags)) {
+ unsigned long end = jiffies;
+ unsigned long start = end - PNFS_DEVICE_RETRY_TIMEOUT;

- d = container_of(node, struct pnfs_block_dev, node);
- if (d->pr_register)
- if (!d->pr_register(d))
- goto out_put;
- return node;
-
-transient:
- end = jiffies;
- start = end - PNFS_DEVICE_RETRY_TIMEOUT;
- if (!time_in_range(node->timestamp_unavailable, start, end)) {
- nfs4_delete_deviceid(node->ld, node->nfs_client, id);
- goto retry;
+ if (!time_in_range(node->timestamp_unavailable, start, end)) {
+ nfs4_delete_deviceid(node->ld, node->nfs_client, id);
+ goto retry;
+ }
+ goto out_put;
}

+ err = bl_register_dev(container_of(node, struct pnfs_block_dev, node));
+ if (err)
+ goto out_put;
+ return node;
+
out_put:
nfs4_put_deviceid_node(node);
- return ERR_PTR(-ENODEV);
+ return ERR_PTR(err);
}

static int
diff --git a/fs/nfs/blocklayout/blocklayout.h b/fs/nfs/blocklayout/blocklayout.h
index cc788e8ce90933..7efbef9d10dba8 100644
--- a/fs/nfs/blocklayout/blocklayout.h
+++ b/fs/nfs/blocklayout/blocklayout.h
@@ -104,6 +104,7 @@ struct pnfs_block_dev {
u64 start;
u64 len;

+ enum pnfs_block_volume_type type;
u32 nr_children;
struct pnfs_block_dev *children;
u64 chunk_size;
@@ -116,7 +117,6 @@ struct pnfs_block_dev {

bool (*map)(struct pnfs_block_dev *dev, u64 offset,
struct pnfs_block_dev_map *map);
- bool (*pr_register)(struct pnfs_block_dev *dev);
};

/* pnfs_block_dev flag bits */
@@ -178,6 +178,7 @@ struct bl_msg_hdr {
#define BL_DEVICE_REQUEST_ERR 0x2 /* User level process fails */

/* dev.c */
+int bl_register_dev(struct pnfs_block_dev *d);
struct nfs4_deviceid_node *bl_alloc_deviceid_node(struct nfs_server *server,
struct pnfs_device *pdev, gfp_t gfp_mask);
void bl_free_deviceid_node(struct nfs4_deviceid_node *d);
diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c
index 16fb64d4af31db..72e061e87e145a 100644
--- a/fs/nfs/blocklayout/dev.c
+++ b/fs/nfs/blocklayout/dev.c
@@ -13,9 +13,74 @@

#define NFSDBG_FACILITY NFSDBG_PNFS_LD

+static void bl_unregister_scsi(struct pnfs_block_dev *dev)
+{
+ struct block_device *bdev = file_bdev(dev->bdev_file);
+ const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+
+ if (!test_and_clear_bit(PNFS_BDEV_REGISTERED, &dev->flags))
+ return;
+
+ if (ops->pr_register(bdev, dev->pr_key, 0, false))
+ pr_err("failed to unregister PR key.\n");
+}
+
+static bool bl_register_scsi(struct pnfs_block_dev *dev)
+{
+ struct block_device *bdev = file_bdev(dev->bdev_file);
+ const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
+ int status;
+
+ if (test_and_set_bit(PNFS_BDEV_REGISTERED, &dev->flags))
+ return true;
+
+ status = ops->pr_register(bdev, 0, dev->pr_key, true);
+ if (status) {
+ pr_err("pNFS: failed to register key for block device %s.",
+ bdev->bd_disk->disk_name);
+ return false;
+ }
+ return true;
+}
+
+static void bl_unregister_dev(struct pnfs_block_dev *dev)
+{
+ if (dev->nr_children) {
+ for (u32 i = 0; i < dev->nr_children; i++)
+ bl_unregister_dev(&dev->children[i]);
+ return;
+ }
+
+ if (dev->type == PNFS_BLOCK_VOLUME_SCSI)
+ bl_unregister_scsi(dev);
+}
+
+int bl_register_dev(struct pnfs_block_dev *dev)
+{
+ if (dev->nr_children) {
+ for (u32 i = 0; i < dev->nr_children; i++) {
+ int ret = bl_register_dev(&dev->children[i]);
+
+ if (ret) {
+ while (i > 0)
+ bl_unregister_dev(&dev->children[--i]);
+ return ret;
+ }
+ }
+
+ return 0;
+ }
+
+ if (dev->type == PNFS_BLOCK_VOLUME_SCSI)
+ return bl_register_scsi(dev);
+ return 0;
+}
+
static void
bl_free_device(struct pnfs_block_dev *dev)
{
+ bl_unregister_dev(dev);
+
if (dev->nr_children) {
int i;

@@ -23,17 +88,6 @@ bl_free_device(struct pnfs_block_dev *dev)
bl_free_device(&dev->children[i]);
kfree(dev->children);
} else {
- if (test_and_clear_bit(PNFS_BDEV_REGISTERED, &dev->flags)) {
- struct block_device *bdev = file_bdev(dev->bdev_file);
- const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
- int error;
-
- error = ops->pr_register(file_bdev(dev->bdev_file),
- dev->pr_key, 0, false);
- if (error)
- pr_err("failed to unregister PR key.\n");
- }
-
if (dev->bdev_file)
fput(dev->bdev_file);
}
@@ -226,30 +280,6 @@ static bool bl_map_stripe(struct pnfs_block_dev *dev, u64 offset,
return true;
}

-/**
- * bl_pr_register_scsi - Register a SCSI PR key for @d
- * @dev: pNFS block device, key to register is already in @d->pr_key
- *
- * Returns true if the device's PR key is registered, otherwise false.
- */
-static bool bl_pr_register_scsi(struct pnfs_block_dev *dev)
-{
- struct block_device *bdev = file_bdev(dev->bdev_file);
- const struct pr_ops *ops = bdev->bd_disk->fops->pr_ops;
- int status;
-
- if (test_and_set_bit(PNFS_BDEV_REGISTERED, &dev->flags))
- return true;
-
- status = ops->pr_register(bdev, 0, dev->pr_key, true);
- if (status) {
- pr_err("pNFS: failed to register key for block device %s.",
- bdev->bd_disk->disk_name);
- return false;
- }
- return true;
-}
-
static int
bl_parse_deviceid(struct nfs_server *server, struct pnfs_block_dev *d,
struct pnfs_block_volume *volumes, int idx, gfp_t gfp_mask);
@@ -392,7 +422,6 @@ bl_parse_scsi(struct nfs_server *server, struct pnfs_block_dev *d,
goto out_blkdev_put;
}

- d->pr_register = bl_pr_register_scsi;
return 0;

out_blkdev_put:
@@ -478,7 +507,9 @@ static int
bl_parse_deviceid(struct nfs_server *server, struct pnfs_block_dev *d,
struct pnfs_block_volume *volumes, int idx, gfp_t gfp_mask)
{
- switch (volumes[idx].type) {
+ d->type = volumes[idx].type;
+
+ switch (d->type) {
case PNFS_BLOCK_VOLUME_SIMPLE:
return bl_parse_simple(server, d, volumes, idx, gfp_mask);
case PNFS_BLOCK_VOLUME_SLICE:
@@ -490,7 +521,7 @@ bl_parse_deviceid(struct nfs_server *server, struct pnfs_block_dev *d,
case PNFS_BLOCK_VOLUME_SCSI:
return bl_parse_scsi(server, d, volumes, idx, gfp_mask);
default:
- dprintk("unsupported volume type: %d\n", volumes[idx].type);
+ dprintk("unsupported volume type: %d\n", d->type);
return -EIO;
}
}