Re: [Qemu-devel] [PATCH v6 27/42] commit: Deal with filters

[Max Reitz]

On 31.08.19 12:44, Vladimir Sementsov-Ogievskiy wrote:
> 09.08.2019 19:13, Max Reitz wrote:
>> This includes some permission limiting (for example, we only need to
>> take the RESIZE permission if the base is smaller than the top).
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>   block/block-backend.c | 16 +++++---
>>   block/commit.c        | 96 +++++++++++++++++++++++++++++++------------
>>   blockdev.c            |  6 ++-
>>   3 files changed, 85 insertions(+), 33 deletions(-)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index c13c5c83b0..0bc592d023 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2180,11 +2180,17 @@ int blk_commit_all(void)
>>           AioContext *aio_context = blk_get_aio_context(blk);
>>   
>>           aio_context_acquire(aio_context);
>> -        if (blk_is_inserted(blk) && blk->root->bs->backing) {
>> -            int ret = bdrv_commit(blk->root->bs);
>> -            if (ret < 0) {
>> -                aio_context_release(aio_context);
>> -                return ret;
>> +        if (blk_is_inserted(blk)) {
>> +            BlockDriverState *non_filter;
>> +
>> +            /* Legacy function, so skip implicit filters */
>> +            non_filter = bdrv_skip_implicit_filters(blk->root->bs);
>> +            if (bdrv_filtered_cow_child(non_filter)) {
>> +                int ret = bdrv_commit(non_filter);
>> +                if (ret < 0) {
>> +                    aio_context_release(aio_context);
>> +                    return ret;
>> +                }
>>               }
> 
> and if non_filter is explicit filter we just skip it. I think we'd better 
> return
> error in this case. For example, just drop if (bdrv_filtered_cow_child) and 
> get
> ENOTSUP from bdrv_commit in this case.

Sounds good, yes.

> And with at least this fixed I'm OK with this patch:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> However some comments below:
> 
>>           }
>>           aio_context_release(aio_context);
>> diff --git a/block/commit.c b/block/commit.c
>> index 5a7672c7c7..40d1c8eeac 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
>>       BlockBackend *top;
>>       BlockBackend *base;
>>       BlockDriverState *base_bs;
>> +    BlockDriverState *above_base;
> 
> you called it base_overlay in mirror, seems better to keep same naming

Indeed.

[...]

>> @@ -315,17 +336,43 @@ void commit_start(const char *job_id, BlockDriverState 
>> *bs,
>>   
>>       s->commit_top_bs = commit_top_bs;
>>   
>> -    /* Block all nodes between top and base, because they will
>> -     * disappear from the chain after this operation. */
>> -    assert(bdrv_chain_contains(top, base));
>> -    for (iter = top; iter != base; iter = backing_bs(iter)) {
>> -        /* XXX BLK_PERM_WRITE needs to be allowed so we don't block 
>> ourselves
>> -         * at s->base (if writes are blocked for a node, they are also 
>> blocked
>> -         * for its backing file). The other options would be a second filter
>> -         * driver above s->base. */
> 
> This code part is absolutely equal to corresponding in block/mirror.c.. It 
> would be great
> to put it into a function and reuse. However its not about these series.

It would probably be great to just drop block/commit.c altogether and
fully merge it into block/mirror.c at some point.

(I suppose we’d just have to check whether there’s any parent who’s
taken the WRITE permission on the top node, and if so, emit READY (and
if not, skip to COMPLETED).)

[...]

>> @@ -412,19 +457,22 @@ int bdrv_commit(BlockDriverState *bs)
>>       if (!drv)
>>           return -ENOMEDIUM;
>>   
>> -    if (!bs->backing) {
>> +    backing_file_bs = bdrv_filtered_cow_bs(bs);
> 
> Hmm just note: if in future we'll have cow child which is not bs->backing, a 
> lot of code will
> fail, as we always assume that cow child is bs->backing. May be, this should 
> be commented in
> bdrv_filtered_cow_child implementation.

I couldn’t see why we’d ever do this.  I hope we never do.

(Aside from just removing bs->file and bs->backing altogether.)

Max

signature.asc
Description: OpenPGP digital signature