On Wed, 02/15 14:42, Vladimir Sementsov-Ogievskiy wrote: > 15.02.2017 14:33, Fam Zheng wrote: > > On Wed, 02/15 14:09, Vladimir Sementsov-Ogievskiy wrote: > > > Currently backup to nbd target is broken, as nbd doesn't have > > > .bdrv_get_info realization. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]> > > > --- > > > > > > v2: add WARNING > > > > > > === > > > > > > Since commit > > > > > > commit 4c9bca7e39a6e07ad02c1dcde3478363344ec60b > > > Author: John Snow <[email protected]> > > > Date: Thu Feb 25 15:58:30 2016 -0500 > > > > > > block/backup: avoid copying less than full target clusters > > > > > > backup to nbd target is broken, we have "Couldn't determine the cluster > > > size of > > > the target image". > > > > > > Proposed NBD protocol extension - NBD_OPT_INFO should finally solve this > > > problem. > > > But until it is not realized, we need allow backup to nbd target due to > > > backward > > > compatibility. > > If you respin, including above in the commit message is even better, IMO. > > > > > Furthermore, is it entirely ok to disallow backup if bds lacks > > > .bdrv_get_info? > > > Which behavior should be default: to fail backup or to use default > > > cluster size? > > > > > > block/backup.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/block/backup.c b/block/backup.c > > > index ea38733..a464b63 100644 > > > --- a/block/backup.c > > > +++ b/block/backup.c > > > @@ -638,7 +638,15 @@ BlockJob *backup_job_create(const char *job_id, > > > BlockDriverState *bs, > > > * 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(target, &bdi); > > > - if (ret < 0 && !target->backing) { > > > + if (ret == -ENOTSUP) { > > > + /* Cluster size is not defined */ > > > + error_report("WARNING: Target block device doesn't provide > > > information " > > > + "about block size and it doesn't have backing file. > > > " > > > + "Default block size of %u bytes is used. If actual > > > block " > > > + "size of target exceeds this default, destination > > > image " > > > + "may become unusable"); > > "Image unusable" might be too strong here because it sounds like a serious > > damage to the image, while actually what we mean is "backup unusable". > > Finally, "... default, backup may be unusable"?
Yes, looks good. Fam
