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

From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org, fam@euphon.net, stefanha@redhat.com,
	kwolf@redhat.com, mreitz@redhat.com, armbru@redhat.com,
	jsnow@redhat.com, eblake@redhat.com, den@openvz.org
Subject: Re: [PATCH v12 06/14] copy-on-read: pass bottom node name to COR driver
Date: Fri, 23 Oct 2020 18:31:54 +0300	[thread overview]
Message-ID: <96d5d3b8-8334-3900-7085-6fd7aeb8190b@virtuozzo.com> (raw)
In-Reply-To: <5cb6baa3-3035-9b50-3e45-a2a48bc8fdd6@virtuozzo.com>

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
>>
> 
> 