Stefano Stabellini
2012-Aug-14 15:05 UTC
[PATCH] libxl/qemu-xen: use cache=writeback for IDE and cache=none for SCSI
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 0c0084f..1c94e80 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -549,10 +549,10 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
if (disks[i].is_cdrom) {
if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY)
drive = libxl__sprintf
- (gc, "if=ide,index=%d,media=cdrom", disk);
+ (gc,
"if=ide,index=%d,media=cdrom,cache=writeback", disk);
else
drive = libxl__sprintf
- (gc,
"file=%s,if=ide,index=%d,media=cdrom,format=%s",
+ (gc,
"file=%s,if=ide,index=%d,media=cdrom,format=%s,cache=writeback",
disks[i].pdev_path, disk, format);
} else {
if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
@@ -575,11 +575,11 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
*/
if (strncmp(disks[i].vdev, "sd", 2) == 0)
drive = libxl__sprintf
- (gc,
"file=%s,if=scsi,bus=0,unit=%d,format=%s",
+ (gc,
"file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=none",
disks[i].pdev_path, disk, format);
else if (disk < 4)
drive = libxl__sprintf
- (gc,
"file=%s,if=ide,index=%d,media=disk,format=%s",
+ (gc,
"file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
disks[i].pdev_path, disk, format);
else
continue; /* Do not emulate this disk */
Stefano Stabellini
2012-Aug-22 11:13 UTC
Re: [PATCH] libxl/qemu-xen: use cache=writeback for IDE and cache=none for SCSI
On Tue, 14 Aug 2012, Stefano Stabellini wrote:> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>ping> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 0c0084f..1c94e80 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -549,10 +549,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > if (disks[i].is_cdrom) { > if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) > drive = libxl__sprintf > - (gc, "if=ide,index=%d,media=cdrom", disk); > + (gc, "if=ide,index=%d,media=cdrom,cache=writeback", disk); > else > drive = libxl__sprintf > - (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s", > + (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s,cache=writeback", > disks[i].pdev_path, disk, format); > } else { > if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) { > @@ -575,11 +575,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > */ > if (strncmp(disks[i].vdev, "sd", 2) == 0) > drive = libxl__sprintf > - (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s", > + (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=none", > disks[i].pdev_path, disk, format); > else if (disk < 4) > drive = libxl__sprintf > - (gc, "file=%s,if=ide,index=%d,media=disk,format=%s", > + (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback", > disks[i].pdev_path, disk, format); > else > continue; /* Do not emulate this disk */ > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >
Ian Campbell
2012-Aug-22 13:27 UTC
Re: [PATCH] libxl/qemu-xen: use cache=writeback for IDE and cache=none for SCSI
On Wed, 2012-08-22 at 12:13 +0100, Stefano Stabellini wrote:> On Tue, 14 Aug 2012, Stefano Stabellini wrote: > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > pingIs this an appropriate change during rcs, is it critical for 4.2 or should it wait for 4.3? I think the changelog here is rather lacking, which makes it hard for me to decide what to do. e.g. the usual stuff: Why are you making this change? What is the impact? etc What is the default for all these cases?> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > > index 0c0084f..1c94e80 100644 > > --- a/tools/libxl/libxl_dm.c > > +++ b/tools/libxl/libxl_dm.c > > @@ -549,10 +549,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > > if (disks[i].is_cdrom) { > > if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) > > drive = libxl__sprintf > > - (gc, "if=ide,index=%d,media=cdrom", disk); > > + (gc, "if=ide,index=%d,media=cdrom,cache=writeback", disk);Why does the cacheability matter for an empty device?> > else > > drive = libxl__sprintf > > - (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s", > > + (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s,cache=writeback",Does writeback mean anything for a r/o device?> > disks[i].pdev_path, disk, format); > > } else { > > if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) { > > @@ -575,11 +575,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > > */ > > if (strncmp(disks[i].vdev, "sd", 2) == 0) > > drive = libxl__sprintf > > - (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s", > > + (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=none", > > disks[i].pdev_path, disk, format); > > else if (disk < 4) > > drive = libxl__sprintf > > - (gc, "file=%s,if=ide,index=%d,media=disk,format=%s", > > + (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",Why is an SCSI disk treated differently to an IDE one wrt caching? Ian.
Ian Campbell
2012-Sep-25 09:27 UTC
Re: [PATCH] libxl/qemu-xen: use cache=writeback for IDE and cache=none for SCSI
On Wed, 2012-08-22 at 14:27 +0100, Ian Campbell wrote:> On Wed, 2012-08-22 at 12:13 +0100, Stefano Stabellini wrote: > > On Tue, 14 Aug 2012, Stefano Stabellini wrote: > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > pingpong?> > Is this an appropriate change during rcs, is it critical for 4.2 or > should it wait for 4.3? > > I think the changelog here is rather lacking, which makes it hard for me > to decide what to do. e.g. the usual stuff: Why are you making this > change? What is the impact? etc > > What is the default for all these cases? > > > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > > > index 0c0084f..1c94e80 100644 > > > --- a/tools/libxl/libxl_dm.c > > > +++ b/tools/libxl/libxl_dm.c > > > @@ -549,10 +549,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > > > if (disks[i].is_cdrom) { > > > if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) > > > drive = libxl__sprintf > > > - (gc, "if=ide,index=%d,media=cdrom", disk); > > > + (gc, "if=ide,index=%d,media=cdrom,cache=writeback", disk); > > Why does the cacheability matter for an empty device? > > > > else > > > drive = libxl__sprintf > > > - (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s", > > > + (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s,cache=writeback", > > Does writeback mean anything for a r/o device? > > > > disks[i].pdev_path, disk, format); > > > } else { > > > if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) { > > > @@ -575,11 +575,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > > > */ > > > if (strncmp(disks[i].vdev, "sd", 2) == 0) > > > drive = libxl__sprintf > > > - (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s", > > > + (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=none", > > > disks[i].pdev_path, disk, format); > > > else if (disk < 4) > > > drive = libxl__sprintf > > > - (gc, "file=%s,if=ide,index=%d,media=disk,format=%s", > > > + (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback", > > Why is an SCSI disk treated differently to an IDE one wrt caching? > > Ian. > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Stefano Stabellini
2012-Sep-25 10:15 UTC
Re: [PATCH] libxl/qemu-xen: use cache=writeback for IDE and cache=none for SCSI
I found this email in my draft mailbox, I must have forgotten to send it. On Wed, 22 Aug 2012, Ian Campbell wrote:> On Wed, 2012-08-22 at 12:13 +0100, Stefano Stabellini wrote: > > On Tue, 14 Aug 2012, Stefano Stabellini wrote: > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > ping > > Is this an appropriate change during rcs, is it critical for 4.2 or > should it wait for 4.3?It should go in one of the 4.2.x releases.> I think the changelog here is rather lacking, which makes it hard for me > to decide what to do. e.g. the usual stuff: Why are you making this > change? What is the impact? etcThis patch makes QEMU upstream behave like QEMU traditional, that changed behavior at the same time this patch was sent (effd5676225761abdab90becac519716515c3be4).> What is the default for all these cases?The default is writethrough, that is very slow.> > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > > > index 0c0084f..1c94e80 100644 > > > --- a/tools/libxl/libxl_dm.c > > > +++ b/tools/libxl/libxl_dm.c > > > @@ -549,10 +549,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > > > if (disks[i].is_cdrom) { > > > if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) > > > drive = libxl__sprintf > > > - (gc, "if=ide,index=%d,media=cdrom", disk); > > > + (gc, "if=ide,index=%d,media=cdrom,cache=writeback", disk); > > Why does the cacheability matter for an empty device?It doesn''t but the default would stay the same when a cdrom is inserted (it is a property of the drive).> > > else > > > drive = libxl__sprintf > > > - (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s", > > > + (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s,cache=writeback", > > Does writeback mean anything for a r/o device?I doesn''t. Do you want me to resend without this hunk?> > > disks[i].pdev_path, disk, format); > > > } else { > > > if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) { > > > @@ -575,11 +575,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > > > */ > > > if (strncmp(disks[i].vdev, "sd", 2) == 0) > > > drive = libxl__sprintf > > > - (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s", > > > + (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=none", > > > disks[i].pdev_path, disk, format); > > > else if (disk < 4) > > > drive = libxl__sprintf > > > - (gc, "file=%s,if=ide,index=%d,media=disk,format=%s", > > > + (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback", > > Why is an SCSI disk treated differently to an IDE one wrt caching? >We chose writeback for IDE because IDE has an explicit flush command that the guest is going to issue when it wants the data to reach the disk. At that point we can do a datasync on the filesystem. I don''t know enough about SCSI to be sure that is the case there as well, so I wanted to stay on the safe side and chose cache=none instead (that means O_DIRECT).
Ian Campbell
2012-Sep-25 11:00 UTC
Re: [PATCH] libxl/qemu-xen: use cache=writeback for IDE and cache=none for SCSI
On Tue, 2012-09-25 at 11:15 +0100, Stefano Stabellini wrote:> I found this email in my draft mailbox, I must have forgotten to send it. > > On Wed, 22 Aug 2012, Ian Campbell wrote: > > On Wed, 2012-08-22 at 12:13 +0100, Stefano Stabellini wrote: > > > On Tue, 14 Aug 2012, Stefano Stabellini wrote: > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > > > ping > > > > Is this an appropriate change during rcs, is it critical for 4.2 or > > should it wait for 4.3? > > It should go in one of the 4.2.x releases. > > > > I think the changelog here is rather lacking, which makes it hard for me > > to decide what to do. e.g. the usual stuff: Why are you making this > > change? What is the impact? etc > > This patch makes QEMU upstream behave like QEMU traditional, that > changed behavior at the same time this patch was sent > (effd5676225761abdab90becac519716515c3be4).Thanks, please can duplicate most of that commit message here as well please, in particular the reference the ML thread. A reference to the trad commit ID might be useful too.> > What is the default for all these cases? > > The default is writethrough, that is very slow.Say "change caching mode from writethrough to writeback" in the commit message? Mention that this is a performance issue.> > > > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > > > > index 0c0084f..1c94e80 100644 > > > > --- a/tools/libxl/libxl_dm.c > > > > +++ b/tools/libxl/libxl_dm.c > > > > @@ -549,10 +549,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > > > > if (disks[i].is_cdrom) { > > > > if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) > > > > drive = libxl__sprintf > > > > - (gc, "if=ide,index=%d,media=cdrom", disk); > > > > + (gc, "if=ide,index=%d,media=cdrom,cache=writeback", disk); > > > > Why does the cacheability matter for an empty device? > > It doesn''t but the default would stay the same when a cdrom is inserted > (it is a property of the drive).OK.> > > > else > > > > drive = libxl__sprintf > > > > - (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s", > > > > + (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s,cache=writeback", > > > > Does writeback mean anything for a r/o device? > > I doesn''t. Do you want me to resend without this hunk?Up to you. I guess it is at least consistent with the other devices. (it feels like this same line is repeated a lot, but lets not tackle that here)> > > > disks[i].pdev_path, disk, format); > > > > } else { > > > > if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) { > > > > @@ -575,11 +575,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > > > > */ > > > > if (strncmp(disks[i].vdev, "sd", 2) == 0) > > > > drive = libxl__sprintf > > > > - (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s", > > > > + (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=none", > > > > disks[i].pdev_path, disk, format); > > > > else if (disk < 4) > > > > drive = libxl__sprintf > > > > - (gc, "file=%s,if=ide,index=%d,media=disk,format=%s", > > > > + (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback", > > > > Why is an SCSI disk treated differently to an IDE one wrt caching? > > > > We chose writeback for IDE because IDE has an explicit flush command > that the guest is going to issue when it wants the data to reach the disk. > At that point we can do a datasync on the filesystem. > I don''t know enough about SCSI to be sure that is the case there as > well, so I wanted to stay on the safe side and chose cache=none instead > (that means O_DIRECT).I''m pretty sure SCSI must have an explicit flush command as well although it might be nice if someone could confirm. And also check that qemu actually flushes... Ian.