| Message ID | 1461731731-6794-1-git-send-email-jfehlig@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 26, 2016 at 10:35:31PM -0600, Jim Fehlig wrote: > qemu commit 91a097e7 forbids specifying the cache mode for empty > drives. Attempting to create a domain with an empty qdisk cdrom > results in > > qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom, > cache=writeback,id=ide-832: Must specify either driver or file > > Change libxl to only emit cache mode when a cdrom target is specified. What happens then when a cdrom is inserted? I cannot seem to find the code in libxl_cdrom_insert that sets the cache mode. Is the default one used then? Roger.
On 04/27/2016 01:38 AM, Roger Pau Monné wrote: > On Tue, Apr 26, 2016 at 10:35:31PM -0600, Jim Fehlig wrote: >> qemu commit 91a097e7 forbids specifying the cache mode for empty >> drives. Attempting to create a domain with an empty qdisk cdrom >> results in >> >> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom, >> cache=writeback,id=ide-832: Must specify either driver or file >> >> Change libxl to only emit cache mode when a cdrom target is specified. > What happens then when a cdrom is inserted? I cannot seem to find the code > in libxl_cdrom_insert that sets the cache mode. I cannot find it either. I suppose it would need to be setup via xenstore, similar to other options like feature_discard. But looking at $qemu-src/hw/block/xen_disk.c, it seems the XenBlkDev struct has no field to specify cache mode. Would qemu's xen_disk need to be extended to support cache mode, followed by a libxl patch to set the cache mode in xenstore? > Is the default one used > then? Yes, the default cache mode (which is already writeback AIUI) would be used if not explicitly specified. Which brings up the option of removing 'cache=writeback' for cdroms altogether. Any opinion on that option? Regards, Jim
On Wed, Apr 27, 2016 at 5:22 PM, Jim Fehlig <jfehlig@suse.com> wrote: > On 04/27/2016 01:38 AM, Roger Pau Monné wrote: >> On Tue, Apr 26, 2016 at 10:35:31PM -0600, Jim Fehlig wrote: >>> qemu commit 91a097e7 forbids specifying the cache mode for empty >>> drives. Attempting to create a domain with an empty qdisk cdrom >>> results in >>> >>> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom, >>> cache=writeback,id=ide-832: Must specify either driver or file >>> >>> Change libxl to only emit cache mode when a cdrom target is specified. >> What happens then when a cdrom is inserted? I cannot seem to find the code >> in libxl_cdrom_insert that sets the cache mode. > > I cannot find it either. I suppose it would need to be setup via xenstore, > similar to other options like feature_discard. But looking at > $qemu-src/hw/block/xen_disk.c, it seems the XenBlkDev struct has no field to > specify cache mode. Would qemu's xen_disk need to be extended to support cache > mode, followed by a libxl patch to set the cache mode in xenstore? > >> Is the default one used >> then? > > Yes, the default cache mode (which is already writeback AIUI) would be used if > not explicitly specified. Which brings up the option of removing > 'cache=writeback' for cdroms altogether. Any opinion on that option? What's the effective difference between caching modes for read-only media anyway? -George
On Thu, Apr 28, 2016 at 09:27:30AM +0100, George Dunlap wrote: > On Wed, Apr 27, 2016 at 5:22 PM, Jim Fehlig <jfehlig@suse.com> wrote: > > On 04/27/2016 01:38 AM, Roger Pau Monné wrote: > >> On Tue, Apr 26, 2016 at 10:35:31PM -0600, Jim Fehlig wrote: > >>> qemu commit 91a097e7 forbids specifying the cache mode for empty > >>> drives. Attempting to create a domain with an empty qdisk cdrom > >>> results in > >>> > >>> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom, > >>> cache=writeback,id=ide-832: Must specify either driver or file > >>> > >>> Change libxl to only emit cache mode when a cdrom target is specified. > >> What happens then when a cdrom is inserted? I cannot seem to find the code > >> in libxl_cdrom_insert that sets the cache mode. > > > > I cannot find it either. I suppose it would need to be setup via xenstore, > > similar to other options like feature_discard. But looking at > > $qemu-src/hw/block/xen_disk.c, it seems the XenBlkDev struct has no field to > > specify cache mode. Would qemu's xen_disk need to be extended to support cache > > mode, followed by a libxl patch to set the cache mode in xenstore? > > > >> Is the default one used > >> then? > > > > Yes, the default cache mode (which is already writeback AIUI) would be used if > > not explicitly specified. Which brings up the option of removing > > 'cache=writeback' for cdroms altogether. Any opinion on that option? > > What's the effective difference between caching modes for read-only > media anyway? That's right, cdroms should always be read-only in which case the cache mode doesn't matter. But I'm not sure if this is enforced in libxl. IMHO, we should make sure ro is enforced with cdrom devices and then we can use the default cache mode. Roger.
Roger Pau Monné wrote: > On Thu, Apr 28, 2016 at 09:27:30AM +0100, George Dunlap wrote: >> On Wed, Apr 27, 2016 at 5:22 PM, Jim Fehlig <jfehlig@suse.com> wrote: >>> On 04/27/2016 01:38 AM, Roger Pau Monné wrote: >>>> On Tue, Apr 26, 2016 at 10:35:31PM -0600, Jim Fehlig wrote: >>>>> qemu commit 91a097e7 forbids specifying the cache mode for empty >>>>> drives. Attempting to create a domain with an empty qdisk cdrom >>>>> results in >>>>> >>>>> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom, >>>>> cache=writeback,id=ide-832: Must specify either driver or file >>>>> >>>>> Change libxl to only emit cache mode when a cdrom target is specified. >>>> What happens then when a cdrom is inserted? I cannot seem to find the code >>>> in libxl_cdrom_insert that sets the cache mode. >>> I cannot find it either. I suppose it would need to be setup via xenstore, >>> similar to other options like feature_discard. But looking at >>> $qemu-src/hw/block/xen_disk.c, it seems the XenBlkDev struct has no field to >>> specify cache mode. Would qemu's xen_disk need to be extended to support cache >>> mode, followed by a libxl patch to set the cache mode in xenstore? >>> >>>> Is the default one used >>>> then? >>> Yes, the default cache mode (which is already writeback AIUI) would be used if >>> not explicitly specified. Which brings up the option of removing >>> 'cache=writeback' for cdroms altogether. Any opinion on that option? >> What's the effective difference between caching modes for read-only >> media anyway? > > That's right, cdroms should always be read-only in which case the cache mode > doesn't matter. But I'm not sure if this is enforced in libxl. xl-disk-configuration.txt states the default 'access=' value for cdrom devices is readonly. It doesn't mention readonly is enforced for cdrom devices, but xlu_disk_parse() in libxlutil unconditionally sets the disk's readwrite field to 0 for cdroms. > IMHO, we should make sure ro is enforced with cdrom devices and then we can > use the default cache mode. I'll send a V2 based on the above findings. Regards, Jim
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index fd12844..df1207b 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -1368,11 +1368,11 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, if (disks[i].is_cdrom) { drive = libxl__sprintf(gc, - "if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i", + "if=ide,index=%d,readonly=%s,media=cdrom,id=ide-%i", disk, disks[i].readwrite ? "off" : "on", dev_number); if (target_path) - drive = libxl__sprintf(gc, "%s,file=%s,format=%s", + drive = libxl__sprintf(gc, "%s,file=%s,format=%s,cache=writeback", drive, target_path, format); } else { /*
qemu commit 91a097e7 forbids specifying the cache mode for empty drives. Attempting to create a domain with an empty qdisk cdrom results in qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom, cache=writeback,id=ide-832: Must specify either driver or file Change libxl to only emit cache mode when a cdrom target is specified. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- tools/libxl/libxl_dm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)