Re: [Qemu-block] [PATCH 3/3] block/stream: introduce a bottom node

[Vladimir Sementsov-Ogievskiy]

29.03.2019 16:29, Andrey Shinkevich wrote:
> The bottom node is the intermediate block device that has the base as its
> backing image. It is used instead of the base node while a block stream
> job is running to avoid dependency on the base that may change due to the
> parallel jobs. The change may take place due to a filter node as well that
> is inserted between the base and the intermediate bottom node. It occurs
> when the base node is the top one for another commit or stream job.

Good to mention here that don't freeze backing child of bottom from this patch
(and iotest change shows it)

> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Signed-off-by: Andrey Shinkevich <address@hidden>
> ---
>   block/stream.c            | 56 
> +++++++++++++++++++++++++----------------------
>   block/trace-events        |  2 +-
>   blockdev.c                | 10 ++++++++-
>   include/block/block_int.h |  6 ++---
>   tests/qemu-iotests/245    |  4 ++--
>   5 files changed, 45 insertions(+), 33 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index c065e99..d4d0737 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -31,7 +31,7 @@ enum {
>   
>   typedef struct StreamBlockJob {
>       BlockJob common;
> -    BlockDriverState *base;
> +    BlockDriverState *bottom;
>       BlockdevOnError on_error;
>       char *backing_file_str;
>       bool bs_read_only;
> @@ -56,7 +56,7 @@ static void stream_abort(Job *job)
>   
>       if (s->chain_frozen) {
>           BlockJob *bjob = &s->common;
> -        bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->base);
> +        bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->bottom);
>       }
>   }
>   
> @@ -65,11 +65,11 @@ static int stream_prepare(Job *job)
>       StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>       BlockJob *bjob = &s->common;
>       BlockDriverState *bs = blk_bs(bjob->blk);
> -    BlockDriverState *base = s->base;
> +    BlockDriverState *base = backing_bs(s->bottom);
>       Error *local_err = NULL;
>       int ret = 0;
>   
> -    bdrv_unfreeze_backing_chain(bs, base);
> +    bdrv_unfreeze_backing_chain(bs, s->bottom);
>       s->chain_frozen = false;
>   
>       if (bs->backing) {
> @@ -112,7 +112,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
>       StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>       BlockBackend *blk = s->common.blk;
>       BlockDriverState *bs = blk_bs(blk);
> -    BlockDriverState *base = s->base;
> +    bool enable_cor = !!backing_bs(s->bottom);

s/!!/!

>       int64_t len;
>       int64_t offset = 0;
>       uint64_t delay_ns = 0;
> @@ -125,6 +125,11 @@ static int coroutine_fn stream_run(Job *job, Error 
> **errp)
>           return 0;
>       }
>   
> +    if (bs == s->bottom) {
> +        /* Nothing to stream */
> +        return 0;
> +    }

I think, with this "if" covers the previous (if (!bs->backing)), so the 
previous should be deleted.

> +
>       len = bdrv_getlength(bs);
>       if (len < 0) {
>           return len;
> @@ -138,7 +143,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
>        * backing chain since the copy-on-read operation does not take base 
> into
>        * account.
>        */
> -    if (!base) {
> +    if (!enable_cor) {

s/!//

>           bdrv_enable_copy_on_read(bs);
>       }
>   
> @@ -161,9 +166,8 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
>           } else if (ret >= 0) {
>               /* Copy if allocated in the intermediate images.  Limit to the
>                * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE).  
> */
> -            ret = bdrv_is_allocated_above(backing_bs(bs), base,
> -                                          offset, n, &n);
> -
> +            ret = bdrv_is_allocated_above_inclusive(backing_bs(bs), 
> s->bottom,
> +                                                    offset, n, &n);
>               /* Finish early if end of backing file has been reached */
>               if (ret == 0 && n == 0) {
>                   n = len - offset;
> @@ -200,7 +204,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
>           }
>       }
>   
> -    if (!base) {
> +    if (!enable_cor) {

s/!//

>           bdrv_disable_copy_on_read(bs);
>       }
>   
> @@ -225,13 +229,14 @@ static const BlockJobDriver stream_job_driver = {
>   };
>   

The rest looks good for me, just use bdrv_find_overlay instead of similar loop 
as Alberto suggested.


-- 
Best regards,
Vladimir