Re: [PATCH v5 2/4] qcow2: add configurations for zoned format extension

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Mon, Oct 30, 2023 at 11:01:26PM +0800, Sam Li wrote:
> Hi Eric,
> 
> Eric Blake <eblake@redhat.com> 于2023年10月30日周一 22:53写道：
> >
> > On Mon, Oct 30, 2023 at 08:18:45PM +0800, Sam Li wrote:
> > > To configure the zoned format feature on the qcow2 driver, it
> > > requires settings as: the device size, zone model, zone size,
> > > zone capacity, number of conventional zones, limits on zone
> > > resources (max append bytes, max open zones, and max_active_zones).
> > >
> > > To create a qcow2 file with zoned format, use command like this:
> > > $ qemu-img create -f qcow2 test.qcow2 -o size=768M -o
> > > zone_size=64M -o zone_capacity=64M -o conventional_zones=0 -o
> > > max_append_bytes=4096 -o max_open_zones=0 -o max_active_zones=0
> > > -o zone_model=host-managed
> > >
> > > Signed-off-by: Sam Li <faithilikerun@gmail.com>
> > >
> > > fix config?
> >
> > Is this comment supposed to be part of the commit message?  If not,...
> 
> No...
> 
> >
> > > ---
> >
> > ...place it here under the divider, so 'git am' won't include it, if there is nothing further to change on this patch.
> >
> > >  block/qcow2.c                    | 205 ++++++++++++++++++++++++++++++-
> > >  block/qcow2.h                    |  37 +++++-
> > >  docs/interop/qcow2.txt           |  67 +++++++++-
> > >  include/block/block_int-common.h |  13 ++
> > >  qapi/block-core.json             |  45 ++++++-
> > >  5 files changed, 362 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > index aa01d9e7b5..cd53268ca7 100644
> > > --- a/block/qcow2.c
> > > +++ b/block/qcow2.c
> > > @@ -73,6 +73,7 @@ typedef struct {
> > >  #define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
> > >  #define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
> > >  #define  QCOW2_EXT_MAGIC_DATA_FILE 0x44415441
> > > +#define  QCOW2_EXT_MAGIC_ZONED_FORMAT 0x007a6264
> > >
> > >  static int coroutine_fn
> > >  qcow2_co_preadv_compressed(BlockDriverState *bs,
> > > @@ -210,6 +211,7 @@ qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> > >      uint64_t offset;
> > >      int ret;
> > >      Qcow2BitmapHeaderExt bitmaps_ext;
> > > +    Qcow2ZonedHeaderExtension zoned_ext;
> > >
> > >      if (need_update_header != NULL) {
> > >          *need_update_header = false;
> > > @@ -431,6 +433,63 @@ qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> > >              break;
> > >          }
> > >
> > > +        case QCOW2_EXT_MAGIC_ZONED_FORMAT:
> > > +        {
> > > +            if (ext.len != sizeof(zoned_ext)) {
> > > +                error_setg(errp, "zoned_ext: Invalid extension length");
> > > +                return -EINVAL;
> > > +            }
> >
> > Do we ever anticipate the struct growing in size in the future to add
> > further features?  Forcing the size to be constant, rather than a
> > minimum, may get in the way of clean upgrades to a future version of
> > the extension header.
> 
> The zoned extension could grow. So ext.len > sizeof(zoned_ext) -> invalid.

No, ext.len >= sizeof(zoned_ext) is valid. The program can extract the
zoned_ext fields that it knows about. Any additional fields are ignored
by the program because it doesn't know how to interpret them. ext.len <
sizeof(zoned_ext) is invalid because required fields are missing.

When zoned_ext is extended to add a new feature the first time, the code
is updated like this:

  if (ext.len < endof(zoned_ext, last_minimum_field)) {
      ...invalid...
  }

  ...handle minimum zoned_ext fields...

  if (ext.len >= endof(zoned_ext, additional_field)) {
      ...handle additional_field...
  }
  ...more additional fields...

This approach is often used by Linux ioctl(2) interfaces. It allows
extensions to the struct without breaking old programs that still use
shorter versions of the struct.

Only optional features can be added using this approach, so it's often
combined with a 'flags' field that indicates which mandatory features
userspace wants and the kernel has provided. That way the kernel can
reject ioctls that require a new feature that the old kernel doesn't
know and the kernel can fill in flags upon returning from ioctl(2) so
userspace knows which functionality was provided by the kernel. qcow2
has feature bits, so I don't think a 'flags' field is required.

Stefan

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