| Message ID | 1456273021-17473-3-git-send-email-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 02/23 19:17, John Snow wrote: > During incremental backups, if the target has a cluster size that is > larger than the backup cluster size and we are backing up to a target > that cannot (for whichever reason) pull clusters up from a backing image, > we may inadvertantly create unusable incremental backup images. > > For example: > > If the bitmap tracks changes at a 64KB granularity and we transmit 64KB > of data at a time but the target uses a 128KB cluster size, it is > possible that only half of a target cluster will be recognized as dirty > by the backup block job. When the cluster is allocated on the target > image but only half populated with data, we lose the ability to > distinguish between zero padding and uninitialized data. > > This does not happen if the target image has a backing file that points > to the last known good backup. > > Even if we have a backing file, though, it's likely going to be faster > to just buffer the redundant data ourselves from the live image than > fetching it from the backing file, so let's just always round up to the > target granularity. > > The same logic applies to backup modes top, none, and full. Copying > fractional clusters without the guarantee of COW is dangerous, but even > if we can rely on COW, it's likely better to just re-copy the data. > > Reported-by: Fam Zheng <famz@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/backup.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/block/backup.c b/block/backup.c > index 76addef..6e9f53d 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -501,6 +501,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > BlockJobTxn *txn, Error **errp) > { > int64_t len; > + BlockDriverInfo bdi; > + int ret; > > assert(bs); > assert(target); > @@ -570,15 +572,30 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > goto error; > } > > - bdrv_op_block_all(target, job->common.blocker); > - > job->on_source_error = on_source_error; > job->on_target_error = on_target_error; > job->target = target; > job->sync_mode = sync_mode; > job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ? > sync_bitmap : NULL; > - job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT; > + > + /* If there is no backing file on the target, we cannot rely on COW if our > + * backup cluster size is smaller than the target cluster size. Even for > + * targets with a backing file, try to avoid COW if possible. */ > + ret = bdrv_get_info(job->target, &bdi); > + if (ret < 0 && !target->backing) { > + error_setg_errno(errp, -ret, > + "Can't determine cluster size of target that has no backing file. " > + "This may create an unusable destination image. Aborting."); Most error messages don't end with a period, but that's not a hard rule AFAICT. > + goto error; > + } else if (ret < 0 && target->backing) { > + /* Not fatal; just trudge on ahead. */ > + job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT; > + } else { > + job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size); > + } > + > + bdrv_op_block_all(target, job->common.blocker); > job->common.len = len; > job->common.co = qemu_coroutine_create(backup_run); > block_job_txn_add_job(txn, &job->common); > -- > 2.4.3 > Reviewed-by: Fam Zheng <famz@redhat.com>
On 02/23/2016 07:27 PM, Fam Zheng wrote: > On Tue, 02/23 19:17, John Snow wrote: >> During incremental backups, if the target has a cluster size that is >> larger than the backup cluster size and we are backing up to a target >> that cannot (for whichever reason) pull clusters up from a backing image, >> we may inadvertantly create unusable incremental backup images. >> >> For example: >> >> If the bitmap tracks changes at a 64KB granularity and we transmit 64KB >> of data at a time but the target uses a 128KB cluster size, it is >> possible that only half of a target cluster will be recognized as dirty >> by the backup block job. When the cluster is allocated on the target >> image but only half populated with data, we lose the ability to >> distinguish between zero padding and uninitialized data. >> >> This does not happen if the target image has a backing file that points >> to the last known good backup. >> >> Even if we have a backing file, though, it's likely going to be faster >> to just buffer the redundant data ourselves from the live image than >> fetching it from the backing file, so let's just always round up to the >> target granularity. >> >> The same logic applies to backup modes top, none, and full. Copying >> fractional clusters without the guarantee of COW is dangerous, but even >> if we can rely on COW, it's likely better to just re-copy the data. >> >> Reported-by: Fam Zheng <famz@redhat.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/backup.c | 23 ++++++++++++++++++++--- >> 1 file changed, 20 insertions(+), 3 deletions(-) >> >> diff --git a/block/backup.c b/block/backup.c >> index 76addef..6e9f53d 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -501,6 +501,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, >> BlockJobTxn *txn, Error **errp) >> { >> int64_t len; >> + BlockDriverInfo bdi; >> + int ret; >> >> assert(bs); >> assert(target); >> @@ -570,15 +572,30 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, >> goto error; >> } >> >> - bdrv_op_block_all(target, job->common.blocker); >> - >> job->on_source_error = on_source_error; >> job->on_target_error = on_target_error; >> job->target = target; >> job->sync_mode = sync_mode; >> job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ? >> sync_bitmap : NULL; >> - job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT; >> + >> + /* If there is no backing file on the target, we cannot rely on COW if our >> + * backup cluster size is smaller than the target cluster size. Even for >> + * targets with a backing file, try to avoid COW if possible. */ >> + ret = bdrv_get_info(job->target, &bdi); >> + if (ret < 0 && !target->backing) { >> + error_setg_errno(errp, -ret, >> + "Can't determine cluster size of target that has no backing file. " >> + "This may create an unusable destination image. Aborting."); > > Most error messages don't end with a period, but that's not a hard rule AFAICT. If that's the only problem, Jeff can touch it up. I want to ask, though: "Are there any cases where we will have no backing file and no bdrv_get_info implementation, but backup should still succeed?" I think the answer is /no/ since everything we claim to support for use with QEMU (instead of qemu-img only) should implement bdrv_get_info. > >> + goto error; >> + } else if (ret < 0 && target->backing) { >> + /* Not fatal; just trudge on ahead. */ >> + job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT; >> + } else { >> + job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size); >> + } >> + >> + bdrv_op_block_all(target, job->common.blocker); >> job->common.len = len; >> job->common.co = qemu_coroutine_create(backup_run); >> block_job_txn_add_job(txn, &job->common); >> -- >> 2.4.3 >> > > Reviewed-by: Fam Zheng <famz@redhat.com> > Thanks.
On 02/23/2016 05:27 PM, Fam Zheng wrote: > On Tue, 02/23 19:17, John Snow wrote: >> During incremental backups, if the target has a cluster size that is >> larger than the backup cluster size and we are backing up to a target >> that cannot (for whichever reason) pull clusters up from a backing image, >> we may inadvertantly create unusable incremental backup images. >> >> + /* If there is no backing file on the target, we cannot rely on COW if our >> + * backup cluster size is smaller than the target cluster size. Even for >> + * targets with a backing file, try to avoid COW if possible. */ >> + ret = bdrv_get_info(job->target, &bdi); >> + if (ret < 0 && !target->backing) { >> + error_setg_errno(errp, -ret, >> + "Can't determine cluster size of target that has no backing file. " >> + "This may create an unusable destination image. Aborting."); > > Most error messages don't end with a period, but that's not a hard rule AFAICT. Ah, but it is: include/qapi/error.h: * The resulting message should be a single phrase, with no newline or * trailing punctuation. * Please don't error_setg(&error_fatal, ...), use error_report() and * exit(), because that's more obvious. * Likewise, don't error_setg(&error_abort, ...), use assert(). */ #define error_setg(errp, fmt, ...) \ The error message should be "Can't determine cluster size of target without backing file"; everything else should be done with error_append_hint(&errp, "Aborting, since this may create an unusable destination image\n")
On 02/23/2016 07:45 PM, Eric Blake wrote: > On 02/23/2016 05:27 PM, Fam Zheng wrote: >> On Tue, 02/23 19:17, John Snow wrote: >>> During incremental backups, if the target has a cluster size that is >>> larger than the backup cluster size and we are backing up to a target >>> that cannot (for whichever reason) pull clusters up from a backing image, >>> we may inadvertantly create unusable incremental backup images. >>> > >>> + /* If there is no backing file on the target, we cannot rely on COW if our >>> + * backup cluster size is smaller than the target cluster size. Even for >>> + * targets with a backing file, try to avoid COW if possible. */ >>> + ret = bdrv_get_info(job->target, &bdi); >>> + if (ret < 0 && !target->backing) { >>> + error_setg_errno(errp, -ret, >>> + "Can't determine cluster size of target that has no backing file. " >>> + "This may create an unusable destination image. Aborting."); >> >> Most error messages don't end with a period, but that's not a hard rule AFAICT. > > Ah, but it is: > > include/qapi/error.h: > > * The resulting message should be a single phrase, with no newline or > * trailing punctuation. > * Please don't error_setg(&error_fatal, ...), use error_report() and > * exit(), because that's more obvious. > * Likewise, don't error_setg(&error_abort, ...), use assert(). > */ > #define error_setg(errp, fmt, ...) \ > ":(.\n" > The error message should be "Can't determine cluster size of target > without backing file"; everything else should be done with > error_append_hint(&errp, "Aborting, since this may create an unusable > destination image\n") > > Tch. I decided I don't like this message anyway. "Couldn't determine the cluster size of the target image, which has no backing file" is more correct. The problem is not why we couldn't determine it, but instead that we were unable to AND there is no backing file, so our inability to determine it is a problem. What's our policy on error message strings that eclipse 80 columns? I was told once (at gunpoint; I live in the USA) to never split up long strings because it makes them harder to grep for.
On 02/23/2016 05:52 PM, John Snow wrote: > "Couldn't determine the cluster size of the target image, which has no > backing file" is more correct. The problem is not why we couldn't > determine it, but instead that we were unable to AND there is no backing > file, so our inability to determine it is a problem. > > What's our policy on error message strings that eclipse 80 columns? I > was told once (at gunpoint; I live in the USA) to never split up long > strings because it makes them harder to grep for. I don't mind error messages longer than 80 columns; and I also don't mind string concatenation to fit the source of the message into reasonable lengths (when I grep for a message, I just grep for the first few words, not the whole message, precisely so that line wraps later in the message don't cause me grief). There's varying opinions from other contributors on whether an error message can be split, but I don't see anything in HACKING that says which style must prevail.
diff --git a/block/backup.c b/block/backup.c index 76addef..6e9f53d 100644 --- a/block/backup.c +++ b/block/backup.c @@ -501,6 +501,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, BlockJobTxn *txn, Error **errp) { int64_t len; + BlockDriverInfo bdi; + int ret; assert(bs); assert(target); @@ -570,15 +572,30 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, goto error; } - bdrv_op_block_all(target, job->common.blocker); - job->on_source_error = on_source_error; job->on_target_error = on_target_error; job->target = target; job->sync_mode = sync_mode; job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ? sync_bitmap : NULL; - job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT; + + /* If there is no backing file on the target, we cannot rely on COW if our + * backup cluster size is smaller than the target cluster size. Even for + * targets with a backing file, try to avoid COW if possible. */ + ret = bdrv_get_info(job->target, &bdi); + if (ret < 0 && !target->backing) { + error_setg_errno(errp, -ret, + "Can't determine cluster size of target that has no backing file. " + "This may create an unusable destination image. Aborting."); + goto error; + } else if (ret < 0 && target->backing) { + /* Not fatal; just trudge on ahead. */ + job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT; + } else { + job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size); + } + + bdrv_op_block_all(target, job->common.blocker); job->common.len = len; job->common.co = qemu_coroutine_create(backup_run); block_job_txn_add_job(txn, &job->common);
During incremental backups, if the target has a cluster size that is larger than the backup cluster size and we are backing up to a target that cannot (for whichever reason) pull clusters up from a backing image, we may inadvertantly create unusable incremental backup images. For example: If the bitmap tracks changes at a 64KB granularity and we transmit 64KB of data at a time but the target uses a 128KB cluster size, it is possible that only half of a target cluster will be recognized as dirty by the backup block job. When the cluster is allocated on the target image but only half populated with data, we lose the ability to distinguish between zero padding and uninitialized data. This does not happen if the target image has a backing file that points to the last known good backup. Even if we have a backing file, though, it's likely going to be faster to just buffer the redundant data ourselves from the live image than fetching it from the backing file, so let's just always round up to the target granularity. The same logic applies to backup modes top, none, and full. Copying fractional clusters without the guarantee of COW is dangerous, but even if we can rely on COW, it's likely better to just re-copy the data. Reported-by: Fam Zheng <famz@redhat.com> Signed-off-by: John Snow <jsnow@redhat.com> --- block/backup.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)