Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces

[Klaus Birkelund]

On Fri, Jul 05, 2019 at 09:23:33AM +0200, Klaus Birkelund Jensen wrote:
> This adds support for multiple namespaces by introducing a new 'nvme-ns'
> device model. The nvme device creates a bus named from the device name
> ('id'). The nvme-ns devices then connect to this and registers
> themselves with the nvme device.
> 
> This changes how an nvme device is created. Example with two namespaces:
> 
>   -drive file=nvme0n1.img,if=none,id=disk1
>   -drive file=nvme0n2.img,if=none,id=disk2
>   -device nvme,serial=deadbeef,id=nvme0
>   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
>   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
> 
> A maximum of 256 namespaces can be configured.
> 
 
Well that was embarrasing.

This patch breaks nvme-test.c. Which I obviously did not run.

In my defense, the test doesn't do much currently, but I'll of course
fix the test for v2.

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces

[Daniel P. Berrangé]

On Fri, Jul 05, 2019 at 03:36:17PM +0200, Klaus Birkelund wrote:
> On Fri, Jul 05, 2019 at 09:23:33AM +0200, Klaus Birkelund Jensen wrote:
> > This adds support for multiple namespaces by introducing a new 'nvme-ns'
> > device model. The nvme device creates a bus named from the device name
> > ('id'). The nvme-ns devices then connect to this and registers
> > themselves with the nvme device.
> > 
> > This changes how an nvme device is created. Example with two namespaces:
> > 
> >   -drive file=nvme0n1.img,if=none,id=disk1
> >   -drive file=nvme0n2.img,if=none,id=disk2
> >   -device nvme,serial=deadbeef,id=nvme0
> >   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
> >   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
> > 
> > A maximum of 256 namespaces can be configured.
> > 
>  
> Well that was embarrasing.
> 
> This patch breaks nvme-test.c. Which I obviously did not run.
> 
> In my defense, the test doesn't do much currently, but I'll of course
> fix the test for v2.

That highlights a more serious problem.  This series changes the syntx
for configuring the nvme device in a way that is not backwards compatible.
So anyone who is using QEMU with NVME will be broken when they upgrade
to the next QEMU release.

I understand why you wanted to restructure things to have a separate
nvme-ns device, but there needs to be some backcompat support in there
for the existing syntax to avoid breaking current users IMHO.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces

[Klaus Birkelund]

On Fri, Jul 05, 2019 at 02:49:29PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jul 05, 2019 at 03:36:17PM +0200, Klaus Birkelund wrote:
> > On Fri, Jul 05, 2019 at 09:23:33AM +0200, Klaus Birkelund Jensen wrote:
> > > This adds support for multiple namespaces by introducing a new 'nvme-ns'
> > > device model. The nvme device creates a bus named from the device name
> > > ('id'). The nvme-ns devices then connect to this and registers
> > > themselves with the nvme device.
> > > 
> > > This changes how an nvme device is created. Example with two namespaces:
> > > 
> > >   -drive file=nvme0n1.img,if=none,id=disk1
> > >   -drive file=nvme0n2.img,if=none,id=disk2
> > >   -device nvme,serial=deadbeef,id=nvme0
> > >   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
> > >   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
> > > 
> > > A maximum of 256 namespaces can be configured.
> > > 
> >  
> > Well that was embarrasing.
> > 
> > This patch breaks nvme-test.c. Which I obviously did not run.
> > 
> > In my defense, the test doesn't do much currently, but I'll of course
> > fix the test for v2.
> 
> That highlights a more serious problem.  This series changes the syntx
> for configuring the nvme device in a way that is not backwards compatible.
> So anyone who is using QEMU with NVME will be broken when they upgrade
> to the next QEMU release.
> 
> I understand why you wanted to restructure things to have a separate
> nvme-ns device, but there needs to be some backcompat support in there
> for the existing syntax to avoid breaking current users IMHO.
> 
 
Hi Daniel,

I raised this issue previously. I suggested that we keep the drive
property for the nvme device and only accept either that or an nvme-ns
device to be configured (but not both).

That would keep backward compatibilty, but enforce the use of nvme-ns
for any setup that requires multiple namespaces.

Would that work?

Cheers,
Klaus

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces

[Daniel P. Berrangé]

On Fri, Jul 05, 2019 at 06:20:14PM +0200, Klaus Birkelund wrote:
> On Fri, Jul 05, 2019 at 02:49:29PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jul 05, 2019 at 03:36:17PM +0200, Klaus Birkelund wrote:
> > > On Fri, Jul 05, 2019 at 09:23:33AM +0200, Klaus Birkelund Jensen wrote:
> > > > This adds support for multiple namespaces by introducing a new 'nvme-ns'
> > > > device model. The nvme device creates a bus named from the device name
> > > > ('id'). The nvme-ns devices then connect to this and registers
> > > > themselves with the nvme device.
> > > > 
> > > > This changes how an nvme device is created. Example with two namespaces:
> > > > 
> > > >   -drive file=nvme0n1.img,if=none,id=disk1
> > > >   -drive file=nvme0n2.img,if=none,id=disk2
> > > >   -device nvme,serial=deadbeef,id=nvme0
> > > >   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
> > > >   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
> > > > 
> > > > A maximum of 256 namespaces can be configured.
> > > > 
> > >  
> > > Well that was embarrasing.
> > > 
> > > This patch breaks nvme-test.c. Which I obviously did not run.
> > > 
> > > In my defense, the test doesn't do much currently, but I'll of course
> > > fix the test for v2.
> > 
> > That highlights a more serious problem.  This series changes the syntx
> > for configuring the nvme device in a way that is not backwards compatible.
> > So anyone who is using QEMU with NVME will be broken when they upgrade
> > to the next QEMU release.
> > 
> > I understand why you wanted to restructure things to have a separate
> > nvme-ns device, but there needs to be some backcompat support in there
> > for the existing syntax to avoid breaking current users IMHO.
> > 
>  
> Hi Daniel,
> 
> I raised this issue previously. I suggested that we keep the drive
> property for the nvme device and only accept either that or an nvme-ns
> device to be configured (but not both).
> 
> That would keep backward compatibilty, but enforce the use of nvme-ns
> for any setup that requires multiple namespaces.
> 
> Would that work?

Yes, that would be viable, as an existing CLI arg usage would continue
to be supported as before.

We could also list the back compat syntax as a deprecation in the docs
(qemu-deprecated.texi) so that in a few releases in the future, we can
drop the old syntax and then use nvme-ns exclusively thereafter.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces

[Paul Durrant]

Hi Klaus,

  I may have missed something but are you planning a v2 of your nvme_ns series soon? I just ran into another issue with this version when trying to use non-consecutive nsid value; the code uses an array of ns indexed by the nsid, and using non-consecutive values causes it to wander off the end of the array when trying to look up an ns by nsid.

  Cheers,

    Paul

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces

[Klaus Jensen]

On Wed, Sep 25, 2019 at 11:33:45AM +0000, Paul Durrant wrote:
> Hi Klaus,
> 
>   I may have missed something but are you planning a v2 of your
>   nvme_ns series soon? I just ran into another issue with this version
>   when trying to use non-consecutive nsid value; the code uses an
>   array of ns indexed by the nsid, and using non-consecutive values
>   causes it to wander off the end of the array when trying to look up
>   an ns by nsid.
> 

Hi Paul,

Yes, v2 will be served momentarily!

The issue you are describing is fixed in my tree!


Thanks,
Klaus

RE: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces

[Paul Durrant]

> -----Original Message-----
> From: Klaus Jensen <its@irrelevant.dk>
> Sent: 25 September 2019 13:15
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; Paul Durrant <paul@xen.org>
> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces
> 
> On Wed, Sep 25, 2019 at 11:33:45AM +0000, Paul Durrant wrote:
> > Hi Klaus,
> >
> >   I may have missed something but are you planning a v2 of your
> >   nvme_ns series soon? I just ran into another issue with this version
> >   when trying to use non-consecutive nsid value; the code uses an
> >   array of ns indexed by the nsid, and using non-consecutive values
> >   causes it to wander off the end of the array when trying to look up
> >   an ns by nsid.
> >
> 
> Hi Paul,
> 
> Yes, v2 will be served momentarily!
> 

Excellent :-)

> The issue you are describing is fixed in my tree!
> 

Cool. I can work around it for the moment, but I'll move onto v2 as soon as you post it.

  Thanks,

    Paul