Re: [Qemu-devel] [PATCH 8/8] nvme: add an OpenChannel 2.0 NVMe device (ocssd)

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6889 bytes --]

On 5/17/19 3:42 AM, Klaus Birkelund Jensen wrote:
> This adds a new 'ocssd' block device that emulates an OpenChannel 2.0
> device. The device is backed by a new 'ocssd' block backend that is
> based on the raw format driver but includes a header that holds the
> device geometry and write data requirements. This new block backend is
> special in that the size is not specified explicitly but in terms of
> sector size, number of chunks, number of parallel units, etc. This
> called for the addition of the `no_size_required` field in `struct
> BlockDriver` to not fail image creation when the size parameter is
> missing.
> 
> The ocssd device is an individual device but shares a lot of code with
> the nvme device. Thus, some core functionality of nvme/nvme.c has been
> exported for use by nvme/ocssd.c.
> 
> Thank you to the following people for their contributions to the
> original qemu-nvme (github.com/OpenChannelSSD/qemu-nvme) implementation.
> 
>   Matias Bjørling <mb@lightnvm.io>
>   Javier González <javier@javigon.com>
>   Simon Andreas Frimann Lund <ocssd@safl.dk>
>   Hans Holmberg <hans@owltronix.com>
>   Jesper Devantier <contact@pseudonymous.me>
>   Young Tack Jin <youngtack.jin@circuitblvd.com>
> 
> Signed-off-by: Klaus Birkelund Jensen <klaus.jensen@cnexlabs.com>
> ---
>  MAINTAINERS                     |   14 +-
>  Makefile.objs                   |    1 +
>  block.c                         |    2 +-
>  block/Makefile.objs             |    2 +-
>  block/nvme.c                    |    2 +-
>  block/ocssd.c                   |  690 ++++++++
>  hw/block/Makefile.objs          |    2 +-
>  hw/block/{ => nvme}/nvme.c      |  192 ++-
>  hw/block/nvme/ocssd.c           | 2647 +++++++++++++++++++++++++++++++
>  hw/block/nvme/ocssd.h           |  140 ++
>  hw/block/nvme/trace-events      |  136 ++
>  hw/block/trace-events           |  109 --
>  include/block/block_int.h       |    3 +
>  include/block/nvme.h            |   12 +-
>  include/block/ocssd.h           |  231 +++
>  {hw => include/hw}/block/nvme.h |   61 +
>  include/hw/pci/pci_ids.h        |    2 +
>  qapi/block-core.json            |   47 +-
>  18 files changed, 4121 insertions(+), 172 deletions(-)
>  create mode 100644 block/ocssd.c
>  rename hw/block/{ => nvme}/nvme.c (94%)
>  create mode 100644 hw/block/nvme/ocssd.c
>  create mode 100644 hw/block/nvme/ocssd.h
>  create mode 100644 hw/block/nvme/trace-events
>  create mode 100644 include/block/ocssd.h
>  rename {hw => include/hw}/block/nvme.h (63%)

Feels big; are you sure this can't be split into smaller pieces to ease
review?

I'm focusing just on the qapi portions:


> +++ b/qapi/block-core.json
> @@ -113,6 +113,44 @@
>        'extents': ['ImageInfo']
>    } }
>  
> +##
> +# @ImageInfoSpecificOcssd:
> +#
> +# @num-ns: number of namespaces
> +#
> +# @namespaces: List of namespsaces

Inconsistent on whether you start with lower or upper case.

> +#
> +# Since: 4.1
> +##
> +{ 'struct': 'ImageInfoSpecificOcssd',
> +  'data': {
> +      'num-ns': 'int',
> +      'sector-size': 'int',
> +      'metadata-size': 'int',
> +      'namespaces': ['ImageInfoSpecificOcssdNS']

Is num-ns redundant information (that is, if I count the length of the
array in namespaces, I don't need num-ns)?

You failed to document sector-size and metadata-size.

> +  } }
> +
> +##
> +# @ImageInfoSpecificOcssdNS:
> +#
> +# @num-grp: number of groups
> +#
> +# @num-pu: number of parallel units per group
> +#
> +# @num-chk: number of chunks per parallel unit
> +#
> +# @num-sec: number of sectors per chunk

Why the abbreviation? Machines don't care if they have to pass something
slightly longer, and if it makes the job easier for the human reading
the machine traces, then spelling things out (such as 'num-groups'),
then abbreviation didn't buy us anything useful.

> +#
> +# Since: 3.1

You missed 3.1 by a long shot. The next release is 4.1.

> +##
> +{ 'struct': 'ImageInfoSpecificOcssdNS',
> +  'data': {
> +      'num-grp': 'int',
> +      'num-pu': 'int',
> +      'num-chk': 'int',
> +      'num-sec': 'int'
> +  } }
> +
>  ##
>  # @ImageInfoSpecific:
>  #
> @@ -124,6 +162,7 @@
>    'data': {
>        'qcow2': 'ImageInfoSpecificQCow2',
>        'vmdk': 'ImageInfoSpecificVmdk',
> +      'ocssd': 'ImageInfoSpecificOcssd',

Perhaps missing a doc comment that ocssd is new to 4.1; although it may
be implied by the documentation adding 'occsd' to the list of overall
block types elsewhere....

>        # If we need to add block driver specific parameters for
>        # LUKS in future, then we'll subclass QCryptoBlockInfoLUKS
>        # to define a ImageInfoSpecificLUKS
> @@ -282,7 +321,7 @@
>  # @drv: the name of the block format used to open the backing device. As of
>  #       0.14.0 this can be: 'blkdebug', 'bochs', 'cloop', 'cow', 'dmg',
>  #       'file', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> -#       'http', 'https', 'luks', 'nbd', 'parallels', 'qcow',
> +#       'http', 'https', 'luks', 'nbd', 'ocssd', 'parallels', 'qcow',
>  #       'qcow2', 'raw', 'vdi', 'vmdk', 'vpc', 'vvfat'
>  #       2.2: 'archipelago' added, 'cow' dropped
>  #       2.3: 'host_floppy' deprecated
> @@ -290,6 +329,7 @@
>  #       2.6: 'luks' added
>  #       2.8: 'replication' added, 'tftp' dropped
>  #       2.9: 'archipelago' dropped
> +#       4.1: 'ocssd' added

...here

>  #
>  # @backing_file: the name of the backing file (for copy-on-write)
>  #
> @@ -2815,8 +2855,8 @@
>    'data': [ 'blkdebug', 'blklogwrites', 'blkverify', 'bochs', 'cloop',
>              'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', 'gluster',
>              'host_cdrom', 'host_device', 'http', 'https', 'iscsi', 'luks',
> -            'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels', 'qcow',
> -            'qcow2', 'qed', 'quorum', 'raw', 'rbd',
> +            'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'ocssd', 'parallels',
> +            'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'rbd',
>              { 'name': 'replication', 'if': 'defined(CONFIG_REPLICATION)' },
>              'sheepdog',
>              'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
> @@ -3917,6 +3957,7 @@
>        'null-aio':   'BlockdevOptionsNull',
>        'null-co':    'BlockdevOptionsNull',
>        'nvme':       'BlockdevOptionsNVMe',
> +      'ocssd':      'BlockdevOptionsGenericFormat',

I guess you don't support qemu creating one of these devices yet?

>        'parallels':  'BlockdevOptionsGenericFormat',
>        'qcow2':      'BlockdevOptionsQcow2',
>        'qcow':       'BlockdevOptionsQcow',
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]