| Message ID | 20200207161231.32707-1-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] block/backup-top: fix flags handling | expand |
On 07.02.20 17:12, Vladimir Sementsov-Ogievskiy wrote: > backup-top "supports" write-unchanged, by skipping CBW operation in > backup_top_co_pwritev. But it forgets to do the same in > backup_top_co_pwrite_zeroes, as well as declare support for > BDRV_REQ_WRITE_UNCHANGED. > > Fix this, and, while being here, declare also support for flags > supported by source child. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > > v3: rebase on master, keep state initialization after check top != NULL. > > v2: restrict flags propagation like it is done in other filters [Eric] > move state variable initialization to the top > block/backup-top.c | 31 ++++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/block/backup-top.c b/block/backup-top.c > index fa78f3256d..1bfb360bd3 100644 > --- a/block/backup-top.c > +++ b/block/backup-top.c [...] > @@ -196,8 +200,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, > return NULL; > } > > - top->total_sectors = source->total_sectors; > state = top->opaque; > + top->total_sectors = source->total_sectors; This looks a bit accidental, but, well, whatever. Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max
07.02.2020 19:30, Max Reitz wrote: > On 07.02.20 17:12, Vladimir Sementsov-Ogievskiy wrote: >> backup-top "supports" write-unchanged, by skipping CBW operation in >> backup_top_co_pwritev. But it forgets to do the same in >> backup_top_co_pwrite_zeroes, as well as declare support for >> BDRV_REQ_WRITE_UNCHANGED. >> >> Fix this, and, while being here, declare also support for flags >> supported by source child. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> >> v3: rebase on master, keep state initialization after check top != NULL. >> >> v2: restrict flags propagation like it is done in other filters [Eric] >> move state variable initialization to the top >> block/backup-top.c | 31 ++++++++++++++++++++----------- >> 1 file changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/block/backup-top.c b/block/backup-top.c >> index fa78f3256d..1bfb360bd3 100644 >> --- a/block/backup-top.c >> +++ b/block/backup-top.c > > [...] > >> @@ -196,8 +200,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, >> return NULL; >> } >> >> - top->total_sectors = source->total_sectors; >> state = top->opaque; >> + top->total_sectors = source->total_sectors; > > This looks a bit accidental, but, well, whatever. I failed to restrict myself in a wish to keep all "top->.. =" initializers went together :) Hmm, I could "state =" intializer to go after them. But it's good to keep it at top too. > > Thanks, applied to my block branch: > > https://git.xanclic.moe/XanClic/qemu/commits/branch/block > Thanks!
diff --git a/block/backup-top.c b/block/backup-top.c index fa78f3256d..1bfb360bd3 100644 --- a/block/backup-top.c +++ b/block/backup-top.c @@ -48,11 +48,17 @@ static coroutine_fn int backup_top_co_preadv( } static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset, - uint64_t bytes) + uint64_t bytes, BdrvRequestFlags flags) { BDRVBackupTopState *s = bs->opaque; - uint64_t end = QEMU_ALIGN_UP(offset + bytes, s->bcs->cluster_size); - uint64_t off = QEMU_ALIGN_DOWN(offset, s->bcs->cluster_size); + uint64_t off, end; + + if (flags & BDRV_REQ_WRITE_UNCHANGED) { + return 0; + } + + off = QEMU_ALIGN_DOWN(offset, s->bcs->cluster_size); + end = QEMU_ALIGN_UP(offset + bytes, s->bcs->cluster_size); return block_copy(s->bcs, off, end - off, NULL); } @@ -60,7 +66,7 @@ static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset, static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes) { - int ret = backup_top_cbw(bs, offset, bytes); + int ret = backup_top_cbw(bs, offset, bytes, 0); if (ret < 0) { return ret; } @@ -71,7 +77,7 @@ static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs, static int coroutine_fn backup_top_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int bytes, BdrvRequestFlags flags) { - int ret = backup_top_cbw(bs, offset, bytes); + int ret = backup_top_cbw(bs, offset, bytes, flags); if (ret < 0) { return ret; } @@ -84,11 +90,9 @@ static coroutine_fn int backup_top_co_pwritev(BlockDriverState *bs, uint64_t bytes, QEMUIOVector *qiov, int flags) { - if (!(flags & BDRV_REQ_WRITE_UNCHANGED)) { - int ret = backup_top_cbw(bs, offset, bytes); - if (ret < 0) { - return ret; - } + int ret = backup_top_cbw(bs, offset, bytes, flags); + if (ret < 0) { + return ret; } return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags); @@ -196,8 +200,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, return NULL; } - top->total_sectors = source->total_sectors; state = top->opaque; + top->total_sectors = source->total_sectors; + top->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED | + (BDRV_REQ_FUA & source->supported_write_flags); + top->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED | + ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & + source->supported_zero_flags); bdrv_ref(target); state->target = bdrv_attach_child(top, target, "target", &child_file, errp);
backup-top "supports" write-unchanged, by skipping CBW operation in backup_top_co_pwritev. But it forgets to do the same in backup_top_co_pwrite_zeroes, as well as declare support for BDRV_REQ_WRITE_UNCHANGED. Fix this, and, while being here, declare also support for flags supported by source child. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- v3: rebase on master, keep state initialization after check top != NULL. v2: restrict flags propagation like it is done in other filters [Eric] move state variable initialization to the top block/backup-top.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-)