From: Klaus Jensen <its@irrelevant.dk>
To: Dmitry Fomichev <dmitry.fomichev@wdc.com>
Cc: "Fam Zheng" <fam@euphon.net>, "Kevin Wolf" <kwolf@redhat.com>,
"Damien Le Moal" <damien.lemoal@wdc.com>,
qemu-block@nongnu.org, "Niklas Cassel" <niklas.cassel@wdc.com>,
"Klaus Jensen" <k.jensen@samsung.com>,
qemu-devel@nongnu.org, "Maxim Levitsky" <mlevitsk@redhat.com>,
"Alistair Francis" <alistair.francis@wdc.com>,
"Keith Busch" <kbusch@kernel.org>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>,
"Matias Bjorling" <matias.bjorling@wdc.com>
Subject: Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set
Date: Thu, 24 Sep 2020 23:07:51 +0200 [thread overview]
Message-ID: <20200924210751.GD1738917@apples.localdomain> (raw)
In-Reply-To: <20200923182021.3724-1-dmitry.fomichev@wdc.com>
[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]
On Sep 24 03:20, Dmitry Fomichev wrote:
> v3 -> v4
>
> - Fix bugs introduced in v2/v3 for QD > 1 operation. Now, all writes
> to a zone happen at the new write pointer variable, zone->w_ptr,
> that is advanced right after submitting the backend i/o. The existing
> zone->d.wp variable is updated upon the successful write completion
> and it is used for zone reporting. Some code has been split from
> nvme_finalize_zoned_write() function to a new function,
> nvme_advance_zone_wp().
>
Same approach that I've used, +1.
> - Make the code compile under mingw. Switch to using QEMU API for
> mmap/msync, i.e. memory_region...(). Since mmap is not available in
> mingw (even though there is mman-win32 library available on Github),
> conditional compilation is added around these calls to avoid
> undefined symbols under mingw. A better fix would be to add stub
> functions to softmmu/memory.c for the case when CONFIG_POSIX is not
> defined, but such change is beyond the scope of this patchset and it
> can be made in a separate patch.
>
Ewwww.
This feels like a hack or at the very least an abuse of the physical
memory management API.
If it really needs to be memory mapped, then I think a hostmem-based
approach similar to what Andrzej did for PMR is needed (I think that
will get rid of the CONFIG_POSIX ifdef at least, but still leave it
slightly tricky to get it to work on all platforms AFAIK). But really,
since we do not require memory semantics for this, then I think the
abstraction is fundamentally wrong.
I am, of course, blowing my own horn, since my implementation uses a
portable blockdev for this.
Another issue is the complete lack of endian conversions. Does it
matter? It depends. Will anyone ever use this on a big endian host and
move the meta data backing file to a little endian host? Probably not.
So does it really matter? Probably not, but it is cutting corners.
> - Make the list of review comments addressed in v2 of the series
> (see below).
>
Very detailed! Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2020-09-24 21:12 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-23 18:20 [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 01/14] hw/block/nvme: Report actual LBA data shift in LBAF Dmitry Fomichev
2020-09-24 12:12 ` Klaus Jensen
2020-09-23 18:20 ` [PATCH v4 02/14] hw/block/nvme: Add Commands Supported and Effects log Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 03/14] hw/block/nvme: Introduce the Namespace Types definitions Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 04/14] hw/block/nvme: Define trace events related to NS Types Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 05/14] hw/block/nvme: Add support for Namespace Types Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 06/14] hw/block/nvme: Add support for active/inactive namespaces Dmitry Fomichev
2020-09-24 12:12 ` Klaus Jensen
2020-09-24 18:17 ` Niklas Cassel
2020-09-24 18:55 ` Klaus Jensen
2020-09-24 19:40 ` Niklas Cassel
2020-09-23 18:20 ` [PATCH v4 07/14] hw/block/nvme: Make Zoned NS Command Set definitions Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 08/14] hw/block/nvme: Define Zoned NS Command Set trace events Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 09/14] hw/block/nvme: Support Zoned Namespace Command Set Dmitry Fomichev
2020-09-25 18:24 ` Klaus Jensen
2020-09-23 18:20 ` [PATCH v4 10/14] hw/block/nvme: Introduce max active and open zone limits Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 11/14] hw/block/nvme: Support Zone Descriptor Extensions Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 12/14] hw/block/nvme: Add injection of Offline/Read-Only zones Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 13/14] hw/block/nvme: Use zone metadata file for persistence Dmitry Fomichev
2020-09-23 18:20 ` [PATCH v4 14/14] hw/block/nvme: Document zoned parameters in usage text Dmitry Fomichev
2020-09-24 21:07 ` Klaus Jensen [this message]
2020-09-28 2:33 ` [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set Dmitry Fomichev
2020-09-28 6:36 ` Klaus Jensen
2020-09-28 21:25 ` Keith Busch
2020-09-28 22:54 ` Damien Le Moal
2020-09-29 10:46 ` Klaus Jensen
2020-09-29 11:13 ` Damien Le Moal
2020-09-29 17:44 ` Keith Busch
2020-09-29 15:43 ` Dmitry Fomichev
2020-09-29 16:36 ` Klaus Jensen
2020-09-29 17:29 ` Keith Busch
2020-09-29 18:00 ` Klaus Jensen
2020-09-29 18:15 ` Keith Busch
2020-09-29 18:18 ` Klaus Jensen
2020-09-29 18:17 ` Matias Bjorling
2020-09-29 18:36 ` Klaus Jensen
2020-09-29 19:42 ` Matias Bjorling
2020-09-29 15:42 ` Dmitry Fomichev
2020-09-29 18:39 ` Klaus Jensen
2020-09-29 19:22 ` Keith Busch
2020-09-29 19:53 ` Dmitry Fomichev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200924210751.GD1738917@apples.localdomain \
--to=its@irrelevant.dk \
--cc=alistair.francis@wdc.com \
--cc=damien.lemoal@wdc.com \
--cc=dmitry.fomichev@wdc.com \
--cc=fam@euphon.net \
--cc=k.jensen@samsung.com \
--cc=kbusch@kernel.org \
--cc=kwolf@redhat.com \
--cc=matias.bjorling@wdc.com \
--cc=mlevitsk@redhat.com \
--cc=niklas.cassel@wdc.com \
--cc=philmd@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).