| Message ID | 1339598189-17933-5-git-send-email-wdongxu@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 13.06.2012 16:36, schrieb Dong Xu Wang: > add-cow will let raw file support snapshot_blkdev indirectly. > > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> Paolo, what do you think about this magic? I think I can see its use especially for HMP because it's quite convenient, but on the other hand it assumes a fixed image path for the new raw image. This isn't going to work for block devices, for example. If we don't do it this way, we need to allow passing image creation options to the snapshotting command so that you can pass a precreated image file. Kevin > --- > blockdev.c | 31 +++++++++++++++++++++++++++---- > docs/live-block-ops.txt | 10 +++++++++- > 2 files changed, 36 insertions(+), 5 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 622ecba..2d89e5e 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -783,15 +783,38 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp) > > /* create new image w/backing file */ > if (mode != NEW_IMAGE_MODE_EXISTING) { > - ret = bdrv_img_create(new_image_file, format, > + if (strcmp(format, "add-cow")) { > + ret = bdrv_img_create(new_image_file, format, > states->old_bs->filename, > states->old_bs->drv->format_name, > NULL, -1, flags); > - if (ret) { > - error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); > - goto delete_and_fail; > + } else { > + char image_file[1024]; > + char option[1024]; > + uint64_t size; > + > + bdrv_get_geometry(states->old_bs, &size); > + size *= BDRV_SECTOR_SIZE; > + > + sprintf(image_file, "%s.raw", new_image_file); > + > + ret = bdrv_img_create(image_file, "raw", NULL, > + NULL, NULL, size, flags); > + if (ret) { > + error_set(errp, QERR_UNDEFINED_ERROR); > + return; > + } > + sprintf(option, "image_file=%s.raw", new_image_file); > + ret = bdrv_img_create(new_image_file, format, > + states->old_bs->filename, > + states->old_bs->drv->format_name, > + option, -1, flags); > } > } > + if (ret) { > + error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); > + goto delete_and_fail; > + } > > /* We will manually add the backing_hd field to the bs later */ > states->new_bs = bdrv_new(""); > diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt > index a257087..c97344b 100644 > --- a/docs/live-block-ops.txt > +++ b/docs/live-block-ops.txt > @@ -2,7 +2,8 @@ LIVE BLOCK OPERATIONS > ===================== > > High level description of live block operations. Note these are not > -supported for use with the raw format at the moment. > +supported for use with the raw format at the moment, but we can use > +add-cow as metadata to suport raw format. > > Snapshot live merge > =================== > @@ -56,3 +57,10 @@ into that image. Example: > (qemu) block_stream ide0-hd0 > > > + > +Raw is not supported, but we can use add-cow in the 1st step: > + > +(qemu) snapshot_blkdev ide0-hd0 /new-path/disk.img add-cow > + > +It will create a raw file named disk.img.raw, with the same virtual size of > +ide0-hd0 first, and then create disk.img.
Il 14/06/2012 12:59, Kevin Wolf ha scritto: > Am 13.06.2012 16:36, schrieb Dong Xu Wang: >> add-cow will let raw file support snapshot_blkdev indirectly. >> >> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > > Paolo, what do you think about this magic? Besides the obvious layering violation (it would be better to add a new method bdrv_ext_snapshot_create perhaps) I don't see very much a problem in it. Passing image creation options sounds like a good idea that we can add in the future as an extension. But honestly, I don't really see the point of add-cow in general... The raw image is anyway not usable without a pass through qemu-io convert, and mirroring will also allow collapsing an image to raw (with the persistent dirty bitmap playing the role of the add-cow metadata). > I think I can see its use especially for HMP because it's quite > convenient, but on the other hand it assumes a fixed image path for the > new raw image. This isn't going to work for block devices, for example. True, but then probably you would use mode='existing', because you need to pre-create the logical volume. > If we don't do it this way, we need to allow passing image creation > options to the snapshotting command so that you can pass a precreated > image file. This sounds like a useful extension anyway, except that passing an unstructured string for image creation options is ugly... Perhaps we can base a better implementation of options on Laszlo's QemuOpts visitor. Paolo > > Kevin > >> --- >> blockdev.c | 31 +++++++++++++++++++++++++++---- >> docs/live-block-ops.txt | 10 +++++++++- >> 2 files changed, 36 insertions(+), 5 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index 622ecba..2d89e5e 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -783,15 +783,38 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp) >> >> /* create new image w/backing file */ >> if (mode != NEW_IMAGE_MODE_EXISTING) { >> - ret = bdrv_img_create(new_image_file, format, >> + if (strcmp(format, "add-cow")) { >> + ret = bdrv_img_create(new_image_file, format, >> states->old_bs->filename, >> states->old_bs->drv->format_name, >> NULL, -1, flags); >> - if (ret) { >> - error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); >> - goto delete_and_fail; >> + } else { >> + char image_file[1024]; >> + char option[1024]; >> + uint64_t size; >> + >> + bdrv_get_geometry(states->old_bs, &size); >> + size *= BDRV_SECTOR_SIZE; >> + >> + sprintf(image_file, "%s.raw", new_image_file); >> + >> + ret = bdrv_img_create(image_file, "raw", NULL, >> + NULL, NULL, size, flags); >> + if (ret) { >> + error_set(errp, QERR_UNDEFINED_ERROR); >> + return; >> + } >> + sprintf(option, "image_file=%s.raw", new_image_file); >> + ret = bdrv_img_create(new_image_file, format, >> + states->old_bs->filename, >> + states->old_bs->drv->format_name, >> + option, -1, flags); >> } >> } >> + if (ret) { >> + error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); >> + goto delete_and_fail; >> + } >> >> /* We will manually add the backing_hd field to the bs later */ >> states->new_bs = bdrv_new(""); >> diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt >> index a257087..c97344b 100644 >> --- a/docs/live-block-ops.txt >> +++ b/docs/live-block-ops.txt >> @@ -2,7 +2,8 @@ LIVE BLOCK OPERATIONS >> ===================== >> >> High level description of live block operations. Note these are not >> -supported for use with the raw format at the moment. >> +supported for use with the raw format at the moment, but we can use >> +add-cow as metadata to suport raw format. >> >> Snapshot live merge >> =================== >> @@ -56,3 +57,10 @@ into that image. Example: >> (qemu) block_stream ide0-hd0 >> >> >> + >> +Raw is not supported, but we can use add-cow in the 1st step: >> + >> +(qemu) snapshot_blkdev ide0-hd0 /new-path/disk.img add-cow >> + >> +It will create a raw file named disk.img.raw, with the same virtual size of >> +ide0-hd0 first, and then create disk.img. > > >
Am 14.06.2012 13:18, schrieb Paolo Bonzini: > Il 14/06/2012 12:59, Kevin Wolf ha scritto: >> Am 13.06.2012 16:36, schrieb Dong Xu Wang: >>> add-cow will let raw file support snapshot_blkdev indirectly. >>> >>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> >> >> Paolo, what do you think about this magic? > > Besides the obvious layering violation (it would be better to add a new > method bdrv_ext_snapshot_create perhaps) I don't see very much a problem > in it. Passing image creation options sounds like a good idea that we > can add in the future as an extension. > > But honestly, I don't really see the point of add-cow in general... The > raw image is anyway not usable without a pass through qemu-io convert, > and mirroring will also allow collapsing an image to raw (with the > persistent dirty bitmap playing the role of the add-cow metadata). Well, the idea was that you stream into add-cow and once the streaming has completed, you can just drop the bitmap. There might be some overlap with mirroring, though when we discussed introducing add-cow, mirrors were not yet on the table. One difference that remains is that with streaming the guest only writes to the target image and can't have any problem with convergence. >> I think I can see its use especially for HMP because it's quite >> convenient, but on the other hand it assumes a fixed image path for the >> new raw image. This isn't going to work for block devices, for example. > > True, but then probably you would use mode='existing', because you need > to pre-create the logical volume. I think it might be convenient to have the raw volume precreated (you need to do that anyway), but create the COW bitmap only during the snapshot command. But yeah, not really important. >> If we don't do it this way, we need to allow passing image creation >> options to the snapshotting command so that you can pass a precreated >> image file. > > This sounds like a useful extension anyway, except that passing an > unstructured string for image creation options is ugly... Perhaps we > can base a better implementation of options on Laszlo's QemuOpts visitor. Yes, I wouldn't want to use a string here, we should use something structured. Image creation still uses the old-style options instead of QemuOpts, but maybe this is the opportunity to convert it. Kevin
On Thu, Jun 14, 2012 at 7:33 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 14.06.2012 13:18, schrieb Paolo Bonzini: >> Il 14/06/2012 12:59, Kevin Wolf ha scritto: >>> Am 13.06.2012 16:36, schrieb Dong Xu Wang: >>>> add-cow will let raw file support snapshot_blkdev indirectly. >>>> >>>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> >>> >>> Paolo, what do you think about this magic? >> >> Besides the obvious layering violation (it would be better to add a new >> method bdrv_ext_snapshot_create perhaps) I don't see very much a problem >> in it. Passing image creation options sounds like a good idea that we >> can add in the future as an extension. >> >> But honestly, I don't really see the point of add-cow in general... The >> raw image is anyway not usable without a pass through qemu-io convert, >> and mirroring will also allow collapsing an image to raw (with the >> persistent dirty bitmap playing the role of the add-cow metadata). > > Well, the idea was that you stream into add-cow and once the streaming > has completed, you can just drop the bitmap. > > There might be some overlap with mirroring, though when we discussed > introducing add-cow, mirrors were not yet on the table. One difference > that remains is that with streaming the guest only writes to the target > image and can't have any problem with convergence. > >>> I think I can see its use especially for HMP because it's quite >>> convenient, but on the other hand it assumes a fixed image path for the >>> new raw image. This isn't going to work for block devices, for example. >> >> True, but then probably you would use mode='existing', because you need >> to pre-create the logical volume. > > I think it might be convenient to have the raw volume precreated (you > need to do that anyway), but create the COW bitmap only during the > snapshot command. But yeah, not really important. > >>> If we don't do it this way, we need to allow passing image creation >>> options to the snapshotting command so that you can pass a precreated >>> image file. >> >> This sounds like a useful extension anyway, except that passing an >> unstructured string for image creation options is ugly... Perhaps we >> can base a better implementation of options on Laszlo's QemuOpts visitor. > > Yes, I wouldn't want to use a string here, we should use something > structured. Image creation still uses the old-style options instead of > QemuOpts, but maybe this is the opportunity to convert it. Kevin, do you mean I need to replace QEMUOptionParameter with QemuOpts? If true, other image formats should also be changed, I noticed Stefan has an un-comleted patch: http://repo.or.cz/w/qemu/stefanha.git/commitdiff/b49babb2c8b476a36357cfd7276ca45a11039ca5 then I can work based on Stefan's patch. > > Kevin >
Am 19.07.2012 04:20, schrieb Dong Xu Wang: > On Thu, Jun 14, 2012 at 7:33 PM, Kevin Wolf <kwolf@redhat.com> wrote: >> Am 14.06.2012 13:18, schrieb Paolo Bonzini: >>> Il 14/06/2012 12:59, Kevin Wolf ha scritto: >>>> Am 13.06.2012 16:36, schrieb Dong Xu Wang: >>>>> add-cow will let raw file support snapshot_blkdev indirectly. >>>>> >>>>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> >>>> >>>> Paolo, what do you think about this magic? >>> >>> Besides the obvious layering violation (it would be better to add a new >>> method bdrv_ext_snapshot_create perhaps) I don't see very much a problem >>> in it. Passing image creation options sounds like a good idea that we >>> can add in the future as an extension. >>> >>> But honestly, I don't really see the point of add-cow in general... The >>> raw image is anyway not usable without a pass through qemu-io convert, >>> and mirroring will also allow collapsing an image to raw (with the >>> persistent dirty bitmap playing the role of the add-cow metadata). >> >> Well, the idea was that you stream into add-cow and once the streaming >> has completed, you can just drop the bitmap. >> >> There might be some overlap with mirroring, though when we discussed >> introducing add-cow, mirrors were not yet on the table. One difference >> that remains is that with streaming the guest only writes to the target >> image and can't have any problem with convergence. >> >>>> I think I can see its use especially for HMP because it's quite >>>> convenient, but on the other hand it assumes a fixed image path for the >>>> new raw image. This isn't going to work for block devices, for example. >>> >>> True, but then probably you would use mode='existing', because you need >>> to pre-create the logical volume. >> >> I think it might be convenient to have the raw volume precreated (you >> need to do that anyway), but create the COW bitmap only during the >> snapshot command. But yeah, not really important. >> >>>> If we don't do it this way, we need to allow passing image creation >>>> options to the snapshotting command so that you can pass a precreated >>>> image file. >>> >>> This sounds like a useful extension anyway, except that passing an >>> unstructured string for image creation options is ugly... Perhaps we >>> can base a better implementation of options on Laszlo's QemuOpts visitor. >> >> Yes, I wouldn't want to use a string here, we should use something >> structured. Image creation still uses the old-style options instead of >> QemuOpts, but maybe this is the opportunity to convert it. > > Kevin, do you mean I need to replace QEMUOptionParameter with QemuOpts? > > If true, other image formats should also be changed, I noticed Stefan > has an un-comleted patch: > http://repo.or.cz/w/qemu/stefanha.git/commitdiff/b49babb2c8b476a36357cfd7276ca45a11039ca5 > > then I can work based on Stefan's patch. Yes, the direction of this patch from Stefan's repo looks good. I'm not sure whether it will immediately allow passing structured image creation options in QMP, though. It might work using the old-style monitor commands, just getting a QDict from the options. Not quite sure how this is going to work with QAPI. Kevin
On Thu, Jul 19, 2012 at 3:20 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote: > On Thu, Jun 14, 2012 at 7:33 PM, Kevin Wolf <kwolf@redhat.com> wrote: >> Am 14.06.2012 13:18, schrieb Paolo Bonzini: >>> Il 14/06/2012 12:59, Kevin Wolf ha scritto: >>>> Am 13.06.2012 16:36, schrieb Dong Xu Wang: >>>>> add-cow will let raw file support snapshot_blkdev indirectly. >>>>> >>>>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> >>>> >>>> Paolo, what do you think about this magic? >>> >>> Besides the obvious layering violation (it would be better to add a new >>> method bdrv_ext_snapshot_create perhaps) I don't see very much a problem >>> in it. Passing image creation options sounds like a good idea that we >>> can add in the future as an extension. >>> >>> But honestly, I don't really see the point of add-cow in general... The >>> raw image is anyway not usable without a pass through qemu-io convert, >>> and mirroring will also allow collapsing an image to raw (with the >>> persistent dirty bitmap playing the role of the add-cow metadata). >> >> Well, the idea was that you stream into add-cow and once the streaming >> has completed, you can just drop the bitmap. >> >> There might be some overlap with mirroring, though when we discussed >> introducing add-cow, mirrors were not yet on the table. One difference >> that remains is that with streaming the guest only writes to the target >> image and can't have any problem with convergence. >> >>>> I think I can see its use especially for HMP because it's quite >>>> convenient, but on the other hand it assumes a fixed image path for the >>>> new raw image. This isn't going to work for block devices, for example. >>> >>> True, but then probably you would use mode='existing', because you need >>> to pre-create the logical volume. >> >> I think it might be convenient to have the raw volume precreated (you >> need to do that anyway), but create the COW bitmap only during the >> snapshot command. But yeah, not really important. >> >>>> If we don't do it this way, we need to allow passing image creation >>>> options to the snapshotting command so that you can pass a precreated >>>> image file. >>> >>> This sounds like a useful extension anyway, except that passing an >>> unstructured string for image creation options is ugly... Perhaps we >>> can base a better implementation of options on Laszlo's QemuOpts visitor. >> >> Yes, I wouldn't want to use a string here, we should use something >> structured. Image creation still uses the old-style options instead of >> QemuOpts, but maybe this is the opportunity to convert it. > > Kevin, do you mean I need to replace QEMUOptionParameter with QemuOpts? > > If true, other image formats should also be changed, I noticed Stefan > has an un-comleted patch: > http://repo.or.cz/w/qemu/stefanha.git/commitdiff/b49babb2c8b476a36357cfd7276ca45a11039ca5 > > then I can work based on Stefan's patch. It's been a while so I don't remember details, but if you want to discuss more let me know and I'll refresh my memory. Stefan
On Thu, 19 Jul 2012 10:17:10 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > Am 19.07.2012 04:20, schrieb Dong Xu Wang: > > On Thu, Jun 14, 2012 at 7:33 PM, Kevin Wolf <kwolf@redhat.com> wrote: > >> Am 14.06.2012 13:18, schrieb Paolo Bonzini: > >>> Il 14/06/2012 12:59, Kevin Wolf ha scritto: > >>>> Am 13.06.2012 16:36, schrieb Dong Xu Wang: > >>>>> add-cow will let raw file support snapshot_blkdev indirectly. > >>>>> > >>>>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > >>>> > >>>> Paolo, what do you think about this magic? > >>> > >>> Besides the obvious layering violation (it would be better to add a new > >>> method bdrv_ext_snapshot_create perhaps) I don't see very much a problem > >>> in it. Passing image creation options sounds like a good idea that we > >>> can add in the future as an extension. > >>> > >>> But honestly, I don't really see the point of add-cow in general... The > >>> raw image is anyway not usable without a pass through qemu-io convert, > >>> and mirroring will also allow collapsing an image to raw (with the > >>> persistent dirty bitmap playing the role of the add-cow metadata). > >> > >> Well, the idea was that you stream into add-cow and once the streaming > >> has completed, you can just drop the bitmap. > >> > >> There might be some overlap with mirroring, though when we discussed > >> introducing add-cow, mirrors were not yet on the table. One difference > >> that remains is that with streaming the guest only writes to the target > >> image and can't have any problem with convergence. > >> > >>>> I think I can see its use especially for HMP because it's quite > >>>> convenient, but on the other hand it assumes a fixed image path for the > >>>> new raw image. This isn't going to work for block devices, for example. > >>> > >>> True, but then probably you would use mode='existing', because you need > >>> to pre-create the logical volume. > >> > >> I think it might be convenient to have the raw volume precreated (you > >> need to do that anyway), but create the COW bitmap only during the > >> snapshot command. But yeah, not really important. > >> > >>>> If we don't do it this way, we need to allow passing image creation > >>>> options to the snapshotting command so that you can pass a precreated > >>>> image file. > >>> > >>> This sounds like a useful extension anyway, except that passing an > >>> unstructured string for image creation options is ugly... Perhaps we > >>> can base a better implementation of options on Laszlo's QemuOpts visitor. > >> > >> Yes, I wouldn't want to use a string here, we should use something > >> structured. Image creation still uses the old-style options instead of > >> QemuOpts, but maybe this is the opportunity to convert it. > > > > Kevin, do you mean I need to replace QEMUOptionParameter with QemuOpts? > > > > If true, other image formats should also be changed, I noticed Stefan > > has an un-comleted patch: > > http://repo.or.cz/w/qemu/stefanha.git/commitdiff/b49babb2c8b476a36357cfd7276ca45a11039ca5 > > > > then I can work based on Stefan's patch. > > Yes, the direction of this patch from Stefan's repo looks good. > > I'm not sure whether it will immediately allow passing structured image > creation options in QMP, though. It might work using the old-style > monitor commands, just getting a QDict from the options. Not quite sure > how this is going to work with QAPI. We did it for netdev_add and it works as you said above. The schema entry looks like this: { 'command': 'netdev_add', 'data': {'type': 'str', 'id': 'str', '*props': '**'}, 'gen': 'no' } Where 'props' is: # @props: #optional a list of properties to be passed to the backend in # the format 'name=value', like 'ifname=tap0,script=no' Then we have the qmp front-end, using the old style monitor signature: int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret); And we have the HMP front-end (which should be used elsewhere too): void netdev_add(QemuOpts *opts, Error **errp); As Paolo said, Laszlo's work will help us to this the right way.
diff --git a/blockdev.c b/blockdev.c index 622ecba..2d89e5e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -783,15 +783,38 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp) /* create new image w/backing file */ if (mode != NEW_IMAGE_MODE_EXISTING) { - ret = bdrv_img_create(new_image_file, format, + if (strcmp(format, "add-cow")) { + ret = bdrv_img_create(new_image_file, format, states->old_bs->filename, states->old_bs->drv->format_name, NULL, -1, flags); - if (ret) { - error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); - goto delete_and_fail; + } else { + char image_file[1024]; + char option[1024]; + uint64_t size; + + bdrv_get_geometry(states->old_bs, &size); + size *= BDRV_SECTOR_SIZE; + + sprintf(image_file, "%s.raw", new_image_file); + + ret = bdrv_img_create(image_file, "raw", NULL, + NULL, NULL, size, flags); + if (ret) { + error_set(errp, QERR_UNDEFINED_ERROR); + return; + } + sprintf(option, "image_file=%s.raw", new_image_file); + ret = bdrv_img_create(new_image_file, format, + states->old_bs->filename, + states->old_bs->drv->format_name, + option, -1, flags); } } + if (ret) { + error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); + goto delete_and_fail; + } /* We will manually add the backing_hd field to the bs later */ states->new_bs = bdrv_new(""); diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt index a257087..c97344b 100644 --- a/docs/live-block-ops.txt +++ b/docs/live-block-ops.txt @@ -2,7 +2,8 @@ LIVE BLOCK OPERATIONS ===================== High level description of live block operations. Note these are not -supported for use with the raw format at the moment. +supported for use with the raw format at the moment, but we can use +add-cow as metadata to suport raw format. Snapshot live merge =================== @@ -56,3 +57,10 @@ into that image. Example: (qemu) block_stream ide0-hd0 + +Raw is not supported, but we can use add-cow in the 1st step: + +(qemu) snapshot_blkdev ide0-hd0 /new-path/disk.img add-cow + +It will create a raw file named disk.img.raw, with the same virtual size of +ide0-hd0 first, and then create disk.img.
add-cow will let raw file support snapshot_blkdev indirectly. Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> --- blockdev.c | 31 +++++++++++++++++++++++++++---- docs/live-block-ops.txt | 10 +++++++++- 2 files changed, 36 insertions(+), 5 deletions(-)