[PATCH V2 1/1] nvme: introduce generic per-namespace chardev
Minwoo Im
minwoo.im.dev at gmail.com
Wed Apr 7 15:11:28 BST 2021
On 21-04-07 15:15:27, Christoph Hellwig wrote:
> On Tue, Apr 06, 2021 at 03:48:41PM +0900, Minwoo Im wrote:
> > +static bool generic_ns = true;
> > +module_param(generic_ns, bool, 0644);
> > +MODULE_PARM_DESC(generic_ns, "support generic namespace character device");
>
> I do not think the module option is a good idea.
Okay. Will make generic device by default.
> > +#ifdef CONFIG_NVME_MULTIPATH
> > +static int nvme_generic_ns_open(struct inode *inode, struct file *file)
> > +{
>
> The ifdef here means that the cases of either a subsystem that can't
> have multiple controllers or the multipath=N option can't be handled
> properly. We really need two different code paths: one with the
> cdev in the namespace, and one with the cdev in the ns_head. Just
> like the gendisk.
Agreed. Kanchan has reported that if multiple controller in CMIC is not
supported, the head disk instance is not allocated and this code will
just return -ENODEV. As you pointed out here, will make it work in the
next version.
> > + return nvme_ns_head_open(generic_ns->head->disk->part0, file->f_mode);
>
> Calling this based on the bdev is a little silly when we can just
> use the trivial underlying functionality.
Then can we have like this ? The following diff is just considering the
ns_head only, but just conceptually, not hanging out with bdev, it's
just get the reference.
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a06634e7bad1..c92328eedc00 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2377,15 +2377,18 @@ static const struct block_device_operations nvme_bdev_ops = {
};
#ifdef CONFIG_NVME_MULTIPATH
-static int nvme_ns_head_open(struct block_device *bdev, fmode_t mode)
+static int nvme_ns_head_get(struct nvme_ns_head *head)
{
- struct nvme_ns_head *head = bdev->bd_disk->private_data;
-
if (!kref_get_unless_zero(&head->ref))
return -ENXIO;
return 0;
}
+static int nvme_ns_head_open(struct block_device *bdev, fmode_t mode)
+{
+ return nvme_ns_head_get(bdev->bd_disk->private_data);
+}
+
static void nvme_ns_head_release(struct gendisk *disk, fmode_t mode)
{
nvme_put_ns_head(disk->private_data);
@@ -3877,7 +3880,7 @@ static int nvme_generic_ns_open(struct inode *inode, struct file *file)
return -ENODEV;
file->private_data = generic_ns;
- return nvme_ns_head_open(generic_ns->head->disk->part0, file->f_mode);
+ return nvme_ns_head_get(generic_ns->head);
}
static int nvme_generic_ns_release(struct inode *inode, struct file *file)
> > + device_initialize(&generic_ns->device);
> > + generic_ns->device.devt = MKDEV(MAJOR(nvme_generic_ns_devt), minor);
> > + generic_ns->device.class = nvme_generic_ns_class;
> > + generic_ns->device.parent = ctrl->device;
>
> We can't reference the controller for something hanging off the ns_head.
Got your point. Let me fix this up.
> > + /*
> > + * If the namespace update fails in a graceful manner, hide the block
> > + * device, but still allow for the generic namespae device to be
> > + * craeted.
> > + */
> > + if (nvme_update_ns_info(ns, id)) {
> > + if (generic_ns)
> > + ns->disk->flags |= GENHD_FL_HIDDEN;
> > + else
> > + goto out_put_disk;
> > + }
>
> We must still fail when the error is one that does not just indicate
> a not supported feature.
Okay. Let me first list up the real error cases.
> > +struct nvme_generic_ns {
> > + struct device device;
> > + struct cdev cdev;
> > + struct nvme_ns_head *head;
> > +
> > + /* targted namespace instance. only valid in non-multipath */
> > + struct nvme_ns *ns;
> > +};
>
> I don't really see point of this extra structure.
As you pointed out above, we just can't rely on the nvme_ns_head where
disk might not be allocated. Earlier, I was trying to put this to
nvme_ns_head, but now possibililties are two. Where do you think for
this generic device fields to be located properly ?
> FYI, I've looked into addressing some of these points and will send
> out an updated patch that sits on top of an ioctl and multipath
> refatoring series I planned a while ago that I resurrected for this.
Sure. Once series are applied, we can make it on top of that series.
Thanks for sharing!
More information about the Linux-nvme
mailing list