Re: [Qemu-devel] Use of PreallocMode in block drivers

[Stefano Garzarella]

On Wed, May 8, 2019 at 1:44 PM Markus Armbruster <address@hidden> wrote:
>
> Stefano Garzarella <address@hidden> writes:
>
> > On Tue, May 07, 2019 at 08:34:51AM +0200, Markus Armbruster wrote:
> >> Cc: Peter for a libvirt perspective.
> >>
> >> Stefano Garzarella <address@hidden> writes:
> >>
> >> > This patch adds the support of preallocation (off/full) for the RBD
> >> > block driver.
> >> > If available, we use rbd_writesame() to quickly fill the image when
> >> > full preallocation is required.
> >> >
> >> > Signed-off-by: Stefano Garzarella <address@hidden>
> >> > ---
> >> >  block/rbd.c          | 149 ++++++++++++++++++++++++++++++++++++++-----
> >> >  qapi/block-core.json |   4 +-
> >> >  2 files changed, 136 insertions(+), 17 deletions(-)
> >> >
> >> > diff --git a/block/rbd.c b/block/rbd.c
> >> > index 0c549c9935..29dd1bb040 100644
> >> > --- a/block/rbd.c
> >> > +++ b/block/rbd.c
> >> > @@ -13,6 +13,7 @@
> >> >
> >> >  #include "qemu/osdep.h"
> >> >
> >> > +#include "qemu/units.h"
> >> >  #include <rbd/librbd.h>
> >> >  #include "qapi/error.h"
> >> >  #include "qemu/error-report.h"
> >> > @@ -331,6 +332,110 @@ static void qemu_rbd_memset(RADOSCB *rcb, int64_t 
> >> > offs)
> >> >      }
> >> >  }
> >> >
> >> > +static int qemu_rbd_do_truncate(rbd_image_t image, int64_t offset,
> >> > +                                PreallocMode prealloc, Error **errp)
> >> > +{
> >> > +    uint64_t current_length;
> >> > +    char *buf = NULL;
> >> > +    int ret;
> >> > +
> >> > +    ret = rbd_get_size(image, &current_length);
> >> > +    if (ret < 0) {
> >> > +        error_setg_errno(errp, -ret, "Failed to get file length");
> >> > +        goto out;
> >> > +    }
> >> > +
> >> > +    if (current_length > offset && prealloc != PREALLOC_MODE_OFF) {
> >> > +        error_setg(errp, "Cannot use preallocation for shrinking 
> >> > files");
> >> > +        ret = -ENOTSUP;
> >> > +        goto out;
> >> > +    }
> >> > +
> >> > +    switch (prealloc) {
> >> > +    case PREALLOC_MODE_FULL: {
> >> [...]
> >> > +    case PREALLOC_MODE_OFF:
> >> [...]
> >> > +    default:
> >> > +        error_setg(errp, "Unsupported preallocation mode: %s",
> >> > +                   PreallocMode_str(prealloc));
> >> > +        ret = -ENOTSUP;
> >> > +        goto out;
> >> > +    }
> >>
> >> Other block drivers also accept only some values of PreallocMode.  Okay.
> >>
> >> I wonder whether management applications need to know which values are
> >> supported.
> >
> > Good point!
>
> We can continue to assume they don't until somebody tells us otherwise.
>
> >> Let me review support in drivers:
> >>
> >> * file (file-win32.c)
> >> * iscsi
> >> * nfs
> >> * qed
> >> * ssh
> >>
> >>   - Reject all but PREALLOC_MODE_OFF
> >>
> >> * copy-on-read
> >> * luks (crypto.c)
> >> * raw
> >>
> >>   - Pass through only
> >>
> >> * file host_cdrom host_device (file-posix.c)
> >>
> >>   - Reject all but PREALLOC_MODE_OFF when shrinking and for non-regular
> >>     files
> >>   - Reject PREALLOC_MODE_FALLOC unless CONFIG_POSIX_FALLOCATE
> >>   - Reject PREALLOC_MODE_METADATA
> >>
> >> * gluster
> >>
> >>   - Reject all but PREALLOC_MODE_OFF when shrinking
> >>   - Reject PREALLOC_MODE_FALLOC unless CONFIG_GLUSTERFS_FALLOCATE
> >>   - Reject PREALLOC_MODE_FULL unless CONFIG_GLUSTERFS_ZEROFILL
> >>   - Reject PREALLOC_MODE_METADATA
> >>
> >> * qcow2
> >>
> >>   - Reject all but PREALLOC_MODE_OFF when shrinking and with a backing
> >>     file
> >>
> >> * rbd with this patch
> >>
> >>   - Reject all but PREALLOC_MODE_OFF when shrinking
> >>   - Reject PREALLOC_MODE_METADATA and PREALLOC_MODE_FALLOC
> >>
> >> * sheepdog
> >>
> >>   - Reject PREALLOC_MODE_METADATA and PREALLOC_MODE_FALLOC
> >>   - Doesn't support shrinking
> >>
> >> * vdi
> >>
> >>   - Reject PREALLOC_MODE_FALLOC and PREALLOC_MODE_FULL
> >>   - Doesn't support shrinking
> >>
> >> * blkdebug
> >> * blklogwrites
> >> * blkverify
> >> * bochs
> >> * cloop
> >> * dmg
> >> * ftp
> >> * ftps
> >> * http
> >> * https
> >> * luks
> >> * nbd
> >> * null-aio
> >> * null-co
> >> * nvme
> >> * parallels
> >> * qcow
> >> * quorum
> >> * replication
> >> * throttle
> >> * vhdx
> >> * vmdk
> >> * vpc
> >> * vvfat
> >> * vxhs
> >>
> >>   - These appear not to use PreallocMode: they don't implement
> >>     .bdrv_co_truncate(), and either don't implement .bdrv_co_create() or
> >>     implement it without a prealloc parameter.
> >>
> >> Looks good to me.
> >>
> >
> > Thanks for the analysis!
> >
> >> > +
> >> > +    ret = 0;
> >> > +
> >> > +out:
> >> > +    g_free(buf);
> >> > +    return ret;
> >> > +}
> >> > +
> >> >  static QemuOptsList runtime_opts = {
> >> >      .name = "rbd",
> >> >      .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> >> [...]
> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> > index 7ccbfff9d0..db25a4065b 100644
> >> > --- a/qapi/block-core.json
> >> > +++ b/qapi/block-core.json
> >> > @@ -4277,13 +4277,15 @@
> >> >  #                   point to a snapshot.
> >> >  # @size             Size of the virtual disk in bytes
> >> >  # @cluster-size     RBD object size
> >> > +# @preallocation    Preallocation mode (allowed values: off, full)
> >> >  #
> >> >  # Since: 2.12
> >> >  ##
> >> >  { 'struct': 'BlockdevCreateOptionsRbd',
> >> >    'data': { 'location':         'BlockdevOptionsRbd',
> >> >              'size':             'size',
> >> > -            '*cluster-size' :   'size' } }
> >> > +            '*cluster-size' :   'size',
> >> > +            '*preallocation':   'PreallocMode' } }
> >> >
> >> >  ##
> >> >  # @BlockdevVmdkSubformat:
> >>
> >> The non-support of values 'metadata' and 'falloc' is not visible in
> >> introspection, only in documentation.  No reason to block this patch, as
> >> the other block drivers have the same introspection weakness (only
> >> sheepdog and vdi bother to document).
> >>
> >> Should we address the introspection weakness?  Only if there's a use for
> >> the information, I think.
> >
> > If the management applications will use that information (or maybe also
> > our help pages), could be useful to have an array of 'PreallocMode'
> > supported per-driver.
>
> Ideally, query-qmp-schema would show only the supported values.
>
> Not hard to do, just tedious: we'd get a number of sub-enums in addition
> to the full one, and we'd have to map from sub-enum to the full one.
>
> QAPI language support for sub-enums would remove most of the tedium.
> Not worthwhile unless the need for sub-enums is actually common.
>
> >> Should we improve documentation for the other block drivers?
> >>
> >
> > Yes, e.g. for Gluster it is not updated.
> > If you agree, I can check and update the documentation of all drivers 
> > following
> > your analysis.
>
> Yes, please!


Hi Markus,
I'm finally updating the documentation of preallocation modes
supported by block drivers and protocols in qapi/block-core.json.
As sheepdog and vdi I'm adding the supported values for each driver or
protocol that supports 'preallocation' parameter during the creation,
I'm also updating the '.help' in the QemuOptsList.

My doubt is: where is better to put the documentation about
preallocation modes supported during the resize? (e.g. some drivers
support only PREALLOC_MODE_OFF when shrinking)

Thanks,
Stefano