* [PATCH] libxl: don't add cache mode for empty drives
@ 2016-04-27 4:35 Jim Fehlig
2016-04-27 7:38 ` Roger Pau Monné
0 siblings, 1 reply; 6+ messages in thread
From: Jim Fehlig @ 2016-04-27 4:35 UTC (permalink / raw)
To: xen-devel; +Cc: wei.liu2, Jim Fehlig, ian.jackson
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(-)
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 {
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] libxl: don't add cache mode for empty drives
2016-04-27 4:35 [PATCH] libxl: don't add cache mode for empty drives Jim Fehlig
@ 2016-04-27 7:38 ` Roger Pau Monné
2016-04-27 16:22 ` Jim Fehlig
0 siblings, 1 reply; 6+ messages in thread
From: Roger Pau Monné @ 2016-04-27 7:38 UTC (permalink / raw)
To: Jim Fehlig; +Cc: ian.jackson, wei.liu2, xen-devel
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libxl: don't add cache mode for empty drives
2016-04-27 7:38 ` Roger Pau Monné
@ 2016-04-27 16:22 ` Jim Fehlig
2016-04-28 8:27 ` George Dunlap
0 siblings, 1 reply; 6+ messages in thread
From: Jim Fehlig @ 2016-04-27 16:22 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: ian.jackson, wei.liu2, xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libxl: don't add cache mode for empty drives
2016-04-27 16:22 ` Jim Fehlig
@ 2016-04-28 8:27 ` George Dunlap
2016-04-28 9:17 ` Roger Pau Monné
0 siblings, 1 reply; 6+ messages in thread
From: George Dunlap @ 2016-04-28 8:27 UTC (permalink / raw)
To: Jim Fehlig
Cc: Wei Liu, xen-devel@lists.xen.org, Ian Jackson,
Roger Pau Monné
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libxl: don't add cache mode for empty drives
2016-04-28 8:27 ` George Dunlap
@ 2016-04-28 9:17 ` Roger Pau Monné
2016-04-28 21:22 ` Jim Fehlig
0 siblings, 1 reply; 6+ messages in thread
From: Roger Pau Monné @ 2016-04-28 9:17 UTC (permalink / raw)
To: George Dunlap; +Cc: Wei Liu, Jim Fehlig, Ian Jackson, xen-devel@lists.xen.org
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libxl: don't add cache mode for empty drives
2016-04-28 9:17 ` Roger Pau Monné
@ 2016-04-28 21:22 ` Jim Fehlig
0 siblings, 0 replies; 6+ messages in thread
From: Jim Fehlig @ 2016-04-28 21:22 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Wei Liu, Ian Jackson, George Dunlap, xen-devel@lists.xen.org
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-04-28 21:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-27 4:35 [PATCH] libxl: don't add cache mode for empty drives Jim Fehlig
2016-04-27 7:38 ` Roger Pau Monné
2016-04-27 16:22 ` Jim Fehlig
2016-04-28 8:27 ` George Dunlap
2016-04-28 9:17 ` Roger Pau Monné
2016-04-28 21:22 ` Jim Fehlig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).