Re: [Qemu-block] [PATCH v2 1/2] block: use internal filter node in backup

[Kevin Wolf]

Am 15.08.2017 um 10:18 hat Manos Pitsidianakis geschrieben:
> block/backup.c currently uses before write notifiers on the targeted
> node. We can create a filter node instead to intercept write requests
> for the backup job on the BDS level, instead of the BlockBackend level.
> 
> This is part of deprecating before write notifiers, which are hard coded
> into the block layer. Block filter drivers are inserted into the graph
> only when a feature is needed. This makes the block layer more modular
> and reuses the block driver abstraction that is already present.
> 
> Signed-off-by: Manos Pitsidianakis <address@hidden>

You make a lot of changes to add bdrv_set_file()/bdrv_append_file()
(which should be separate patches, by the way), but if we look at the
only actual user of the functions:

> @@ -606,17 +744,33 @@ BlockJob *backup_job_create(const char *job_id, 
> BlockDriverState *bs,
>                           bdrv_get_device_name(bs));
>          goto error;
>      }
> +    /* Setup before write filter */
> +    filter =
> +        bdrv_new_open_driver(&backup_top,
> +                             NULL, bdrv_get_flags(bs), NULL, &error_abort);
> +    filter->implicit = true;
> +    filter->total_sectors = bs->total_sectors;
> +    bdrv_set_aio_context(filter, bdrv_get_aio_context(bs));
> +
> +    /* Insert before write notifier in the BDS chain */
> +    bdrv_ref(filter);
> +    bdrv_drained_begin(bs);
> +    bdrv_append_file(filter, bs, &error_abort);
> +    bdrv_drained_end(bs);

Is there a reason why you can't simply do:

    filter->file = bdrv_attach_child(filter, bs, "file", &child_file, 
&local_err);
    bdrv_replace_node(filter->file, filter, &error_abort);

Effectively these two lines are all that your new functions boil down to
because only this one special case is ever used.


Did you consider that using filter->file for the backup filter rather
than filter->backing like in mirror and commit mean that the backing
file chain is broken? Are we sure that all functions that operate on the
backing file chain can already skip filters?

For example, bdrv_co_get_block_status_above() doesn't seem to do the
right thing yet, it would consider the filter as the base file where the
chain ends.

Attaching as filter->backing feels a bit safer to me.

> @@ -532,6 +551,124 @@ static const BlockJobDriver backup_job_driver = {
>      .drain                  = backup_drain,
>  };
>  
> +static int coroutine_fn backup_top_co_preadv(BlockDriverState *bs,
> +                                             uint64_t offset,
> +                                             uint64_t bytes,
> +                                             QEMUIOVector *qiov, int flags)
> +{
> +    return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
> +}
> +
> +static int coroutine_fn backup_top_co_pwritev(BlockDriverState *bs,
> +                                              uint64_t offset,
> +                                              uint64_t bytes,
> +                                              QEMUIOVector *qiov, int flags)
> +{
> +    int ret = 0;
> +    BackupBlockJob *job = bs->opaque;
> +    if (job) {
> +        assert(bs == blk_bs(job->common.blk));
> +        assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> +        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> +        ret = backup_do_cow(job, offset, bytes, NULL, true);
> +    }

Is it worth making this a static inline function instead of having three
copies of the same code?

> +    return ret < 0 ? ret : bdrv_co_pwritev(bs->file, offset, bytes,
> +                                           qiov, flags);
> +}
> +
> +static int coroutine_fn backup_top_co_pwrite_zeroes(BlockDriverState *bs,
> +                                                    int64_t offset,
> +                                                    int bytes,
> +                                                    BdrvRequestFlags flags)
> +{
> +    int ret = 0;
> +    BackupBlockJob *job = bs->opaque;
> +    if (job) {
> +        assert(bs == blk_bs(job->common.blk));
> +        assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> +        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> +        ret = backup_do_cow(job, offset, bytes, NULL, true);
> +    }
> +
> +    return ret < 0 ? ret : bdrv_co_pwrite_zeroes(bs->file, offset, bytes,
> +                                                 flags);
> +}
> +
> +static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs,
> +                                               int64_t offset, int bytes)
> +{
> +    int ret = 0;
> +    BackupBlockJob *job = bs->opaque;
> +    if (job) {
> +        assert(bs == blk_bs(job->common.blk));
> +        assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
> +        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> +        ret = backup_do_cow(job, offset, bytes, NULL, true);
> +    }
> +
> +    return ret < 0 ? ret : bdrv_co_pdiscard(bs->file->bs, offset, bytes);
> +}
> +
> +static int backup_top_co_flush(BlockDriverState *bs)
> +{
> +    return bdrv_co_flush(bs->file->bs);
> +}
> +
> +static int backup_top_open(BlockDriverState *bs, QDict *options,
> +                           int flags, Error **errp)
> +{
> +    return 0;
> +}
> +
> +static void backup_top_close(BlockDriverState *bs)
> +{
> +}
> +
> +static int64_t backup_top_getlength(BlockDriverState *bs)
> +{
> +    return bs->file ? bdrv_getlength(bs->file->bs) : 0;
> +}
> +
> +static bool backup_recurse_is_first_non_filter(BlockDriverState *bs,
> +                                               BlockDriverState *candidate)
> +{
> +    return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
> +}
> +
> +static int64_t coroutine_fn backup_co_get_block_status(BlockDriverState *bs,
> +                                                       int64_t sector_num,
> +                                                       int nb_sectors,
> +                                                       int *pnum,
> +                                                       BlockDriverState 
> **file)
> +{
> +    assert(bs->file && bs->file->bs);
> +    *pnum = nb_sectors;
> +    *file = bs->file->bs;
> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> +           (sector_num << BDRV_SECTOR_BITS);
> +}

This is just a duplicate of bdrv_co_get_block_status_from_file().

Kevin