[PATCH V2 0/2] nvmet: add revalidate support

[PATCH V2 0/2] nvmet: add revalidate support

[Chaitanya Kulkarni]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 4484 bytes --]

Hi,

This patch series adds support for the NVMeOF target bdev-ns and
file-ns size re-validation.

The first patch adds bdev/file backend revalidation helpers which
was posted by Anthony Iliopoulos. I've fixed the comments posted on V1
by keeping the authorship of the patch.

The 2nd patch is needed since the change of size detection on the target
should generate the AEN to the host. Right now there is no mechanism
that allows us to add callbacks for the block and file backend so that
we will get the notification (if anyone knows please let me know, I'll
be happy rework this series). So this just adds a simple per namespace
thread which checks for the size change and generates AEN when needed.

I'm open to use different thread model.

Without second Patch:-
-------------------
1. Build null_blk based XFS file system :-
[ 3854.406993] XFS (nullb0): Unmounting Filesystem
[ 3865.803947] null_blk: module loaded
[ 3869.870398] XFS (nullb0): Mounting V5 Filesystem
[ 3869.873697] XFS (nullb0): Ending clean mount
[ 3869.873826] xfs filesystem being mounted at /mnt/backend supports timestamps until 2038 (0x7fffffff)
2. Add nvmet file backed namespaces and connect to host with nvme_loop.ko.
[ 3889.258370] nvmet: adding nsid 1 to subsystem fs
[ 3890.276583] nvmet: creating controller 1 for subsystem fs for NQN nqn.2014-08.org.nvmexpress:uuid:f67d5ee8-b2df-44a3-81f2-c827705db7e2.
[ 3890.277202] nvme nvme1: creating 64 I/O queues.
[ 3890.293196] nvme nvme1: new ctrl: "fs"
[ 3890.295363] nvme1n1: detected capacity change from 0 to 10737418240
# dmesg -c 
# dmesg -c 
3. Verify the backend file information :-
# cat /sys/kernel/config/nvmet/subsystems/fs/namespaces/1/device_path
/mnt/backend/nvme1n1
# file /mnt/backend/nvme1n1
/mnt/backend/nvme1n1: data
# ls -lrth /mnt/backend/nvme1n1
-rw-r--r--. 1 root root 10G Apr  7 20:51 /mnt/backend/nvme1n1
4. Change the size of the backend file :-
# truncate -s 5G /mnt/backend/nvme1n1
# dmesg -c 
# dmesg -c 
5. Check the host size block device size.
# lsblk | grep nvme1n1
nvme1n1           259:11   0   10G  0 disk <-----------------------
#

With the second patch:-
--------------------
1. Build null_blk based XFS file system :-
[ 3979.363104] null_blk: module loaded
[ 3983.418615] XFS (nullb0): Mounting V5 Filesystem
[ 3983.421752] XFS (nullb0): Ending clean mount
[ 3983.421887] xfs filesystem being mounted at /mnt/backend supports timestamps until 2038 (0x7fffffff)
2. Add nvmet file backed namespaces and connect to host with nvme_loop.ko.
[ 4002.801019] nvmet: adding nsid 1 to subsystem fs
[ 4003.818375] nvmet: creating controller 1 for subsystem fs for NQN nqn.2014-08.org.nvmexpress:uuid:3c1d7670-7f65-41b5-b488-9249220b76e8.
[ 4003.819076] nvme nvme1: creating 64 I/O queues.
[ 4003.834461] nvme nvme1: new ctrl: "fs"
[ 4003.837448] nvme1n1: detected capacity change from 0 to 10737418240
# dmesg -c 
3. Verify the backend file information :-
# cat /sys/kernel/config/nvmet/subsystems/fs/namespaces/1/device_path
/mnt/backend/nvme1n1
# file /mnt/backend/nvme1n1
/mnt/backend/nvme1n1: data
# ls -lrth /mnt/backend/nvme1n1
-rw-r--r--. 1 root root 10G Apr  7 20:53 /mnt/backend/nvme1n1
4. Change the size of the backend file :-
# truncate -s 5G /mnt/backend/nvme1n1
# dmesg -c 
[ 4041.181065] nvme nvme1: rescanning namespaces.
[ 4041.529446] nvme1n1: detected capacity change from 10737418240 to 5368709120
# lsblk | grep nvme1n1
5. Check the host size block device size.
nvme1n1           259:12   0    5G  0 disk <-----------------------
# 

Regards,
Chaitanya

Changes from V1 :-

1. Just use ns->size = i_size_read(ns->bdev->bd_inode) in the
   nvmet_bdev_ns_revalidate().
2. Remove !file check and use fill line for vfs_getattr() call in
   nvmet_file_ns_revalidate().
3. Add wrapper nvmet_ns_revalidate().
4. Add 2nd patch to introduce per namespace thread to monitor the size by
   calling nvmet_ns_revalidate() and generate AEN when size change is
   detected.  
5. Change return type of the nvmet_[bdev|file]ns_revalidate() from void
   to bool.

Anthony Iliopoulos (1):
  nvmet: add ns revalidation support

Chaitanya Kulkarni (1):
  nvmet: add per ns thread to generate AEN

 drivers/nvme/target/admin-cmd.c   |  2 ++
 drivers/nvme/target/core.c        | 34 +++++++++++++++++++++++++++++++
 drivers/nvme/target/io-cmd-bdev.c | 12 +++++++++++
 drivers/nvme/target/io-cmd-file.c | 16 +++++++++++++++
 drivers/nvme/target/nvmet.h       |  4 ++++
 5 files changed, 68 insertions(+)

-- 
2.22.1



[-- Attachment #2: Type: text/plain, Size: 158 bytes --]

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

[PATCH V2 1/2] nvmet: add ns revalidation support

[Chaitanya Kulkarni]

From: Anthony Iliopoulos <ailiop@suse.com>

Add support for detecting capacity changes on nvmet blockdev and file
backed namespaces. This allows for emulating and testing online resizing
of nvme devices and filesystems on top.

Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
[Fix comments posted on V1]
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/admin-cmd.c   |  5 +++++
 drivers/nvme/target/io-cmd-bdev.c |  5 +++++
 drivers/nvme/target/io-cmd-file.c | 11 +++++++++++
 drivers/nvme/target/nvmet.h       |  2 ++
 4 files changed, 23 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 72a7e41f3018..ba01d8e6e9c9 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -468,6 +468,11 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	if (!ns)
 		goto done;
 
+	if (ns->bdev)
+		nvmet_bdev_ns_revalidate(ns);
+	else
+		nvmet_file_ns_revalidate(ns);
+
 	/*
 	 * nuse = ncap = nsze isn't always true, but we have no way to find
 	 * that out from the underlying device.
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index ea0e596be15d..0427e040e3dd 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -75,6 +75,11 @@ void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
 	}
 }
 
+void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
+{
+	ns->size = i_size_read(ns->bdev->bd_inode);
+}
+
 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 cd5670b83118..14364383d2fe 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -80,6 +80,17 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 	return ret;
 }
 
+void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
+{
+	struct kstat stat;
+
+	if (vfs_getattr(&ns->file->f_path, &stat, STATX_SIZE,
+			AT_STATX_FORCE_SYNC))
+		return;
+
+	ns->size = stat.size;
+}
+
 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 eda28b22a2c8..a3ffdcca4efb 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -487,6 +487,8 @@ void nvmet_file_ns_disable(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);
+void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns);
+void nvmet_file_ns_revalidate(struct nvmet_ns *ns);
 
 static inline u32 nvmet_rw_len(struct nvmet_req *req)
 {
-- 
2.22.1


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

[PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Chaitanya Kulkarni]

The change of size detection on the target should generate the AEN to
the host. Right now there is no mechanism that allows us to add
callbacks for the block and file backend so that we will get the
notification for change of the size for block device and file backend.
This patch adds a simple per namespace thread that checks for the size
change and generates AEN when needed.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/admin-cmd.c   |  5 +----
 drivers/nvme/target/core.c        | 34 +++++++++++++++++++++++++++++++
 drivers/nvme/target/io-cmd-bdev.c |  9 +++++++-
 drivers/nvme/target/io-cmd-file.c | 15 +++++++++-----
 drivers/nvme/target/nvmet.h       |  6 ++++--
 5 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ba01d8e6e9c9..1e4ce77f1ac4 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -468,10 +468,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	if (!ns)
 		goto done;
 
-	if (ns->bdev)
-		nvmet_bdev_ns_revalidate(ns);
-	else
-		nvmet_file_ns_revalidate(ns);
+	nvmet_ns_revalidate(ns);
 
 	/*
 	 * nuse = ncap = nsze isn't always true, but we have no way to find
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 576de773b4db..d851b8da0ef0 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -9,6 +9,7 @@
 #include <linux/rculist.h>
 #include <linux/pci-p2pdma.h>
 #include <linux/scatterlist.h>
+#include <linux/delay.h>
 
 #define CREATE_TRACE_POINTS
 #include "trace.h"
@@ -514,6 +515,33 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
 		ns->nsid);
 }
 
+bool nvmet_ns_revalidate(struct nvmet_ns *ns)
+{
+	if (ns->bdev)
+		return nvmet_bdev_ns_revalidate(ns);
+
+	return nvmet_file_ns_revalidate(ns);
+}
+
+static int nvmet_ns_resize_thread(void *data)
+{
+	struct nvmet_ns *ns = (struct nvmet_ns *) data;
+
+	while (!kthread_should_park()) {
+		if (nvmet_ns_revalidate(ns)) {
+			mutex_lock(&ns->subsys->lock);
+			nvmet_ns_changed(ns->subsys, ns->nsid);
+			mutex_unlock(&ns->subsys->lock);
+		}
+		schedule();
+		/* XXX: use better sleep wakeup mechanism */
+		msleep(100);
+	}
+
+	kthread_parkme();
+	return 0;
+}
+
 int nvmet_ns_enable(struct nvmet_ns *ns)
 {
 	struct nvmet_subsys *subsys = ns->subsys;
@@ -572,6 +600,10 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
 
 	nvmet_ns_changed(subsys, ns->nsid);
 	ns->enabled = true;
+	ns->resize_thread = kthread_create(nvmet_ns_resize_thread, ns,
+					  "nvmet_ns_resize%s/%d",
+					  ns->device_path, ns->nsid);
+	wake_up_process(ns->resize_thread);
 	ret = 0;
 out_unlock:
 	mutex_unlock(&subsys->lock);
@@ -593,6 +625,8 @@ void nvmet_ns_disable(struct nvmet_ns *ns)
 	if (!ns->enabled)
 		goto out_unlock;
 
+	kthread_park(ns->resize_thread);
+	kthread_stop(ns->resize_thread);
 	ns->enabled = false;
 	list_del_rcu(&ns->dev_link);
 	if (ns->nsid == subsys->max_nsid)
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index 0427e040e3dd..e4791e57d397 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -75,9 +75,16 @@ void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
 	}
 }
 
-void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
+bool nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
 {
+	bool change;
+
+	mutex_lock(&ns->subsys->lock);
+	change = ns->size != i_size_read(ns->bdev->bd_inode) ? true : false;
 	ns->size = i_size_read(ns->bdev->bd_inode);
+	mutex_unlock(&ns->subsys->lock);
+
+	return change;
 }
 
 static u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts)
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 14364383d2fe..01d3d150585d 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -80,15 +80,20 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 	return ret;
 }
 
-void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
+bool nvmet_file_ns_revalidate(struct nvmet_ns *ns)
 {
+	struct path *f_path = &ns->file->f_path;
+	bool change = false;
 	struct kstat stat;
 
-	if (vfs_getattr(&ns->file->f_path, &stat, STATX_SIZE,
-			AT_STATX_FORCE_SYNC))
-		return;
+	mutex_lock(&ns->subsys->lock);
+	if (vfs_getattr(f_path, &stat, STATX_SIZE, AT_STATX_FORCE_SYNC) == 0) {
+		change = ns->size != stat.size ? true : false;
+		ns->size = stat.size;
+	}
+	mutex_unlock(&ns->subsys->lock);
 
-	ns->size = stat.size;
+	return change;
 }
 
 static void nvmet_file_init_bvec(struct bio_vec *bv, struct scatterlist *sg)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index a3ffdcca4efb..3236eae00cae 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -54,6 +54,7 @@ struct nvmet_ns {
 	struct percpu_ref	ref;
 	struct block_device	*bdev;
 	struct file		*file;
+	struct task_struct	*resize_thread;
 	bool			readonly;
 	u32			nsid;
 	u32			blksize_shift;
@@ -487,8 +488,9 @@ void nvmet_file_ns_disable(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);
-void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns);
-void nvmet_file_ns_revalidate(struct nvmet_ns *ns);
+bool nvmet_bdev_ns_revalidate(struct nvmet_ns *ns);
+bool nvmet_file_ns_revalidate(struct nvmet_ns *ns);
+bool nvmet_ns_revalidate(struct nvmet_ns *ns);
 
 static inline u32 nvmet_rw_len(struct nvmet_req *req)
 {
-- 
2.22.1


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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Sagi Grimberg]

On 4/7/20 9:16 PM, Chaitanya Kulkarni wrote:
> The change of size detection on the target should generate the AEN to
> the host. Right now there is no mechanism that allows us to add
> callbacks for the block and file backend so that we will get the
> notification for change of the size for block device and file backend.
> This patch adds a simple per namespace thread that checks for the size
> change and generates AEN when needed.

kthread? per ns?! I really don't think this is the way to go Chaitanya..

I'd suggest to expose a revalidate configfs attribute and have nvmetcli
install a udev rule that triggers a write to this attribute. Balbir
already got the udev notification for disk resize (see
set_capacity_revalidate_and_notify).

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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Chaitanya Kulkarni]

On 4/7/20 10:33 PM, Sagi Grimberg wrote:
>
> On 4/7/20 9:16 PM, Chaitanya Kulkarni wrote:
>> The change of size detection on the target should generate the AEN to
>> the host. Right now there is no mechanism that allows us to add
>> callbacks for the block and file backend so that we will get the
>> notification for change of the size for block device and file backend.
>> This patch adds a simple per namespace thread that checks for the size
>> change and generates AEN when needed.
> kthread? per ns?! I really don't think this is the way to go Chaitanya..

I hacked it up quickly, may be we can use a global kthread which does period

susbsys and ns scan, but AEN needs to be issued to the host either way.

> I'd suggest to expose a revalidate configfs attribute and have nvmetcli
> install a udev rule that triggers a write to this attribute. Balbir
> already got the udev notification for disk resize (see
> set_capacity_revalidate_and_notify).

Okay we can do that. So you are saying generate the AEN from configs
attribute.

Christoph, do you have any suggestions/preference ? otherwise I'll send V3.

Please confirm.



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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Chaitanya Kulkarni]

On 4/7/20 10:43 PM, Chaitanya Kulkarni wrote:
> On 4/7/20 10:33 PM, Sagi Grimberg wrote:
>> On 4/7/20 9:16 PM, Chaitanya Kulkarni wrote:
>>> The change of size detection on the target should generate the AEN to
>>> the host. Right now there is no mechanism that allows us to add
>>> callbacks for the block and file backend so that we will get the
>>> notification for change of the size for block device and file backend.
>>> This patch adds a simple per namespace thread that checks for the size
>>> change and generates AEN when needed.
>> kthread? per ns?! I really don't think this is the way to go Chaitanya..
> I hacked it up quickly, may be we can use a global kthread which does period
>
> susbsys and ns scan, but AEN needs to be issued to the host either way.
>
BTW I'd like to know what is the downside of the having a global kthread
approach

for subsys and their ns scanning to generate AER when re-validate ?



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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Sagi Grimberg]

>> On 4/7/20 9:16 PM, Chaitanya Kulkarni wrote:
>>> The change of size detection on the target should generate the AEN to
>>> the host. Right now there is no mechanism that allows us to add
>>> callbacks for the block and file backend so that we will get the
>>> notification for change of the size for block device and file backend.
>>> This patch adds a simple per namespace thread that checks for the size
>>> change and generates AEN when needed.
>> kthread? per ns?! I really don't think this is the way to go Chaitanya..
> 
> I hacked it up quickly, may be we can use a global kthread which does period
> 
> susbsys and ns scan, but AEN needs to be issued to the host either way.

Seems like such an overkill to dedicate a full kthread for this...

But maybe it won't be such a strange thing to dedicate a maintenance
thread that would do stuff like this. My main issue with his is that it
is polling which to me seems like a band-aid really...

>> I'd suggest to expose a revalidate configfs attribute and have nvmetcli
>> install a udev rule that triggers a write to this attribute. Balbir
>> already got the udev notification for disk resize (see
>> set_capacity_revalidate_and_notify).
> 
> Okay we can do that. So you are saying generate the AEN from configs
> attribute.

Yes.. that won't work for file backed namespaces though...

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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Chaitanya Kulkarni]

On 04/07/2020 10:55 PM, Sagi Grimberg wrote:
>
>>> On 4/7/20 9:16 PM, Chaitanya Kulkarni wrote:
>>>> The change of size detection on the target should generate the AEN to
>>>> the host. Right now there is no mechanism that allows us to add
>>>> callbacks for the block and file backend so that we will get the
>>>> notification for change of the size for block device and file backend.
>>>> This patch adds a simple per namespace thread that checks for the size
>>>> change and generates AEN when needed.
>>> kthread? per ns?! I really don't think this is the way to go Chaitanya..
>>
>> I hacked it up quickly, may be we can use a global kthread which does period
>>
>> susbsys and ns scan, but AEN needs to be issued to the host either way.
>
> Seems like such an overkill to dedicate a full kthread for this...
>
> But maybe it won't be such a strange thing to dedicate a maintenance
> thread that would do stuff like this. My main issue with his is that it
> is polling which to me seems like a band-aid really...

It is, but having a lightweight maintenance thread is very simple way
to implement this for bdev and file without involving userspace or fancy
hacks.

What are your concerns about polling (as in sleep() and schedule()
calls wrt to the code)? Can you please elaborate ?

>
>>> I'd suggest to expose a revalidate configfs attribute and have nvmetcli
>>> install a udev rule that triggers a write to this attribute. Balbir
>>> already got the udev notification for disk resize (see
>>> set_capacity_revalidate_and_notify).
>>
>> Okay we can do that. So you are saying generate the AEN from configs
>> attribute.
>
> Yes.. that won't work for file backed namespaces though...
>

Isn't that defeats the purpose of having a file backend which provides
more flexibility to the user ?

I think we should at least try an implementation which has global
maintenance thread, measure the impact and then discard it before going
to userspace.

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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Sagi Grimberg]

>> But maybe it won't be such a strange thing to dedicate a maintenance
>> thread that would do stuff like this. My main issue with his is that it
>> is polling which to me seems like a band-aid really...
> 
> It is, but having a lightweight maintenance thread is very simple way
> to implement this for bdev and file

Simple at first sight, my main concern is what the impact may be...

> without involving userspace or fancy hacks.

Wouldn't call these hacks really, we have several examples of
disk resize triggering events to usespace.

> What are your concerns about polling (as in sleep() and schedule()
> calls wrt to the code)? Can you please elaborate ?

What is the right polling scheme? what is the impact on the system 
performance? What happens when you have a lot of namespaces?
I view the polling scheme a band-aid resulting for lack of event driven
infrastructure for disk resize.

>>>> I'd suggest to expose a revalidate configfs attribute and have nvmetcli
>>>> install a udev rule that triggers a write to this attribute. Balbir
>>>> already got the udev notification for disk resize (see
>>>> set_capacity_revalidate_and_notify).
>>>
>>> Okay we can do that. So you are saying generate the AEN from configs
>>> attribute.
>>
>> Yes.. that won't work for file backed namespaces though...
>>
> 
> Isn't that defeats the purpose of having a file backend which provides
> more flexibility to the user ?

We could have something that leverages fsnotify to address files...

> I think we should at least try an implementation which has global
> maintenance thread, measure the impact and then discard it before going
> to userspace.

I'm not too fond of the idea in general, but I could be convinced
otherwise. I'm interested to learn what others think.

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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Chaitanya Kulkarni]

On 04/09/2020 09:07 PM, Sagi Grimberg wrote:
>
>>> But maybe it won't be such a strange thing to dedicate a maintenance
>>> thread that would do stuff like this. My main issue with his is that it
>>> is polling which to me seems like a band-aid really...
>>
>> It is, but having a lightweight maintenance thread is very simple way
>> to implement this for bdev and file
>
> Simple at first sight, my main concern is what the impact may be...
>
I've posted a new version with fine grained tunables. This should
make it easy for user to use in their environment, instead of forcing
it.
>> without involving userspace or fancy hacks.
>
> Wouldn't call these hacks really, we have several examples of
> disk resize triggering events to usespace.
>
Yes, not fancy, bad choice of word. My point is let us try in-kernel
approach and avoid adding new configfs attributes if possible, if
whatever reason it fails we can always add userspace.
>> What are your concerns about polling (as in sleep() and schedule()
>> calls wrt to the code)? Can you please elaborate ?
>
> What is the right polling scheme? what is the impact on the system
> performance? What happens when you have a lot of namespaces?
> I view the polling scheme a band-aid resulting for lack of event driven
> infrastructure for disk resize.
>
Yes is, the right thing to do is have f/inotify for file and in-kernel
gendisk event framework for bdev. We don't have such a scheme. I did 
spend some time into how in-kernel disk events are implemented, it will
take some time to get that it upstream. I'm working on that. Meanwhile
this will allow us fix existing problem.
>>>>> I'd suggest to expose a revalidate configfs attribute and have nvmetcli
>>>>> install a udev rule that triggers a write to this attribute. Balbir
>>>>> already got the udev notification for disk resize (see
>>>>> set_capacity_revalidate_and_notify).
>>>>
>>>> Okay we can do that. So you are saying generate the AEN from configs
>>>> attribute.
>>>
>>> Yes.. that won't work for file backed namespaces though...
True.
>>>
>>
>> Isn't that defeats the purpose of having a file backend which provides
>> more flexibility to the user ?
>
> We could have something that leverages fsnotify to address files...
>
True and for bdev we need to update in-kernel disk event framework.
I've already raised this approach when reviewing Balbilr's series, I've
started looking into this but will take some time.
>> I think we should at least try an implementation which has global
>> maintenance thread, measure the impact and then discard it before going
>> to userspace.
>
> I'm not too fond of the idea in general, but I could be convinced
> otherwise. I'm interested to learn what others think.
>
Lets start the discussion on V3 and see how that goes.


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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Anthony Iliopoulos]

On Tue, Apr 07, 2020 at 10:33:07PM -0700, Sagi Grimberg wrote:
> 
> 
> On 4/7/20 9:16 PM, Chaitanya Kulkarni wrote:
> > The change of size detection on the target should generate the AEN to
> > the host. Right now there is no mechanism that allows us to add
> > callbacks for the block and file backend so that we will get the
> > notification for change of the size for block device and file backend.
> > This patch adds a simple per namespace thread that checks for the size
> > change and generates AEN when needed.
> 
> kthread? per ns?! I really don't think this is the way to go Chaitanya..
> 
> I'd suggest to expose a revalidate configfs attribute and have nvmetcli
> install a udev rule that triggers a write to this attribute. Balbir
> already got the udev notification for disk resize (see
> set_capacity_revalidate_and_notify).

So in the original design I had assumed that the burden of AEN
generation falls on userspace, given that the resizing of either a block
device or a file is something that userspace would initiate anyway. As
such, the same process could simply issue a nvme rescan ioctl to effect
the capacity changes (this is why in the original patch I had wired up
revalidate with identify).

With the udev notification patchset in place one could trigger the
rescan for bdev-backed targets (e.g. do a nvme ns-rescan, or toggle
the existing nvmet ns enable configfs attr to induce the change). Some
revalidation actions will necessarily remain in userspace, e.g. in
blktests where loopback bdev is used, it requires a LOOP_SET_CAPACITY
ioctl to reflect the changes (losetup -c).

For file-backed targets, userspace may leverage inotify to achieve the
same. Otherwise a size change notifier would be required to trigger a
rescan (e.g. see fs/attr.c:notify_change()). The issue though here is
that there is no immediate way to associate a filp to a namespace, which
means that the handler would need to iterate all controllers and every
namespace to match it - not very nice, and probably isn't worth the
complexity.

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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Chaitanya Kulkarni]

On 04/08/2020 02:55 AM, Anthony Iliopoulos wrote:
> So in the original design I had assumed that the burden of AEN
> generation falls on userspace, given that the resizing of either a block
> device or a file is something that userspace would initiate anyway. As
> such, the same process could simply issue a nvme rescan ioctl to effect
> the capacity changes (this is why in the original patch I had wired up
> revalidate with identify).
>
> With the udev notification patchset in place one could trigger the
> rescan for bdev-backed targets (e.g. do a nvme ns-rescan, or toggle
> the existing nvmet ns enable configfs attr to induce the change). Some
> revalidation actions will necessarily remain in userspace, e.g. in
> blktests where loopback bdev is used, it requires a LOOP_SET_CAPACITY
> ioctl to reflect the changes (losetup -c).
>

> For file-backed targets, userspace may leverage inotify to achieve the
> same. Otherwise a size change notifier would be required to trigger a
> rescan (e.g. see fs/attr.c:notify_change()). The issue though here is
> that there is no immediate way to associate a filp to a namespace, which
> means that the handler would need to iterate all controllers and every
> namespace to match it - not very nice, and probably isn't worth the
> complexity.
>

We need this for file back-end since it provides user flexibility about
managing the ns.

I think we should at-least try global maintenance thread, if that didn't 
work out we can always get back to the user-space solution.




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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Anthony Iliopoulos]

On Wed, Apr 08, 2020 at 03:25:38PM +0000, Chaitanya Kulkarni wrote:
> On 04/08/2020 02:55 AM, Anthony Iliopoulos wrote:
> > So in the original design I had assumed that the burden of AEN
> > generation falls on userspace, given that the resizing of either a block
> > device or a file is something that userspace would initiate anyway. As
> > such, the same process could simply issue a nvme rescan ioctl to effect
> > the capacity changes (this is why in the original patch I had wired up
> > revalidate with identify).
> >
> > With the udev notification patchset in place one could trigger the
> > rescan for bdev-backed targets (e.g. do a nvme ns-rescan, or toggle
> > the existing nvmet ns enable configfs attr to induce the change). Some
> > revalidation actions will necessarily remain in userspace, e.g. in
> > blktests where loopback bdev is used, it requires a LOOP_SET_CAPACITY
> > ioctl to reflect the changes (losetup -c).
> >
> 
> > For file-backed targets, userspace may leverage inotify to achieve the
> > same. Otherwise a size change notifier would be required to trigger a
> > rescan (e.g. see fs/attr.c:notify_change()). The issue though here is
> > that there is no immediate way to associate a filp to a namespace, which
> > means that the handler would need to iterate all controllers and every
> > namespace to match it - not very nice, and probably isn't worth the
> > complexity.
> >
> 
> We need this for file back-end since it provides user flexibility about
> managing the ns.
> 
> I think we should at-least try global maintenance thread, if that didn't 
> work out we can always get back to the user-space solution.

Sure, but please note that this doesn't cover all use-cases, notably the
bdev-over-loopback example that I mentioned (truncate on the file that
backs loopback won't be reflected to its bdev w/o losetup -c, so the
maintenance thread will never pick up any change). Are there any
scenarios using non-loopback-based backing bdevs that admit resizing?


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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Chaitanya Kulkarni]

On 04/08/2020 10:08 AM, Anthony Iliopoulos wrote:
> Sure, but please note that this doesn't cover all use-cases, notably the
> bdev-over-loopback example that I mentioned (truncate on the file that
> backs loopback won't be reflected to its bdev w/o losetup -c, so the
> maintenance thread will never pick up any change). Are there any
> scenarios using non-loopback-based backing bdevs that admit resizing?

Not that I'm aware of, if anyone is please let me know.

Regarding loopback, are you saying that on the target side you are
using loop device backed by the file as a namespace device_path ?

Can you please describe the use case for loopback ?




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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Anthony Iliopoulos]

On Wed, Apr 08, 2020 at 11:28:01PM +0000, Chaitanya Kulkarni wrote:
> On 04/08/2020 10:08 AM, Anthony Iliopoulos wrote:
> > Sure, but please note that this doesn't cover all use-cases, notably the
> > bdev-over-loopback example that I mentioned (truncate on the file that
> > backs loopback won't be reflected to its bdev w/o losetup -c, so the
> > maintenance thread will never pick up any change). Are there any
> > scenarios using non-loopback-based backing bdevs that admit resizing?
> 
> Not that I'm aware of, if anyone is please let me know.
> 
> Regarding loopback, are you saying that on the target side you are
> using loop device backed by the file as a namespace device_path ?
> 
> Can you please describe the use case for loopback ?

I was primarily thinking of blktests, where loop blockdev is used for
testing bdev-backed namespaces. For testing filesystem resizing on top
of nvmet (which was my use-case), it really makes no difference whether
the ns is bdev or file-backed.

I'm just pointing out that if the loop blockdev is the only kind of
resizable backing used for nvmet-bdev, then the maintenance kthread will
only solve half of the problem: userspace would still need to issue a
capacity change ioctl to the loop blockdev after resizing its underlying
file, before the kthread could see any changes.

But I suppose there may exist other use-cases I am unaware of, given
that if the ultimate backing is a file, then connecting this to a loop
blockdev and doing a bdev-backed nvmet instead of a file-backed one
directly to the backing file, doesn't bring any benefit.

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

Re: [PATCH V2 2/2] nvmet: add per ns thread to generate AEN

[Chaitanya Kulkarni]

On 04/09/2020 02:41 AM, Anthony Iliopoulos wrote:
> I was primarily thinking of blktests, where loop blockdev is used for
> testing bdev-backed namespaces. For testing filesystem resizing on top
> of nvmet (which was my use-case), it really makes no difference whether
> the ns is bdev or file-backed.
>
For testing we can getaway with file backend, it will exercise same
code path.
> I'm just pointing out that if the loop blockdev is the only kind of
> resizable backing used for nvmet-bdev, then the maintenance kthread will
> only solve half of the problem: userspace would still need to issue a
> capacity change ioctl to the loop blockdev after resizing its underlying
> file, before the kthread could see any changes.
>
For nvmet it expects generic block deivce or generic backend, we should
avoid to rely on such functionality if we can and this is for testing 
case I'd use filebackend than loop device, since it uses same code path,
and easy to setup test with minimum steps.
> But I suppose there may exist other use-cases I am unaware of, given
> that if the ultimate backing is a file, then connecting this to a loop
> blockdev and doing a bdev-backed nvmet instead of a file-backed one
> directly to the backing file, doesn't bring any benefit.
>
Yes, I don't see any benefit yet except adding another level of 
indirection. (and possibly crypto support, but that can be solved
by using ext4 and user based per file encryption).



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

Re: [PATCH V2 0/2] nvmet: add revalidate support

[Chaitanya Kulkarni]

On 4/7/20 9:16 PM, Chaitanya Kulkarni wrote:
> Hi,
>
> This patch series adds support for the NVMeOF target bdev-ns and
> file-ns size re-validation.
>
> The first patch adds bdev/file backend revalidation helpers which
> was posted by Anthony Iliopoulos. I've fixed the comments posted on V1
> by keeping the authorship of the patch.
>
> The 2nd patch is needed since the change of size detection on the target
> should generate the AEN to the host. Right now there is no mechanism
> that allows us to add callbacks for the block and file backend so that
> we will get the notification (if anyone knows please let me know, I'll
> be happy rework this series). So this just adds a simple per namespace
> thread which checks for the size change and generates AEN when needed.
>
> I'm open to use different thread model.
>

FYI I've generated this series on Linus's Linux 5.6-rc6 tag as my nvme
git repo

is messed up. If everything looks okay I'll re-base and send V3 on nvme tree

tomorrow.



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