[Qemu-devel] [PATCH 0/5] nbd: Use BlockBackend

[Qemu-devel] [PATCH 0/5] nbd: Use BlockBackend

[Max Reitz]

>From the block layer's perspective, the nbd server is pretty similar to
a guest device. Therefore, it should use BlockBackend to access block
devices, just like any other guest device does.

This series consequently makes the nbd server use BlockBackend for
referencing block devices.


Max Reitz (5):
  block: Lift more functions into BlockBackend
  block: Add AioContextNotifier functions to BB
  block: Add blk_add_close_notifier() for BB
  nbd: Change external interface to BlockBackend
  nbd: Use BlockBackend internally

 block/block-backend.c          | 38 +++++++++++++++++++++++++
 blockdev-nbd.c                 | 15 +++++-----
 include/block/nbd.h            |  7 ++---
 include/sysemu/block-backend.h | 12 ++++++++
 nbd.c                          | 63 +++++++++++++++++++++---------------------
 qemu-nbd.c                     |  2 +-
 6 files changed, 94 insertions(+), 43 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/5] block: Lift more functions into BlockBackend

[Max Reitz]

There are already some blk_aio_* functions, so we might as well have
blk_co_* functions (as far as we need them). This patch adds
blk_co_flush(), blk_co_discard(), and also blk_invalidate_cache() (which
is not a blk_co_* function but is needed nonetheless).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 15 +++++++++++++++
 include/sysemu/block-backend.h |  3 +++
 2 files changed, 18 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index d0692b1..89f69b7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -497,6 +497,16 @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
     return bdrv_aio_ioctl(blk->bs, req, buf, cb, opaque);
 }
 
+int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors)
+{
+    return bdrv_co_discard(blk->bs, sector_num, nb_sectors);
+}
+
+int blk_co_flush(BlockBackend *blk)
+{
+    return bdrv_co_flush(blk->bs);
+}
+
 int blk_flush(BlockBackend *blk)
 {
     return bdrv_flush(blk->bs);
@@ -549,6 +559,11 @@ void blk_set_enable_write_cache(BlockBackend *blk, bool wce)
     bdrv_set_enable_write_cache(blk->bs, wce);
 }
 
+void blk_invalidate_cache(BlockBackend *blk, Error **errp)
+{
+    bdrv_invalidate_cache(blk->bs, errp);
+}
+
 int blk_is_inserted(BlockBackend *blk)
 {
     return bdrv_is_inserted(blk->bs);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 52d13c1..0c46b82 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -108,6 +108,8 @@ int blk_aio_multiwrite(BlockBackend *blk, BlockRequest *reqs, int num_reqs);
 int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
 BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
                           BlockCompletionFunc *cb, void *opaque);
+int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
+int blk_co_flush(BlockBackend *blk);
 int blk_flush(BlockBackend *blk);
 int blk_flush_all(void);
 void blk_drain_all(void);
@@ -120,6 +122,7 @@ int blk_is_read_only(BlockBackend *blk);
 int blk_is_sg(BlockBackend *blk);
 int blk_enable_write_cache(BlockBackend *blk);
 void blk_set_enable_write_cache(BlockBackend *blk, bool wce);
+void blk_invalidate_cache(BlockBackend *blk, Error **errp);
 int blk_is_inserted(BlockBackend *blk);
 void blk_lock_medium(BlockBackend *blk, bool locked);
 void blk_eject(BlockBackend *blk, bool eject_flag);
-- 
1.9.3

[Qemu-devel] [PATCH 2/5] block: Add AioContextNotifier functions to BB

[Max Reitz]

Because all BlockDriverStates behind a single BlockBackend reside in a
single AioContext, it is fine to just pass these functions
(blk_add_aio_context_notifier() and blk_remove_aio_context_notifier())
through to the root BlockDriverState.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 18 ++++++++++++++++++
 include/sysemu/block-backend.h |  8 ++++++++
 2 files changed, 26 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 89f69b7..7a7f690 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -624,6 +624,24 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
     bdrv_set_aio_context(blk->bs, new_context);
 }
 
+void blk_add_aio_context_notifier(BlockBackend *blk,
+        void (*attached_aio_context)(AioContext *new_context, void *opaque),
+        void (*detach_aio_context)(void *opaque), void *opaque)
+{
+    bdrv_add_aio_context_notifier(blk->bs, attached_aio_context,
+                                  detach_aio_context, opaque);
+}
+
+void blk_remove_aio_context_notifier(BlockBackend *blk,
+                                     void (*attached_aio_context)(AioContext *,
+                                                                  void *),
+                                     void (*detach_aio_context)(void *),
+                                     void *opaque)
+{
+    bdrv_remove_aio_context_notifier(blk->bs, attached_aio_context,
+                                     detach_aio_context, opaque);
+}
+
 void blk_io_plug(BlockBackend *blk)
 {
     bdrv_io_plug(blk->bs);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 0c46b82..d9c1337 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -135,6 +135,14 @@ void blk_op_block_all(BlockBackend *blk, Error *reason);
 void blk_op_unblock_all(BlockBackend *blk, Error *reason);
 AioContext *blk_get_aio_context(BlockBackend *blk);
 void blk_set_aio_context(BlockBackend *blk, AioContext *new_context);
+void blk_add_aio_context_notifier(BlockBackend *blk,
+        void (*attached_aio_context)(AioContext *new_context, void *opaque),
+        void (*detach_aio_context)(void *opaque), void *opaque);
+void blk_remove_aio_context_notifier(BlockBackend *blk,
+                                     void (*attached_aio_context)(AioContext *,
+                                                                  void *),
+                                     void (*detach_aio_context)(void *),
+                                     void *opaque);
 void blk_io_plug(BlockBackend *blk);
 void blk_io_unplug(BlockBackend *blk);
 BlockAcctStats *blk_get_stats(BlockBackend *blk);
-- 
1.9.3

[Qemu-devel] [PATCH 3/5] block: Add blk_add_close_notifier() for BB

[Max Reitz]

Adding something like a "delete notifier" to a BlockBackend would not
make much sense, because whoever is interested in registering there will
probably hold a reference to that BlockBackend; therefore, the notifier
will never be called (or only when the notifiee already relinquished its
reference and thus most probably is no longer interested in that
notification).

Therefore, this patch just passes through the close notifier interface
of the root BDS. This will be called when the device is ejected, for
instance, and therefore does make sense.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 5 +++++
 include/sysemu/block-backend.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 7a7f690..ef16d73 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -642,6 +642,11 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
                                      detach_aio_context, opaque);
 }
 
+void blk_add_close_notifier(BlockBackend *blk, Notifier *notify)
+{
+    bdrv_add_close_notifier(blk->bs, notify);
+}
+
 void blk_io_plug(BlockBackend *blk)
 {
     bdrv_io_plug(blk->bs);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index d9c1337..8871a02 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -143,6 +143,7 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
                                                                   void *),
                                      void (*detach_aio_context)(void *),
                                      void *opaque);
+void blk_add_close_notifier(BlockBackend *blk, Notifier *notify);
 void blk_io_plug(BlockBackend *blk);
 void blk_io_unplug(BlockBackend *blk);
 BlockAcctStats *blk_get_stats(BlockBackend *blk);
-- 
1.9.3

[Qemu-devel] [PATCH 4/5] nbd: Change external interface to BlockBackend

[Max Reitz]

Substitute BlockDriverState by BlockBackend in every globally visible
function provided by nbd.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev-nbd.c      | 15 ++++++++-------
 include/block/nbd.h |  7 +++----
 nbd.c               | 11 ++++++-----
 qemu-nbd.c          |  2 +-
 4 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 06f901e..22e95d1 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -10,6 +10,7 @@
  */
 
 #include "sysemu/blockdev.h"
+#include "sysemu/block-backend.h"
 #include "hw/block/block.h"
 #include "monitor/monitor.h"
 #include "qapi/qmp/qerror.h"
@@ -73,7 +74,7 @@ static void nbd_close_notifier(Notifier *n, void *data)
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
                         Error **errp)
 {
-    BlockDriverState *bs;
+    BlockBackend *blk;
     NBDExport *exp;
     NBDCloseNotifier *n;
 
@@ -87,12 +88,12 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         return;
     }
 
-    bs = bdrv_find(device);
-    if (!bs) {
+    blk = blk_by_name(device);
+    if (!blk) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
-    if (!bdrv_is_inserted(bs)) {
+    if (!blk_is_inserted(blk)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return;
     }
@@ -100,18 +101,18 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     if (!has_writable) {
         writable = false;
     }
-    if (bdrv_is_read_only(bs)) {
+    if (blk_is_read_only(blk)) {
         writable = false;
     }
 
-    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL);
+    exp = nbd_export_new(blk, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL);
 
     nbd_export_set_name(exp, device);
 
     n = g_new0(NBDCloseNotifier, 1);
     n->n.notify = nbd_close_notifier;
     n->exp = exp;
-    bdrv_add_close_notifier(bs, &n->n);
+    blk_add_close_notifier(blk, &n->n);
     QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
 }
 
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9e835d2..348302c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -85,14 +85,13 @@ int nbd_disconnect(int fd);
 typedef struct NBDExport NBDExport;
 typedef struct NBDClient NBDClient;
 
-NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
-                          off_t size, uint32_t nbdflags,
-                          void (*close)(NBDExport *));
+NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
+                          uint32_t nbdflags, void (*close)(NBDExport *));
 void nbd_export_close(NBDExport *exp);
 void nbd_export_get(NBDExport *exp);
 void nbd_export_put(NBDExport *exp);
 
-BlockDriverState *nbd_export_get_blockdev(NBDExport *exp);
+BlockBackend *nbd_export_get_blockdev(NBDExport *exp);
 
 NBDExport *nbd_export_find(const char *name);
 void nbd_export_set_name(NBDExport *exp, const char *name);
diff --git a/nbd.c b/nbd.c
index a7bce45..3fd5743 100644
--- a/nbd.c
+++ b/nbd.c
@@ -19,6 +19,7 @@
 #include "block/nbd.h"
 #include "block/block.h"
 #include "block/block_int.h"
+#include "sysemu/block-backend.h"
 
 #include "block/coroutine.h"
 
@@ -957,10 +958,10 @@ static void bs_aio_detach(void *opaque)
     exp->ctx = NULL;
 }
 
-NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
-                          off_t size, uint32_t nbdflags,
-                          void (*close)(NBDExport *))
+NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
+                          uint32_t nbdflags, void (*close)(NBDExport *))
 {
+    BlockDriverState *bs = blk_bs(blk);
     NBDExport *exp = g_malloc0(sizeof(NBDExport));
     exp->refcount = 1;
     QTAILQ_INIT(&exp->clients);
@@ -1056,9 +1057,9 @@ void nbd_export_put(NBDExport *exp)
     }
 }
 
-BlockDriverState *nbd_export_get_blockdev(NBDExport *exp)
+BlockBackend *nbd_export_get_blockdev(NBDExport *exp)
 {
-    return exp->bs;
+    return exp->bs->blk;
 }
 
 void nbd_export_close_all(void)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 5cd6c6d..60ce50f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -730,7 +730,7 @@ int main(int argc, char **argv)
         }
     }
 
-    exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, nbd_export_closed);
+    exp = nbd_export_new(blk, dev_offset, fd_size, nbdflags, nbd_export_closed);
 
     if (sockpath) {
         fd = unix_socket_incoming(sockpath);
-- 
1.9.3

[Qemu-devel] [PATCH 5/5] nbd: Use BlockBackend internally

[Max Reitz]

With all externally visible functions changed to use BlockBackend, this
patch makes nbd use BlockBackend for everything internally as well.

While touching them, substitute 512 by BDRV_SECTOR_SIZE in the calls to
blk_read(), blk_write() and blk_co_discard().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 nbd.c | 56 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/nbd.c b/nbd.c
index 3fd5743..53cf82b 100644
--- a/nbd.c
+++ b/nbd.c
@@ -17,8 +17,6 @@
  */
 
 #include "block/nbd.h"
-#include "block/block.h"
-#include "block/block_int.h"
 #include "sysemu/block-backend.h"
 
 #include "block/coroutine.h"
@@ -102,7 +100,7 @@ struct NBDExport {
     int refcount;
     void (*close)(NBDExport *exp);
 
-    BlockDriverState *bs;
+    BlockBackend *blk;
     char *name;
     off_t dev_offset;
     off_t size;
@@ -930,7 +928,7 @@ static void nbd_request_put(NBDRequest *req)
     nbd_client_put(client);
 }
 
-static void bs_aio_attached(AioContext *ctx, void *opaque)
+static void blk_aio_attached(AioContext *ctx, void *opaque)
 {
     NBDExport *exp = opaque;
     NBDClient *client;
@@ -944,7 +942,7 @@ static void bs_aio_attached(AioContext *ctx, void *opaque)
     }
 }
 
-static void bs_aio_detach(void *opaque)
+static void blk_aio_detach(void *opaque)
 {
     NBDExport *exp = opaque;
     NBDClient *client;
@@ -961,24 +959,23 @@ static void bs_aio_detach(void *opaque)
 NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
                           uint32_t nbdflags, void (*close)(NBDExport *))
 {
-    BlockDriverState *bs = blk_bs(blk);
     NBDExport *exp = g_malloc0(sizeof(NBDExport));
     exp->refcount = 1;
     QTAILQ_INIT(&exp->clients);
-    exp->bs = bs;
+    exp->blk = blk;
     exp->dev_offset = dev_offset;
     exp->nbdflags = nbdflags;
-    exp->size = size == -1 ? bdrv_getlength(bs) : size;
+    exp->size = size == -1 ? blk_getlength(blk) : size;
     exp->close = close;
-    exp->ctx = bdrv_get_aio_context(bs);
-    bdrv_ref(bs);
-    bdrv_add_aio_context_notifier(bs, bs_aio_attached, bs_aio_detach, exp);
+    exp->ctx = blk_get_aio_context(blk);
+    blk_ref(blk);
+    blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
     /*
      * NBD exports are used for non-shared storage migration.  Make sure
      * that BDRV_O_INCOMING is cleared and the image is ready for write
      * access since the export could be available before migration handover.
      */
-    bdrv_invalidate_cache(bs, NULL);
+    blk_invalidate_cache(blk, NULL);
     return exp;
 }
 
@@ -1025,11 +1022,11 @@ void nbd_export_close(NBDExport *exp)
     }
     nbd_export_set_name(exp, NULL);
     nbd_export_put(exp);
-    if (exp->bs) {
-        bdrv_remove_aio_context_notifier(exp->bs, bs_aio_attached,
-                                         bs_aio_detach, exp);
-        bdrv_unref(exp->bs);
-        exp->bs = NULL;
+    if (exp->blk) {
+        blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
+                                        blk_aio_detach, exp);
+        blk_unref(exp->blk);
+        exp->blk = NULL;
     }
 }
 
@@ -1059,7 +1056,7 @@ void nbd_export_put(NBDExport *exp)
 
 BlockBackend *nbd_export_get_blockdev(NBDExport *exp)
 {
-    return exp->bs->blk;
+    return exp->blk;
 }
 
 void nbd_export_close_all(void)
@@ -1138,7 +1135,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
 
     command = request->type & NBD_CMD_MASK_COMMAND;
     if (command == NBD_CMD_READ || command == NBD_CMD_WRITE) {
-        req->data = qemu_blockalign(client->exp->bs, request->len);
+        req->data = blk_blockalign(client->exp->blk, request->len);
     }
     if (command == NBD_CMD_WRITE) {
         TRACE("Reading %u byte(s)", request->len);
@@ -1204,7 +1201,7 @@ static void nbd_trip(void *opaque)
         TRACE("Request type is READ");
 
         if (request.type & NBD_CMD_FLAG_FUA) {
-            ret = bdrv_co_flush(exp->bs);
+            ret = blk_co_flush(exp->blk);
             if (ret < 0) {
                 LOG("flush failed");
                 reply.error = -ret;
@@ -1212,8 +1209,9 @@ static void nbd_trip(void *opaque)
             }
         }
 
-        ret = bdrv_read(exp->bs, (request.from + exp->dev_offset) / 512,
-                        req->data, request.len / 512);
+        ret = blk_read(exp->blk,
+                       (request.from + exp->dev_offset) / BDRV_SECTOR_SIZE,
+                       req->data, request.len / BDRV_SECTOR_SIZE);
         if (ret < 0) {
             LOG("reading from file failed");
             reply.error = -ret;
@@ -1235,8 +1233,9 @@ static void nbd_trip(void *opaque)
 
         TRACE("Writing to device");
 
-        ret = bdrv_write(exp->bs, (request.from + exp->dev_offset) / 512,
-                         req->data, request.len / 512);
+        ret = blk_write(exp->blk,
+                        (request.from + exp->dev_offset) / BDRV_SECTOR_SIZE,
+                        req->data, request.len / BDRV_SECTOR_SIZE);
         if (ret < 0) {
             LOG("writing to file failed");
             reply.error = -ret;
@@ -1244,7 +1243,7 @@ static void nbd_trip(void *opaque)
         }
 
         if (request.type & NBD_CMD_FLAG_FUA) {
-            ret = bdrv_co_flush(exp->bs);
+            ret = blk_co_flush(exp->blk);
             if (ret < 0) {
                 LOG("flush failed");
                 reply.error = -ret;
@@ -1263,7 +1262,7 @@ static void nbd_trip(void *opaque)
     case NBD_CMD_FLUSH:
         TRACE("Request type is FLUSH");
 
-        ret = bdrv_co_flush(exp->bs);
+        ret = blk_co_flush(exp->blk);
         if (ret < 0) {
             LOG("flush failed");
             reply.error = -ret;
@@ -1274,8 +1273,9 @@ static void nbd_trip(void *opaque)
         break;
     case NBD_CMD_TRIM:
         TRACE("Request type is TRIM");
-        ret = bdrv_co_discard(exp->bs, (request.from + exp->dev_offset) / 512,
-                              request.len / 512);
+        ret = blk_co_discard(exp->blk, (request.from + exp->dev_offset)
+                                       / BDRV_SECTOR_SIZE,
+                             request.len / BDRV_SECTOR_SIZE);
         if (ret < 0) {
             LOG("discard failed");
             reply.error = -ret;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/5] block: Add AioContextNotifier functions to BB

[Paolo Bonzini]

On 17/11/2014 16:30, Max Reitz wrote:
> Because all BlockDriverStates behind a single BlockBackend reside in a
> single AioContext, it is fine to just pass these functions
> (blk_add_aio_context_notifier() and blk_remove_aio_context_notifier())
> through to the root BlockDriverState.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

The logical question then is: are there any function in BlockDriverState
that do not make sense as a BlockBackend API?

Paolo


> ---
>  block/block-backend.c          | 18 ++++++++++++++++++
>  include/sysemu/block-backend.h |  8 ++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 89f69b7..7a7f690 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -624,6 +624,24 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
>      bdrv_set_aio_context(blk->bs, new_context);
>  }
>  
> +void blk_add_aio_context_notifier(BlockBackend *blk,
> +        void (*attached_aio_context)(AioContext *new_context, void *opaque),
> +        void (*detach_aio_context)(void *opaque), void *opaque)
> +{
> +    bdrv_add_aio_context_notifier(blk->bs, attached_aio_context,
> +                                  detach_aio_context, opaque);
> +}
> +
> +void blk_remove_aio_context_notifier(BlockBackend *blk,
> +                                     void (*attached_aio_context)(AioContext *,
> +                                                                  void *),
> +                                     void (*detach_aio_context)(void *),
> +                                     void *opaque)
> +{
> +    bdrv_remove_aio_context_notifier(blk->bs, attached_aio_context,
> +                                     detach_aio_context, opaque);
> +}
> +
>  void blk_io_plug(BlockBackend *blk)
>  {
>      bdrv_io_plug(blk->bs);
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 0c46b82..d9c1337 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -135,6 +135,14 @@ void blk_op_block_all(BlockBackend *blk, Error *reason);
>  void blk_op_unblock_all(BlockBackend *blk, Error *reason);
>  AioContext *blk_get_aio_context(BlockBackend *blk);
>  void blk_set_aio_context(BlockBackend *blk, AioContext *new_context);
> +void blk_add_aio_context_notifier(BlockBackend *blk,
> +        void (*attached_aio_context)(AioContext *new_context, void *opaque),
> +        void (*detach_aio_context)(void *opaque), void *opaque);
> +void blk_remove_aio_context_notifier(BlockBackend *blk,
> +                                     void (*attached_aio_context)(AioContext *,
> +                                                                  void *),
> +                                     void (*detach_aio_context)(void *),
> +                                     void *opaque);
>  void blk_io_plug(BlockBackend *blk);
>  void blk_io_unplug(BlockBackend *blk);
>  BlockAcctStats *blk_get_stats(BlockBackend *blk);
> 

Re: [Qemu-devel] [PATCH 5/5] nbd: Use BlockBackend internally

[Paolo Bonzini]

On 17/11/2014 16:30, Max Reitz wrote:
> With all externally visible functions changed to use BlockBackend, this
> patch makes nbd use BlockBackend for everything internally as well.
> 
> While touching them, substitute 512 by BDRV_SECTOR_SIZE in the calls to
> blk_read(), blk_write() and blk_co_discard().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

qemu-nbd.c has more uses of "bs" that probably should be changed to
"blk".  Not sure if it should be in this patch, patch 4 or an additional
patch 6 though.

If the third possibility, series

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> ---
>  nbd.c | 56 ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index 3fd5743..53cf82b 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -17,8 +17,6 @@
>   */
>  
>  #include "block/nbd.h"
> -#include "block/block.h"
> -#include "block/block_int.h"
>  #include "sysemu/block-backend.h"
>  
>  #include "block/coroutine.h"
> @@ -102,7 +100,7 @@ struct NBDExport {
>      int refcount;
>      void (*close)(NBDExport *exp);
>  
> -    BlockDriverState *bs;
> +    BlockBackend *blk;
>      char *name;
>      off_t dev_offset;
>      off_t size;
> @@ -930,7 +928,7 @@ static void nbd_request_put(NBDRequest *req)
>      nbd_client_put(client);
>  }
>  
> -static void bs_aio_attached(AioContext *ctx, void *opaque)
> +static void blk_aio_attached(AioContext *ctx, void *opaque)
>  {
>      NBDExport *exp = opaque;
>      NBDClient *client;
> @@ -944,7 +942,7 @@ static void bs_aio_attached(AioContext *ctx, void *opaque)
>      }
>  }
>  
> -static void bs_aio_detach(void *opaque)
> +static void blk_aio_detach(void *opaque)
>  {
>      NBDExport *exp = opaque;
>      NBDClient *client;
> @@ -961,24 +959,23 @@ static void bs_aio_detach(void *opaque)
>  NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
>                            uint32_t nbdflags, void (*close)(NBDExport *))
>  {
> -    BlockDriverState *bs = blk_bs(blk);
>      NBDExport *exp = g_malloc0(sizeof(NBDExport));
>      exp->refcount = 1;
>      QTAILQ_INIT(&exp->clients);
> -    exp->bs = bs;
> +    exp->blk = blk;
>      exp->dev_offset = dev_offset;
>      exp->nbdflags = nbdflags;
> -    exp->size = size == -1 ? bdrv_getlength(bs) : size;
> +    exp->size = size == -1 ? blk_getlength(blk) : size;
>      exp->close = close;
> -    exp->ctx = bdrv_get_aio_context(bs);
> -    bdrv_ref(bs);
> -    bdrv_add_aio_context_notifier(bs, bs_aio_attached, bs_aio_detach, exp);
> +    exp->ctx = blk_get_aio_context(blk);
> +    blk_ref(blk);
> +    blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
>      /*
>       * NBD exports are used for non-shared storage migration.  Make sure
>       * that BDRV_O_INCOMING is cleared and the image is ready for write
>       * access since the export could be available before migration handover.
>       */
> -    bdrv_invalidate_cache(bs, NULL);
> +    blk_invalidate_cache(blk, NULL);
>      return exp;
>  }
>  
> @@ -1025,11 +1022,11 @@ void nbd_export_close(NBDExport *exp)
>      }
>      nbd_export_set_name(exp, NULL);
>      nbd_export_put(exp);
> -    if (exp->bs) {
> -        bdrv_remove_aio_context_notifier(exp->bs, bs_aio_attached,
> -                                         bs_aio_detach, exp);
> -        bdrv_unref(exp->bs);
> -        exp->bs = NULL;
> +    if (exp->blk) {
> +        blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
> +                                        blk_aio_detach, exp);
> +        blk_unref(exp->blk);
> +        exp->blk = NULL;
>      }
>  }
>  
> @@ -1059,7 +1056,7 @@ void nbd_export_put(NBDExport *exp)
>  
>  BlockBackend *nbd_export_get_blockdev(NBDExport *exp)
>  {
> -    return exp->bs->blk;
> +    return exp->blk;
>  }
>  
>  void nbd_export_close_all(void)
> @@ -1138,7 +1135,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
>  
>      command = request->type & NBD_CMD_MASK_COMMAND;
>      if (command == NBD_CMD_READ || command == NBD_CMD_WRITE) {
> -        req->data = qemu_blockalign(client->exp->bs, request->len);
> +        req->data = blk_blockalign(client->exp->blk, request->len);
>      }
>      if (command == NBD_CMD_WRITE) {
>          TRACE("Reading %u byte(s)", request->len);
> @@ -1204,7 +1201,7 @@ static void nbd_trip(void *opaque)
>          TRACE("Request type is READ");
>  
>          if (request.type & NBD_CMD_FLAG_FUA) {
> -            ret = bdrv_co_flush(exp->bs);
> +            ret = blk_co_flush(exp->blk);
>              if (ret < 0) {
>                  LOG("flush failed");
>                  reply.error = -ret;
> @@ -1212,8 +1209,9 @@ static void nbd_trip(void *opaque)
>              }
>          }
>  
> -        ret = bdrv_read(exp->bs, (request.from + exp->dev_offset) / 512,
> -                        req->data, request.len / 512);
> +        ret = blk_read(exp->blk,
> +                       (request.from + exp->dev_offset) / BDRV_SECTOR_SIZE,
> +                       req->data, request.len / BDRV_SECTOR_SIZE);
>          if (ret < 0) {
>              LOG("reading from file failed");
>              reply.error = -ret;
> @@ -1235,8 +1233,9 @@ static void nbd_trip(void *opaque)
>  
>          TRACE("Writing to device");
>  
> -        ret = bdrv_write(exp->bs, (request.from + exp->dev_offset) / 512,
> -                         req->data, request.len / 512);
> +        ret = blk_write(exp->blk,
> +                        (request.from + exp->dev_offset) / BDRV_SECTOR_SIZE,
> +                        req->data, request.len / BDRV_SECTOR_SIZE);
>          if (ret < 0) {
>              LOG("writing to file failed");
>              reply.error = -ret;
> @@ -1244,7 +1243,7 @@ static void nbd_trip(void *opaque)
>          }
>  
>          if (request.type & NBD_CMD_FLAG_FUA) {
> -            ret = bdrv_co_flush(exp->bs);
> +            ret = blk_co_flush(exp->blk);
>              if (ret < 0) {
>                  LOG("flush failed");
>                  reply.error = -ret;
> @@ -1263,7 +1262,7 @@ static void nbd_trip(void *opaque)
>      case NBD_CMD_FLUSH:
>          TRACE("Request type is FLUSH");
>  
> -        ret = bdrv_co_flush(exp->bs);
> +        ret = blk_co_flush(exp->blk);
>          if (ret < 0) {
>              LOG("flush failed");
>              reply.error = -ret;
> @@ -1274,8 +1273,9 @@ static void nbd_trip(void *opaque)
>          break;
>      case NBD_CMD_TRIM:
>          TRACE("Request type is TRIM");
> -        ret = bdrv_co_discard(exp->bs, (request.from + exp->dev_offset) / 512,
> -                              request.len / 512);
> +        ret = blk_co_discard(exp->blk, (request.from + exp->dev_offset)
> +                                       / BDRV_SECTOR_SIZE,
> +                             request.len / BDRV_SECTOR_SIZE);
>          if (ret < 0) {
>              LOG("discard failed");
>              reply.error = -ret;
> 

Re: [Qemu-devel] [PATCH 2/5] block: Add AioContextNotifier functions to BB

[Max Reitz]

On 2014-11-17 at 18:26, Paolo Bonzini wrote:
> On 17/11/2014 16:30, Max Reitz wrote:
>> Because all BlockDriverStates behind a single BlockBackend reside in a
>> single AioContext, it is fine to just pass these functions
>> (blk_add_aio_context_notifier() and blk_remove_aio_context_notifier())
>> through to the root BlockDriverState.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> The logical question then is: are there any function in BlockDriverState
> that do not make sense as a BlockBackend API?

Well, surely bdrv_swap(), or bdrv_drop_intermediate(). These are 
functions which work on multiple BDSs (in the same tree, that is, behind 
the same BB) so they don't make sense on the BB.

Others could simply be passed through to the root BDS but it somehow 
doesn't make sense to execute them on the BB. For instance, 
bdrv_set_key(); this is something for an individual BDS, in contrast to 
other operations like reading and writing which will probably be passed 
through the BDS tree; or bdrv_get_info().

The functions added in this patch do make sense on a BB level: Since all 
BDSs behind one BB are always in the same AioContext, it makes sense to 
consider that the BB's AioContext.

The next patch is more difficult to justify. Closing a BDS is somehow 
passed through, but at first glance it doesn't make a whole lot of sense 
on the BB level. However, when you consider (as far as I looked into it) 
that a BDS is only closed when there are either no references to it 
(which will not happen as long as it has a BB) or when it is ejected, it 
suddenly does make sense: "Ejecting" really is something for the BB, so 
it makes sense to wait for that event (even though the name "close 
notifier" doesn't sound much like it...). Maybe I should sometimes take 
a deeper look into when a BDS belonging to a BB may be closed and if 
it's really only due to ejection, rename the "close notifiers" to 
something like "eject notifiers" (on the BB level).

Max

Re: [Qemu-devel] [PATCH 5/5] nbd: Use BlockBackend internally

[Max Reitz]

On 2014-11-17 at 18:29, Paolo Bonzini wrote:
>
> On 17/11/2014 16:30, Max Reitz wrote:
>> With all externally visible functions changed to use BlockBackend, this
>> patch makes nbd use BlockBackend for everything internally as well.
>>
>> While touching them, substitute 512 by BDRV_SECTOR_SIZE in the calls to
>> blk_read(), blk_write() and blk_co_discard().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> qemu-nbd.c has more uses of "bs" that probably should be changed to
> "blk".  Not sure if it should be in this patch, patch 4 or an additional
> patch 6 though.

Good point. I'll go for patch 6.

> If the third possibility, series
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks!

Max

Re: [Qemu-devel] [PATCH 2/5] block: Add AioContextNotifier functions to BB

[Paolo Bonzini]

On 18/11/2014 10:26, Max Reitz wrote:
> However, when you consider (as far as I looked into it) that a BDS is
> only closed when there are either no references to it (which will not
> happen as long as it has a BB) or when it is ejected, it suddenly does
> make sense: "Ejecting" really is something for the BB, so it makes sense
> to wait for that event (even though the name "close notifier" doesn't
> sound much like it...).

I agree, and indeed the notifier was added to deal with ejection.

Thanks for clarifying.  I guess if QEMU were written in C++ we would
have a hierarchy like this:

    BlockDevice (abstract)
        BlockBackend
        BlockDriverState

and there would be no duplication of function names; the BlockBackend
implementation would still have to forward to the top BlockDriverState.

Paolo