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

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

[Mikhail Malygin]

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

Signed-off-by: Mikhail Malygin <m.malygin@yadro.com>
---
 drivers/nvme/target/configfs.c    | 14 +++++++++++
 drivers/nvme/target/core.c        | 23 +++++++++++++++++
 drivers/nvme/target/io-cmd-bdev.c | 16 ++++++++++--
 drivers/nvme/target/io-cmd-file.c | 42 ++++++++++++++++++++++---------
 drivers/nvme/target/nvmet.h       |  3 +++
 5 files changed, 84 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a45bd3b..06566ad1d197 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -545,6 +545,19 @@ 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 = 0;
+
+	ret = nvmet_ns_revalidate(ns);
+
+	return ret ? ret : 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 +565,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..1d90803e3c9a 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -620,6 +620,29 @@ 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;
+	int ret = 0;
+
+	mutex_lock(&subsys->lock);
+	if(!ns->enabled){
+		goto out_unlock;
+	}
+
+	if(ns->bdev){
+		ret = nvmet_bdev_ns_revalidate(ns);
+	}else if(ns->file){
+		ret = nvmet_file_ns_revalidate(ns);
+	}
+
+	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..ea55bbe93813 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);
 }
 
+static 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;
 }
 
@@ -75,6 +81,12 @@ void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
 	}
 }
 
+int nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
+{
+	nvmet_bdev_ns_read_size(ns);
+	return 0;
+}
+
 static u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts)
 {
 	u16 status = NVME_SC_SUCCESS;
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 05453f5d1448..84a2d664d39a 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)
 	}
 }
 
+static int nvmet_file_ns_read_size(struct nvmet_ns *ns)
+{
+	int ret;
+	struct kstat stat;
+
+	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,18 +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);
-	if (ret)
-		goto err;
+	ret = nvmet_file_ns_read_size(ns);
 
-	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);
+	if(ret){
+		goto err;
+	}
 
 	ns->bvec_cache = kmem_cache_create("nvmet-bvec",
 			NVMET_MAX_MPOOL_BVEC * sizeof(struct bio_vec),
@@ -80,6 +93,11 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 	return ret;
 }
 
+int nvmet_file_ns_revalidate(struct nvmet_ns *ns)
+{
+	return nvmet_file_ns_read_size(ns);
+}
+
 static void nvmet_file_init_bvec(struct bio_vec *bv, struct scatterlist *sg)
 {
 	bv->bv_page = sg_page(sg);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index c51f8dd01dc4..a9453e63e5dd 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);
+int nvmet_bdev_ns_revalidate(struct nvmet_ns *ns);
+int nvmet_file_ns_revalidate(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

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

[Sagi Grimberg]

> +int nvmet_ns_revalidate(struct nvmet_ns *ns)
> +{
> +	struct nvmet_subsys *subsys = ns->subsys;
> +	int ret = 0;
> +
> +	mutex_lock(&subsys->lock);
> +	if(!ns->enabled){
> +		goto out_unlock;
> +	}
> +
> +	if(ns->bdev){
> +		ret = nvmet_bdev_ns_revalidate(ns);
> +	}else if(ns->file){
> +		ret = nvmet_file_ns_revalidate(ns);
> +	}
> +
> +	nvmet_ns_changed(subsys, ns->nsid);

Shouldn't that happen only of the size changed?

So what is the model here? have some udev rule to
trigger this?

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

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

[Christoph Hellwig]

On Fri, Sep 27, 2019 at 10:24:17AM -0700, Sagi Grimberg wrote:
>> +int nvmet_ns_revalidate(struct nvmet_ns *ns)
>> +{
>> +	struct nvmet_subsys *subsys = ns->subsys;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&subsys->lock);
>> +	if(!ns->enabled){
>> +		goto out_unlock;
>> +	}
>> +
>> +	if(ns->bdev){
>> +		ret = nvmet_bdev_ns_revalidate(ns);
>> +	}else if(ns->file){
>> +		ret = nvmet_file_ns_revalidate(ns);
>> +	}
>> +
>> +	nvmet_ns_changed(subsys, ns->nsid);
>
> Shouldn't that happen only of the size changed?

s/of/if/ ?

I guess we could do a sanity check first.

> So what is the model here? have some udev rule to
> trigger this?

Well, even if there was a udev rule we can't directly rely on it.
Mikhail actually talked with me about this at SDC, and I suggested to
just implement a simple file to revalidate from userspace, as we have no
good way to figure out if it changed in the kernel code.

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

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

[Sagi Grimberg]

>>> +int nvmet_ns_revalidate(struct nvmet_ns *ns)
>>> +{
>>> +	struct nvmet_subsys *subsys = ns->subsys;
>>> +	int ret = 0;
>>> +
>>> +	mutex_lock(&subsys->lock);
>>> +	if(!ns->enabled){
>>> +		goto out_unlock;
>>> +	}
>>> +
>>> +	if(ns->bdev){
>>> +		ret = nvmet_bdev_ns_revalidate(ns);
>>> +	}else if(ns->file){
>>> +		ret = nvmet_file_ns_revalidate(ns);
>>> +	}
>>> +
>>> +	nvmet_ns_changed(subsys, ns->nsid);
>>
>> Shouldn't that happen only of the size changed?
> 
> s/of/if/ ?

Yes..

> I guess we could do a sanity check first.
> 
>> So what is the model here? have some udev rule to
>> trigger this?
> 
> Well, even if there was a udev rule we can't directly rely on it.

What do you mean we can't rely on it?

I do understand that we'd have no triggers for file-backed namespaces.

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

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

[Christoph Hellwig]

On Fri, Sep 27, 2019 at 03:34:24PM -0700, Sagi Grimberg wrote:
>> Well, even if there was a udev rule we can't directly rely on it.
>
> What do you mean we can't rely on it?
>
> I do understand that we'd have no triggers for file-backed namespaces.

Do we have any generic way supported by all block device drivers to
notify resizes?  For files we could at least use dnotify/inotify/fsnotify/
etc in userspace.

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

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

[m.malygin]

From: Mikhail Malygin <m.malygin@yadro.com>

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

Signed-off-by: Mikhail Malygin <m.malygin@yadro.com>
---
 drivers/nvme/target/configfs.c    | 14 +++++++++++
 drivers/nvme/target/core.c        | 31 +++++++++++++++++++++++
 drivers/nvme/target/io-cmd-bdev.c | 16 ++++++++++--
 drivers/nvme/target/io-cmd-file.c | 42 ++++++++++++++++++++++---------
 drivers/nvme/target/nvmet.h       |  3 +++
 5 files changed, 92 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a45bd3b..06566ad1d197 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -545,6 +545,19 @@ 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 = 0;
+
+	ret = nvmet_ns_revalidate(ns);
+
+	return ret ? ret : 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 +565,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..2f5933c2d300 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -620,6 +620,37 @@ 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;
+
+	mutex_lock(&subsys->lock);
+	if(!ns->enabled){
+		goto out_unlock;
+	}
+
+	if(ns->bdev){
+		ret = nvmet_bdev_ns_revalidate(ns);
+	}else if(ns->file){
+		ret = nvmet_file_ns_revalidate(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..ea55bbe93813 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);
 }
 
+static 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;
 }
 
@@ -75,6 +81,12 @@ void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
 	}
 }
 
+int nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
+{
+	nvmet_bdev_ns_read_size(ns);
+	return 0;
+}
+
 static u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts)
 {
 	u16 status = NVME_SC_SUCCESS;
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 05453f5d1448..84a2d664d39a 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)
 	}
 }
 
+static int nvmet_file_ns_read_size(struct nvmet_ns *ns)
+{
+	int ret;
+	struct kstat stat;
+
+	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,18 +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);
-	if (ret)
-		goto err;
+	ret = nvmet_file_ns_read_size(ns);
 
-	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);
+	if(ret){
+		goto err;
+	}
 
 	ns->bvec_cache = kmem_cache_create("nvmet-bvec",
 			NVMET_MAX_MPOOL_BVEC * sizeof(struct bio_vec),
@@ -80,6 +93,11 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 	return ret;
 }
 
+int nvmet_file_ns_revalidate(struct nvmet_ns *ns)
+{
+	return nvmet_file_ns_read_size(ns);
+}
+
 static void nvmet_file_init_bvec(struct bio_vec *bv, struct scatterlist *sg)
 {
 	bv->bv_page = sg_page(sg);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index c51f8dd01dc4..a9453e63e5dd 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);
+int nvmet_bdev_ns_revalidate(struct nvmet_ns *ns);
+int nvmet_file_ns_revalidate(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

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

[Mikhail Malygin]

Just posted an updated version with size check before triggering ns_changed.

Regarding the model, I’m managing both block devices and targets from userspace daemon, so
just need any reliable way to update target ns size once I resized block device.

> On 27 Sep 2019, at 20:24, Sagi Grimberg <sagi@grimberg.me> wrote:
> 
> 
>> +int nvmet_ns_revalidate(struct nvmet_ns *ns)
>> +{
>> +	struct nvmet_subsys *subsys = ns->subsys;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&subsys->lock);
>> +	if(!ns->enabled){
>> +		goto out_unlock;
>> +	}
>> +
>> +	if(ns->bdev){
>> +		ret = nvmet_bdev_ns_revalidate(ns);
>> +	}else if(ns->file){
>> +		ret = nvmet_file_ns_revalidate(ns);
>> +	}
>> +
>> +	nvmet_ns_changed(subsys, ns->nsid);
> 
> Shouldn't that happen only of the size changed?
> 
> So what is the model here? have some udev rule to
> trigger this?

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

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

[Sagi Grimberg]

This looks fine to me,

Christoph, you happy with this version?

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

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

[Christoph Hellwig]

On Fri, Oct 04, 2019 at 05:03:36PM -0700, Sagi Grimberg wrote:
> This looks fine to me,
> 
> Christoph, you happy with this version?
> 

Conceptually yes, but it really needs to at least follow the kernel
style.  I'm also not sure I'm happy about all the extra indirections,
but I can be talked into it.

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

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

[m.malygin]

From: Mikhail Malygin <m.malygin@yadro.com>

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

Signed-off-by: Mikhail Malygin <m.malygin@yadro.com>
---
 drivers/nvme/target/configfs.c    | 14 +++++++++++
 drivers/nvme/target/core.c        | 27 +++++++++++++++++++++
 drivers/nvme/target/io-cmd-bdev.c | 16 +++++++++++--
 drivers/nvme/target/io-cmd-file.c | 39 ++++++++++++++++++++++---------
 drivers/nvme/target/nvmet.h       |  3 +++
 5 files changed, 86 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a45bd3b..06566ad1d197 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -545,6 +545,19 @@ 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 = 0;
+
+	ret = nvmet_ns_revalidate(ns);
+
+	return ret ? ret : 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 +565,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..593cd919bd95 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;
+
+	mutex_lock(&subsys->lock);
+	if (!ns->enabled)
+		goto out_unlock;
+
+	if (ns->bdev)
+		ret = nvmet_bdev_ns_revalidate(ns);
+	else if (ns->file)
+		ret = nvmet_file_ns_revalidate(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..ea55bbe93813 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);
 }
 
+static 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;
 }
 
@@ -75,6 +81,12 @@ void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
 	}
 }
 
+int nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
+{
+	nvmet_bdev_ns_read_size(ns);
+	return 0;
+}
+
 static u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts)
 {
 	u16 status = NVME_SC_SUCCESS;
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 05453f5d1448..148b2343f9a1 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)
 	}
 }
 
+static int nvmet_file_ns_read_size(struct nvmet_ns *ns)
+{
+	int ret;
+	struct kstat stat;
+
+	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);
+
 	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);
@@ -80,6 +92,11 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 	return ret;
 }
 
+int nvmet_file_ns_revalidate(struct nvmet_ns *ns)
+{
+	return nvmet_file_ns_read_size(ns);
+}
+
 static void nvmet_file_init_bvec(struct bio_vec *bv, struct scatterlist *sg)
 {
 	bv->bv_page = sg_page(sg);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index c51f8dd01dc4..a9453e63e5dd 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);
+int nvmet_bdev_ns_revalidate(struct nvmet_ns *ns);
+int nvmet_file_ns_revalidate(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

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

[Mikhail Malygin]

Just posted updated version with code style cleanup.

> On 6 Oct 2019, at 13:12, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Fri, Oct 04, 2019 at 05:03:36PM -0700, Sagi Grimberg wrote:
>> This looks fine to me,
>> 
>> Christoph, you happy with this version?
>> 
> 
> Conceptually yes, but it really needs to at least follow the kernel
> style.  I'm also not sure I'm happy about all the extra indirections,
> but I can be talked into it.


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

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

[Christoph Hellwig]

> +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 = 0;
> +
> +	ret = nvmet_ns_revalidate(ns);
> +
> +	return ret ? ret : count;

Nit: no need to initialize ret, and we can use a normal if here:

	ret = nvmet_ns_revalidate(ns);
	if (ret)
		return ret;
	return count;

> +int nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
> +{
> +	nvmet_bdev_ns_read_size(ns);
> +	return 0;
> +}

I think we can just remove this wrapper.

> +static int nvmet_file_ns_read_size(struct nvmet_ns *ns)
> +{
> +	int ret;
> +	struct kstat stat;
> +
> +	ret = vfs_getattr(&ns->file->f_path,
> +			&stat, STATX_SIZE, AT_STATX_FORCE_SYNC);

Nit: you can add more arguments to the first line:

	ret = vfs_getattr(&ns->file->f_path, &stat, STATX_SIZE,
			  AT_STATX_FORCE_SYNC);

> +int nvmet_file_ns_revalidate(struct nvmet_ns *ns)
> +{
> +	return nvmet_file_ns_read_size(ns);
> +}

And we can just remove this wrapper as well.

Or in fact rename the low-level functions to *_revalidate which
might be a little more obvious.  But either way I don't think we need
small wrappers.

Otherwise this looks good to me, thanks for doing the work!

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

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

[m.malygin]

From: Mikhail Malygin <m.malygin@yadro.com>

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

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;
+
+	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;
+
+	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;
+
+	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);
+
 	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

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

[Mikhail Malygin]

Thanks for the detailed feedback! Posted an updated version to the ml.

> On 7 Oct 2019, at 19:56, Christoph Hellwig <hch@infradead.org> wrote:
> 
>> +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 = 0;
>> +
>> +	ret = nvmet_ns_revalidate(ns);
>> +
>> +	return ret ? ret : count;
> 
> Nit: no need to initialize ret, and we can use a normal if here:
> 
> 	ret = nvmet_ns_revalidate(ns);
> 	if (ret)
> 		return ret;
> 	return count;
> 
>> +int nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
>> +{
>> +	nvmet_bdev_ns_read_size(ns);
>> +	return 0;
>> +}
> 
> I think we can just remove this wrapper.
> 
>> +static int nvmet_file_ns_read_size(struct nvmet_ns *ns)
>> +{
>> +	int ret;
>> +	struct kstat stat;
>> +
>> +	ret = vfs_getattr(&ns->file->f_path,
>> +			&stat, STATX_SIZE, AT_STATX_FORCE_SYNC);
> 
> Nit: you can add more arguments to the first line:
> 
> 	ret = vfs_getattr(&ns->file->f_path, &stat, STATX_SIZE,
> 			  AT_STATX_FORCE_SYNC);
> 
>> +int nvmet_file_ns_revalidate(struct nvmet_ns *ns)
>> +{
>> +	return nvmet_file_ns_read_size(ns);
>> +}
> 
> And we can just remove this wrapper as well.
> 
> Or in fact rename the low-level functions to *_revalidate which
> might be a little more obvious.  But either way I don't think we need
> small wrappers.
> 
> Otherwise this looks good to me, thanks for doing the work!


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

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

[Chaitanya Kulkarni]

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

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

[Christoph Hellwig]

On Mon, Oct 07, 2019 at 10:57:09PM +0300, m.malygin@yadro.com wrote:
> From: Mikhail Malygin <m.malygin@yadro.com>
> 
> 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.
> 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
> 
> Signed-off-by: Mikhail Malygin <m.malygin@yadro.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

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

[m.malygin]

From: Mikhail Malygin <m.malygin@yadro.com>

Currently device size is cached in ns->size field on namespace enable, so
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 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 | 33 ++++++++++++++++++++-----------
 drivers/nvme/target/nvmet.h       |  3 +++
 5 files changed, 76 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..67c42465fd1e 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;
+	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;
+	}
+
+	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 32008d85172b..0dcb1450df24 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..fea4176b8921 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)
+{
+	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,10 @@ 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);
 	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

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

[Mikhail Malygin]

Thanks, posted an updated version.

> On 8 Oct 2019, at 05:30, Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com> wrote:
> 
> 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

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

[Chaitanya Kulkarni]

Thanks foe this patch Mikhail.

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 10/8/19 5:29 AM, m.malygin@yadro.com wrote:
> From: Mikhail Malygin<m.malygin@yadro.com>
> 
> Currently device size is cached in ns->size field on namespace enable, so
> 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 initiator may perform namespace rescan


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