| Message ID | 20190807141226.193501-11-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2-bitmaps: rewrite reopening logic | expand |
On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote: > The only reason I can imagine for this strange code at the very-end of > bdrv_reopen_commit is the fact that bs->read_only updated after > calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same > time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong > check for being writable, when actually it only need writable file > child not self. > > So, as it's fixed, let's move things to correct place. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/block_int.h | 6 ------ > block.c | 19 ------------------- > block/qcow2.c | 15 ++++++++++++++- > 3 files changed, 14 insertions(+), 26 deletions(-) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 3aa1e832a8..18a1e81194 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -531,12 +531,6 @@ struct BlockDriver { > uint64_t parent_perm, uint64_t parent_shared, > uint64_t *nperm, uint64_t *nshared); > > - /** > - * Bitmaps should be marked as 'IN_USE' in the image on reopening image > - * as rw. This handler should realize it. It also should unset readonly > - * field of BlockDirtyBitmap's in case of success. > - */ > - int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); > bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, > const char *name, > uint32_t granularity, > diff --git a/block.c b/block.c > index d59f9f97cb..395bc88045 100644 > --- a/block.c > +++ b/block.c > @@ -3925,16 +3925,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) > BlockDriver *drv; > BlockDriverState *bs; > BdrvChild *child; > - bool old_can_write, new_can_write; > > assert(reopen_state != NULL); > bs = reopen_state->bs; > drv = bs->drv; > assert(drv != NULL); > > - old_can_write = > - !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE); > - > /* If there are any driver level actions to take */ > if (drv->bdrv_reopen_commit) { > drv->bdrv_reopen_commit(reopen_state); > @@ -3978,21 +3974,6 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) > } > > bdrv_refresh_limits(bs, NULL); > - > - new_can_write = > - !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE); > - if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) { > - Error *local_err = NULL; > - if (drv->bdrv_reopen_bitmaps_rw(bs, &local_err) < 0) { > - /* This is not fatal, bitmaps just left read-only, so all following > - * writes will fail. User can remove read-only bitmaps to unblock > - * writes. > - */ > - error_reportf_err(local_err, > - "%s: Failed to make dirty bitmaps writable: ", > - bdrv_get_node_name(bs)); > - } > - } > } > > /* > diff --git a/block/qcow2.c b/block/qcow2.c > index 5c1187e2f9..9e6210c282 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1828,6 +1828,20 @@ fail: > static void qcow2_reopen_commit(BDRVReopenState *state) > { > qcow2_update_options_commit(state->bs, state->opaque); > + if (state->flags & BDRV_O_RDWR) { > + Error *local_err = NULL; > + > + if (qcow2_reopen_bitmaps_rw(state->bs, &local_err) < 0) { > + /* > + * This is not fatal, bitmaps just left read-only, so all following > + * writes will fail. User can remove read-only bitmaps to unblock > + * writes or retry reopen. > + */ > + error_reportf_err(local_err, > + "%s: Failed to make dirty bitmaps writable: ", > + bdrv_get_node_name(state->bs)); > + } > + } > g_free(state->opaque); > } > > @@ -5229,7 +5243,6 @@ BlockDriver bdrv_qcow2 = { > .bdrv_detach_aio_context = qcow2_detach_aio_context, > .bdrv_attach_aio_context = qcow2_attach_aio_context, > > - .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw, > .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap, > .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap, > }; > Makes sense to me -- bitmap reopen should happen when a specific driver needs to reopen. It was a weird top-level driver callback. Reviewed-by: John Snow <jsnow@redhat.com> Max, can you please review this one as well?
On 07.08.19 16:12, Vladimir Sementsov-Ogievskiy wrote: > The only reason I can imagine for this strange code at the very-end of > bdrv_reopen_commit is the fact that bs->read_only updated after > calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same > time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong > check for being writable, when actually it only need writable file > child not self. > > So, as it's fixed, let's move things to correct place. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/block_int.h | 6 ------ > block.c | 19 ------------------- > block/qcow2.c | 15 ++++++++++++++- > 3 files changed, 14 insertions(+), 26 deletions(-) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 3aa1e832a8..18a1e81194 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -531,12 +531,6 @@ struct BlockDriver { > uint64_t parent_perm, uint64_t parent_shared, > uint64_t *nperm, uint64_t *nshared); > > - /** > - * Bitmaps should be marked as 'IN_USE' in the image on reopening image > - * as rw. This handler should realize it. It also should unset readonly > - * field of BlockDirtyBitmap's in case of success. > - */ > - int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); > bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, > const char *name, > uint32_t granularity, > diff --git a/block.c b/block.c > index d59f9f97cb..395bc88045 100644 > --- a/block.c > +++ b/block.c > @@ -3925,16 +3925,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) > BlockDriver *drv; > BlockDriverState *bs; > BdrvChild *child; > - bool old_can_write, new_can_write; > > assert(reopen_state != NULL); > bs = reopen_state->bs; > drv = bs->drv; > assert(drv != NULL); > > - old_can_write = > - !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE); > - > /* If there are any driver level actions to take */ > if (drv->bdrv_reopen_commit) { > drv->bdrv_reopen_commit(reopen_state); > @@ -3978,21 +3974,6 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) > } > > bdrv_refresh_limits(bs, NULL); > - > - new_can_write = > - !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE); > - if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) { > - Error *local_err = NULL; > - if (drv->bdrv_reopen_bitmaps_rw(bs, &local_err) < 0) { > - /* This is not fatal, bitmaps just left read-only, so all following > - * writes will fail. User can remove read-only bitmaps to unblock > - * writes. > - */ > - error_reportf_err(local_err, > - "%s: Failed to make dirty bitmaps writable: ", > - bdrv_get_node_name(bs)); > - } > - } > } > > /* > diff --git a/block/qcow2.c b/block/qcow2.c > index 5c1187e2f9..9e6210c282 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1828,6 +1828,20 @@ fail: > static void qcow2_reopen_commit(BDRVReopenState *state) > { > qcow2_update_options_commit(state->bs, state->opaque); > + if (state->flags & BDRV_O_RDWR) { > + Error *local_err = NULL; > + > + if (qcow2_reopen_bitmaps_rw(state->bs, &local_err) < 0) { > + /* > + * This is not fatal, bitmaps just left read-only, so all following > + * writes will fail. User can remove read-only bitmaps to unblock > + * writes or retry reopen. > + */ It’s still my impression that this is absolutely fatal, because that means the node isn’t actually writable, and that means the reopen effectively failed. But again, it doesn’t make things worse. Assuming the RW -> RW transition is a no-op now (the previous patch claims to handle that case): Acked-by: Max Reitz <mreitz@redhat.com> > + error_reportf_err(local_err, > + "%s: Failed to make dirty bitmaps writable: ", > + bdrv_get_node_name(state->bs)); > + } > + } > g_free(state->opaque); > } > > @@ -5229,7 +5243,6 @@ BlockDriver bdrv_qcow2 = { > .bdrv_detach_aio_context = qcow2_detach_aio_context, > .bdrv_attach_aio_context = qcow2_attach_aio_context, > > - .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw, > .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap, > .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap, > }; >
diff --git a/include/block/block_int.h b/include/block/block_int.h index 3aa1e832a8..18a1e81194 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -531,12 +531,6 @@ struct BlockDriver { uint64_t parent_perm, uint64_t parent_shared, uint64_t *nperm, uint64_t *nshared); - /** - * Bitmaps should be marked as 'IN_USE' in the image on reopening image - * as rw. This handler should realize it. It also should unset readonly - * field of BlockDirtyBitmap's in case of success. - */ - int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, const char *name, uint32_t granularity, diff --git a/block.c b/block.c index d59f9f97cb..395bc88045 100644 --- a/block.c +++ b/block.c @@ -3925,16 +3925,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) BlockDriver *drv; BlockDriverState *bs; BdrvChild *child; - bool old_can_write, new_can_write; assert(reopen_state != NULL); bs = reopen_state->bs; drv = bs->drv; assert(drv != NULL); - old_can_write = - !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE); - /* If there are any driver level actions to take */ if (drv->bdrv_reopen_commit) { drv->bdrv_reopen_commit(reopen_state); @@ -3978,21 +3974,6 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) } bdrv_refresh_limits(bs, NULL); - - new_can_write = - !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE); - if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) { - Error *local_err = NULL; - if (drv->bdrv_reopen_bitmaps_rw(bs, &local_err) < 0) { - /* This is not fatal, bitmaps just left read-only, so all following - * writes will fail. User can remove read-only bitmaps to unblock - * writes. - */ - error_reportf_err(local_err, - "%s: Failed to make dirty bitmaps writable: ", - bdrv_get_node_name(bs)); - } - } } /* diff --git a/block/qcow2.c b/block/qcow2.c index 5c1187e2f9..9e6210c282 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1828,6 +1828,20 @@ fail: static void qcow2_reopen_commit(BDRVReopenState *state) { qcow2_update_options_commit(state->bs, state->opaque); + if (state->flags & BDRV_O_RDWR) { + Error *local_err = NULL; + + if (qcow2_reopen_bitmaps_rw(state->bs, &local_err) < 0) { + /* + * This is not fatal, bitmaps just left read-only, so all following + * writes will fail. User can remove read-only bitmaps to unblock + * writes or retry reopen. + */ + error_reportf_err(local_err, + "%s: Failed to make dirty bitmaps writable: ", + bdrv_get_node_name(state->bs)); + } + } g_free(state->opaque); } @@ -5229,7 +5243,6 @@ BlockDriver bdrv_qcow2 = { .bdrv_detach_aio_context = qcow2_detach_aio_context, .bdrv_attach_aio_context = qcow2_attach_aio_context, - .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw, .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap, .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap, };
The only reason I can imagine for this strange code at the very-end of bdrv_reopen_commit is the fact that bs->read_only updated after calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong check for being writable, when actually it only need writable file child not self. So, as it's fixed, let's move things to correct place. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block_int.h | 6 ------ block.c | 19 ------------------- block/qcow2.c | 15 ++++++++++++++- 3 files changed, 14 insertions(+), 26 deletions(-)