[libvirt] bug: try to take disk snapshot for LVM2 Volume
Dave Allan
dallan at redhat.com
Wed Nov 16 03:20:31 UTC 2011
On Tue, Nov 15, 2011 at 05:26:59PM -0700, Eric Blake wrote:
> On 11/15/2011 04:18 PM, Eric Blake wrote:
> >> 2. Do the 'virsh snapshot-create abc --disk-only' with libvirt-0.9.6 or
> >> upper.
> >
> > But here, since you failed to use the --xmlfile option with XML
> > describing the new qcow2 file name (or used snapshot-create-as with the
> > --diskspec option), you are asking libvirt to create the name for the
> > new qcow2 file, and to create it in the same directory as the existing
> > disk image. That is, you asked libvirt to create /dev/sdc.1317357844
> > (with that suffix being the timestamp of your attempt).
> >
>
> >>
> >> And 'snapshot-create ... --disk-only' works well for image format file,
> >> e.g. qcow2, raw and etc.
> >
> > It works for any backing file format. Where it needs help is knowing
> > what file name to use - if your backing file is a block device instead
> > of a regular file, then it is up to you to help libvirt out by giving it
> > a sensible file name for the new qcow2 image, either with the --xmlfile
> > option of snapshot-create, or the --diskspec option of snapshot-create-as.
> >
> >>
> >> So, I think that the restriction is needed for the taking snapshot
> >> disk-only for the disk volume.
> >
> > This is where I disagree with your conclusion. Taking a disk-only
> > snapshot of block devices works just fine, provided that you tell
> > libvirt what file name to use for the qcow2 file it creates.
>
> After working on this some more, I think that identifying problematic
> file systems, like devtmpfs, is too tricky to be portable. But I think
> we can meet halfway - right now, the libvirt code blindly generates a
> filename if one was not provided, regardless of the file type of the
> backing file it will be wrapping. But maybe it would be sufficient to
> make the command error out if no qcow2 filename is provided and the
> backing file is not a regular file. As in this patch:
Ok, now I understand the situation; thanks to everyone for the
explanations. I'm somewhat uncomfortable using file type as a proxy
for "troublesome filesystem" since although they are linked in this
case, I'm not sure they're linked in all cases. Maybe I'm wrong about
that. If there is no portable way to determine whether a particular
filesystem is going to be a problem, I would just clearly document the
limitations of letting libvirt choose the filename and the importance
of not using that functionality on filesystems that cannot hold a
useful snapshot.
Dave
> From 099080c24eb1ed78c836e5823d351ab2980dc523 Mon Sep 17 00:00:00 2001
> From: Eric Blake <eblake at redhat.com>
> Date: Tue, 15 Nov 2011 17:19:20 -0700
> Subject: [PATCH] snapshot: refuse to generate names for non-regular backing
> files
>
> For whatever reason, the kernel allows you to create a regular
> file named /dev/sdc.12345; although this file will disappear the
> next time devtmpfs is remounted. If you let libvirt generate
> the name of the external snapshot for a disk image originally
> using the block device /dev/sdc, then the domain will be rendered
> unbootable once the qcow2 file is lost on the next devtmpfs
> remount. In this case, the user should have used 'virsh
> snapshot-create --xmlfile' or 'virsh snapshot-create-as --diskspec'
> to specify the name for the qcow2 file in a sane location, rather
> than relying on libvirt generating a name that is most likely to
> be wrong. We can help avoid naive mistakes by enforcing that
> the user provide the external name for any backing file that is
> not a regular file.
>
> * src/conf/domain_conf.c (virDomainSnapshotAlignDisks): Only
> generate names if backing file exists as regular file.
> Reported by MATSUDA Daiki.
> ---
> src/conf/domain_conf.c | 15 +++++++++++++--
> 1 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6b78d97..1cef2be 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12203,7 +12203,8 @@
> virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
>
> qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), disksorter);
>
> - /* Generate any default external file names. */
> + /* Generate any default external file names, but only if the
> + * backing file is a regular file. */
> for (i = 0; i < def->ndisks; i++) {
> virDomainSnapshotDiskDefPtr disk = &def->disks[i];
>
> @@ -12211,14 +12212,24 @@
> virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
> !disk->file) {
> const char *original = def->dom->disks[i]->src;
> const char *tmp;
> + struct stat sb;
>
> if (!original) {
> virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("cannot generate external backup
> name "
> + _("cannot generate external
> snapshot name "
> "for disk '%s' without source"),
> disk->name);
> goto cleanup;
> }
> + if (stat(original, &sb) < 0 || !S_ISREG(sb.st_mode)) {
> + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("source for disk '%s' is not a
> regular "
> + "file; refusing to generate
> external "
> + "snapshot name"),
> + disk->name);
> + goto cleanup;
> + }
> +
> tmp = strrchr(original, '.');
> if (!tmp || strchr(tmp, '/')) {
> ignore_value(virAsprintf(&disk->file, "%s.%s",
> --
> 1.7.7.1
>
>
>
> --
> Eric Blake eblake at redhat.com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list