On 08.12.2020 15:21, Christoph Hellwig wrote:
A bunch of nitpicks (mostly naming as usual, sorry..):
No worries. Thanks for taking the time.
+static int __nvme_ns_ioctl(struct gendisk *disk, unsigned int cmd,
+ unsigned long arg)
{
What about nvme_disk_ioctl instead as that is what it operates on?
Sure.
+static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
+ unsigned int cmd, unsigned long arg)
+{
+ return __nvme_ns_ioctl(bdev->bd_disk, cmd, arg);
+}
+
+static long nvme_cdev_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return __nvme_ns_ioctl((struct gendisk *)file->private_data, cmd, arg);
+}
No need for the cast.
Also can we keep all the char device methods together close to the
struct file_operations declaration? I just prefer to keep the code
a little grouped.
Perfect.
-static int nvme_open(struct block_device *bdev, fmode_t mode)
+static int __nvme_open(struct nvme_ns *ns)
{
- struct nvme_ns *ns = bdev->bd_disk->private_data;
-
#ifdef CONFIG_NVME_MULTIPATH
/* should never be called due to GENHD_FL_HIDDEN */
if (WARN_ON_ONCE(ns->head->disk))
@@ -1846,12 +1859,24 @@ static int nvme_open(struct block_device *bdev, fmode_t mode)
return -ENXIO;
}
+static void __nvme_release(struct nvme_ns *ns)
+{
+ module_put(ns->ctrl->ops->module);
+ nvme_put_ns(ns);
+}
nvme_ns_open and nvme_ns_release?
ok.
+
+static int nvme_open(struct block_device *bdev, fmode_t mode)
+{
+ struct nvme_ns *ns = bdev->bd_disk->private_data;
+
+ return __nvme_open(ns);
+}
+
static void nvme_release(struct gendisk *disk, fmode_t mode)
{
struct nvme_ns *ns = disk->private_data;
- module_put(ns->ctrl->ops->module);
- nvme_put_ns(ns);
+ __nvme_release(ns);
No need for the local ns variable in both cases.
ok.
+static int nvme_cdev_open(struct inode *inode, struct file *file)
+{
+ struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
+ int ret;
+
+ ret = __nvme_open(ns);
+ if (!ret)
+ file->private_data = ns->disk;
+
+ return ret;
Do we need the ->private_data assignment at all? I think the ioctl
handler could just grab it directly from i_cdev.
Mmmm. Good point. I'll try that.
+ sprintf(cdisk_name, "nvme%dn%dc", ctrl->instance, ns->head->instance);
And the most important naming decision is this. I have two issues with
naming still:
- we aready use the c for controller in the hidden disk naming. Although
that is in a different position, but I think this not super intuitive.
- this is missing multipath support entirely, so once we want to add
multipath support we'll run into issues. So maybe use something
based off the hidden node naming? E.g.:
sprintf(disk_name, "nvme-generic-%dc%dn%d", ctrl->subsys->instance,
ctrl->instance, ns->head->instance);
Perfect. Sounds like a good compromise to still keep the original hidden
disk. Keith is happy too, so we have a plan.
+ /* When the device does not support any of the features required by the
+ * kernel (or viceversa), hide the block device. We can still rely on
+ * the namespace char device for submitting IOCTLs
+ */
Normal kernel comment style is the opening
/*
on its own line.
OK.
if (nvme_update_ns_info(ns, id))
- goto out_put_disk;
+ disk->flags |= GENHD_FL_HIDDEN;
I don't think we can do this based on all the error returns. I think
we'll have to move the flags manipulation into nvme_update_ns_info to
also cover the revalidate case.
Ok.
I am working on the multipath part. I'll send a V3 with all these
comments and then a follow-up patch with multipath.
