Message ID | 20200203134213.2173-1-vsementsov@virtuozzo.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] block/backup-top: fix flags handling | expand |
On 2/3/20 7:42 AM, 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> > --- Reviewed-by: Eric Blake <eblake@redhat.com>
03.02.2020 18:42, Eric Blake wrote: > On 2/3/20 7:42 AM, 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> >> --- > > Reviewed-by: Eric Blake <eblake@redhat.com> > Thanks!
On 03.02.20 14:42, 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> > --- > > v2: restrict flags propagation like it is done in other filters [Eric] > move state variable initialization to the top > > block/backup-top.c | 32 ++++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 12 deletions(-) > > diff --git a/block/backup-top.c b/block/backup-top.c > index 9aed2eb4c0..a4cec60859 100644 > --- a/block/backup-top.c > +++ b/block/backup-top.c [...] > @@ -186,17 +190,21 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, > Error **errp) > { > Error *local_err = NULL; > - BDRVBackupTopState *state; > BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter, > filter_node_name, > BDRV_O_RDWR, errp); > + BDRVBackupTopState *state = top->opaque; > > if (!top) { > return NULL; > } If top can be NULL, then we shouldn’t dereference it before (with state = top->opaque). (Pulling up the initialization of @state is also unrelated to this patch, I don’t exactly know why you’re doing it here.) Max
06.02.2020 19:31, Max Reitz wrote: > On 03.02.20 14:42, 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> >> --- >> >> v2: restrict flags propagation like it is done in other filters [Eric] >> move state variable initialization to the top >> >> block/backup-top.c | 32 ++++++++++++++++++++------------ >> 1 file changed, 20 insertions(+), 12 deletions(-) >> >> diff --git a/block/backup-top.c b/block/backup-top.c >> index 9aed2eb4c0..a4cec60859 100644 >> --- a/block/backup-top.c >> +++ b/block/backup-top.c > > > [...] > >> @@ -186,17 +190,21 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, >> Error **errp) >> { >> Error *local_err = NULL; >> - BDRVBackupTopState *state; >> BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter, >> filter_node_name, >> BDRV_O_RDWR, errp); >> + BDRVBackupTopState *state = top->opaque; >> >> if (!top) { >> return NULL; >> } > > If top can be NULL, then we shouldn’t dereference it before (with > state = top->opaque). Oh yes. > > (Pulling up the initialization of @state is also unrelated to this > patch, I don’t exactly know why you’re doing it here.) > I just though, that it is small good style enhancement, but it isn't :) Let's keep state variable handling as it was.
diff --git a/block/backup-top.c b/block/backup-top.c index 9aed2eb4c0..a4cec60859 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); @@ -186,17 +190,21 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, Error **errp) { Error *local_err = NULL; - BDRVBackupTopState *state; BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter, filter_node_name, BDRV_O_RDWR, errp); + BDRVBackupTopState *state = top->opaque; if (!top) { return NULL; } top->total_sectors = source->total_sectors; - state = top->opaque; + 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> --- v2: restrict flags propagation like it is done in other filters [Eric] move state variable initialization to the top block/backup-top.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)