Re: [Qemu-devel] [PATCH 1/2] block: use internal filter node in backup

2017-08-15 Thread Manos Pitsidianakis

On Tue, Aug 15, 2017 at 10:44:15AM +0300, Vladimir Sementsov-Ogievskiy wrote:

15.08.2017 09:19, Manos Pitsidianakis wrote:

block/backup.c currently uses before write notifiers on the targeted
node. We can create a filter node instead to intercept write requests
for the backup job on the BDS level, instead of the BlockBackend level.



Hi Manos!

Looks interesting but what is the real benefit of it? It doesn't look 
like it simplifies the code.. Also, is it affect performance?

I think it worth describing in commit message.


The goal is to refactor the block layer into being more modular instead 
of having hard coded features. I don't suspect this will have any kind 
of performance gains, this is a minor change. Before write notifiers are 
not needed if you can intercept the write requests on the way.


signature.asc
Description: PGP signature


Re: [Qemu-devel] [PATCH 1/2] block: use internal filter node in backup

2017-08-15 Thread Vladimir Sementsov-Ogievskiy

15.08.2017 09:19, Manos Pitsidianakis wrote:

block/backup.c currently uses before write notifiers on the targeted
node. We can create a filter node instead to intercept write requests
for the backup job on the BDS level, instead of the BlockBackend level.



Hi Manos!

Looks interesting but what is the real benefit of it? It doesn't look 
like it simplifies the code.. Also, is it affect performance?

I think it worth describing in commit message.


--
Best regards,
Vladimir




[Qemu-devel] [PATCH 1/2] block: use internal filter node in backup

2017-08-14 Thread Manos Pitsidianakis
block/backup.c currently uses before write notifiers on the targeted
node. We can create a filter node instead to intercept write requests
for the backup job on the BDS level, instead of the BlockBackend level.

Signed-off-by: Manos Pitsidianakis 
---
 block.c|  89 +--
 block/backup.c | 207 -
 block/io.c |  10 +--
 block/mirror.c |   4 +-
 blockdev.c |   2 +-
 include/block/block.h  |   8 +-
 tests/qemu-iotests/141.out |   2 +-
 7 files changed, 277 insertions(+), 45 deletions(-)

diff --git a/block.c b/block.c
index 2de1c29eb3..81bd51b670 100644
--- a/block.c
+++ b/block.c
@@ -2088,6 +2088,38 @@ static void bdrv_parent_cb_resize(BlockDriverState *bs)
 }
 
 /*
+ * Sets the file link of a BDS. A new reference is created; callers
+ * which don't need their own reference any more must call bdrv_unref().
+ */
+void bdrv_set_file(BlockDriverState *bs, BlockDriverState *file_bs,
+   Error **errp)
+{
+if (file_bs) {
+bdrv_ref(file_bs);
+}
+
+if (bs->file) {
+bdrv_unref_child(bs, bs->file);
+}
+
+if (!file_bs) {
+bs->file = NULL;
+goto out;
+}
+
+bs->file = bdrv_attach_child(bs, file_bs, "file", &child_file,
+ errp);
+if (!bs->file) {
+bdrv_unref(file_bs);
+}
+
+bdrv_refresh_filename(bs);
+
+out:
+bdrv_refresh_limits(bs, NULL);
+}
+
+/*
  * Sets the backing file link of a BDS. A new reference is created; callers
  * which don't need their own reference any more must call bdrv_unref().
  */
@@ -2355,12 +2387,12 @@ static BlockDriverState 
*bdrv_append_temp_snapshot(BlockDriverState *bs,
 goto out;
 }
 
-/* bdrv_append() consumes a strong reference to bs_snapshot
+/* bdrv_append_backing() consumes a strong reference to bs_snapshot
  * (i.e. it will call bdrv_unref() on it) even on error, so in
  * order to be able to return one, we have to increase
  * bs_snapshot's refcount here */
 bdrv_ref(bs_snapshot);
-bdrv_append(bs_snapshot, bs, &local_err);
+bdrv_append_backing(bs_snapshot, bs, &local_err);
 if (local_err) {
 error_propagate(errp, local_err);
 bs_snapshot = NULL;
@@ -3142,7 +3174,7 @@ static bool should_update_child(BdrvChild *c, 
BlockDriverState *to)
 return false;
 }
 
-if (c->role == &child_backing) {
+if (c->role == &child_backing || c->role == &child_file) {
 /* If @from is a backing file of @to, ignore the child to avoid
  * creating a loop. We only want to change the pointer of other
  * parents. */
@@ -3213,6 +3245,45 @@ out:
 }
 
 /*
+ * Add new bs node at the top of a BDS chain while the chain is
+ * live, while keeping required fields on the top layer.
+ *
+ * This will modify the BlockDriverState fields, and swap contents
+ * between bs_new and bs_top. Both bs_new and bs_top are modified.
+ *
+ * bs_new must not be attached to a BlockBackend.
+ *
+ * bdrv_append_file() takes ownership of a bs_new reference and unrefs it
+ * because that's what the callers commonly need. bs_new will be referenced by
+ * the old parents of bs_top after bdrv_append_file() returns. If the caller
+ * needs to keep a reference of its own, it must call bdrv_ref().
+ */
+void bdrv_append_file(BlockDriverState *bs_new, BlockDriverState *bs_top,
+  Error **errp)
+{
+Error *local_err = NULL;
+
+bdrv_ref(bs_top);
+bdrv_set_file(bs_new, bs_top, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+bdrv_set_file(bs_new, NULL, &error_abort);
+goto out;
+}
+bdrv_replace_node(bs_top, bs_new, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+/* bs_new is now referenced by its new parents, we don't need the
+ * additional reference any more. */
+out:
+bdrv_unref(bs_top);
+bdrv_unref(bs_new);
+}
+
+/*
  * Add new bs contents at the top of an image chain while the chain is
  * live, while keeping required fields on the top layer.
  *
@@ -3223,13 +3294,13 @@ out:
  *
  * This function does not create any image files.
  *
- * bdrv_append() takes ownership of a bs_new reference and unrefs it because
- * that's what the callers commonly need. bs_new will be referenced by the old
- * parents of bs_top after bdrv_append() returns. If the caller needs to keep a
- * reference of its own, it must call bdrv_ref().
+ * bdrv_append_backing() takes ownership of a bs_new reference and unrefs it
+ * because that's what the callers commonly need. bs_new will be referenced by
+ * the old parents of bs_top after bdrv_append_backing() returns. If the caller
+ * needs to keep a reference of its own, it must call bdrv_ref().
  */
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
- Error **errp)