nvme: enable char device per namespace

[Javier González]

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.