[PATCH] libxl: don't add cache mode for empty drives

[PATCH] libxl: don't add cache mode for empty drives

[Jim Fehlig]

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

Re: [PATCH] libxl: don't add cache mode for empty drives

[Roger Pau Monné]

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

Re: [PATCH] libxl: don't add cache mode for empty drives

[Jim Fehlig]

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

Re: [PATCH] libxl: don't add cache mode for empty drives

[George Dunlap]

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

Re: [PATCH] libxl: don't add cache mode for empty drives

[Roger Pau Monné]

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

Re: [PATCH] libxl: don't add cache mode for empty drives

[Jim Fehlig]

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