diff mbox series

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

Message ID 1603390423-980205-7-git-send-email-andrey.shinkevich@virtuozzo.com
State New
Headers show
Series Apply COR-filter to the block-stream permanently | expand

Commit Message

Xingtao Yao (Fujitsu)" via Oct. 22, 2020, 6:13 p.m. UTC
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 <andrey.shinkevich@virtuozzo.com>
---
 block/copy-on-read.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Vladimir Sementsov-Ogievskiy Oct. 23, 2020, 2:45 p.m. UTC | #1
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 <andrey.shinkevich@virtuozzo.com>

> ---

>   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, &child_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
Andrey Shinkevich Oct. 23, 2020, 3:31 p.m. UTC | #2
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 <andrey.shinkevich@virtuozzo.com>
>> ---
>>   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, 
>> &child_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
>>
> 
>
Vladimir Sementsov-Ogievskiy Oct. 23, 2020, 4:01 p.m. UTC | #3
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 <andrey.shinkevich@virtuozzo.com>
>>> ---
>>>   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, &child_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 <vsementsov@virtuozzo.com>

> 
>>> +            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
>>>
>>
>>
diff mbox series

Patch

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, &child_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