[Qemu-devel] [PATCH v4 00/10] New block driver: blklogwrites

[Qemu-devel] [PATCH v4 00/10] New block driver: blklogwrites

[Ari Sundholm]

This patch series adds a new block driver, blklogwrites, to QEMU. The
driver is given two block devices: a raw device backed by an image or a
host block device, and a log device, typically backed by a file, on
which writes to the raw device are logged.

The logging format used is the same as in the dm-log-writes target of
the Linux kernel device mapper. The log reflects the writes that have
been performed on the guest block device and flushed. To be strict, the
log may contain writes that have not been flushed yet, but they are
technically outside the bounds of the log until the next flush updates
the metadata in the log superblock. We believe these semantics to be
useful even though they may not be completely identical to those of
dm-log-writes.

This functionality can be used for crash consistency and fs consistency
testing in filesystem drivers, including on non-Linux guests and older
guests running Linux versions without support for dm-log-writes. This
is simple and useful. Admittedly this and more advanced things could
perhaps be done by extending the quorum driver, but this approach would
require re-engineering the functionality and involve a more complicated
setup, so we offer this simple solution which we have found useful
internally.

In the first patch of this series, two block permission constants are
moved from block.c to include/block/block.h to make them available
outside of block.c. The next patch uses these constants.

The driver requires all requests to be aligned to the sector size. In
the second patch of this series, which includes the bulk of the
blklogwrites driver, this sector size is assumed to be equal to
BDRV_SECTOR_SIZE for simplicity.

In patches 3-8, a mechanism to pass the block configuration of a block
backend to a block driver is introduced, and this mechanism is
integrated in various hw/block drivers.

In patch 9, blklogwrites is changed to use the block configuration of
the relevant block backend to set its block limits.

Finally, in patch 10, blklogwrites is improved to use the logical
sector size of the block backend for logging. This means that the
sector size in the log metadata will be set to the logical sector size,
and offsets and sizes of writes will be expressed as a multiple of that
instead of BDRV_SECTOR_SIZE.

v4:
  - Check return value of snprintf() (flagged by patchew)
  - Don't use C99 for loop initial declaration (flagged by patchew, but
    wasn't QEMU supposed to be C99? Oh well.)
  - Add proper "Since" fields for blklogwrites in block-core.json
  - Rebase on current upstream master

v3:
  - Rebase on current upstream master

v2:
  - Incorporate review feedback
  - Add patches to apply block configurations in more block device
    drivers

Aapo Vienamo (1):
  block: Add blklogwrites

Ari Sundholm (9):
  block: Move two block permission constants to the relevant enum
  block: Add a mechanism for passing a block driver a block
    configuration
  hw/scsi/scsi-disk: Always apply block configuration to block driver
  hw/ide/qdev: Always apply block configuration to block driver
  hw/block/virtio-blk: Always apply block configuration to block driver
  hw/block/nvme: Always apply block configuration to block driver
  hw/block/fdc: Always apply block configuration to block driver
  block/blklogwrites: Use block limits from the backend block
    configuration
  block/blklogwrites: Use the block device logical sector size when
    logging writes

 MAINTAINERS               |   6 +
 block.c                   |   6 -
 block/Makefile.objs       |   1 +
 block/blklogwrites.c      | 426 ++++++++++++++++++++++++++++++++++++++++++++++
 block/io.c                |  22 +++
 hw/block/block.c          |  12 +-
 hw/block/fdc.c            |   4 +
 hw/block/nvme.c           |   1 +
 hw/block/virtio-blk.c     |   1 +
 hw/ide/qdev.c             |   2 +
 hw/scsi/scsi-disk.c       |   1 +
 include/block/block.h     |  17 ++
 include/block/block_int.h |   9 +
 include/hw/block/block.h  |   1 +
 qapi/block-core.json      |  30 +++-
 15 files changed, 526 insertions(+), 13 deletions(-)
 create mode 100644 block/blklogwrites.c

-- 
2.7.4

[Qemu-devel] [PATCH v4 01/10] block: Move two block permission constants to the relevant enum

[Ari Sundholm]

This allows using the two constants outside of block.c, which will
happen in a subsequent patch.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 block.c               | 6 ------
 include/block/block.h | 7 +++++++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 501b64c..c8cffe1 100644
--- a/block.c
+++ b/block.c
@@ -1914,12 +1914,6 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
     return 0;
 }
 
-#define DEFAULT_PERM_PASSTHROUGH (BLK_PERM_CONSISTENT_READ \
-                                 | BLK_PERM_WRITE \
-                                 | BLK_PERM_WRITE_UNCHANGED \
-                                 | BLK_PERM_RESIZE)
-#define DEFAULT_PERM_UNCHANGED (BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH)
-
 void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c,
                                const BdrvChildRole *role,
                                BlockReopenQueue *reopen_queue,
diff --git a/include/block/block.h b/include/block/block.h
index 6cc6c7e..312ae01 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -225,6 +225,13 @@ enum {
     BLK_PERM_GRAPH_MOD          = 0x10,
 
     BLK_PERM_ALL                = 0x1f,
+
+    DEFAULT_PERM_PASSTHROUGH    = BLK_PERM_CONSISTENT_READ
+                                 | BLK_PERM_WRITE
+                                 | BLK_PERM_WRITE_UNCHANGED
+                                 | BLK_PERM_RESIZE,
+
+    DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
 };
 
 char *bdrv_perm_names(uint64_t perm);
-- 
2.7.4

[Qemu-devel] [PATCH v4 02/10] block: Add blklogwrites

[Ari Sundholm]

From: Aapo Vienamo <aapo@tuxera.com>

Implements a block device write logging system, similar to Linux kernel
device mapper dm-log-writes. The write operations that are performed
on a block device are logged to a file or another block device. The
write log format is identical to the dm-log-writes format. Currently,
log markers are not supported.

This functionality can be used for crash consistency and fs consistency
testing. By implementing it in qemu, tests utilizing write logs can be
be used to test non-Linux drivers and older kernels.

The implementation is based on the blkverify and blkdebug block drivers.

Signed-off-by: Aapo Vienamo <aapo@tuxera.com>
Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 MAINTAINERS          |   6 +
 block/Makefile.objs  |   1 +
 block/blklogwrites.c | 389 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json |  30 +++-
 4 files changed, 420 insertions(+), 6 deletions(-)
 create mode 100644 block/blklogwrites.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4c73c16..4b8d1a8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2015,6 +2015,12 @@ S: Supported
 F: block/quorum.c
 L: qemu-block@nongnu.org
 
+blklogwrites
+M: Ari Sundholm <ari@tuxera.com>
+L: qemu-block@nongnu.org
+S: Supported
+F: block/blklogwrites.c
+
 blkverify
 M: Stefan Hajnoczi <stefanha@redhat.com>
 L: qemu-block@nongnu.org
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 899bfb5..c8337bf 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -5,6 +5,7 @@ block-obj-y += qed-check.o
 block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o
 block-obj-y += quorum.o
 block-obj-y += parallels.o blkdebug.o blkverify.o blkreplay.o
+block-obj-y += blklogwrites.o
 block-obj-y += block-backend.o snapshot.o qapi.o
 block-obj-$(CONFIG_WIN32) += file-win32.o win32-aio.o
 block-obj-$(CONFIG_POSIX) += file-posix.o
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
new file mode 100644
index 0000000..1b969b0
--- /dev/null
+++ b/block/blklogwrites.c
@@ -0,0 +1,389 @@
+/*
+ * Write logging blk driver based on blkverify and blkdebug.
+ *
+ * Copyright (c) 2017 Tuomas Tynkkynen <tuomas@tuxera.com>
+ * Copyright (c) 2018 Aapo Vienamo <aapo@tuxera.com>
+ * Copyright (c) 2018 Ari Sundholm <ari@tuxera.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/sockets.h" /* for EINPROGRESS on Windows */
+#include "block/block_int.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qstring.h"
+#include "qemu/cutils.h"
+#include "qemu/option.h"
+
+/* Disk format stuff - taken from Linux drivers/md/dm-log-writes.c */
+
+#define LOG_FLUSH_FLAG (1 << 0)
+#define LOG_FUA_FLAG (1 << 1)
+#define LOG_DISCARD_FLAG (1 << 2)
+#define LOG_MARK_FLAG (1 << 3)
+
+#define WRITE_LOG_VERSION 1ULL
+#define WRITE_LOG_MAGIC 0x6a736677736872ULL
+
+/* All fields are little-endian. */
+struct log_write_super {
+    uint64_t magic;
+    uint64_t version;
+    uint64_t nr_entries;
+    uint32_t sectorsize;
+};
+
+struct log_write_entry {
+    uint64_t sector;
+    uint64_t nr_sectors;
+    uint64_t flags;
+    uint64_t data_len;
+};
+
+/* End of disk format structures. */
+
+typedef struct {
+    BdrvChild *log_file;
+    uint64_t cur_log_sector;
+    uint64_t nr_entries;
+} BDRVBlkLogWritesState;
+
+static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
+                               Error **errp)
+{
+    BDRVBlkLogWritesState *s = bs->opaque;
+    Error *local_err = NULL;
+    int ret;
+
+    /* Open the raw file */
+    bs->file = bdrv_open_child(NULL, options, "raw", bs, &child_file, false,
+                               &local_err);
+    if (local_err) {
+        ret = -EINVAL;
+        error_propagate(errp, local_err);
+        goto fail;
+    }
+
+    s->cur_log_sector = 1;
+    s->nr_entries = 0;
+
+    /* Open the log file */
+    s->log_file = bdrv_open_child(NULL, options, "log", bs, &child_file, false,
+                                  &local_err);
+    if (local_err) {
+        ret = -EINVAL;
+        error_propagate(errp, local_err);
+        goto fail;
+    }
+
+    ret = 0;
+fail:
+    if (ret < 0) {
+        bdrv_unref_child(bs, bs->file);
+        bs->file = NULL;
+    }
+    return ret;
+}
+
+static void blk_log_writes_close(BlockDriverState *bs)
+{
+    BDRVBlkLogWritesState *s = bs->opaque;
+
+    bdrv_unref_child(bs, s->log_file);
+    s->log_file = NULL;
+}
+
+static int64_t blk_log_writes_getlength(BlockDriverState *bs)
+{
+    return bdrv_getlength(bs->file->bs);
+}
+
+static void blk_log_writes_refresh_filename(BlockDriverState *bs,
+                                            QDict *options)
+{
+    BDRVBlkLogWritesState *s = bs->opaque;
+
+    /* bs->file->bs has already been refreshed */
+    bdrv_refresh_filename(s->log_file->bs);
+
+    if (bs->file->bs->full_open_options
+        && s->log_file->bs->full_open_options)
+    {
+        QDict *opts = qdict_new();
+        qdict_put_obj(opts, "driver",
+                      QOBJECT(qstring_from_str("blklogwrites")));
+
+        qobject_ref(bs->file->bs->full_open_options);
+        qdict_put_obj(opts, "raw", QOBJECT(bs->file->bs->full_open_options));
+        qobject_ref(s->log_file->bs->full_open_options);
+        qdict_put_obj(opts, "log",
+                      QOBJECT(s->log_file->bs->full_open_options));
+
+        bs->full_open_options = opts;
+    }
+
+    if (bs->file->bs->exact_filename[0]
+        && s->log_file->bs->exact_filename[0])
+    {
+        int ret = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                           "blklogwrites:%s:%s",
+                           bs->file->bs->exact_filename,
+                           s->log_file->bs->exact_filename);
+
+        if (ret >= sizeof(bs->exact_filename)) {
+            /* An overflow makes the filename unusable, so do not report any */
+            bs->exact_filename[0] = '\0';
+        }
+    }
+}
+
+static void blk_log_writes_child_perm(BlockDriverState *bs, BdrvChild *c,
+                                      const BdrvChildRole *role,
+                                      BlockReopenQueue *ro_q,
+                                      uint64_t perm, uint64_t shrd,
+                                      uint64_t *nperm, uint64_t *nshrd)
+{
+    if (!c) {
+        *nperm = perm & DEFAULT_PERM_PASSTHROUGH;
+        *nshrd = (shrd & DEFAULT_PERM_PASSTHROUGH) | DEFAULT_PERM_UNCHANGED;
+        return;
+    }
+
+    if (!strcmp(c->name, "log")) {
+        bdrv_format_default_perms(bs, c, role, ro_q, perm, shrd, nperm, nshrd);
+    } else {
+        bdrv_filter_default_perms(bs, c, role, ro_q, perm, shrd, nperm, nshrd);
+    }
+}
+
+static void blk_log_writes_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    if (bs->bl.request_alignment < BDRV_SECTOR_SIZE) {
+        bs->bl.request_alignment = BDRV_SECTOR_SIZE;
+
+        if (bs->bl.pdiscard_alignment &&
+                bs->bl.pdiscard_alignment < bs->bl.request_alignment)
+            bs->bl.pdiscard_alignment = bs->bl.request_alignment;
+        if (bs->bl.pwrite_zeroes_alignment &&
+                bs->bl.pwrite_zeroes_alignment < bs->bl.request_alignment)
+            bs->bl.pwrite_zeroes_alignment = bs->bl.request_alignment;
+    }
+}
+
+static int coroutine_fn
+blk_log_writes_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+                         QEMUIOVector *qiov, int flags)
+{
+    return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
+}
+
+typedef struct BlkLogWritesFileReq {
+    BlockDriverState *bs;
+    uint64_t offset;
+    uint64_t bytes;
+    int file_flags;
+    QEMUIOVector *qiov;
+    int (*func)(struct BlkLogWritesFileReq *r);
+    int file_ret;
+} BlkLogWritesFileReq;
+
+typedef struct {
+    BlockDriverState *bs;
+    QEMUIOVector *qiov;
+    struct log_write_entry entry;
+    uint64_t zero_size;
+    int log_ret;
+} BlkLogWritesLogReq;
+
+static void coroutine_fn blk_log_writes_co_do_log(BlkLogWritesLogReq *lr)
+{
+    BDRVBlkLogWritesState *s = lr->bs->opaque;
+    uint64_t cur_log_offset = s->cur_log_sector << BDRV_SECTOR_BITS;
+
+    s->nr_entries++;
+    s->cur_log_sector +=
+            ROUND_UP(lr->qiov->size, BDRV_SECTOR_SIZE) >> BDRV_SECTOR_BITS;
+
+    lr->log_ret = bdrv_co_pwritev(s->log_file, cur_log_offset, lr->qiov->size,
+                                  lr->qiov, 0);
+
+    /* Logging for the "write zeroes" operation */
+    if (lr->log_ret == 0 && lr->zero_size) {
+        cur_log_offset = s->cur_log_sector << BDRV_SECTOR_BITS;
+        s->cur_log_sector +=
+                ROUND_UP(lr->zero_size, BDRV_SECTOR_SIZE) >> BDRV_SECTOR_BITS;
+
+        lr->log_ret = bdrv_co_pwrite_zeroes(s->log_file, cur_log_offset,
+                                            lr->zero_size, 0);
+    }
+
+    /* Update super block on flush */
+    if (lr->log_ret == 0 && lr->entry.flags & LOG_FLUSH_FLAG) {
+        struct log_write_super super = {
+            .magic      = cpu_to_le64(WRITE_LOG_MAGIC),
+            .version    = cpu_to_le64(WRITE_LOG_VERSION),
+            .nr_entries = cpu_to_le64(s->nr_entries),
+            .sectorsize = cpu_to_le32(1 << BDRV_SECTOR_BITS),
+        };
+        static const char zeroes[BDRV_SECTOR_SIZE - sizeof(super)] = { '\0' };
+        QEMUIOVector qiov;
+
+        qemu_iovec_init(&qiov, 2);
+        qemu_iovec_add(&qiov, &super, sizeof(super));
+        qemu_iovec_add(&qiov, (void *)zeroes, sizeof(zeroes));
+
+        lr->log_ret =
+            bdrv_co_pwritev(s->log_file, 0, BDRV_SECTOR_SIZE, &qiov, 0);
+        if (lr->log_ret == 0) {
+            lr->log_ret = bdrv_co_flush(s->log_file->bs);
+        }
+        qemu_iovec_destroy(&qiov);
+    }
+}
+
+static void coroutine_fn blk_log_writes_co_do_file(BlkLogWritesFileReq *fr)
+{
+    fr->file_ret = fr->func(fr);
+}
+
+static int coroutine_fn
+blk_log_writes_co_log(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+                      QEMUIOVector *qiov, int flags,
+                      int (*file_func)(BlkLogWritesFileReq *r),
+                      uint64_t entry_flags, bool is_zero_write)
+{
+    QEMUIOVector log_qiov;
+    size_t niov = qiov ? qiov->niov : 0;
+    size_t i;
+    BlkLogWritesFileReq fr = {
+        .bs     = bs,
+        .offset = offset,
+        .bytes  = bytes,
+        .file_flags = flags,
+        .qiov   = qiov,
+        .func   = file_func,
+    };
+    BlkLogWritesLogReq lr = {
+        .bs             = bs,
+        .qiov           = &log_qiov,
+        .entry = {
+            .sector     = cpu_to_le64(offset >> BDRV_SECTOR_BITS),
+            .nr_sectors = cpu_to_le64(bytes >> BDRV_SECTOR_BITS),
+            .flags      = cpu_to_le64(entry_flags),
+            .data_len   = 0,
+        },
+        .zero_size = is_zero_write ? bytes : 0,
+    };
+    static const char zeroes[BDRV_SECTOR_SIZE - sizeof(struct log_write_entry)]
+        = { '\0' };
+
+    assert(QEMU_IS_ALIGNED(offset, bs->bl.request_alignment));
+    assert(QEMU_IS_ALIGNED(bytes, bs->bl.request_alignment));
+
+    qemu_iovec_init(&log_qiov, niov + 2);
+    qemu_iovec_add(&log_qiov, &lr.entry, sizeof(lr.entry));
+    qemu_iovec_add(&log_qiov, (void *)zeroes, sizeof(zeroes));
+    for (i = 0; i < niov; ++i) {
+        qemu_iovec_add(&log_qiov, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
+    }
+
+    blk_log_writes_co_do_file(&fr);
+    blk_log_writes_co_do_log(&lr);
+
+    qemu_iovec_destroy(&log_qiov);
+
+    if (lr.log_ret < 0) {
+        return lr.log_ret;
+    }
+
+    return fr.file_ret;
+}
+
+static int coroutine_fn
+blk_log_writes_co_do_file_pwritev(BlkLogWritesFileReq *fr)
+{
+    return bdrv_co_pwritev(fr->bs->file, fr->offset, fr->bytes,
+                           fr->qiov, fr->file_flags);
+}
+
+static int coroutine_fn
+blk_log_writes_co_do_file_pwrite_zeroes(BlkLogWritesFileReq *fr)
+{
+    return bdrv_co_pwrite_zeroes(fr->bs->file, fr->offset, fr->bytes,
+                                 fr->file_flags);
+}
+
+static int coroutine_fn blk_log_writes_co_do_file_flush(BlkLogWritesFileReq *fr)
+{
+    return bdrv_co_flush(fr->bs->file->bs);
+}
+
+static int coroutine_fn
+blk_log_writes_co_do_file_pdiscard(BlkLogWritesFileReq *fr)
+{
+    return bdrv_co_pdiscard(fr->bs->file->bs, fr->offset, fr->bytes);
+}
+
+static int coroutine_fn
+blk_log_writes_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+                          QEMUIOVector *qiov, int flags)
+{
+    return blk_log_writes_co_log(bs, offset, bytes, qiov, flags,
+                                 blk_log_writes_co_do_file_pwritev, 0, false);
+}
+
+static int coroutine_fn
+blk_log_writes_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int bytes,
+                                BdrvRequestFlags flags)
+{
+    return blk_log_writes_co_log(bs, offset, bytes, NULL, flags,
+                                 blk_log_writes_co_do_file_pwrite_zeroes, 0,
+                                 true);
+}
+
+static int coroutine_fn blk_log_writes_co_flush_to_disk(BlockDriverState *bs)
+{
+    return blk_log_writes_co_log(bs, 0, 0, NULL, 0,
+                                 blk_log_writes_co_do_file_flush,
+                                 LOG_FLUSH_FLAG, false);
+}
+
+static int coroutine_fn
+blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int count)
+{
+    return blk_log_writes_co_log(bs, offset, count, NULL, 0,
+                                 blk_log_writes_co_do_file_pdiscard,
+                                 LOG_DISCARD_FLAG, false);
+}
+
+static BlockDriver bdrv_blk_log_writes = {
+    .format_name            = "blklogwrites",
+    .protocol_name          = "blklogwrites",
+    .instance_size          = sizeof(BDRVBlkLogWritesState),
+
+    .bdrv_file_open         = blk_log_writes_open,
+    .bdrv_close             = blk_log_writes_close,
+    .bdrv_getlength         = blk_log_writes_getlength,
+    .bdrv_refresh_filename  = blk_log_writes_refresh_filename,
+    .bdrv_child_perm        = blk_log_writes_child_perm,
+    .bdrv_refresh_limits    = blk_log_writes_refresh_limits,
+
+    .bdrv_co_preadv         = blk_log_writes_co_preadv,
+    .bdrv_co_pwritev        = blk_log_writes_co_pwritev,
+    .bdrv_co_pwrite_zeroes  = blk_log_writes_co_pwrite_zeroes,
+    .bdrv_co_flush_to_disk  = blk_log_writes_co_flush_to_disk,
+    .bdrv_co_pdiscard       = blk_log_writes_co_pdiscard,
+    .bdrv_co_block_status   = bdrv_co_block_status_from_file,
+
+    .is_filter              = true,
+};
+
+static void bdrv_blk_log_writes_init(void)
+{
+    bdrv_register(&bdrv_blk_log_writes);
+}
+
+block_init(bdrv_blk_log_writes_init);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4b1de47..459d017 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2425,16 +2425,17 @@
 # @throttle: Since 2.11
 # @nvme: Since 2.12
 # @copy-on-read: Since 3.0
+# @blklogwrites: Since 3.0
 #
 # Since: 2.9
 ##
 { 'enum': 'BlockdevDriver',
-  'data': [ 'blkdebug', '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', 'replication', 'sheepdog', 'ssh',
-            'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
+  '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', 'replication', 'sheepdog',
+            'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
 
 ##
 # @BlockdevOptionsFile:
@@ -2949,6 +2950,21 @@
             '*set-state': ['BlkdebugSetStateOptions'] } }
 
 ##
+# @BlockdevOptionsBlklogwrites:
+#
+# Driver specific block device options for blklogwrites.
+#
+# @raw:     block device
+#
+# @log:     block device used to log writes on @raw
+#
+# Since: 3.0
+##
+{ 'struct': 'BlockdevOptionsBlklogwrites',
+  'data': { 'raw': 'BlockdevRef',
+            'log': 'BlockdevRef' } }
+
+##
 # @BlockdevOptionsBlkverify:
 #
 # Driver specific block device options for blkverify.
@@ -3443,6 +3459,7 @@
   'discriminator': 'driver',
   'data': {
       'blkdebug':   'BlockdevOptionsBlkdebug',
+      'blklogwrites': 'BlockdevOptionsBlklogwrites',
       'blkverify':  'BlockdevOptionsBlkverify',
       'bochs':      'BlockdevOptionsGenericFormat',
       'cloop':      'BlockdevOptionsGenericFormat',
@@ -3971,6 +3988,7 @@
   'discriminator': 'driver',
   'data': {
       'blkdebug':       'BlockdevCreateNotSupported',
+      'blklogwrites':   'BlockdevCreateNotSupported',
       'blkverify':      'BlockdevCreateNotSupported',
       'bochs':          'BlockdevCreateNotSupported',
       'cloop':          'BlockdevCreateNotSupported',
-- 
2.7.4

[Qemu-devel] [PATCH v4 03/10] block: Add a mechanism for passing a block driver a block configuration

[Ari Sundholm]

A block driver may need to know about the block configuration, most
critically the sector sizes, of a block backend for alignment purposes
or for some other reason. It doesn't seem like qemu has an existing
mechanism for a block backend to convey the required information to
the relevant block driver when it is being realized.

The need for this mechanism rises from the fact that a drive may not
have a backend at the time it is created, as devices are created after
drives during qemu startup. Therefore, a driver requiring information
about the block configuration can get this information when a backend
is created for it at the earliest. The most natural place for this to
take place seems to be in the realization functions of the various
block device drivers, such as scsi-hd. The interface proposed here
allows the block driver to receive information about the block
configuration and the associated backend through a new callback.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 block/io.c                | 22 ++++++++++++++++++++++
 hw/block/block.c          | 12 +++++++++++-
 include/block/block.h     | 10 ++++++++++
 include/block/block_int.h |  9 +++++++++
 include/hw/block/block.h  |  1 +
 5 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index b7beaee..3a06843 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2932,3 +2932,25 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
     bdrv_dec_in_flight(dst_bs);
     return ret;
 }
+
+void bdrv_apply_blkconf(BlockDriverState *bs, BlockConf *conf)
+{
+    BlockDriver *drv = bs->drv;
+
+    if (!drv) {
+        return;
+    }
+
+    if (drv->bdrv_apply_blkconf) {
+        drv->bdrv_apply_blkconf(bs, conf);
+        return;
+    }
+
+    if (bs->file && bs->file->bs) {
+        bdrv_apply_blkconf(bs->file->bs, conf);
+    }
+
+    if (bs->drv->supports_backing && backing_bs(bs)) {
+        bdrv_apply_blkconf(backing_bs(bs), conf);
+    }
+}
diff --git a/hw/block/block.c b/hw/block/block.c
index b91e2b6..8c87dbf 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -38,7 +38,7 @@ void blkconf_blocksizes(BlockConf *conf)
     /* fill in detected values if they are not defined via qemu command line */
     if (!conf->physical_block_size) {
         if (!backend_ret) {
-           conf->physical_block_size = blocksizes.phys;
+            conf->physical_block_size = blocksizes.phys;
         } else {
             conf->physical_block_size = BDRV_SECTOR_SIZE;
         }
@@ -52,6 +52,16 @@ void blkconf_blocksizes(BlockConf *conf)
     }
 }
 
+void blkconf_apply_to_blkdrv(BlockConf *conf)
+{
+    BlockBackend *blk = conf->blk;
+    BlockDriverState *bs = blk_bs(blk);
+
+    if (bs) {
+        bdrv_apply_blkconf(bs, conf);
+    }
+}
+
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
                                    bool resizable, Error **errp)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 312ae01..5d0a8ba 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -10,6 +10,7 @@
 #include "block/dirty-bitmap.h"
 #include "block/blockjob.h"
 #include "qemu/hbitmap.h"
+#include "hw/block/block.h"
 
 /* block.c */
 typedef struct BlockDriver BlockDriver;
@@ -650,4 +651,13 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host);
 int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
                                     BdrvChild *dst, uint64_t dst_offset,
                                     uint64_t bytes, BdrvRequestFlags flags);
+
+/*
+ * bdrv_apply_blkconf:
+ *
+ * Recursively finds the highest-level block drivers among the files and
+ * backing files that accept a block configuration and applies the given block
+ * configuration to them.
+ */
+void bdrv_apply_blkconf(BlockDriverState *bs, BlockConf *conf);
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 888b7f7..cb82745 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -464,6 +464,15 @@ struct BlockDriver {
     void (*bdrv_abort_perm_update)(BlockDriverState *bs);
 
     /**
+     * Called to inform the driver of the block configuration of the virtual
+     * block device.
+     *
+     * This function is called by a block device realization function if the
+     * device wants to inform the block driver of its block configuration.
+     */
+    void (*bdrv_apply_blkconf)(BlockDriverState *bs, BlockConf *conf);
+
+    /**
      * Returns in @nperm and @nshared the permissions that the driver for @bs
      * needs on its child @c, based on the cumulative permissions requested by
      * the parents in @parent_perm and @parent_shared.
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index d4f4dff..2861995 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -77,6 +77,7 @@ bool blkconf_geometry(BlockConf *conf, int *trans,
                       unsigned cyls_max, unsigned heads_max, unsigned secs_max,
                       Error **errp);
 void blkconf_blocksizes(BlockConf *conf);
+void blkconf_apply_to_blkdrv(BlockConf *conf);
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
                                    bool resizable, Error **errp);
 
-- 
2.7.4

[Qemu-devel] [PATCH v4 04/10] hw/scsi/scsi-disk: Always apply block configuration to block driver

[Ari Sundholm]

This allows the block driver to use the block configuration of the new
SCSI device. One use for this information is to set request limits
using this information.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 hw/scsi/scsi-disk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ded23d3..d50517e 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2413,6 +2413,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
         blk_set_dev_ops(s->qdev.conf.blk, &scsi_disk_block_ops, s);
     }
     blk_set_guest_block_size(s->qdev.conf.blk, s->qdev.blocksize);
+    blkconf_apply_to_blkdrv(&s->qdev.conf);
 
     blk_iostatus_enable(s->qdev.conf.blk);
 }
-- 
2.7.4

[Qemu-devel] [PATCH v4 05/10] hw/ide/qdev: Always apply block configuration to block driver

[Ari Sundholm]

This allows the block driver to use the block configuration of the new
IDE device. One use for this information is to set request limits
using this information.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 hw/ide/qdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index f395d24..7da1d28 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -214,6 +214,8 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
         dev->serial = g_strdup(s->drive_serial_str);
     }
 
+    blkconf_apply_to_blkdrv(&dev->conf);
+
     add_boot_device_path(dev->conf.bootindex, &dev->qdev,
                          dev->unit ? "/disk@1" : "/disk@0");
 }
-- 
2.7.4

[Qemu-devel] [PATCH v4 06/10] hw/block/virtio-blk: Always apply block configuration to block driver

[Ari Sundholm]

This allows the block driver to use the block configuration of the new
VirtIO block device. One use for this information is to set request
limits using this information.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 hw/block/virtio-blk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 50b5c86..51e7b86 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -975,6 +975,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
     blk_set_dev_ops(s->blk, &virtio_block_ops, s);
     blk_set_guest_block_size(s->blk, s->conf.conf.logical_block_size);
+    blkconf_apply_to_blkdrv(&s->conf.conf);
 
     blk_iostatus_enable(s->blk);
 }
-- 
2.7.4

[Qemu-devel] [PATCH v4 07/10] hw/block/nvme: Always apply block configuration to block driver

[Ari Sundholm]

This allows the block driver to use the block configuration of the new
NVMe device. One use for this information is to set request limits
using this information.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 hw/block/nvme.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 811084b..3b00dff 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1225,6 +1225,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
                                        false, errp)) {
         return;
     }
+    blkconf_apply_to_blkdrv(&n->conf);
 
     pci_conf = pci_dev->config;
     pci_conf[PCI_INTERRUPT_PIN] = 1;
-- 
2.7.4

[Qemu-devel] [PATCH v4 08/10] hw/block/fdc: Always apply block configuration to block driver

[Ari Sundholm]

This allows the block driver to use the block configuration of the new
floppy device. One use for this information is to set request limits
using this information.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 hw/block/fdc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index cd29e27..b11eeda 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -482,6 +482,8 @@ static void fd_change_cb(void *opaque, bool load, Error **errp)
                                            errp)) {
             return;
         }
+
+        blkconf_apply_to_blkdrv(drive->conf);
     }
 
     drive->media_changed = 1;
@@ -594,6 +596,8 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
     pick_drive_type(drive);
     dev->type = drive->drive;
 
+    blkconf_apply_to_blkdrv(&dev->conf);
+
     fd_revalidate(drive);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v4 09/10] block/blklogwrites: Use block limits from the backend block configuration

[Ari Sundholm]

This is to ensure that writes are aligned properly for logging writes
to the virtual block device. This is important because the
dm-log-writes log format has a granularity of one sector for both the
offset and the size of each write. By using the logical sector size
for alignment, the log records the writes more faithfully for those
devices that have a non-512 logical sector size.

Note that even with this patch, blklogwrites still uses
BDRV_SECTOR_SIZE for logging. This will change in a subsequent patch
which will introduce support for non-512 sector sizes.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 block/blklogwrites.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 1b969b0..216367f 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -173,6 +173,25 @@ static void blk_log_writes_refresh_limits(BlockDriverState *bs, Error **errp)
     }
 }
 
+static void blk_log_writes_apply_blkconf(BlockDriverState *bs, BlockConf *conf)
+{
+    assert(bs && conf && conf->blk);
+
+    bs->bl.request_alignment = conf->logical_block_size;
+    if (conf->discard_granularity != (uint32_t)-1) {
+        bs->bl.pdiscard_alignment = conf->discard_granularity;
+    }
+
+    if (bs->bl.pdiscard_alignment &&
+            bs->bl.pdiscard_alignment < bs->bl.request_alignment) {
+        bs->bl.pdiscard_alignment = bs->bl.request_alignment;
+    }
+    if (bs->bl.pwrite_zeroes_alignment &&
+            bs->bl.pwrite_zeroes_alignment < bs->bl.request_alignment) {
+        bs->bl.pwrite_zeroes_alignment = bs->bl.request_alignment;
+    }
+}
+
 static int coroutine_fn
 blk_log_writes_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
                          QEMUIOVector *qiov, int flags)
@@ -370,6 +389,7 @@ static BlockDriver bdrv_blk_log_writes = {
     .bdrv_refresh_filename  = blk_log_writes_refresh_filename,
     .bdrv_child_perm        = blk_log_writes_child_perm,
     .bdrv_refresh_limits    = blk_log_writes_refresh_limits,
+    .bdrv_apply_blkconf     = blk_log_writes_apply_blkconf,
 
     .bdrv_co_preadv         = blk_log_writes_co_preadv,
     .bdrv_co_pwritev        = blk_log_writes_co_pwritev,
-- 
2.7.4

[Qemu-devel] [PATCH v4 10/10] block/blklogwrites: Use the block device logical sector size when logging writes

[Ari Sundholm]

The guest OS may perform writes which are aligned to the logical
sector size instead of the physical one, so logging at this granularity
records the writes performed on the block device most faithfully.

Signed-off-by: Ari Sundholm <ari@tuxera.com>
---
 block/blklogwrites.c | 47 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 216367f..decf5e5 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -47,6 +47,8 @@ struct log_write_entry {
 
 typedef struct {
     BdrvChild *log_file;
+    uint32_t sectorsize;
+    uint32_t sectorbits;
     uint64_t cur_log_sector;
     uint64_t nr_entries;
 } BDRVBlkLogWritesState;
@@ -67,6 +69,8 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    s->sectorsize = BDRV_SECTOR_SIZE; /* May be updated later */
+    s->sectorbits = BDRV_SECTOR_BITS;
     s->cur_log_sector = 1;
     s->nr_entries = 0;
 
@@ -173,11 +177,20 @@ static void blk_log_writes_refresh_limits(BlockDriverState *bs, Error **errp)
     }
 }
 
+static inline uint32_t blk_log_writes_log2(uint32_t value)
+{
+    assert(value > 0);
+    return 31 - clz32(value);
+}
+
 static void blk_log_writes_apply_blkconf(BlockDriverState *bs, BlockConf *conf)
 {
-    assert(bs && conf && conf->blk);
+    BDRVBlkLogWritesState *s = bs->opaque;
+    assert(bs && conf && conf->blk && s);
 
-    bs->bl.request_alignment = conf->logical_block_size;
+    s->sectorsize = conf->logical_block_size;
+    s->sectorbits = blk_log_writes_log2(s->sectorsize);
+    bs->bl.request_alignment = s->sectorsize;
     if (conf->discard_granularity != (uint32_t)-1) {
         bs->bl.pdiscard_alignment = conf->discard_granularity;
     }
@@ -220,20 +233,20 @@ typedef struct {
 static void coroutine_fn blk_log_writes_co_do_log(BlkLogWritesLogReq *lr)
 {
     BDRVBlkLogWritesState *s = lr->bs->opaque;
-    uint64_t cur_log_offset = s->cur_log_sector << BDRV_SECTOR_BITS;
+    uint64_t cur_log_offset = s->cur_log_sector << s->sectorbits;
 
     s->nr_entries++;
     s->cur_log_sector +=
-            ROUND_UP(lr->qiov->size, BDRV_SECTOR_SIZE) >> BDRV_SECTOR_BITS;
+            ROUND_UP(lr->qiov->size, s->sectorsize) >> s->sectorbits;
 
     lr->log_ret = bdrv_co_pwritev(s->log_file, cur_log_offset, lr->qiov->size,
                                   lr->qiov, 0);
 
     /* Logging for the "write zeroes" operation */
     if (lr->log_ret == 0 && lr->zero_size) {
-        cur_log_offset = s->cur_log_sector << BDRV_SECTOR_BITS;
+        cur_log_offset = s->cur_log_sector << s->sectorbits;
         s->cur_log_sector +=
-                ROUND_UP(lr->zero_size, BDRV_SECTOR_SIZE) >> BDRV_SECTOR_BITS;
+                ROUND_UP(lr->zero_size, s->sectorsize) >> s->sectorbits;
 
         lr->log_ret = bdrv_co_pwrite_zeroes(s->log_file, cur_log_offset,
                                             lr->zero_size, 0);
@@ -245,21 +258,22 @@ static void coroutine_fn blk_log_writes_co_do_log(BlkLogWritesLogReq *lr)
             .magic      = cpu_to_le64(WRITE_LOG_MAGIC),
             .version    = cpu_to_le64(WRITE_LOG_VERSION),
             .nr_entries = cpu_to_le64(s->nr_entries),
-            .sectorsize = cpu_to_le32(1 << BDRV_SECTOR_BITS),
+            .sectorsize = cpu_to_le32(s->sectorsize),
         };
-        static const char zeroes[BDRV_SECTOR_SIZE - sizeof(super)] = { '\0' };
+        void *zeroes = g_malloc0(s->sectorsize - sizeof(super));
         QEMUIOVector qiov;
 
         qemu_iovec_init(&qiov, 2);
         qemu_iovec_add(&qiov, &super, sizeof(super));
-        qemu_iovec_add(&qiov, (void *)zeroes, sizeof(zeroes));
+        qemu_iovec_add(&qiov, zeroes, s->sectorsize - sizeof(super));
 
         lr->log_ret =
-            bdrv_co_pwritev(s->log_file, 0, BDRV_SECTOR_SIZE, &qiov, 0);
+            bdrv_co_pwritev(s->log_file, 0, s->sectorsize, &qiov, 0);
         if (lr->log_ret == 0) {
             lr->log_ret = bdrv_co_flush(s->log_file->bs);
         }
         qemu_iovec_destroy(&qiov);
+        g_free(zeroes);
     }
 }
 
@@ -277,6 +291,7 @@ blk_log_writes_co_log(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     QEMUIOVector log_qiov;
     size_t niov = qiov ? qiov->niov : 0;
     size_t i;
+    BDRVBlkLogWritesState *s = bs->opaque;
     BlkLogWritesFileReq fr = {
         .bs     = bs,
         .offset = offset,
@@ -289,22 +304,23 @@ blk_log_writes_co_log(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         .bs             = bs,
         .qiov           = &log_qiov,
         .entry = {
-            .sector     = cpu_to_le64(offset >> BDRV_SECTOR_BITS),
-            .nr_sectors = cpu_to_le64(bytes >> BDRV_SECTOR_BITS),
+            .sector     = cpu_to_le64(offset >> s->sectorbits),
+            .nr_sectors = cpu_to_le64(bytes >> s->sectorbits),
             .flags      = cpu_to_le64(entry_flags),
             .data_len   = 0,
         },
         .zero_size = is_zero_write ? bytes : 0,
     };
-    static const char zeroes[BDRV_SECTOR_SIZE - sizeof(struct log_write_entry)]
-        = { '\0' };
+    void *zeroes = g_malloc0(s->sectorsize - sizeof(lr.entry));
 
+    assert((1 << s->sectorbits) == s->sectorsize);
+    assert(bs->bl.request_alignment == s->sectorsize);
     assert(QEMU_IS_ALIGNED(offset, bs->bl.request_alignment));
     assert(QEMU_IS_ALIGNED(bytes, bs->bl.request_alignment));
 
     qemu_iovec_init(&log_qiov, niov + 2);
     qemu_iovec_add(&log_qiov, &lr.entry, sizeof(lr.entry));
-    qemu_iovec_add(&log_qiov, (void *)zeroes, sizeof(zeroes));
+    qemu_iovec_add(&log_qiov, zeroes, s->sectorsize - sizeof(lr.entry));
     for (i = 0; i < niov; ++i) {
         qemu_iovec_add(&log_qiov, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
     }
@@ -313,6 +329,7 @@ blk_log_writes_co_log(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     blk_log_writes_co_do_log(&lr);
 
     qemu_iovec_destroy(&log_qiov);
+    g_free(zeroes);
 
     if (lr.log_ret < 0) {
         return lr.log_ret;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 03/10] block: Add a mechanism for passing a block driver a block configuration

[Ari Sundholm]

Ping regarding this patch and the ones following it in the series. I 
would very much appreciate any feedback on the new callback and related 
plumbing I'm proposing for passing things like the guest block device 
sector size to block drivers (such as the filter type one proposed 
earlier in this series). Alternative ideas are also welcome.

Thank you,
Ari Sundholm
ari@tuxera.com

On 06/08/2018 03:32 PM, Ari Sundholm wrote:
> A block driver may need to know about the block configuration, most
> critically the sector sizes, of a block backend for alignment purposes
> or for some other reason. It doesn't seem like qemu has an existing
> mechanism for a block backend to convey the required information to
> the relevant block driver when it is being realized.
> 
> The need for this mechanism rises from the fact that a drive may not
> have a backend at the time it is created, as devices are created after
> drives during qemu startup. Therefore, a driver requiring information
> about the block configuration can get this information when a backend
> is created for it at the earliest. The most natural place for this to
> take place seems to be in the realization functions of the various
> block device drivers, such as scsi-hd. The interface proposed here
> allows the block driver to receive information about the block
> configuration and the associated backend through a new callback.
> 
> Signed-off-by: Ari Sundholm <ari@tuxera.com>
> ---
>   block/io.c                | 22 ++++++++++++++++++++++
>   hw/block/block.c          | 12 +++++++++++-
>   include/block/block.h     | 10 ++++++++++
>   include/block/block_int.h |  9 +++++++++
>   include/hw/block/block.h  |  1 +
>   5 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/block/io.c b/block/io.c
> index b7beaee..3a06843 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2932,3 +2932,25 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
>       bdrv_dec_in_flight(dst_bs);
>       return ret;
>   }
> +
> +void bdrv_apply_blkconf(BlockDriverState *bs, BlockConf *conf)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    if (!drv) {
> +        return;
> +    }
> +
> +    if (drv->bdrv_apply_blkconf) {
> +        drv->bdrv_apply_blkconf(bs, conf);
> +        return;
> +    }
> +
> +    if (bs->file && bs->file->bs) {
> +        bdrv_apply_blkconf(bs->file->bs, conf);
> +    }
> +
> +    if (bs->drv->supports_backing && backing_bs(bs)) {
> +        bdrv_apply_blkconf(backing_bs(bs), conf);
> +    }
> +}
> diff --git a/hw/block/block.c b/hw/block/block.c
> index b91e2b6..8c87dbf 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -38,7 +38,7 @@ void blkconf_blocksizes(BlockConf *conf)
>       /* fill in detected values if they are not defined via qemu command line */
>       if (!conf->physical_block_size) {
>           if (!backend_ret) {
> -           conf->physical_block_size = blocksizes.phys;
> +            conf->physical_block_size = blocksizes.phys;
>           } else {
>               conf->physical_block_size = BDRV_SECTOR_SIZE;
>           }
> @@ -52,6 +52,16 @@ void blkconf_blocksizes(BlockConf *conf)
>       }
>   }
>   
> +void blkconf_apply_to_blkdrv(BlockConf *conf)
> +{
> +    BlockBackend *blk = conf->blk;
> +    BlockDriverState *bs = blk_bs(blk);
> +
> +    if (bs) {
> +        bdrv_apply_blkconf(bs, conf);
> +    }
> +}
> +
>   bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
>                                      bool resizable, Error **errp)
>   {
> diff --git a/include/block/block.h b/include/block/block.h
> index 312ae01..5d0a8ba 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -10,6 +10,7 @@
>   #include "block/dirty-bitmap.h"
>   #include "block/blockjob.h"
>   #include "qemu/hbitmap.h"
> +#include "hw/block/block.h"
>   
>   /* block.c */
>   typedef struct BlockDriver BlockDriver;
> @@ -650,4 +651,13 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host);
>   int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
>                                       BdrvChild *dst, uint64_t dst_offset,
>                                       uint64_t bytes, BdrvRequestFlags flags);
> +
> +/*
> + * bdrv_apply_blkconf:
> + *
> + * Recursively finds the highest-level block drivers among the files and
> + * backing files that accept a block configuration and applies the given block
> + * configuration to them.
> + */
> +void bdrv_apply_blkconf(BlockDriverState *bs, BlockConf *conf);
>   #endif
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 888b7f7..cb82745 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -464,6 +464,15 @@ struct BlockDriver {
>       void (*bdrv_abort_perm_update)(BlockDriverState *bs);
>   
>       /**
> +     * Called to inform the driver of the block configuration of the virtual
> +     * block device.
> +     *
> +     * This function is called by a block device realization function if the
> +     * device wants to inform the block driver of its block configuration.
> +     */
> +    void (*bdrv_apply_blkconf)(BlockDriverState *bs, BlockConf *conf);
> +
> +    /**
>        * Returns in @nperm and @nshared the permissions that the driver for @bs
>        * needs on its child @c, based on the cumulative permissions requested by
>        * the parents in @parent_perm and @parent_shared.
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index d4f4dff..2861995 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -77,6 +77,7 @@ bool blkconf_geometry(BlockConf *conf, int *trans,
>                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
>                         Error **errp);
>   void blkconf_blocksizes(BlockConf *conf);
> +void blkconf_apply_to_blkdrv(BlockConf *conf);
>   bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
>                                      bool resizable, Error **errp);
>   
> 

Re: [Qemu-devel] [PATCH v4 00/10] New block driver: blklogwrites

[Ari Sundholm]

ping. Any comments or suggestions would be welcome whenever you have the 
time. :)

Thank you,
Ari Sundholm
ari@tuxera.com


On 06/08/2018 03:32 PM, Ari Sundholm wrote:
> This patch series adds a new block driver, blklogwrites, to QEMU. The
> driver is given two block devices: a raw device backed by an image or a
> host block device, and a log device, typically backed by a file, on
> which writes to the raw device are logged.
> 
> The logging format used is the same as in the dm-log-writes target of
> the Linux kernel device mapper. The log reflects the writes that have
> been performed on the guest block device and flushed. To be strict, the
> log may contain writes that have not been flushed yet, but they are
> technically outside the bounds of the log until the next flush updates
> the metadata in the log superblock. We believe these semantics to be
> useful even though they may not be completely identical to those of
> dm-log-writes.
> 
> This functionality can be used for crash consistency and fs consistency
> testing in filesystem drivers, including on non-Linux guests and older
> guests running Linux versions without support for dm-log-writes. This
> is simple and useful. Admittedly this and more advanced things could
> perhaps be done by extending the quorum driver, but this approach would
> require re-engineering the functionality and involve a more complicated
> setup, so we offer this simple solution which we have found useful
> internally.
> 
> In the first patch of this series, two block permission constants are
> moved from block.c to include/block/block.h to make them available
> outside of block.c. The next patch uses these constants.
> 
> The driver requires all requests to be aligned to the sector size. In
> the second patch of this series, which includes the bulk of the
> blklogwrites driver, this sector size is assumed to be equal to
> BDRV_SECTOR_SIZE for simplicity.
> 
> In patches 3-8, a mechanism to pass the block configuration of a block
> backend to a block driver is introduced, and this mechanism is
> integrated in various hw/block drivers.
> 
> In patch 9, blklogwrites is changed to use the block configuration of
> the relevant block backend to set its block limits.
> 
> Finally, in patch 10, blklogwrites is improved to use the logical
> sector size of the block backend for logging. This means that the
> sector size in the log metadata will be set to the logical sector size,
> and offsets and sizes of writes will be expressed as a multiple of that
> instead of BDRV_SECTOR_SIZE.
> 
> v4:
>    - Check return value of snprintf() (flagged by patchew)
>    - Don't use C99 for loop initial declaration (flagged by patchew, but
>      wasn't QEMU supposed to be C99? Oh well.)
>    - Add proper "Since" fields for blklogwrites in block-core.json
>    - Rebase on current upstream master
> 
> v3:
>    - Rebase on current upstream master
> 
> v2:
>    - Incorporate review feedback
>    - Add patches to apply block configurations in more block device
>      drivers
> 
> Aapo Vienamo (1):
>    block: Add blklogwrites
> 
> Ari Sundholm (9):
>    block: Move two block permission constants to the relevant enum
>    block: Add a mechanism for passing a block driver a block
>      configuration
>    hw/scsi/scsi-disk: Always apply block configuration to block driver
>    hw/ide/qdev: Always apply block configuration to block driver
>    hw/block/virtio-blk: Always apply block configuration to block driver
>    hw/block/nvme: Always apply block configuration to block driver
>    hw/block/fdc: Always apply block configuration to block driver
>    block/blklogwrites: Use block limits from the backend block
>      configuration
>    block/blklogwrites: Use the block device logical sector size when
>      logging writes
> 
>   MAINTAINERS               |   6 +
>   block.c                   |   6 -
>   block/Makefile.objs       |   1 +
>   block/blklogwrites.c      | 426 ++++++++++++++++++++++++++++++++++++++++++++++
>   block/io.c                |  22 +++
>   hw/block/block.c          |  12 +-
>   hw/block/fdc.c            |   4 +
>   hw/block/nvme.c           |   1 +
>   hw/block/virtio-blk.c     |   1 +
>   hw/ide/qdev.c             |   2 +
>   hw/scsi/scsi-disk.c       |   1 +
>   include/block/block.h     |  17 ++
>   include/block/block_int.h |   9 +
>   include/hw/block/block.h  |   1 +
>   qapi/block-core.json      |  30 +++-
>   15 files changed, 526 insertions(+), 13 deletions(-)
>   create mode 100644 block/blklogwrites.c
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v4 10/10] block/blklogwrites: Use the block device logical sector size when logging writes

[Alberto Garcia]

On Fri 08 Jun 2018 02:32:28 PM CEST, Ari Sundholm wrote:
> The guest OS may perform writes which are aligned to the logical
> sector size instead of the physical one, so logging at this granularity
> records the writes performed on the block device most faithfully.
>
> Signed-off-by: Ari Sundholm <ari@tuxera.com>
> ---
>  block/blklogwrites.c | 47 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> index 216367f..decf5e5 100644
> --- a/block/blklogwrites.c
> +++ b/block/blklogwrites.c
> @@ -47,6 +47,8 @@ struct log_write_entry {
>  
>  typedef struct {
>      BdrvChild *log_file;
> +    uint32_t sectorsize;
> +    uint32_t sectorbits;
>      uint64_t cur_log_sector;
>      uint64_t nr_entries;
>  } BDRVBlkLogWritesState;
> @@ -67,6 +69,8 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> +    s->sectorsize = BDRV_SECTOR_SIZE; /* May be updated later */
> +    s->sectorbits = BDRV_SECTOR_BITS;
>      s->cur_log_sector = 1;
>      s->nr_entries = 0;

I haven't looked closely into this series so sorry if there's something
that I'm overlooking, but this caught my attention: why do you have a
patch that improves a driver that you introduced earlier in this same
series?

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v4 10/10] block/blklogwrites: Use the block device logical sector size when logging writes

[Ari Sundholm]

On 06/18/2018 06:36 PM, Alberto Garcia wrote:
> On Fri 08 Jun 2018 02:32:28 PM CEST, Ari Sundholm wrote:
>> The guest OS may perform writes which are aligned to the logical
>> sector size instead of the physical one, so logging at this granularity
>> records the writes performed on the block device most faithfully.
>>
>> Signed-off-by: Ari Sundholm <ari@tuxera.com>
>> ---
>>   block/blklogwrites.c | 47 ++++++++++++++++++++++++++++++++---------------
>>   1 file changed, 32 insertions(+), 15 deletions(-)
>>
>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>> index 216367f..decf5e5 100644
>> --- a/block/blklogwrites.c
>> +++ b/block/blklogwrites.c
>> @@ -47,6 +47,8 @@ struct log_write_entry {
>>   
>>   typedef struct {
>>       BdrvChild *log_file;
>> +    uint32_t sectorsize;
>> +    uint32_t sectorbits;
>>       uint64_t cur_log_sector;
>>       uint64_t nr_entries;
>>   } BDRVBlkLogWritesState;
>> @@ -67,6 +69,8 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>           goto fail;
>>       }
>>   
>> +    s->sectorsize = BDRV_SECTOR_SIZE; /* May be updated later */
>> +    s->sectorbits = BDRV_SECTOR_BITS;
>>       s->cur_log_sector = 1;
>>       s->nr_entries = 0;
> 
> I haven't looked closely into this series so sorry if there's something
> that I'm overlooking, but this caught my attention: why do you have a
> patch that improves a driver that you introduced earlier in this same
> series?
> 

I guess there is no other real reason than that it reflects the 
development history of the driver more closely: the code by the original 
author did not contain proper support for non-512 sector sizes. I then 
added the parts needed for this.

The series could be restructured in the following manner if that makes 
it more reviewable (prerequisites first, then the entire driver):
- Patch 1: as before
- Patches 2-7: introduction of the mechanism to pass block 
configurations to drivers + changes to block device implementations to 
make them pass on this information (current patches 3-8)
- Patch 8: Introduction of the blklogwrites driver (current patches 2, 9 
and 10, squashed)

Does that sound like a good idea?

> Berto
> 

Thank you,
Ari Sundholm
ari@tuxera.com

Re: [Qemu-devel] [Qemu-block] [PATCH v4 10/10] block/blklogwrites: Use the block device logical sector size when logging writes

[Alberto Garcia]

On Mon 18 Jun 2018 05:53:50 PM CEST, Ari Sundholm wrote:
> On 06/18/2018 06:36 PM, Alberto Garcia wrote:
>> On Fri 08 Jun 2018 02:32:28 PM CEST, Ari Sundholm wrote:
>>> The guest OS may perform writes which are aligned to the logical
>>> sector size instead of the physical one, so logging at this granularity
>>> records the writes performed on the block device most faithfully.
>>>
>>> Signed-off-by: Ari Sundholm <ari@tuxera.com>
>>> ---
>>>   block/blklogwrites.c | 47 ++++++++++++++++++++++++++++++++---------------
>>>   1 file changed, 32 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>>> index 216367f..decf5e5 100644
>>> --- a/block/blklogwrites.c
>>> +++ b/block/blklogwrites.c
>>> @@ -47,6 +47,8 @@ struct log_write_entry {
>>>   
>>>   typedef struct {
>>>       BdrvChild *log_file;
>>> +    uint32_t sectorsize;
>>> +    uint32_t sectorbits;
>>>       uint64_t cur_log_sector;
>>>       uint64_t nr_entries;
>>>   } BDRVBlkLogWritesState;
>>> @@ -67,6 +69,8 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>>           goto fail;
>>>       }
>>>   
>>> +    s->sectorsize = BDRV_SECTOR_SIZE; /* May be updated later */
>>> +    s->sectorbits = BDRV_SECTOR_BITS;
>>>       s->cur_log_sector = 1;
>>>       s->nr_entries = 0;
>> 
>> I haven't looked closely into this series so sorry if there's something
>> that I'm overlooking, but this caught my attention: why do you have a
>> patch that improves a driver that you introduced earlier in this same
>> series?
>> 
>
> I guess there is no other real reason than that it reflects the
> development history of the driver more closely: the code by the
> original author did not contain proper support for non-512 sector
> sizes. I then added the parts needed for this.

Ah I see, so the driver was originally written by someone else and you
improved it.

As I said I haven't looked closely into the code (I hope I'll have time
to do it after I'm done with my blockdev-reopen work) so perhaps there's
good reason for this in this case, but in general I think that reviews
are simpler if one doesn't need to review code that is going to be
modified in the following patch.

> The series could be restructured in the following manner if that makes
> it more reviewable (prerequisites first, then the entire driver):

I just want to clarify that the code looks reviewable as it is now, it's
not like this is a blocker (for reviews at least) :)

> - Patch 1: as before
> - Patches 2-7: introduction of the mechanism to pass block
> configurations to drivers + changes to block device implementations to
> make them pass on this information (current patches 3-8)
> - Patch 8: Introduction of the blklogwrites driver (current patches 2,
> 9 and 10, squashed)
>
> Does that sound like a good idea?

It does, yes.

Thanks!

Berto