Re: [PATCH V2 1/1] nvme: introduce generic per-namespace chardev

[Minwoo Im <minwoo.im.dev@gmail.com>]

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!

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme