| Message ID | 1430207220-24458-26-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
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 <den@openvz.org> > CC: Roman Kagan <rkagan@parallels.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Stefan Hajnoczi <stefanha@redhat.com> > --- > 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.
On Tue, Apr 28, 2015 at 01:59:56PM +0300, Roman Kagan wrote: > 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 <den@openvz.org> > > CC: Roman Kagan <rkagan@parallels.com> > > CC: Kevin Wolf <kwolf@redhat.com> > > CC: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > block/parallels.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 77 insertions(+), 6 deletions(-) > > 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. I mean, Reviewed-by: Roman Kagan <rkagan@parallels.com> Roman.
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 <den@openvz.org> > CC: Roman Kagan <rkagan@parallels.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Stefan Hajnoczi <stefanha@redhat.com> > --- > block/parallels.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 77 insertions(+), 6 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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; @@ -581,6 +622,31 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, } } + opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, &local_err); + if (local_err != NULL) { + goto fail_options; + } + + qemu_opts_absorb_qdict(opts, options, &local_err); + if (local_err != NULL) { + goto fail_options; + } + + s->prealloc_size = + qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0); + s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS); + buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE); + s->prealloc_mode = qapi_enum_parse(prealloc_mode_lookup, buf, + PRL_PREALLOC_MODE_MAX, PRL_PREALLOC_MODE_FALLOCATE, &local_err); + g_free(buf); + if (local_err != NULL) { + goto fail_options; + } + if (!bdrv_has_zero_init(bs->file) || + bdrv_truncate(bs->file, bdrv_getlength(bs->file)) != 0) { + s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE; + } + if (flags & BDRV_O_RDWR) { s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC); ret = parallels_update_header(bs); @@ -602,6 +668,11 @@ fail_format: fail: qemu_vfree(s->header); return ret; + +fail_options: + error_propagate(errp, local_err); + ret = -EINVAL; + goto fail; }
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 <den@openvz.org> CC: Roman Kagan <rkagan@parallels.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Stefan Hajnoczi <stefanha@redhat.com> --- block/parallels.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 77 insertions(+), 6 deletions(-)