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 <[email protected]>
---
drivers/nvme/target/admin-cmd.c | 5 +++++
drivers/nvme/target/io-cmd-bdev.c | 10 ++++++++++
drivers/nvme/target/io-cmd-file.c | 14 ++++++++++++++
drivers/nvme/target/nvmet.h | 2 ++
4 files changed, 31 insertions(+)
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 9d6f75cfa77c..4c79aa804887 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -486,6 +486,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..1f8c00d68aa9 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -75,6 +75,16 @@ void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
}
}
+void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
+{
+ loff_t size;
+
+ size = i_size_read(ns->bdev->bd_inode);
+
+ if (ns->size != size)
+ ns->size = size;
+}
+
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..c102437db72a 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -80,6 +80,20 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
return ret;
}
+void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
+{
+ struct kstat stat;
+
+ if (!ns->file)
+ return;
+
+ 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 421dff3ea143..8b479d932a7b 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -498,6 +498,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.26.0
> +void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
> +{
> + loff_t size;
> +
> + size = i_size_read(ns->bdev->bd_inode);
> +
> + if (ns->size != size)
> + ns->size = size;
Why is the if useful?
> +}
> +
> 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..c102437db72a 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c
> @@ -80,6 +80,20 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
> return ret;
> }
>
> +void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
> +{
> + struct kstat stat;
> +
> + if (!ns->file)
> + return;
When is !ns->file expected?
On Thu, Apr 02, 2020 at 09:30:52PM +0200, Anthony Iliopoulos wrote:
> 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 <[email protected]>
I vaguely remember seeing a very similar patch before, is this a repost?
> +void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
> +{
> + loff_t size;
> +
> + size = i_size_read(ns->bdev->bd_inode);
> +
> + if (ns->size != size)
> + ns->size = size;
This can be:
ns->size = i_size_read(ns->bdev->bd_inode);
> +void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
> +{
> + struct kstat stat;
> +
> + if (!ns->file)
> + return;
Shouldn't this always be non-NULL?
> +
> + if (vfs_getattr(&ns->file->f_path,
> + &stat, STATX_SIZE, AT_STATX_FORCE_SYNC))
> + return;
Use up the full line:
if (vfs_getattr(&ns->file->f_path, &stat, STATX_SIZE,
AT_STATX_FORCE_SYNC))
Also shouldn't there be error handling? If we can't stat the file
the namespace is toast.
On 04/02/2020 12:31 PM, Anthony Iliopoulos wrote:
> +void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
> +{
> + loff_t size;
> +
> + size = i_size_read(ns->bdev->bd_inode);
> +
> + if (ns->size != size)
> + ns->size = size;
How about following one line which is still readable ?
ns->size = i_size_read(ns->bdev->bd_inode);
> +}
> +
On 04/02/2020 12:31 PM, Anthony Iliopoulos wrote:
> 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<[email protected]>
Since we are adding this code for testing I'd like to see test cases
for both bdev and file.
All the testcases are in blktests/test/nvme/ with nvme-loop for NVMeOF.
On 04/02/2020 12:31 PM, Anthony Iliopoulos wrote:
> +void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
> +{
> + struct kstat stat;
> +
> + if (!ns->file)
> + return;
No need for the above check.
> +
> + if (vfs_getattr(&ns->file->f_path,
> + &stat, STATX_SIZE, AT_STATX_FORCE_SYNC))
> + return;
Error handling needed ? What should we set ns->size if vfs_getattr
fails ? in case of error what nvmet_identify_ns() report success or a
failure ?
> +
> + ns->size = stat.size;
bdev checks the ns->size != size, why the same check is not here ?
Just remove the check from bdev and see my comment there.
> +}
On 04/02/2020 12:31 PM, Anthony Iliopoulos wrote:
> 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<[email protected]>
> ---
Please ignore my duplicate comments from other, I try not to read
other's comments before the review.
On Fri, Apr 03, 2020 at 08:43:31AM +0200, Christoph Hellwig wrote:
> On Thu, Apr 02, 2020 at 09:30:52PM +0200, Anthony Iliopoulos wrote:
> > 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 <[email protected]>
>
> I vaguely remember seeing a very similar patch before, is this a repost?
Not a repost, but you're right, apparently there was a similar patch
posted before: [email protected], which instead
triggers revalidation via configfs.
> > +void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns)
> > +{
> > + loff_t size;
> > +
> > + size = i_size_read(ns->bdev->bd_inode);
> > +
> > + if (ns->size != size)
> > + ns->size = size;
>
> This can be:
>
> ns->size = i_size_read(ns->bdev->bd_inode);
Fixed.
> > +void nvmet_file_ns_revalidate(struct nvmet_ns *ns)
> > +{
> > + struct kstat stat;
> > +
> > + if (!ns->file)
> > + return;
>
> Shouldn't this always be non-NULL?
Right, this would be unset only during nvmet_ns_disable, and by that
time the ns is off the list, so identify should never see this being
non-NULL. Removed.
> > +
> > + if (vfs_getattr(&ns->file->f_path,
> > + &stat, STATX_SIZE, AT_STATX_FORCE_SYNC))
> > + return;
>
> Use up the full line:
>
> if (vfs_getattr(&ns->file->f_path, &stat, STATX_SIZE,
> AT_STATX_FORCE_SYNC))
Fixed.
> Also shouldn't there be error handling? If we can't stat the file
> the namespace is toast.
Indeed, I think it makes sense to fail identify at that point with
NVME_SC_INVALID_NS.
If you'd rather go with this patch instead of the configfs approach,
I'll post a v2 with the fixes, and some associated blktests that
Chaitanya requested.
Thank you all for the reviews!