Message ID | 1274347924-9188-2-git-send-email-Jes.Sorensen@redhat.com |
---|---|
State | New |
Headers | show |
On 05/20/2010 11:32 AM, Jes.Sorensen@redhat.com wrote: > + if (bdrv_flags & BDRV_O_NOCACHE) { > + fprintf(stderr, "qemu: failed to open disk image %s as " > + "nocache (O_DIRECT) retrying as write-back\n", file); > + bdrv_flags &= BDRV_O_NOCACHE; Missing ~ here. > + bdrv_flags |= BDRV_O_CACHE_WB; > + if (bdrv_open(dinfo->bdrv, file, bdrv_flags, drv)< 0) > + goto error_open; > + } else { I think the retry should be done silently if no cache= option is given. That is cache=none will be the default but: - if it is not specified and not supported by the image, fall back to writeback with no warning. However, this is just a QoI issue and can be fixed later. - if it is specified and not supported by the image, either fall back to writeback with a warning, or fail altogether. The former would be a change in behavior, so it has to be documented somewhere if it changes. Or maybe add BDRV_O_CACHE_WT and let the backend decide the default? Paolo
On 05/20/2010 10:24 AM, Paolo Bonzini wrote: > On 05/20/2010 11:32 AM, Jes.Sorensen@redhat.com wrote: >> + if (bdrv_flags & BDRV_O_NOCACHE) { >> + fprintf(stderr, "qemu: failed to open disk image %s as " >> + "nocache (O_DIRECT) retrying as write-back\n", >> file); >> + bdrv_flags &= BDRV_O_NOCACHE; > > Missing ~ here. > >> + bdrv_flags |= BDRV_O_CACHE_WB; >> + if (bdrv_open(dinfo->bdrv, file, bdrv_flags, drv)< 0) >> + goto error_open; >> + } else { > > I think the retry should be done silently if no cache= option is > given. That is cache=none will be the default but: > > - if it is not specified and not supported by the image, fall back to > writeback with no warning. However, this is just a QoI issue and can > be fixed later. > > - if it is specified and not supported by the image, either fall back > to writeback with a warning, or fail altogether. The former would be > a change in behavior, so it has to be documented somewhere if it changes. > > Or maybe add BDRV_O_CACHE_WT and let the backend decide the default? It used to be that we had a CACHE_DEFAULT which allowed qcow2 to do CACHE_WB by default whereas everything else did CACHE_WT. The same technique could be used to let physical devices do NOCACHE by default. Regards, Anthony Liguori > Paolo >
diff --git a/vl.c b/vl.c index d77b47c..f3a7d63 100644 --- a/vl.c +++ b/vl.c @@ -787,7 +787,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, int max_devs; int index; int ro = 0; - int bdrv_flags = 0; + int bdrv_flags = BDRV_O_NOCACHE; int on_read_error, on_write_error; const char *devaddr; DriveInfo *dinfo; @@ -910,11 +910,11 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, if ((buf = qemu_opt_get(opts, "cache")) != NULL) { if (!strcmp(buf, "off") || !strcmp(buf, "none")) { - bdrv_flags |= BDRV_O_NOCACHE; + /* default */ } else if (!strcmp(buf, "writeback")) { bdrv_flags |= BDRV_O_CACHE_WB; } else if (!strcmp(buf, "writethrough")) { - /* this is the default */ + bdrv_flags &= ~BDRV_O_CACHE_MASK; } else { fprintf(stderr, "qemu: invalid cache option\n"); return NULL; @@ -1120,15 +1120,28 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, bdrv_flags |= ro ? 0 : BDRV_O_RDWR; if (bdrv_open(dinfo->bdrv, file, bdrv_flags, drv) < 0) { - fprintf(stderr, "qemu: could not open disk image %s: %s\n", - file, strerror(errno)); - return NULL; + if (bdrv_flags & BDRV_O_NOCACHE) { + fprintf(stderr, "qemu: failed to open disk image %s as " + "nocache (O_DIRECT) retrying as write-back\n", file); + bdrv_flags &= BDRV_O_NOCACHE; + bdrv_flags |= BDRV_O_CACHE_WB; + if (bdrv_open(dinfo->bdrv, file, bdrv_flags, drv) < 0) + goto error_open; + } else { + goto error_open; + } } if (bdrv_key_required(dinfo->bdrv)) autostart = 0; *fatal_error = 0; return dinfo; + +error_open: + fprintf(stderr, "qemu: could not open disk image %s: %s\n", + file, strerror(errno)); + return NULL; + } static int drive_init_func(QemuOpts *opts, void *opaque)