Re: [PATCH] nvmet: add revalidation support to bdev and file backed namespaces

From: Anthony Iliopoulos
Date: Mon Apr 06 2020 - 04:03:15 EST


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 <ailiop@xxxxxxxx>
>
> 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: 20191008122904.20438-1-m.malygin@xxxxxxxxx, 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!