Re: [PATCH v4] nvmet: add revalidate ns sysfs attribute to handle device resize

[Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>]

Hi Mikhail,

Please some inline nit comments.

On 10/7/19 12:57 PM, m.malygin@yadro.com wrote:
> From: Mikhail Malygin <m.malygin@yadro.com>
> nit :-
> Currently device size is cached in ns->size field on namespace enable, so
> any device size change after that can't bee seen by initiator.
nit :- any device size change after that can't bee seen by the
initiator.
> This patch adds revalidate namespace attribute. Once it is written,
> target refreshes ns->size property and calls nvmet_ns_changed
> so initator may perform namespace rescan

nit :- so initiator may perform namespace rescan.

> 
> Signed-off-by: Mikhail Malygin <m.malygin@yadro.com>
> ---
>   drivers/nvme/target/configfs.c    | 16 +++++++++++++++
>   drivers/nvme/target/core.c        | 27 ++++++++++++++++++++++++
>   drivers/nvme/target/io-cmd-bdev.c | 10 +++++++--
>   drivers/nvme/target/io-cmd-file.c | 34 +++++++++++++++++++++----------
>   drivers/nvme/target/nvmet.h       |  3 +++
>   5 files changed, 77 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 98613a45bd3b..337e967190c5 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -545,6 +545,21 @@ static ssize_t nvmet_ns_buffered_io_store(struct config_item *item,
>   
>   CONFIGFS_ATTR(nvmet_ns_, buffered_io);
>   
> +static ssize_t nvmet_ns_revalidate_store(struct config_item *item,
> +		const char *page, size_t count)
> +{
> +	struct nvmet_ns *ns = to_nvmet_ns(item);
> +	int ret;
> +
> +	ret = nvmet_ns_revalidate(ns);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +CONFIGFS_ATTR_WO(nvmet_ns_, revalidate);
> +
>   static struct configfs_attribute *nvmet_ns_attrs[] = {
>   	&nvmet_ns_attr_device_path,
>   	&nvmet_ns_attr_device_nguid,
> @@ -552,6 +567,7 @@ static struct configfs_attribute *nvmet_ns_attrs[] = {
>   	&nvmet_ns_attr_ana_grpid,
>   	&nvmet_ns_attr_enable,
>   	&nvmet_ns_attr_buffered_io,
> +	&nvmet_ns_attr_revalidate,
>   #ifdef CONFIG_PCI_P2PDMA
>   	&nvmet_ns_attr_p2pmem,
>   #endif
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 3a67e244e568..122bcbdb5c05 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -620,6 +620,33 @@ void nvmet_ns_disable(struct nvmet_ns *ns)
>   	mutex_unlock(&subsys->lock);
>   }
>   
> +int nvmet_ns_revalidate(struct nvmet_ns *ns)
> +{
> +	struct nvmet_subsys *subsys = ns->subsys;
> +	loff_t ns_size = ns->size;
> +	u32 ns_blksize_shift = ns->blksize_shift;
> +	int ret = 0;
nit:- Please consider :-
	struct nvmet_subsys *subsys = ns->subsys;
	u32 ns_blksize_shift = ns->blksize_shift;
	loff_t ns_size = ns->size;
	int ret = 0;
> +
> +	mutex_lock(&subsys->lock);
> +	if (!ns->enabled)
> +		goto out_unlock;
> +
> +	if (ns->bdev)
> +		nvmet_bdev_ns_read_size(ns);
> +	else if (ns->file)
> +		ret = nvmet_file_ns_read_size(ns);
> +
> +	if (ret)
> +		goto out_unlock;

Nit:- since ret gets initialized as a part of the ns->file == true
why not ?
	if (ns->bdev)
		nvmet_bdev_ns_read_size(ns);
	else if (ns->file) {
		ret = nvmet_file_ns_read_size(ns);
		if (ret)
			goto out_unlock
	}
> +
> +	if (ns->size != ns_size || ns->blksize_shift != ns_blksize_shift)
> +		nvmet_ns_changed(subsys, ns->nsid);
> +
> +out_unlock:
> +	mutex_unlock(&subsys->lock);
> +	return ret;
> +}
> +
>   void nvmet_ns_free(struct nvmet_ns *ns)
>   {
>   	nvmet_ns_disable(ns);
> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
> index de0bff70ebb6..ac8229deeccc 100644
> --- a/drivers/nvme/target/io-cmd-bdev.c
> +++ b/drivers/nvme/target/io-cmd-bdev.c
> @@ -47,6 +47,12 @@ void nvmet_bdev_set_limits(struct block_device *bdev, struct nvme_id_ns *id)
>   	id->nows = to0based(ql->io_opt / ql->logical_block_size);
>   }
>   
> +void nvmet_bdev_ns_read_size(struct nvmet_ns *ns)
> +{
> +	ns->size = i_size_read(ns->bdev->bd_inode);
> +	ns->blksize_shift = blksize_bits(bdev_logical_block_size(ns->bdev));
> +}
> +
>   int nvmet_bdev_ns_enable(struct nvmet_ns *ns)
>   {
>   	int ret;
> @@ -62,8 +68,8 @@ int nvmet_bdev_ns_enable(struct nvmet_ns *ns)
>   		ns->bdev = NULL;
>   		return ret;
>   	}
> -	ns->size = i_size_read(ns->bdev->bd_inode);
> -	ns->blksize_shift = blksize_bits(bdev_logical_block_size(ns->bdev));
> +
> +	nvmet_bdev_ns_read_size(ns);
>   	return 0;
>   }
>   
> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 05453f5d1448..cff39fea6d85 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c
> @@ -27,10 +27,30 @@ void nvmet_file_ns_disable(struct nvmet_ns *ns)
>   	}
>   }
>   
> +int nvmet_file_ns_read_size(struct nvmet_ns *ns)
> +{
> +	int ret;
> +	struct kstat stat;
nit again :-
	struct kstat stat;
	int ret;
> +
> +	ret = vfs_getattr(&ns->file->f_path, &stat, STATX_SIZE,
> +				AT_STATX_FORCE_SYNC);
> +	if (ret)
> +		return ret;
> +
> +	ns->size = stat.size;
> +	/*
> +	 * i_blkbits can be greater than the universally accepted upper bound,
> +	 * so make sure we export a sane namespace lba_shift.
> +	 */
> +	ns->blksize_shift = min_t(u8,
> +			file_inode(ns->file)->i_blkbits, 12);
> +
> +	return 0;
> +}
> +
>   int nvmet_file_ns_enable(struct nvmet_ns *ns)
>   {
>   	int flags = O_RDWR | O_LARGEFILE;
> -	struct kstat stat;
>   	int ret;
>   
>   	if (!ns->buffered_io)
> @@ -43,19 +63,11 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
>   		return PTR_ERR(ns->file);
>   	}
>   
> -	ret = vfs_getattr(&ns->file->f_path,
> -			&stat, STATX_SIZE, AT_STATX_FORCE_SYNC);
> +	ret = nvmet_file_ns_read_size(ns);
nit:- no need for following new line.
> +
>   	if (ret)
>   		goto err;
>   
> -	ns->size = stat.size;
> -	/*
> -	 * i_blkbits can be greater than the universally accepted upper bound,
> -	 * so make sure we export a sane namespace lba_shift.
> -	 */
> -	ns->blksize_shift = min_t(u8,
> -			file_inode(ns->file)->i_blkbits, 12);
> -
>   	ns->bvec_cache = kmem_cache_create("nvmet-bvec",
>   			NVMET_MAX_MPOOL_BVEC * sizeof(struct bio_vec),
>   			0, SLAB_HWCACHE_ALIGN, NULL);
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index c51f8dd01dc4..ccdfdcfce65b 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -408,6 +408,7 @@ struct nvmet_ns *nvmet_find_namespace(struct nvmet_ctrl *ctrl, __le32 nsid);
>   void nvmet_put_namespace(struct nvmet_ns *ns);
>   int nvmet_ns_enable(struct nvmet_ns *ns);
>   void nvmet_ns_disable(struct nvmet_ns *ns);
> +int nvmet_ns_revalidate(struct nvmet_ns *ns);
>   struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid);
>   void nvmet_ns_free(struct nvmet_ns *ns);
>   
> @@ -485,6 +486,8 @@ int nvmet_bdev_ns_enable(struct nvmet_ns *ns);
>   int nvmet_file_ns_enable(struct nvmet_ns *ns);
>   void nvmet_bdev_ns_disable(struct nvmet_ns *ns);
>   void nvmet_file_ns_disable(struct nvmet_ns *ns);
> +void nvmet_bdev_ns_read_size(struct nvmet_ns *ns);
> +int nvmet_file_ns_read_size(struct nvmet_ns *ns);
>   u16 nvmet_bdev_flush(struct nvmet_req *req);
>   u16 nvmet_file_flush(struct nvmet_req *req);
>   void nvmet_ns_changed(struct nvmet_subsys *subsys, u32 nsid);
> 


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