Re: [PATCH v12 06/14] copy-on-read: pass bottom node name to COR driver

[Vladimir Sementsov-Ogievskiy]

23.10.2020 18:31, Andrey Shinkevich wrote:


On 23.10.2020 17:45, Vladimir Sementsov-Ogievskiy wrote:

22.10.2020 21:13, Andrey Shinkevich wrote:

We are going to use the COR-filter for a block-stream job.
To limit COR operations by the base node in the backing chain during
stream job, pass the bottom node name, that is the first non-filter
overlay of the base, to the copy-on-read driver as the base node itself
may change due to possible concurrent jobs.
The rest of the functionality will be implemented in the patch that
follows.

Signed-off-by: Andrey Shinkevich 
---
  block/copy-on-read.c | 16 
  1 file changed, 16 insertions(+)

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 618c4c4..3d8e4db 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,18 +24,24 @@
  #include "block/block_int.h"
  #include "qemu/module.h"
  #include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qdict.h"
  #include "block/copy-on-read.h"
  typedef struct BDRVStateCOR {
  bool active;
+    BlockDriverState *bottom_bs;
  } BDRVStateCOR;
  static int cor_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
  {
+    BlockDriverState *bottom_bs = NULL;
  BDRVStateCOR *state = bs->opaque;
+    /* Find a bottom node name, if any */
+    const char *bottom_node = qdict_get_try_str(options, "bottom");
  bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
 BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -51,7 +57,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, 
int flags,
  ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
  bs->file->bs->supported_zero_flags);
+    if (bottom_node) {
+    bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
+    if (!bottom_bs) {
+    error_setg(errp, QERR_BASE_NOT_FOUND, bottom_node);


QERR_BASE_NOT_FOUND is unrelated here. Also, I see a comment in qerror.h that 
such macros should not be used in new code. And don't forget to drop qerror.h 
include line.



I have been surprized because I don't have it in my branch and instead I do:
error_setg(errp, "Bottom node '%s' not found", bottom_node);


OK, with it:

Reviewed-by: Vladimir Sementsov-Ogievskiy 




+    qdict_del(options, "bottom");


this may be moved above "bottom_bs = ..", to not call it after "if" in separate.



Please, see the "Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to 
COR driver".


Hm, assume that it may free the string itself, OK then.




+    return -EINVAL;
+    }
+    qdict_del(options, "bottom");
+    }
  state->active = true;
+    state->bottom_bs = bottom_bs;
  /*
   * We don't need to call bdrv_child_refresh_perms() now as the permissions







--
Best regards,
Vladimir

Re: [PATCH v12 06/14] copy-on-read: pass bottom node name to COR driver

[Andrey Shinkevich]

On 23.10.2020 17:45, Vladimir Sementsov-Ogievskiy wrote:

22.10.2020 21:13, Andrey Shinkevich wrote:

We are going to use the COR-filter for a block-stream job.
To limit COR operations by the base node in the backing chain during
stream job, pass the bottom node name, that is the first non-filter
overlay of the base, to the copy-on-read driver as the base node itself
may change due to possible concurrent jobs.
The rest of the functionality will be implemented in the patch that
follows.

Signed-off-by: Andrey Shinkevich 
---
  block/copy-on-read.c | 16 
  1 file changed, 16 insertions(+)

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 618c4c4..3d8e4db 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,18 +24,24 @@
  #include "block/block_int.h"
  #include "qemu/module.h"
  #include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qdict.h"
  #include "block/copy-on-read.h"
  typedef struct BDRVStateCOR {
  bool active;
+    BlockDriverState *bottom_bs;
  } BDRVStateCOR;
  static int cor_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
  {
+    BlockDriverState *bottom_bs = NULL;
  BDRVStateCOR *state = bs->opaque;
+    /* Find a bottom node name, if any */
+    const char *bottom_node = qdict_get_try_str(options, "bottom");
  bs->file = bdrv_open_child(NULL, options, "file", bs, 
_of_bds,
 BDRV_CHILD_FILTERED | 
BDRV_CHILD_PRIMARY,
@@ -51,7 +57,17 @@ static int cor_open(BlockDriverState *bs, QDict 
*options, int flags,

  ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
  bs->file->bs->supported_zero_flags);
+    if (bottom_node) {
+    bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
+    if (!bottom_bs) {
+    error_setg(errp, QERR_BASE_NOT_FOUND, bottom_node);


QERR_BASE_NOT_FOUND is unrelated here. Also, I see a comment in qerror.h 
that such macros should not be used in new code. And don't forget to 
drop qerror.h include line.




I have been surprized because I don't have it in my branch and instead I do:
error_setg(errp, "Bottom node '%s' not found", bottom_node);


+    qdict_del(options, "bottom");


this may be moved above "bottom_bs = ..", to not call it after "if" in 
separate.




Please, see the "Re: [PATCH v11 04/13] copy-on-read: pass overlay base 
node name to COR driver".



+    return -EINVAL;
+    }
+    qdict_del(options, "bottom");
+    }
  state->active = true;
+    state->bottom_bs = bottom_bs;
  /*
   * We don't need to call bdrv_child_refresh_perms() now as the 
permissions

Re: [PATCH v12 06/14] copy-on-read: pass bottom node name to COR driver

[Vladimir Sementsov-Ogievskiy]

22.10.2020 21:13, Andrey Shinkevich wrote:

We are going to use the COR-filter for a block-stream job.
To limit COR operations by the base node in the backing chain during
stream job, pass the bottom node name, that is the first non-filter
overlay of the base, to the copy-on-read driver as the base node itself
may change due to possible concurrent jobs.
The rest of the functionality will be implemented in the patch that
follows.

Signed-off-by: Andrey Shinkevich 
---
  block/copy-on-read.c | 16 
  1 file changed, 16 insertions(+)

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 618c4c4..3d8e4db 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,18 +24,24 @@
  #include "block/block_int.h"
  #include "qemu/module.h"
  #include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qdict.h"
  #include "block/copy-on-read.h"
  
  
  typedef struct BDRVStateCOR {

  bool active;
+BlockDriverState *bottom_bs;
  } BDRVStateCOR;
  
  
  static int cor_open(BlockDriverState *bs, QDict *options, int flags,

  Error **errp)
  {
+BlockDriverState *bottom_bs = NULL;
  BDRVStateCOR *state = bs->opaque;
+/* Find a bottom node name, if any */
+const char *bottom_node = qdict_get_try_str(options, "bottom");
  
  bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,

 BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -51,7 +57,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, 
int flags,
  ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
  bs->file->bs->supported_zero_flags);
  
+if (bottom_node) {

+bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
+if (!bottom_bs) {
+error_setg(errp, QERR_BASE_NOT_FOUND, bottom_node);


QERR_BASE_NOT_FOUND is unrelated here. Also, I see a comment in qerror.h that 
such macros should not be used in new code. And don't forget to drop qerror.h 
include line.


+qdict_del(options, "bottom");


this may be moved above "bottom_bs = ..", to not call it after "if" in separate.


+return -EINVAL;
+}
+qdict_del(options, "bottom");
+}
  state->active = true;
+state->bottom_bs = bottom_bs;
  
  /*

   * We don't need to call bdrv_child_refresh_perms() now as the permissions




--
Best regards,
Vladimir

[PATCH v12 06/14] copy-on-read: pass bottom node name to COR driver

[Andrey Shinkevich via]

We are going to use the COR-filter for a block-stream job.
To limit COR operations by the base node in the backing chain during
stream job, pass the bottom node name, that is the first non-filter
overlay of the base, to the copy-on-read driver as the base node itself
may change due to possible concurrent jobs.
The rest of the functionality will be implemented in the patch that
follows.

Signed-off-by: Andrey Shinkevich 
---
 block/copy-on-read.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 618c4c4..3d8e4db 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,18 +24,24 @@
 #include "block/block_int.h"
 #include "qemu/module.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qdict.h"
 #include "block/copy-on-read.h"
 
 
 typedef struct BDRVStateCOR {
 bool active;
+BlockDriverState *bottom_bs;
 } BDRVStateCOR;
 
 
 static int cor_open(BlockDriverState *bs, QDict *options, int flags,
 Error **errp)
 {
+BlockDriverState *bottom_bs = NULL;
 BDRVStateCOR *state = bs->opaque;
+/* Find a bottom node name, if any */
+const char *bottom_node = qdict_get_try_str(options, "bottom");
 
 bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -51,7 +57,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, 
int flags,
 ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
 bs->file->bs->supported_zero_flags);
 
+if (bottom_node) {
+bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
+if (!bottom_bs) {
+error_setg(errp, QERR_BASE_NOT_FOUND, bottom_node);
+qdict_del(options, "bottom");
+return -EINVAL;
+}
+qdict_del(options, "bottom");
+}
 state->active = true;
+state->bottom_bs = bottom_bs;
 
 /*
  * We don't need to call bdrv_child_refresh_perms() now as the permissions
-- 
1.8.3.1