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

From: Christoph Hellwig <hch@lst.de>
To: Minwoo Im <minwoo.im.dev@gmail.com>
Cc: linux-nvme@lists.infradead.org, "Keith Busch" <kbusch@kernel.org>,
	"Jens Axboe" <axboe@fb.com>, "Christoph Hellwig" <hch@lst.de>,
	"Sagi Grimberg" <sagi@grimberg.me>,
	"Kanchan Joshi" <joshiiitr@gmail.com>,
	"Javier González" <javier.gonz@samsung.com>
Subject: Re: [PATCH V2 1/1] nvme: introduce generic per-namespace chardev
Date: Wed, 7 Apr 2021 15:15:27 +0200	[thread overview]
Message-ID: <20210407131527.GA15142@lst.de> (raw)
In-Reply-To: <20210406064841.103393-2-minwoo.im.dev@gmail.com>

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.

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

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

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

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

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

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.

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