Re: [Qemu-devel] [Qemu-block] [RFC] nvme: how to support multiple namespaces

[Markus Armbruster]

Klaus Birkelund <address@hidden> writes:

> On Mon, Jun 24, 2019 at 12:18:45PM +0200, Kevin Wolf wrote:
>> Am 24.06.2019 um 10:01 hat Klaus Birkelund geschrieben:
>> > On Thu, Jun 20, 2019 at 05:37:24PM +0200, Laszlo Ersek wrote:
>> > > On 06/17/19 10:12, Klaus Birkelund wrote:
>> > > > Hi all,
>> > > > 
>> > > > I'm thinking about how to support multiple namespaces in the NVMe
>> > > > device. My first idea was to add a "namespaces" property array to the
>> > > > device that references blockdevs, but as Laszlo writes below, this 
>> > > > might
>> > > > not be the best idea. It also makes it troublesome to add per-namespace
>> > > > parameters (which is something I will be required to do for other
>> > > > reasons). Some of you might remember my first attempt at this that
>> > > > included adding a new block driver (derived from raw) that could be
>> > > > given certain parameters that would then be stored in the image. But I
>> > > > understand that this is a no-go, and I can see why.
>> > > > 
>> > > > I guess the optimal way would be such that the parameters was something
>> > > > like:
>> > > > 
>> > > >    -blockdev 
>> > > > raw,node-name=blk_ns1,file.driver=file,file.filename=blk_ns1.img
>> > > >    -blockdev 
>> > > > raw,node-name=blk_ns2,file.driver=file,file.filename=blk_ns2.img
>> > > >    -device nvme-ns,drive=blk_ns1,ns-specific-options 
>> > > > (nsfeat,mc,dlfeat)...
>> > > >    -device nvme-ns,drive=blk_ns2,...
>> > > >    -device nvme,...
>> > > > 
>> > > > My question is how to state the parent/child relationship between the
>> > > > nvme and nvme-ns devices. I've been looking at how ide and virtio does
>> > > > this, and maybe a "bus" is the right way to go?
>> > > 
>> > > I've added Markus to the address list, because of this question. No
>> > > other (new) comments from me on the thread starter at this time, just
>> > > keeping the full context.
>> > > 
>> > 
>> > Hi all,
>> > 
>> > I've succesfully implemented this by introducing a new 'nvme-ns' device
>> > model. The nvme device creates a bus named from the device id ('id'
>> > parameter) and the nvme-ns devices are then registered on this.
>> > 
>> > This results in an nvme device being creates like this (two namespaces
>> > example):
>> > 
>> >   -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
>> > 
>> > How does that look as a way forward?
>> 
>> This looks very similar to what other devices do (one bus controller
>> that has multiple devices on its but), so I like it.
>> 
>> The thing that is special here is that -device nvme is already a block
>> device by itself that can take a drive property. So how does this play
>> together? Can I choose to either specify a drive directly for the nvme
>> device or nvme-ns devices, but when I do both, I will get an error? What
>> happens if I don't specify a drive for nvme, but also don't add nvme-ns
>> devices?
>> 
>
> Hi Kevin,
>
> Yes, the nvme device is already a block device. My current patch removes
> that property from the nvme device. I guess this breaks backward
> compatibiltiy. We could accept a drive for the nvme device only if no
> nvme-ns devices are configured and connected on the bus.

Sounds awful :)

> I'm not entirely sure on the spec, but my gut tells me that an nvme
> device without any namespaces is technically a valid device, although it
> is a bit useless.

So maybe the device actually consists of a controller part (no drive
property) and namespace parts (one drive property each).

If yes, then the existing nvme device model is flawed.  Suggest to
deprecate and start over.  This should be possible without duplicating
code.

The alternative is bad magic, like the one you sketched above.  We
usually come to regret such magic.

Whether the controller device should be a composite device containing
the namespace parts is a separate question.

> I will post my patch (as part of a larger series) and we can discuss it
> there.

Yes, please.

> Thanks for the feedback!
>
> Klaus