Re: [Qemu-devel] [PATCH v3 32/44] mirror: Use real permissions in mirror/active commit block job

[Max Reitz]

On 28.02.2017 13:54, Kevin Wolf wrote:
> The mirror block job is mainly used for two different scenarios:
> Mirroring to an otherwise unused, independent target node, or for active
> commit where the target node is part of the backing chain of the source.
> 
> Similarly to the commit block job patch, we need to insert a new filter
> node to keep the permissions correct during active commit.
> 
> Note that one change this implies is that job->blk points to
> mirror_top_bs as its root now, and mirror_top_bs (rather than the actual
> source node) contains the bs->job pointer. This requires qemu-img commit
> to get the job by name now rather than just taking bs->job.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/mirror.c             | 216 
> ++++++++++++++++++++++++++++++++++++++-------
>  qemu-img.c                 |   6 +-
>  tests/qemu-iotests/141     |   2 +-
>  tests/qemu-iotests/141.out |   4 +-
>  4 files changed, 190 insertions(+), 38 deletions(-)

BTW, seeing this commit made me remember that the additional functions I
requested for the dummy block driver (flush etc.) were not meant to be
limited to mirror; the commit node would need get_block_status, too, of
course (not necessarily flush, discard, or write_zeroes, because the
commit node doesn't allow writing anyway). Can be fixed during freeze,
though.

> diff --git a/block/mirror.c b/block/mirror.c
> index beaac6f..a2f22e1 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c

[...]

> @@ -983,6 +1005,77 @@ static const BlockJobDriver commit_active_job_driver = {
>      .drain                  = mirror_drain,
>  };
>  
> +static int coroutine_fn bdrv_mirror_top_preadv(BlockDriverState *bs,
> +    uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
> +{
> +    return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
> +}
> +
> +static int coroutine_fn bdrv_mirror_top_pwritev(BlockDriverState *bs,
> +    uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
> +{
> +    return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
> +}
> +
> +static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
> +{
> +    return bdrv_co_flush(bs->backing->bs);
> +}
> +
> +static int64_t coroutine_fn bdrv_mirror_top_get_block_status(
> +    BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
> +    BlockDriverState **file)
> +{
> +    *pnum = nb_sectors;
> +    *file = bs->backing->bs;
> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |

BDRV_BLOCK_RAW makes bdrv_co_get_block_status() fall through to
bs->file, though.

I'm willing to turn two blind eyes on this, though, and consider that to
be a bug in bdrv_co_get_block_status(); it should fall through to *file.

OTOH, as I said before, the bs->backing without bs->file thing also
breaks bdrv_refresh_filename(). We could fix that, too, by falling
through to bs->backing if there is no bs->file, but that would be a
hack. The other solution is to implement .bdrv_refresh_filename(), of
course.

Since I only have two eyes I can turn blind, I can't really give an R-b, so:

Acked-by: Max Reitz <address@hidden>

> +           (sector_num << BDRV_SECTOR_BITS);
> +}

signature.asc
Description: OpenPGP digital signature