Re: [PATCH 00/10] hw/block/nvme: namespace types and zoned namespaces

[Klaus Jensen]

On Jun 30 12:59, Niklas Cassel wrote:
> On Tue, Jun 30, 2020 at 12:01:29PM +0200, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Hi all,
> 
> Hello Klaus,
> 

Hi Niklas,

> > 
> >   * the controller uses timers to autonomously finish zones (wrt. FRL)
> 
> AFAICT, Dmitry's patches does this as well.
> 

Hmm, yeah. Something is going on at least. It's not really clear to me
why it works or what is happening with that admin completion queue
timer, but I'll dig through it.

> > 
> > I've been on paternity leave for a month, so I havn't been around to
> > review Dmitry's patches, but I have started that process now. I would
> > also be happy to work with Dmitry & Friends on merging our versions to
> > get the best of both worlds if it makes sense.
> > 
> > This series and all preparatory patch sets (the ones I've been posting
> > yesterday and today) are available on my GitHub[2]. Unfortunately
> > Patchew got screwed up in the middle of me sending patches and it never
> > picked up v2 of "hw/block/nvme: support multiple namespaces" because it
> > was getting late and I made a mistake with the CC's. So my posted series
> > don't apply according to Patchew, but they actually do if you follow the
> > Based-on's (... or just grab [2]).
> > 
> > 
> >   [1]: Message-Id: <20200617213415.22417-1-dmitry.fomichev@wdc.com>
> >   [2]: https://github.com/birkelund/qemu/tree/for-master/nvme
> > 
> > 
> > Based-on: <20200630043122.1307043-1-its@irrelevant.dk>
> > ("[PATCH 0/3] hw/block/nvme: bump to v1.4")
> 
> Is this the only patch series that this series depends on?
> 
> In the beginning of the cover letter, you mentioned
> "NVMe v1.4 mandatory support", "SGLs", "multiple namespaces",
> and "and mostly just overall clean up".
> 

No, its a string of series that all has a Based-on tag (that is, "[PATCH
0/3] hw/block/nvme: bump to v1.4" has another Based-on tag that points
to the dependency of that). The point was to have patchew nicely apply
everything, but it broke midway...

As Philippe pointed out, all of the patch sets are integrated in the
GitHub tree, applied to QEMU master.

> 
> I think that you have done a great job getting the NVMe
> driver out of a frankenstate, and made it compliant with
> a proper spec (NVMe 1.4).
> 
> I'm also a big fan of the refactoring so that the driver
> handles more than one namespace, and the new bus model.
> 

Well, thanks! :)

> I know that you first sent your
> "nvme: support NVMe v1.3d, SGLs and multiple namespaces"
> patch series July, last year.
> 
> Looking at your outstanding patch series on patchwork:
> https://patchwork.kernel.org/project/qemu-devel/list/?submitter=188679
> 
> (Feel free to correct me if I have misunderstood anything.)
> 
> I see that these are related to your patch series from July last year:
> hw/block/nvme: bump to v1.3
> hw/block/nvme: support scatter gather lists
> hw/block/nvme: support multiple namespaces
> hw/block/nvme: bump to v1.4
> 

Yeah this stuff has been around for a while so the history on patchwork
is a mess.

> 
> This patch series seems minor and could probably be merged immediately:
> hw/block/nvme: handle transient dma errors
> 

Sure, but it's nicer in combination with the previous series
("hw/block/nvme: AIO and address mapping refactoring"). What I /can/ do
is rip out "hw/block/nvme: allow multiple aios per command" as that
patch might require more time for reviews. The rest of that series are
clean ups and a couple of bug fixes.

> 
> This patch series looks a bit weird:
> hw/block/nvme: AIO and address mapping refactoring
> 
> Since it looks like a V1 post, and was first posted yesterday.
> However, 2 out of the 17 patches in are Acked-by: Keith.
> (Perhaps some of your previously posted patches was put inside
> this new patch series?)
> 

Yes that and reviewers requested a lot of separation, so basically the
patch set ballooned.

> 
> This patch series:
> hw/block/nvme: namespace types and zoned namespaces
> 
> Which was first posted today. Up until earlier today, we haven't seen
> any patches from you related to ZNS (only overall NVMe cleanups).
> Dmitry's ZNS patches have been on the list since 2020-06-16.
> 

Yeah, as I mentioned in my cover letter, I was on leave, so I wasn't
around for the big ZNS release day either. But, honestly, I think this
is irrelevant - code should be merged based on technical reasons (not
technicalities).

> 
> Just a friendly suggestion, how about:
> 
> 1) We get your
> 
> hw/block/nvme: bump to v1.3
> hw/block/nvme: support scatter gather lists
> hw/block/nvme: support multiple namespaces
> hw/block/nvme: bump to v1.4
> 
> patch series merged.
> 

Blowing my own horn here, but yeah, it seems like everyone would like to
see this merged.

> 2) We get Dmitry's patch series merged.
> 
> Shared 4:th) If there is any feature that you miss in Dmitry's patch series,
> perhaps you could send patches to add what you are missing.
>

Looks like the two version are pretty much on par in terms of features.

> Shared 4:th) Your other patch series:
> hw/block/nvme: AIO and address mapping refactoring could get merged.
> 

As stated above I think its only a single commit ("hw/block/nvme: allow
multiple aios per command") that is controversial in that series.

> 
> Please don't take this suggestion the wrong way, I'm simply trying
> to come up with a way to move forward from here.
> 

Absolutely - I totally get that you want to move forward with Dmitry's
series, but I'd like to finish my review before committing to anything.


Cheers,
Klaus