Re: [Xen-devel] [PATCH] libxl/qemu-xen: use cache=writeback for IDE and cache=none for SCSI

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > > 
> > > 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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel