Re: [Qemu-devel] [PATCH 25/27] block/parallels: add prealloc-mode and prealloc-size open paramemets

[Roman Kagan]

On Tue, Apr 28, 2015 at 10:46:58AM +0300, Denis V. Lunev wrote:
> This is preparational commit for tweaks in Parallels image expansion.
> The idea is that enlarge via truncate by one data block is slow. It
> would be much better to use fallocate via bdrv_write_zeroes and
> expand by some significant amount at once.
> 
> Original idea with sequential file writing to the end of the file without
> fallocate/truncate would be slower than this approach if the image is
> expanded with several operations:
> - each image expanding means file metadata update, i.e. filesystem
>   journal write. Truncate/write to newly truncated space update file
>   metadata twice thus truncate removal helps. With fallocate call
>   inside bdrv_write_zeroes file metadata is updated only once and
>   this should happen infrequently thus this approach is the best one
>   for the image expansion
> - tail writes are ordered, i.e. the guest IO queue could not be sent
>   immediately to the host introducing additional IO delays
> 
> This patch just adds proper parameters into BDRVParallelsState and
> performs options parsing in parallels_open.
> 
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Roman Kagan <address@hidden>
> CC: Kevin Wolf <address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> ---
>  block/parallels.c | 83 
> +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 77 insertions(+), 6 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 05fe030..440938e 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -31,6 +31,7 @@
>  #include "block/block_int.h"
>  #include "qemu/module.h"
>  #include "qemu/bitmap.h"
> +#include "qapi/util.h"
>  
>  /**************************************************************/
>  
> @@ -56,6 +57,20 @@ typedef struct ParallelsHeader {
>      char padding[12];
>  } QEMU_PACKED ParallelsHeader;
>  
> +
> +typedef enum ParallelsPreallocMode {
> +    PRL_PREALLOC_MODE_FALLOCATE = 0,
> +    PRL_PREALLOC_MODE_TRUNCATE = 1,
> +    PRL_PREALLOC_MODE_MAX = 2,
> +} ParallelsPreallocMode;
> +
> +static const char *prealloc_mode_lookup[] = {
> +    "falloc",
> +    "truncate",
> +    NULL,
> +};
> +
> +
>  typedef struct BDRVParallelsState {
>      /** Locking is conservative, the lock protects
>       *   - image file extending (truncate, fallocate)
> @@ -73,14 +88,40 @@ typedef struct BDRVParallelsState {
>      uint32_t *bat_bitmap;
>      unsigned int bat_size;
>  
> +    uint64_t prealloc_size;
> +    ParallelsPreallocMode prealloc_mode;
> +
>      unsigned int tracks;
>  
>      unsigned int off_multiplier;
> -
> -    bool has_truncate;
>  } BDRVParallelsState;
>  
>  
> +#define PARALLELS_OPT_PREALLOC_MODE     "prealloc-mode"
> +#define PARALLELS_OPT_PREALLOC_SIZE     "prealloc-size"
> +
> +static QemuOptsList parallels_runtime_opts = {
> +    .name = "parallels",
> +    .head = QTAILQ_HEAD_INITIALIZER(parallels_runtime_opts.head),
> +    .desc = {
> +        {
> +            .name = PARALLELS_OPT_PREALLOC_SIZE,
> +            .type = QEMU_OPT_SIZE,
> +            .help = "Preallocation size on image expansion",
> +            .def_value_str = "128MiB",
> +        },
> +        {
> +            .name = PARALLELS_OPT_PREALLOC_MODE,
> +            .type = QEMU_OPT_STRING,
> +            .help = "Preallocation mode on image expansion "
> +                    "(allowed values: falloc, truncate)",
> +            .def_value_str = "falloc",
> +        },
> +        { /* end of list */ },
> +    },
> +};
> +
> +
>  static int64_t bat2sect(BDRVParallelsState *s, uint32_t idx)
>  {
>      return (uint64_t)le32_to_cpu(s->bat_bitmap[idx]) * s->off_multiplier;
> @@ -159,7 +200,7 @@ static int64_t allocate_cluster(BlockDriverState *bs, 
> int64_t sector_num)
>      }
>  
>      pos = bdrv_getlength(bs->file) >> BDRV_SECTOR_BITS;
> -    if (s->has_truncate) {
> +    if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
>          ret = bdrv_truncate(bs->file, (pos + s->tracks) << BDRV_SECTOR_BITS);
>      } else {
>          ret = bdrv_write_zeroes(bs->file, pos, s->tracks, 0);
> @@ -509,6 +550,9 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      BDRVParallelsState *s = bs->opaque;
>      ParallelsHeader ph;
>      int ret, size;
> +    QemuOpts *opts = NULL;
> +    Error *local_err = NULL;
> +    char *buf;
>  
>      ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
>      if (ret < 0) {
> @@ -567,9 +611,6 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      }
>      s->bat_bitmap = (uint32_t *)(s->header + 1);
>  
> -    s->has_truncate = bdrv_has_zero_init(bs->file) &&
> -                      bdrv_truncate(bs->file, bdrv_getlength(bs->file)) == 0;
> -
>      if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>          /* Image was not closed correctly. The check is mandatory */
>          s->header_unclean = true;

It may be slightly more logical to leave truncate support out of patch
09 sticking with bdrv_write_zeros there, and introduce it all in this
patch.

Otherwise looks good to me.

Roman.