| Message ID | 20220120162043.2693196-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2: add keep-dirty open option | expand |
On 20.01.2022 19:20, Vladimir Sementsov-Ogievskiy wrote: > Consider the case: > > Thirdparty component works with qcow2 image, and dirty bit is set. > > Thirdparty component want to start qemu-img to do some manipulation. > Ofcourse, third party component flushes refcounts and other metadata > before starting QEMU. > > But the component don't want to clear dirty bit, as this breaks > transactionability of the operation: we'll have to set it again but it > may fail. Clearing the dirty bit is unrecoverable action and can't be > transactional. That's a problem. > > The solution is a new qcow2 open option: keep-dirty. When set: > 1. On qcow2 open, ignore dirty bit and don't do check: caller is > responsible for refcounts being valid. > 2. Never clear dirty bit during QEMU execution, including close. > > Details: > > 1. For simplicity let's just not allow keep-dirty without lazy > refcounts. > > 2. Don't allow to open with keep-dirty when dirty bit is unset. This > may mean some error in user logic. > > 3. For implementation do the following: dirty flag > in s->incompatible_features behaves same way as without keep-dirty > option: it actually designate status of refcounts dirtiness. But we > never clear the flag in the image, and we remember that it is always > set. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qapi/block-core.json | 5 ++++ > block/qcow2.h | 2 ++ > block/qcow2.c | 66 ++++++++++++++++++++++++++++++++++++-------- > 3 files changed, 61 insertions(+), 12 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 9a5a3641d0..3e35357182 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3228,6 +3228,10 @@ > # @lazy-refcounts: whether to enable the lazy refcounts > # feature (default is taken from the image file) > # > +# @keep-dirty: whether to not touch dirty bit. When set, QEMU doesn't > +# check refcounts on qcow2 open (ignoring dirty bit) and doesn't > +# clear dirty bit on qcow2 close. (since 7.0) > +# > # @pass-discard-request: whether discard requests to the qcow2 > # device should be forwarded to the data source > # > @@ -3276,6 +3280,7 @@ > { 'struct': 'BlockdevOptionsQcow2', > 'base': 'BlockdevOptionsGenericCOWFormat', > 'data': { '*lazy-refcounts': 'bool', > + '*keep-dirty': 'bool', > '*pass-discard-request': 'bool', > '*pass-discard-snapshot': 'bool', > '*pass-discard-other': 'bool', > diff --git a/block/qcow2.h b/block/qcow2.h > index fd48a89d45..696e13377a 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -130,6 +130,7 @@ > > #define QCOW2_OPT_DATA_FILE "data-file" > #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts" > +#define QCOW2_OPT_KEEP_DIRTY "keep-dirty" > #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request" > #define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot" > #define QCOW2_OPT_DISCARD_OTHER "pass-discard-other" > @@ -376,6 +377,7 @@ typedef struct BDRVQcow2State { > int flags; > int qcow_version; > bool use_lazy_refcounts; > + bool keep_dirty; > int refcount_order; > int refcount_bits; > uint64_t refcount_max; > diff --git a/block/qcow2.c b/block/qcow2.c > index d509016756..1c42103fb9 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -514,15 +514,17 @@ int qcow2_mark_dirty(BlockDriverState *bs) > return 0; /* already dirty */ > } > > - val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY); > - ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features), > - &val, sizeof(val)); > - if (ret < 0) { > - return ret; > - } > - ret = bdrv_flush(bs->file->bs); > - if (ret < 0) { > - return ret; > + if (!s->keep_dirty) { > + val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY); > + ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features), > + &val, sizeof(val)); > + if (ret < 0) { > + return ret; > + } > + ret = bdrv_flush(bs->file->bs); > + if (ret < 0) { > + return ret; > + } > } > > /* Only treat image as dirty if the header was updated successfully */ > @@ -549,7 +551,13 @@ static int qcow2_mark_clean(BlockDriverState *bs) > return ret; > } > > - return qcow2_update_header(bs); > + if (!s->keep_dirty) { > + /* > + * No reason to update the header if we don't want to clear dirty > + * bit. > + */ > + return qcow2_update_header(bs); > + } > } > return 0; > } > @@ -709,6 +717,11 @@ static QemuOptsList qcow2_runtime_opts = { > .type = QEMU_OPT_BOOL, > .help = "Postpone refcount updates", > }, > + { > + .name = QCOW2_OPT_KEEP_DIRTY, > + .type = QEMU_OPT_BOOL, > + .help = "Keep dirty bit untouched", > + }, > { > .name = QCOW2_OPT_DISCARD_REQUEST, > .type = QEMU_OPT_BOOL, > @@ -966,6 +979,7 @@ typedef struct Qcow2ReopenState { > Qcow2Cache *refcount_block_cache; > int l2_slice_size; /* Number of entries in a slice of the L2 table */ > bool use_lazy_refcounts; > + bool keep_dirty; > int overlap_check; > bool discard_passthrough[QCOW2_DISCARD_MAX]; > uint64_t cache_clean_interval; > @@ -1088,6 +1102,13 @@ static int qcow2_update_options_prepare(BlockDriverState *bs, > } > } > > + r->keep_dirty = qemu_opt_get_bool(opts, QCOW2_OPT_KEEP_DIRTY, false); > + if (r->keep_dirty && !r->use_lazy_refcounts) { > + error_setg(errp, "keep-dirty requires lazy refcounts"); > + ret = -EINVAL; > + goto fail; Vladimir, according to documentation dirty bit and lazy refcounts are independent: incompatible_features: Bit 0: Dirty bit. If this bit is set then refcounts may be inconsistent, make sure to scan L1/L2 tables to repair refcounts before accessing the image. compatible_features: Bit 0: Lazy refcounts bit. If this bit is set then lazy refcount updates can be used. This means marking the image file dirty and postponing refcount metadata updates. while this patch assumes existence of some dependence between them. Is there a real reason to introduce such the connection? [snip] Kirill
27.01.2022 12:43, Kirill Tkhai wrote: > On 20.01.2022 19:20, Vladimir Sementsov-Ogievskiy wrote: >> Consider the case: >> >> Thirdparty component works with qcow2 image, and dirty bit is set. >> >> Thirdparty component want to start qemu-img to do some manipulation. >> Ofcourse, third party component flushes refcounts and other metadata >> before starting QEMU. >> >> But the component don't want to clear dirty bit, as this breaks >> transactionability of the operation: we'll have to set it again but it >> may fail. Clearing the dirty bit is unrecoverable action and can't be >> transactional. That's a problem. >> >> The solution is a new qcow2 open option: keep-dirty. When set: >> 1. On qcow2 open, ignore dirty bit and don't do check: caller is >> responsible for refcounts being valid. >> 2. Never clear dirty bit during QEMU execution, including close. >> >> Details: >> >> 1. For simplicity let's just not allow keep-dirty without lazy >> refcounts. >> >> 2. Don't allow to open with keep-dirty when dirty bit is unset. This >> may mean some error in user logic. >> >> 3. For implementation do the following: dirty flag >> in s->incompatible_features behaves same way as without keep-dirty >> option: it actually designate status of refcounts dirtiness. But we >> never clear the flag in the image, and we remember that it is always >> set. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qapi/block-core.json | 5 ++++ >> block/qcow2.h | 2 ++ >> block/qcow2.c | 66 ++++++++++++++++++++++++++++++++++++-------- >> 3 files changed, 61 insertions(+), 12 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 9a5a3641d0..3e35357182 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -3228,6 +3228,10 @@ >> # @lazy-refcounts: whether to enable the lazy refcounts >> # feature (default is taken from the image file) >> # >> +# @keep-dirty: whether to not touch dirty bit. When set, QEMU doesn't >> +# check refcounts on qcow2 open (ignoring dirty bit) and doesn't >> +# clear dirty bit on qcow2 close. (since 7.0) >> +# >> # @pass-discard-request: whether discard requests to the qcow2 >> # device should be forwarded to the data source >> # >> @@ -3276,6 +3280,7 @@ >> { 'struct': 'BlockdevOptionsQcow2', >> 'base': 'BlockdevOptionsGenericCOWFormat', >> 'data': { '*lazy-refcounts': 'bool', >> + '*keep-dirty': 'bool', >> '*pass-discard-request': 'bool', >> '*pass-discard-snapshot': 'bool', >> '*pass-discard-other': 'bool', >> diff --git a/block/qcow2.h b/block/qcow2.h >> index fd48a89d45..696e13377a 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -130,6 +130,7 @@ >> >> #define QCOW2_OPT_DATA_FILE "data-file" >> #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts" >> +#define QCOW2_OPT_KEEP_DIRTY "keep-dirty" >> #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request" >> #define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot" >> #define QCOW2_OPT_DISCARD_OTHER "pass-discard-other" >> @@ -376,6 +377,7 @@ typedef struct BDRVQcow2State { >> int flags; >> int qcow_version; >> bool use_lazy_refcounts; >> + bool keep_dirty; >> int refcount_order; >> int refcount_bits; >> uint64_t refcount_max; >> diff --git a/block/qcow2.c b/block/qcow2.c >> index d509016756..1c42103fb9 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -514,15 +514,17 @@ int qcow2_mark_dirty(BlockDriverState *bs) >> return 0; /* already dirty */ >> } >> >> - val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY); >> - ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features), >> - &val, sizeof(val)); >> - if (ret < 0) { >> - return ret; >> - } >> - ret = bdrv_flush(bs->file->bs); >> - if (ret < 0) { >> - return ret; >> + if (!s->keep_dirty) { >> + val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY); >> + ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features), >> + &val, sizeof(val)); >> + if (ret < 0) { >> + return ret; >> + } >> + ret = bdrv_flush(bs->file->bs); >> + if (ret < 0) { >> + return ret; >> + } >> } >> >> /* Only treat image as dirty if the header was updated successfully */ >> @@ -549,7 +551,13 @@ static int qcow2_mark_clean(BlockDriverState *bs) >> return ret; >> } >> >> - return qcow2_update_header(bs); >> + if (!s->keep_dirty) { >> + /* >> + * No reason to update the header if we don't want to clear dirty >> + * bit. >> + */ >> + return qcow2_update_header(bs); >> + } >> } >> return 0; >> } >> @@ -709,6 +717,11 @@ static QemuOptsList qcow2_runtime_opts = { >> .type = QEMU_OPT_BOOL, >> .help = "Postpone refcount updates", >> }, >> + { >> + .name = QCOW2_OPT_KEEP_DIRTY, >> + .type = QEMU_OPT_BOOL, >> + .help = "Keep dirty bit untouched", >> + }, >> { >> .name = QCOW2_OPT_DISCARD_REQUEST, >> .type = QEMU_OPT_BOOL, >> @@ -966,6 +979,7 @@ typedef struct Qcow2ReopenState { >> Qcow2Cache *refcount_block_cache; >> int l2_slice_size; /* Number of entries in a slice of the L2 table */ >> bool use_lazy_refcounts; >> + bool keep_dirty; >> int overlap_check; >> bool discard_passthrough[QCOW2_DISCARD_MAX]; >> uint64_t cache_clean_interval; >> @@ -1088,6 +1102,13 @@ static int qcow2_update_options_prepare(BlockDriverState *bs, >> } >> } >> >> + r->keep_dirty = qemu_opt_get_bool(opts, QCOW2_OPT_KEEP_DIRTY, false); >> + if (r->keep_dirty && !r->use_lazy_refcounts) { >> + error_setg(errp, "keep-dirty requires lazy refcounts"); >> + ret = -EINVAL; >> + goto fail; > > Vladimir, according to documentation dirty bit and lazy refcounts are independent: > > incompatible_features: > Bit 0: Dirty bit. If this bit is set then refcounts > may be inconsistent, make sure to scan L1/L2 > tables to repair refcounts before accessing the > image. > > compatible_features: > Bit 0: Lazy refcounts bit. If this bit is set then > lazy refcount updates can be used. This means > marking the image file dirty and postponing > refcount metadata updates. > > while this patch assumes existence of some dependence between them. > Is there a real reason to introduce such the connection? > Hmm, somehow I thought that there was some documented dependency.. But seems you are right. Honestly, I don't know real scenarios where non-lazy refcounts make sense. In Virtuozzo we always use lazy refcounts turned on by bit in the image. OK, I'll drop a dependency for new option.
diff --git a/qapi/block-core.json b/qapi/block-core.json index 9a5a3641d0..3e35357182 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3228,6 +3228,10 @@ # @lazy-refcounts: whether to enable the lazy refcounts # feature (default is taken from the image file) # +# @keep-dirty: whether to not touch dirty bit. When set, QEMU doesn't +# check refcounts on qcow2 open (ignoring dirty bit) and doesn't +# clear dirty bit on qcow2 close. (since 7.0) +# # @pass-discard-request: whether discard requests to the qcow2 # device should be forwarded to the data source # @@ -3276,6 +3280,7 @@ { 'struct': 'BlockdevOptionsQcow2', 'base': 'BlockdevOptionsGenericCOWFormat', 'data': { '*lazy-refcounts': 'bool', + '*keep-dirty': 'bool', '*pass-discard-request': 'bool', '*pass-discard-snapshot': 'bool', '*pass-discard-other': 'bool', diff --git a/block/qcow2.h b/block/qcow2.h index fd48a89d45..696e13377a 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -130,6 +130,7 @@ #define QCOW2_OPT_DATA_FILE "data-file" #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts" +#define QCOW2_OPT_KEEP_DIRTY "keep-dirty" #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request" #define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot" #define QCOW2_OPT_DISCARD_OTHER "pass-discard-other" @@ -376,6 +377,7 @@ typedef struct BDRVQcow2State { int flags; int qcow_version; bool use_lazy_refcounts; + bool keep_dirty; int refcount_order; int refcount_bits; uint64_t refcount_max; diff --git a/block/qcow2.c b/block/qcow2.c index d509016756..1c42103fb9 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -514,15 +514,17 @@ int qcow2_mark_dirty(BlockDriverState *bs) return 0; /* already dirty */ } - val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY); - ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features), - &val, sizeof(val)); - if (ret < 0) { - return ret; - } - ret = bdrv_flush(bs->file->bs); - if (ret < 0) { - return ret; + if (!s->keep_dirty) { + val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY); + ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features), + &val, sizeof(val)); + if (ret < 0) { + return ret; + } + ret = bdrv_flush(bs->file->bs); + if (ret < 0) { + return ret; + } } /* Only treat image as dirty if the header was updated successfully */ @@ -549,7 +551,13 @@ static int qcow2_mark_clean(BlockDriverState *bs) return ret; } - return qcow2_update_header(bs); + if (!s->keep_dirty) { + /* + * No reason to update the header if we don't want to clear dirty + * bit. + */ + return qcow2_update_header(bs); + } } return 0; } @@ -709,6 +717,11 @@ static QemuOptsList qcow2_runtime_opts = { .type = QEMU_OPT_BOOL, .help = "Postpone refcount updates", }, + { + .name = QCOW2_OPT_KEEP_DIRTY, + .type = QEMU_OPT_BOOL, + .help = "Keep dirty bit untouched", + }, { .name = QCOW2_OPT_DISCARD_REQUEST, .type = QEMU_OPT_BOOL, @@ -966,6 +979,7 @@ typedef struct Qcow2ReopenState { Qcow2Cache *refcount_block_cache; int l2_slice_size; /* Number of entries in a slice of the L2 table */ bool use_lazy_refcounts; + bool keep_dirty; int overlap_check; bool discard_passthrough[QCOW2_DISCARD_MAX]; uint64_t cache_clean_interval; @@ -1088,6 +1102,13 @@ static int qcow2_update_options_prepare(BlockDriverState *bs, } } + r->keep_dirty = qemu_opt_get_bool(opts, QCOW2_OPT_KEEP_DIRTY, false); + if (r->keep_dirty && !r->use_lazy_refcounts) { + error_setg(errp, "keep-dirty requires lazy refcounts"); + ret = -EINVAL; + goto fail; + } + /* Overlap check options */ opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP); opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE); @@ -1214,6 +1235,7 @@ static void qcow2_update_options_commit(BlockDriverState *bs, s->overlap_check = r->overlap_check; s->use_lazy_refcounts = r->use_lazy_refcounts; + s->keep_dirty = r->keep_dirty; for (i = 0; i < QCOW2_DISCARD_MAX; i++) { s->discard_passthrough[i] = r->discard_passthrough[i]; @@ -1810,7 +1832,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE; /* Repair image if dirty */ - if (!(flags & BDRV_O_CHECK) && bdrv_is_writable(bs) && + if (!s->keep_dirty && !(flags & BDRV_O_CHECK) && bdrv_is_writable(bs) && (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) { BdrvCheckResult result = {0}; @@ -1825,6 +1847,20 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, } } + if (s->keep_dirty) { + if (!(s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) { + error_setg(errp, "keep-dirty behaviour is requested but image " + "is not dirty"); + ret = -EINVAL; + goto fail; + } + /* + * User guarantees that refcounts are valid. So, consider them valid, + * keeping dirty bit set in the header. + */ + s->incompatible_features &= ~QCOW2_INCOMPAT_DIRTY; + } + #ifdef DEBUG_ALLOC { BdrvCheckResult result = {0}; @@ -2826,6 +2862,7 @@ int qcow2_update_header(BlockDriverState *bs) uint32_t refcount_table_clusters; size_t header_length; Qcow2UnknownHeaderExtension *uext; + uint64_t incompatible_features; buf = qemu_blockalign(bs, buflen); @@ -2846,6 +2883,11 @@ int qcow2_update_header(BlockDriverState *bs) goto fail; } + incompatible_features = s->incompatible_features; + if (s->keep_dirty) { + incompatible_features |= QCOW2_INCOMPAT_DIRTY; + } + *header = (QCowHeader) { /* Version 2 fields */ .magic = cpu_to_be32(QCOW_MAGIC), @@ -2863,7 +2905,7 @@ int qcow2_update_header(BlockDriverState *bs) .snapshots_offset = cpu_to_be64(s->snapshots_offset), /* Version 3 fields */ - .incompatible_features = cpu_to_be64(s->incompatible_features), + .incompatible_features = cpu_to_be64(incompatible_features), .compatible_features = cpu_to_be64(s->compatible_features), .autoclear_features = cpu_to_be64(s->autoclear_features), .refcount_order = cpu_to_be32(s->refcount_order),
Consider the case: Thirdparty component works with qcow2 image, and dirty bit is set. Thirdparty component want to start qemu-img to do some manipulation. Ofcourse, third party component flushes refcounts and other metadata before starting QEMU. But the component don't want to clear dirty bit, as this breaks transactionability of the operation: we'll have to set it again but it may fail. Clearing the dirty bit is unrecoverable action and can't be transactional. That's a problem. The solution is a new qcow2 open option: keep-dirty. When set: 1. On qcow2 open, ignore dirty bit and don't do check: caller is responsible for refcounts being valid. 2. Never clear dirty bit during QEMU execution, including close. Details: 1. For simplicity let's just not allow keep-dirty without lazy refcounts. 2. Don't allow to open with keep-dirty when dirty bit is unset. This may mean some error in user logic. 3. For implementation do the following: dirty flag in s->incompatible_features behaves same way as without keep-dirty option: it actually designate status of refcounts dirtiness. But we never clear the flag in the image, and we remember that it is always set. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- qapi/block-core.json | 5 ++++ block/qcow2.h | 2 ++ block/qcow2.c | 66 ++++++++++++++++++++++++++++++++++++-------- 3 files changed, 61 insertions(+), 12 deletions(-)