[PATCH] Fix restore handling checks

[PATCH] Fix restore handling checks

[Michal Novotny]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

Hi,
this is the patch to fix restore handling to implement some more checks 
to support more checks than for UUID and name duplicity. This patch 
basically disallows the migration/restore of IDE drives with the 
read-only flag since this is not supported according to the ATAPI/IDE 
specifications so we should disallow this for both domain creation and 
domain migration/restore. This patch implements it for both create and 
restore/migrate functionality.

Also, the check whether the host machine does have enough memory 
available for the guest has been implemented which can be the real issue 
when you try to migrate a guest from one machine to another that is not 
having enough memory for this guest. The guest memory gets transferred 
but it fails to run so it's not running on either of those machines 
(i.e. domain is not on the destination nor source host machine).

I did try it with restore functionality now since I've been able to make 
it working for save once so I'm currently using one save image for the 
testing but unfortunately I'm having many issues with the common 
migration and save functionality since I've been able to make it working 
once to save it correctly. Fortunately the restores for this one 
particular save image is working fine. I was also thinking about 2 
concurrent migrations to the guest and/or save with the concurrent 
migration and it should be the issue (although it's not been tested 
because of reasons described above) since the domain gets created and 
it's available in the XendDomain list (i.e. xc.domain_getinfo() list) so 
it shouldn't be an issue here.

Michal

Signed-off-by: Michal Novotny <minovotn@redhat.com>

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat


[-- Attachment #2: xen-fix-restore-handling-for-ide-drives-and-memory.patch --]
[-- Type: text/x-patch, Size: 4945 bytes --]

diff -r 31708477f0a9 tools/python/xen/xend/XendCheckpoint.py
--- a/tools/python/xen/xend/XendCheckpoint.py	Mon Jun 21 09:59:10 2010 +0100
+++ b/tools/python/xen/xend/XendCheckpoint.py	Mon Jun 21 18:23:46 2010 +0200
@@ -64,6 +64,70 @@
               list.insert (i+1, value)
     return
 
+def get_avail_memory():
+    """Get available memory for new guest creation (in KiB, for restore)"""
+    from xen.xend import XendOptions
+
+    # First get total memory in KiB
+    xc = xen.lowlevel.xc.xc()
+    info = xc.domain_getinfo()
+    total_mem  = xc.physinfo()['total_memory']
+    del xc
+
+    # Count memory of all running guests in KiB
+    mem_used = 0L
+    for x in info:
+        if x['domid'] != 0:
+            mem_used += x['mem_kb']
+
+    # Get minimal memory for dom0 and convert to KiB
+    min_mem = XendOptions.instance().get_dom0_min_mem() * 1024
+
+    return total_mem - mem_used - min_mem
+
+def check_for_restore_bail(msg):
+    raise VmError("Cannot restore: %s" % msg)
+
+def check_for_enough_mem(val):
+    mem_avail = int(get_avail_memory() / 1024)
+    log.debug("Available memory: %s MiB, guest requires: %s MiB" % (mem_avail, val))
+    return val > mem_avail
+
+def check_for_restore_is_hvm(cfg):
+    for item in cfg:
+        if (type(item) == list):
+           if item[0] == 'image':
+               return type(item[1]) == list and item[1][0] == 'hvm'
+
+def check_for_restore_hvm_have_readonly_ide(cfg):
+    """Check the configuration for read-only IDE devices
+       Fail if such a device is found."""
+    is_ide = True
+    if type(cfg) == list and cfg[0] in ('tap', 'vbd'):
+        for p in cfg:
+            if (type(p) != str):
+                if p[0] == 'dev':
+                    is_ide = (p[1].find('hd') >= 0)
+                if p[0] == 'mode' and is_ide:
+                    if p[1] == 'r':
+                        return True
+    return False
+
+def check_for_restore(cfg):
+    is_hvm = check_for_restore_is_hvm(cfg)
+    name = None
+    for item in cfg:
+        if (type(item) == list):
+            if item[0] == 'name':
+                name = item[1]
+            # Check for enough memory to create the guest
+            if item[0] == 'memory':
+                if not check_for_enough_mem(item[1]):
+                    check_for_restore_bail('Host machine doesn\'t have enough memory for %s' % name)
+            # We disable read-only IDE drives only for HVM guests
+            if item[0] == 'device' and is_hvm:
+                if check_for_restore_hvm_have_readonly_ide(item[1]):
+                    check_for_restore_bail('HVM domains cannot be using read-only IDE drives')
 
 def save(fd, dominfo, network, live, dst, checkpoint=False, node=-1):
     from xen.xend import XendDomain
@@ -220,6 +284,7 @@
             othervm = xd.domain_lookup_nr(domconfig["uuid"])
         if othervm is not None and othervm.domid is not None: 
             raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
+        check_for_restore(vmconfig)
 
     if dominfo:
         dominfo.resume()
diff -r 31708477f0a9 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Mon Jun 21 09:59:10 2010 +0100
+++ b/tools/python/xen/xend/XendDomainInfo.py	Mon Jun 21 18:23:46 2010 +0200
@@ -82,6 +82,31 @@
 log = logging.getLogger("xend.XendDomainInfo")
 #log.setLevel(logging.TRACE)
 
+def cfg_is_hvm(cfg):
+    for item in cfg:
+        if (type(item) == list):
+           if item[0] == 'image':
+               return type(item[1]) == list and item[1][0] == 'hvm'
+
+def cfg_hvm_have_readonly_ide_devices(cfg):
+    """Check the configuration for read-only IDE devices
+       Fail if such a device is found."""
+    if not cfg_is_hvm(cfg):
+        return False
+
+    is_ide = True
+    for item in cfg:
+        if (type(item) == list):
+            if item[0] == 'device':
+                if type(item[1]) == list and item[1][0] in ('tap', 'vbd'):
+                    for p in item[1]:
+                        if (type(p) != str):
+                            if p[0] == 'dev':
+                                is_ide = (p[1].find('hd') >= 0)
+                        if p[0] == 'mode' and is_ide:
+                            if p[1] == 'r':
+                                return True
+    return False
 
 def create(config):
     """Creates and start a VM using the supplied configuration. 
@@ -100,6 +125,10 @@
         othervm = XendDomain.instance().domain_lookup_nr(domconfig["uuid"])
     if othervm is not None and othervm.domid is not None:
         raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
+
+    if cfg_hvm_have_readonly_ide_devices(config):
+        raise VmError("HVM domains cannot be using read-only IDE drives")
+
     log.debug("XendDomainInfo.create(%s)", scrub_password(config))
     vm = XendDomainInfo(domconfig)
     try:

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Fix restore handling checks

[Keir Fraser]

On 21/06/2010 17:30, "Michal Novotny" <minovotn@redhat.com> wrote:

> Hi,
> this is the patch to fix restore handling to implement some more checks
> to support more checks than for UUID and name duplicity. This patch
> basically disallows the migration/restore of IDE drives with the
> read-only flag since this is not supported according to the ATAPI/IDE
> specifications so we should disallow this for both domain creation and
> domain migration/restore.

What about CD-ROMs? This would break my test domain config, for example.

> This patch implements it for both create and
> restore/migrate functionality.
> 
> Also, the check whether the host machine does have enough memory
> available for the guest has been implemented which can be the real issue
> when you try to migrate a guest from one machine to another that is not
> having enough memory for this guest. The guest memory gets transferred
> but it fails to run so it's not running on either of those machines
> (i.e. domain is not on the destination nor source host machine).

Failed restore should get reported back to the host that is saving the guest
state, and cause that machine to resume execution of the original VM. Does
that not work for you?

Possibly checking up front for available memory on the target is a good
idea, but it shouldn't be *essential* if the error handling is up to par.

 -- Keir

> I did try it with restore functionality now since I've been able to make
> it working for save once so I'm currently using one save image for the
> testing but unfortunately I'm having many issues with the common
> migration and save functionality since I've been able to make it working
> once to save it correctly. Fortunately the restores for this one
> particular save image is working fine. I was also thinking about 2
> concurrent migrations to the guest and/or save with the concurrent
> migration and it should be the issue (although it's not been tested
> because of reasons described above) since the domain gets created and
> it's available in the XendDomain list (i.e. xc.domain_getinfo() list) so
> it shouldn't be an issue here.
> 
> Michal
> 
> Signed-off-by: Michal Novotny <minovotn@redhat.com>

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/21/2010 08:04 PM, Keir Fraser wrote:
> On 21/06/2010 17:30, "Michal Novotny"<minovotn@redhat.com>  wrote:
>
>    
>> Hi,
>> this is the patch to fix restore handling to implement some more checks
>> to support more checks than for UUID and name duplicity. This patch
>> basically disallows the migration/restore of IDE drives with the
>> read-only flag since this is not supported according to the ATAPI/IDE
>> specifications so we should disallow this for both domain creation and
>> domain migration/restore.
>>      
> What about CD-ROMs? This would break my test domain config, for example.
>
>    

Right, there's the exception for CD-ROMs according to the spec. I should 
implement this as well but read-only IDE disk devices are not supported 
according to the IDE specs.

>> This patch implements it for both create and
>> restore/migrate functionality.
>>
>> Also, the check whether the host machine does have enough memory
>> available for the guest has been implemented which can be the real issue
>> when you try to migrate a guest from one machine to another that is not
>> having enough memory for this guest. The guest memory gets transferred
>> but it fails to run so it's not running on either of those machines
>> (i.e. domain is not on the destination nor source host machine).
>>      
> Failed restore should get reported back to the host that is saving the guest
> state, and cause that machine to resume execution of the original VM. Does
> that not work for you?
>
> Possibly checking up front for available memory on the target is a good
> idea, but it shouldn't be *essential* if the error handling is up to par.
>
>   -- Keir
>    

It was not working but I'm still coping with some issues that are 
save/restore related so maybe this is the reason why I was unable to see 
this behaviour.

Michal

>    
>> I did try it with restore functionality now since I've been able to make
>> it working for save once so I'm currently using one save image for the
>> testing but unfortunately I'm having many issues with the common
>> migration and save functionality since I've been able to make it working
>> once to save it correctly. Fortunately the restores for this one
>> particular save image is working fine. I was also thinking about 2
>> concurrent migrations to the guest and/or save with the concurrent
>> migration and it should be the issue (although it's not been tested
>> because of reasons described above) since the domain gets created and
>> it's available in the XendDomain list (i.e. xc.domain_getinfo() list) so
>> it shouldn't be an issue here.
>>
>> Michal
>>
>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>>      
>
>    


-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

[-- Attachment #1: Type: text/plain, Size: 3186 bytes --]

On 06/22/2010 07:43 AM, Michal Novotny wrote:
> On 06/21/2010 08:04 PM, Keir Fraser wrote:
>> On 21/06/2010 17:30, "Michal Novotny"<minovotn@redhat.com>  wrote:
>>
>>> Hi,
>>> this is the patch to fix restore handling to implement some more checks
>>> to support more checks than for UUID and name duplicity. This patch
>>> basically disallows the migration/restore of IDE drives with the
>>> read-only flag since this is not supported according to the ATAPI/IDE
>>> specifications so we should disallow this for both domain creation and
>>> domain migration/restore.
>> What about CD-ROMs? This would break my test domain config, for example.
>>
>
> Right, there's the exception for CD-ROMs according to the spec. I 
> should implement this as well but read-only IDE disk devices are not 
> supported according to the IDE specs.


This is the updated version of my patch to allow read-only CD-ROM 
devices as you had a good point that read-only is supported for CD-ROM 
IDE drives (but only for them).


>
>>> This patch implements it for both create and
>>> restore/migrate functionality.
>>>
>>> Also, the check whether the host machine does have enough memory
>>> available for the guest has been implemented which can be the real 
>>> issue
>>> when you try to migrate a guest from one machine to another that is not
>>> having enough memory for this guest. The guest memory gets transferred
>>> but it fails to run so it's not running on either of those machines
>>> (i.e. domain is not on the destination nor source host machine).
>> Failed restore should get reported back to the host that is saving 
>> the guest
>> state, and cause that machine to resume execution of the original VM. 
>> Does
>> that not work for you?
>>
>> Possibly checking up front for available memory on the target is a good
>> idea, but it shouldn't be *essential* if the error handling is up to 
>> par.
>>
>>   -- Keir
>

I was able to make it working now and this functionality seems to be 
working now however I think that the preliminary check before the 
transfer itself is a good idea. As far as I had it implemented already I 
just did some modifications to allow CD-ROM IDE read-only drives and 
this is the updated version.

Michal

>
>>> I did try it with restore functionality now since I've been able to 
>>> make
>>> it working for save once so I'm currently using one save image for the
>>> testing but unfortunately I'm having many issues with the common
>>> migration and save functionality since I've been able to make it 
>>> working
>>> once to save it correctly. Fortunately the restores for this one
>>> particular save image is working fine. I was also thinking about 2
>>> concurrent migrations to the guest and/or save with the concurrent
>>> migration and it should be the issue (although it's not been tested
>>> because of reasons described above) since the domain gets created and
>>> it's available in the XendDomain list (i.e. xc.domain_getinfo() 
>>> list) so
>>> it shouldn't be an issue here.
>>>
>>> Michal
>>>
>>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>>
>
>


-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat


[-- Attachment #2: xen-fix-restore-handling-for-ide-drives-and-memory.patch --]
[-- Type: text/x-patch, Size: 5129 bytes --]

diff -r 72c6228b5f0f tools/python/xen/xend/XendCheckpoint.py
--- a/tools/python/xen/xend/XendCheckpoint.py	Mon Jun 21 19:19:25 2010 +0100
+++ b/tools/python/xen/xend/XendCheckpoint.py	Tue Jun 22 08:07:48 2010 +0200
@@ -64,6 +64,71 @@
               list.insert (i+1, value)
     return
 
+def get_avail_memory():
+    """Get available memory for new guest creation (in KiB, for restore)"""
+    from xen.xend import XendOptions
+
+    # First get total memory in KiB
+    xc = xen.lowlevel.xc.xc()
+    info = xc.domain_getinfo()
+    total_mem  = xc.physinfo()['total_memory']
+    del xc
+
+    # Count memory of all running guests in KiB
+    mem_used = 0L
+    for x in info:
+        if x['domid'] != 0:
+            mem_used += x['mem_kb']
+
+    # Get minimal memory for dom0 and convert to KiB
+    min_mem = XendOptions.instance().get_dom0_min_mem() * 1024
+
+    return total_mem - mem_used - min_mem
+
+def check_for_restore_bail(msg):
+    raise VmError("Cannot restore: %s" % msg)
+
+def check_for_enough_mem(val):
+    mem_avail = int(get_avail_memory() / 1024)
+    log.debug("Available memory: %s MiB, guest requires: %s MiB" % (mem_avail, val))
+    return val > mem_avail
+
+def check_for_restore_is_hvm(cfg):
+    for item in cfg:
+        if (type(item) == list):
+           if item[0] == 'image':
+               return type(item[1]) == list and item[1][0] == 'hvm'
+
+def check_for_restore_hvm_have_readonly_ide(cfg):
+    """Check the configuration for read-only IDE devices
+       Fail if such a device is found."""
+    disallow = None
+    if type(cfg) == list and cfg[0] in ('tap', 'vbd'):
+        for p in cfg:
+            if (type(p) != str):
+                if p[0] == 'dev':
+                    disallow = ((p[1].find('hd') >= 0) and
+                                (p[1].find('cdrom') == -1))
+                if p[0] == 'mode' and disallow:
+                    if p[1] == 'r':
+                        return True
+    return False
+
+def check_for_restore(cfg):
+    is_hvm = check_for_restore_is_hvm(cfg)
+    name = None
+    for item in cfg:
+        if (type(item) == list):
+            if item[0] == 'name':
+                name = item[1]
+            # Check for enough memory to create the guest
+            if item[0] == 'memory':
+                if not check_for_enough_mem(item[1]):
+                    check_for_restore_bail('Host machine doesn\'t have enough memory for %s' % name)
+            # We disable read-only IDE drives only for HVM guests
+            if item[0] == 'device' and is_hvm:
+                if check_for_restore_hvm_have_readonly_ide(item[1]):
+                    check_for_restore_bail('HVM domains cannot be using read-only IDE drives')
 
 def save(fd, dominfo, network, live, dst, checkpoint=False, node=-1):
     from xen.xend import XendDomain
@@ -220,6 +285,7 @@
             othervm = xd.domain_lookup_nr(domconfig["uuid"])
         if othervm is not None and othervm.domid is not None: 
             raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
+        check_for_restore(vmconfig)
 
     if dominfo:
         dominfo.resume()
diff -r 72c6228b5f0f tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Mon Jun 21 19:19:25 2010 +0100
+++ b/tools/python/xen/xend/XendDomainInfo.py	Tue Jun 22 08:07:48 2010 +0200
@@ -82,6 +82,32 @@
 log = logging.getLogger("xend.XendDomainInfo")
 #log.setLevel(logging.TRACE)
 
+def cfg_is_hvm(cfg):
+    for item in cfg:
+        if (type(item) == list):
+           if item[0] == 'image':
+               return type(item[1]) == list and item[1][0] == 'hvm'
+
+def cfg_hvm_have_readonly_ide_disks(cfg):
+    """Check whether the configuration for read-only IDE disks since
+       they are not supported according to IDE specifications."""
+    if not cfg_is_hvm(cfg):
+        return False
+
+    disallow = None
+    for item in cfg:
+        if (type(item) == list):
+            if item[0] == 'device':
+                if type(item[1]) == list and item[1][0] in ('tap', 'vbd'):
+                    for p in item[1]:
+                        if (type(p) != str):
+                            if p[0] == 'dev':
+                              disallow = ((p[1].find('hd') >= 0) and
+                                          (p[1].find('cdrom') == -1))
+                        if p[0] == 'mode' and disallow:
+                            if p[1] == 'r':
+                                return True
+    return False
 
 def create(config):
     """Creates and start a VM using the supplied configuration. 
@@ -100,6 +126,10 @@
         othervm = XendDomain.instance().domain_lookup_nr(domconfig["uuid"])
     if othervm is not None and othervm.domid is not None:
         raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
+
+    if cfg_hvm_have_readonly_ide_disks(config):
+        raise VmError("HVM domains cannot be using read-only IDE drives")
+
     log.debug("XendDomainInfo.create(%s)", scrub_password(config))
     vm = XendDomainInfo(domconfig)
     try:

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Fix restore handling checks

[Keir Fraser]

Okay, hopefully someone will be able to Ack this patch with better knwoledge
of xend than me.

 -- Keir


On 22/06/2010 07:11, "Michal Novotny" <minovotn@redhat.com> wrote:

> On 06/22/2010 07:43 AM, Michal Novotny wrote:
>> On 06/21/2010 08:04 PM, Keir Fraser wrote:
>>> On 21/06/2010 17:30, "Michal Novotny"<minovotn@redhat.com>  wrote:
>>> 
>>>> Hi,
>>>> this is the patch to fix restore handling to implement some more checks
>>>> to support more checks than for UUID and name duplicity. This patch
>>>> basically disallows the migration/restore of IDE drives with the
>>>> read-only flag since this is not supported according to the ATAPI/IDE
>>>> specifications so we should disallow this for both domain creation and
>>>> domain migration/restore.
>>> What about CD-ROMs? This would break my test domain config, for example.
>>> 
>> 
>> Right, there's the exception for CD-ROMs according to the spec. I
>> should implement this as well but read-only IDE disk devices are not
>> supported according to the IDE specs.
> 
> 
> This is the updated version of my patch to allow read-only CD-ROM
> devices as you had a good point that read-only is supported for CD-ROM
> IDE drives (but only for them).
> 
> 
>> 
>>>> This patch implements it for both create and
>>>> restore/migrate functionality.
>>>> 
>>>> Also, the check whether the host machine does have enough memory
>>>> available for the guest has been implemented which can be the real
>>>> issue
>>>> when you try to migrate a guest from one machine to another that is not
>>>> having enough memory for this guest. The guest memory gets transferred
>>>> but it fails to run so it's not running on either of those machines
>>>> (i.e. domain is not on the destination nor source host machine).
>>> Failed restore should get reported back to the host that is saving
>>> the guest
>>> state, and cause that machine to resume execution of the original VM.
>>> Does
>>> that not work for you?
>>> 
>>> Possibly checking up front for available memory on the target is a good
>>> idea, but it shouldn't be *essential* if the error handling is up to
>>> par.
>>> 
>>>   -- Keir
>> 
> 
> I was able to make it working now and this functionality seems to be
> working now however I think that the preliminary check before the
> transfer itself is a good idea. As far as I had it implemented already I
> just did some modifications to allow CD-ROM IDE read-only drives and
> this is the updated version.
> 
> Michal
> 
>> 
>>>> I did try it with restore functionality now since I've been able to
>>>> make
>>>> it working for save once so I'm currently using one save image for the
>>>> testing but unfortunately I'm having many issues with the common
>>>> migration and save functionality since I've been able to make it
>>>> working
>>>> once to save it correctly. Fortunately the restores for this one
>>>> particular save image is working fine. I was also thinking about 2
>>>> concurrent migrations to the guest and/or save with the concurrent
>>>> migration and it should be the issue (although it's not been tested
>>>> because of reasons described above) since the domain gets created and
>>>> it's available in the XendDomain list (i.e. xc.domain_getinfo()
>>>> list) so
>>>> it shouldn't be an issue here.
>>>> 
>>>> Michal
>>>> 
>>>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>>> 
>> 
>> 
> 

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/22/2010 08:14 AM, Keir Fraser wrote:
> Okay, hopefully someone will be able to Ack this patch with better knwoledge
> of xend than me.
>
>   -- Keir
>
>    
Ok, good. If it will be accepted and put into xen-staging/xen-unstable 
tree please let me know about it.

Thanks,
Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]

On 06/22/2010 08:17 AM, Michal Novotny wrote:
> On 06/22/2010 08:14 AM, Keir Fraser wrote:
>> Okay, hopefully someone will be able to Ack this patch with better 
>> knwoledge
>> of xend than me.
>>
>>   -- Keir
>>
> Ok, good. If it will be accepted and put into xen-staging/xen-unstable 
> tree please let me know about it.
>
> Thanks,
> Michal
>

Well, this is new version of the patch since I've tried now to both 
migrate from RHEL-5 host (python-2.4) to RHEL-6 host (python-2.6) using 
the RHEL-5 host as a source machine for migrations and also RHEL-6 host 
to restore the the guest locally using `xm restore` to see whether the 
guest memory calculated correctly and it did pass when there was enough 
memory to create the guest and failed when there was not enough memory 
to create the guest. Also, some fixing for comparing the values was 
necessary since it was treated as string comparison on one of the hosts.

Both the host machines were having the latest Xen-4.1 installed.

Michal

Signed-off-by: Michal Novotny <minovotn@redhat.com>

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat


[-- Attachment #2: xen-fix-restore-handling-for-ide-drives-and-memory.patch --]
[-- Type: text/x-patch, Size: 5742 bytes --]

diff -r a24dbfcbdf69 tools/python/xen/xend/XendCheckpoint.py
--- a/tools/python/xen/xend/XendCheckpoint.py	Tue Jun 22 07:19:38 2010 +0100
+++ b/tools/python/xen/xend/XendCheckpoint.py	Tue Jun 22 12:52:56 2010 +0200
@@ -64,6 +64,78 @@ def insert_after(list, pred, value):
               list.insert (i+1, value)
     return
 
+def get_avail_memory():
+    """Get available memory for new guest creation (in KiB, for restore)"""
+    from xen.xend import XendOptions
+
+    # First get total memory in KiB
+    xc = xen.lowlevel.xc.xc()
+    info = xc.domain_getinfo()
+    total_mem  = xc.physinfo()['total_memory']
+    del xc
+
+    # Count memory of all running guests in KiB
+    mem_used = 0L
+    for x in info:
+        if x['domid'] != 0:
+            # If blocked & paused & !paused & no online_vcpus we most
+            # probably migrate so we get maxmem instead (since mem_kb
+            # is having currently transferred amount of memory so we
+            # cannot use it for good calculations)
+            if (x['blocked'] == 1 and x['paused'] == 1 and
+                x['running'] == 0 and x['online_vcpus'] == 0):
+                log.debug("Domain %d seems to be restoring" % x['domid'])
+                mem_used += x['maxmem_kb']
+            else:
+                mem_used += x['mem_kb']
+
+    # Get minimal memory for dom0 and convert to KiB
+    min_mem = XendOptions.instance().get_dom0_min_mem() * 1024
+
+    return total_mem - mem_used - min_mem
+
+def check_for_restore_bail(msg):
+    raise VmError("Cannot restore: %s" % msg)
+
+def check_for_enough_mem(val):
+    mem_avail = get_avail_memory() / 1024
+    log.debug("Available memory: %s MiB, guest requires: %s MiB" % (mem_avail, val))
+    return int(mem_avail) > int(val)
+
+def check_for_restore_is_hvm(cfg):
+    for item in cfg:
+        if (type(item) == list):
+           if item[0] == 'image':
+               return type(item[1]) == list and item[1][0] == 'hvm'
+
+def check_for_restore_hvm_have_readonly_ide(cfg):
+    """Check the configuration for read-only IDE devices
+       Fail if such a device is found."""
+    disallow = None
+    if type(cfg) == list and cfg[0] in ('tap', 'vbd'):
+        for p in cfg:
+            if (type(p) != str):
+                if p[0] == 'dev':
+                    disallow = ((p[1].find('hd') >= 0) and
+                                (p[1].find('cdrom') == -1))
+                if p[0] == 'mode' and disallow:
+                    if p[1] == 'r':
+                        return True
+    return False
+
+def check_for_restore(cfg):
+    is_hvm = check_for_restore_is_hvm(cfg)
+    name = None
+    for item in cfg:
+        if (type(item) == list):
+            # Check for enough memory to create the guest
+            if item[0] == 'memory':
+                if not check_for_enough_mem(item[1]):
+                    check_for_restore_bail('Host machine doesn\'t have enough memory to create the guest')
+            # We disable read-only IDE drives only for HVM guests
+            if item[0] == 'device' and is_hvm:
+                if check_for_restore_hvm_have_readonly_ide(item[1]):
+                    check_for_restore_bail('HVM domains cannot be using read-only IDE drives')
 
 def save(fd, dominfo, network, live, dst, checkpoint=False, node=-1):
     from xen.xend import XendDomain
@@ -220,6 +292,7 @@ def restore(xd, fd, dominfo = None, paus
             othervm = xd.domain_lookup_nr(domconfig["uuid"])
         if othervm is not None and othervm.domid is not None: 
             raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
+        check_for_restore(vmconfig)
 
     if dominfo:
         dominfo.resume()
diff -r a24dbfcbdf69 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Tue Jun 22 07:19:38 2010 +0100
+++ b/tools/python/xen/xend/XendDomainInfo.py	Tue Jun 22 12:52:56 2010 +0200
@@ -82,6 +82,32 @@ log = logging.getLogger("xend.XendDomain
 log = logging.getLogger("xend.XendDomainInfo")
 #log.setLevel(logging.TRACE)
 
+def cfg_is_hvm(cfg):
+    for item in cfg:
+        if (type(item) == list):
+           if item[0] == 'image':
+               return type(item[1]) == list and item[1][0] == 'hvm'
+
+def cfg_hvm_have_readonly_ide_disks(cfg):
+    """Check whether the configuration for read-only IDE disks since
+       they are not supported according to IDE specifications."""
+    if not cfg_is_hvm(cfg):
+        return False
+
+    disallow = None
+    for item in cfg:
+        if (type(item) == list):
+            if item[0] == 'device':
+                if type(item[1]) == list and item[1][0] in ('tap', 'vbd'):
+                    for p in item[1]:
+                        if (type(p) != str):
+                            if p[0] == 'dev':
+                              disallow = ((p[1].find('hd') >= 0) and
+                                          (p[1].find('cdrom') == -1))
+                        if p[0] == 'mode' and disallow:
+                            if p[1] == 'r':
+                                return True
+    return False
 
 def create(config):
     """Creates and start a VM using the supplied configuration. 
@@ -100,6 +126,10 @@ def create(config):
         othervm = XendDomain.instance().domain_lookup_nr(domconfig["uuid"])
     if othervm is not None and othervm.domid is not None:
         raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
+
+    if cfg_hvm_have_readonly_ide_disks(config):
+        raise VmError("HVM domains cannot be using read-only IDE drives")
+
     log.debug("XendDomainInfo.create(%s)", scrub_password(config))
     vm = XendDomainInfo(domconfig)
     try:

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Fix restore handling checks

[Konrad Rzeszutek Wilk]

On Tue, Jun 22, 2010 at 02:56:24PM +0200, Michal Novotny wrote:
> On 06/22/2010 08:17 AM, Michal Novotny wrote:
> >On 06/22/2010 08:14 AM, Keir Fraser wrote:
> >>Okay, hopefully someone will be able to Ack this patch with
> >>better knwoledge
> >>of xend than me.
> >>
> >>  -- Keir
> >>
> >Ok, good. If it will be accepted and put into
> >xen-staging/xen-unstable tree please let me know about it.
> >
> >Thanks,
> >Michal
> >
> 
> Well, this is new version of the patch since I've tried now to both
> migrate from RHEL-5 host (python-2.4) to RHEL-6 host (python-2.6)
> using the RHEL-5 host as a source machine for migrations and also
> RHEL-6 host to restore the the guest locally using `xm restore` to
> see whether the guest memory calculated correctly and it did pass
> when there was enough memory to create the guest and failed when
> there was not enough memory to create the guest. Also, some fixing
> for comparing the values was necessary since it was treated as
> string comparison on one of the hosts.
> 
> Both the host machines were having the latest Xen-4.1 installed.
> 
> Michal
> 
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> 
> -- 
> Michal Novotny<minovotn@redhat.com>, RHCE
> Virtualization Team (xen userspace), Red Hat
> 

> diff -r a24dbfcbdf69 tools/python/xen/xend/XendCheckpoint.py
> --- a/tools/python/xen/xend/XendCheckpoint.py	Tue Jun 22 07:19:38 2010 +0100
> +++ b/tools/python/xen/xend/XendCheckpoint.py	Tue Jun 22 12:52:56 2010 +0200
> @@ -64,6 +64,78 @@ def insert_after(list, pred, value):
>                list.insert (i+1, value)
>      return
>  
> +def get_avail_memory():
> +    """Get available memory for new guest creation (in KiB, for restore)"""
> +    from xen.xend import XendOptions
> +
> +    # First get total memory in KiB
> +    xc = xen.lowlevel.xc.xc()
> +    info = xc.domain_getinfo()
> +    total_mem  = xc.physinfo()['total_memory']
> +    del xc
> +
> +    # Count memory of all running guests in KiB
> +    mem_used = 0L
> +    for x in info:
> +        if x['domid'] != 0:
> +            # If blocked & paused & !paused & no online_vcpus we most
> +            # probably migrate so we get maxmem instead (since mem_kb
> +            # is having currently transferred amount of memory so we
> +            # cannot use it for good calculations)
> +            if (x['blocked'] == 1 and x['paused'] == 1 and
> +                x['running'] == 0 and x['online_vcpus'] == 0):
> +                log.debug("Domain %d seems to be restoring" % x['domid'])
> +                mem_used += x['maxmem_kb']
> +            else:
> +                mem_used += x['mem_kb']
> +
> +    # Get minimal memory for dom0 and convert to KiB
> +    min_mem = XendOptions.instance().get_dom0_min_mem() * 1024
> +
> +    return total_mem - mem_used - min_mem
> +
> +def check_for_restore_bail(msg):
> +    raise VmError("Cannot restore: %s" % msg)
> +
> +def check_for_enough_mem(val):
> +    mem_avail = get_avail_memory() / 1024
> +    log.debug("Available memory: %s MiB, guest requires: %s MiB" % (mem_avail, val))
> +    return int(mem_avail) > int(val)
> +
> +def check_for_restore_is_hvm(cfg):
> +    for item in cfg:
> +        if (type(item) == list):
> +           if item[0] == 'image':
> +               return type(item[1]) == list and item[1][0] == 'hvm'

There is no chance that 'hvm' would be in different case? Say 'HVM' ?

> +
> +def check_for_restore_hvm_have_readonly_ide(cfg):
> +    """Check the configuration for read-only IDE devices
> +       Fail if such a device is found."""
> +    disallow = None
> +    if type(cfg) == list and cfg[0] in ('tap', 'vbd'):
> +        for p in cfg:
> +            if (type(p) != str):

Is that right? should it not == list ?
> +                if p[0] == 'dev':
> +                    disallow = ((p[1].find('hd') >= 0) and
> +                                (p[1].find('cdrom') == -1))
> +                if p[0] == 'mode' and disallow:
> +                    if p[1] == 'r':
> +                        return True
> +    return False
> +
> +def check_for_restore(cfg):
> +    is_hvm = check_for_restore_is_hvm(cfg)
> +    name = None
> +    for item in cfg:
> +        if (type(item) == list):
> +            # Check for enough memory to create the guest
> +            if item[0] == 'memory':
> +                if not check_for_enough_mem(item[1]):
> +                    check_for_restore_bail('Host machine doesn\'t have enough memory to create the guest')
> +            # We disable read-only IDE drives only for HVM guests
> +            if item[0] == 'device' and is_hvm:
> +                if check_for_restore_hvm_have_readonly_ide(item[1]):
> +                    check_for_restore_bail('HVM domains cannot be using read-only IDE drives')
>  
>  def save(fd, dominfo, network, live, dst, checkpoint=False, node=-1):
>      from xen.xend import XendDomain
> @@ -220,6 +292,7 @@ def restore(xd, fd, dominfo = None, paus
>              othervm = xd.domain_lookup_nr(domconfig["uuid"])
>          if othervm is not None and othervm.domid is not None: 
>              raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
> +        check_for_restore(vmconfig)
>  
>      if dominfo:
>          dominfo.resume()
> diff -r a24dbfcbdf69 tools/python/xen/xend/XendDomainInfo.py
> --- a/tools/python/xen/xend/XendDomainInfo.py	Tue Jun 22 07:19:38 2010 +0100
> +++ b/tools/python/xen/xend/XendDomainInfo.py	Tue Jun 22 12:52:56 2010 +0200
> @@ -82,6 +82,32 @@ log = logging.getLogger("xend.XendDomain
>  log = logging.getLogger("xend.XendDomainInfo")
>  #log.setLevel(logging.TRACE)
>  
> +def cfg_is_hvm(cfg):
> +    for item in cfg:
> +        if (type(item) == list):
> +           if item[0] == 'image':
> +               return type(item[1]) == list and item[1][0] == 'hvm'
> +
> +def cfg_hvm_have_readonly_ide_disks(cfg):
> +    """Check whether the configuration for read-only IDE disks since
> +       they are not supported according to IDE specifications."""
> +    if not cfg_is_hvm(cfg):
> +        return False
> +
> +    disallow = None
> +    for item in cfg:
> +        if (type(item) == list):
> +            if item[0] == 'device':
> +                if type(item[1]) == list and item[1][0] in ('tap', 'vbd'):
> +                    for p in item[1]:
> +                        if (type(p) != str):
> +                            if p[0] == 'dev':
> +                              disallow = ((p[1].find('hd') >= 0) and
> +                                          (p[1].find('cdrom') == -1))
> +                        if p[0] == 'mode' and disallow:
> +                            if p[1] == 'r':
> +                                return True
> +    return False
>  
>  def create(config):
>      """Creates and start a VM using the supplied configuration. 
> @@ -100,6 +126,10 @@ def create(config):
>          othervm = XendDomain.instance().domain_lookup_nr(domconfig["uuid"])
>      if othervm is not None and othervm.domid is not None:
>          raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
> +
> +    if cfg_hvm_have_readonly_ide_disks(config):
> +        raise VmError("HVM domains cannot be using read-only IDE drives")
> +
>      log.debug("XendDomainInfo.create(%s)", scrub_password(config))
>      vm = XendDomainInfo(domconfig)
>      try:

Otherwise looks good to me.

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/22/2010 04:10 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Jun 22, 2010 at 02:56:24PM +0200, Michal Novotny wrote:
>    
>> On 06/22/2010 08:17 AM, Michal Novotny wrote:
>>      
>>> On 06/22/2010 08:14 AM, Keir Fraser wrote:
>>>        
>>>> Okay, hopefully someone will be able to Ack this patch with
>>>> better knwoledge
>>>> of xend than me.
>>>>
>>>>   -- Keir
>>>>
>>>>          
>>> Ok, good. If it will be accepted and put into
>>> xen-staging/xen-unstable tree please let me know about it.
>>>
>>> Thanks,
>>> Michal
>>>
>>>        
>> Well, this is new version of the patch since I've tried now to both
>> migrate from RHEL-5 host (python-2.4) to RHEL-6 host (python-2.6)
>> using the RHEL-5 host as a source machine for migrations and also
>> RHEL-6 host to restore the the guest locally using `xm restore` to
>> see whether the guest memory calculated correctly and it did pass
>> when there was enough memory to create the guest and failed when
>> there was not enough memory to create the guest. Also, some fixing
>> for comparing the values was necessary since it was treated as
>> string comparison on one of the hosts.
>>
>> Both the host machines were having the latest Xen-4.1 installed.
>>
>> Michal
>>
>> Signed-off-by: Michal Novotny<minovotn@redhat.com>
>>
>> -- 
>> Michal Novotny<minovotn@redhat.com>, RHCE
>> Virtualization Team (xen userspace), Red Hat
>>
>>      
>    
>> diff -r a24dbfcbdf69 tools/python/xen/xend/XendCheckpoint.py
>> --- a/tools/python/xen/xend/XendCheckpoint.py	Tue Jun 22 07:19:38 2010 +0100
>> +++ b/tools/python/xen/xend/XendCheckpoint.py	Tue Jun 22 12:52:56 2010 +0200
>> @@ -64,6 +64,78 @@ def insert_after(list, pred, value):
>>                 list.insert (i+1, value)
>>       return
>>
>> +def get_avail_memory():
>> +    """Get available memory for new guest creation (in KiB, for restore)"""
>> +    from xen.xend import XendOptions
>> +
>> +    # First get total memory in KiB
>> +    xc = xen.lowlevel.xc.xc()
>> +    info = xc.domain_getinfo()
>> +    total_mem  = xc.physinfo()['total_memory']
>> +    del xc
>> +
>> +    # Count memory of all running guests in KiB
>> +    mem_used = 0L
>> +    for x in info:
>> +        if x['domid'] != 0:
>> +            # If blocked&  paused&  !paused&  no online_vcpus we most
>> +            # probably migrate so we get maxmem instead (since mem_kb
>> +            # is having currently transferred amount of memory so we
>> +            # cannot use it for good calculations)
>> +            if (x['blocked'] == 1 and x['paused'] == 1 and
>> +                x['running'] == 0 and x['online_vcpus'] == 0):
>> +                log.debug("Domain %d seems to be restoring" % x['domid'])
>> +                mem_used += x['maxmem_kb']
>> +            else:
>> +                mem_used += x['mem_kb']
>> +
>> +    # Get minimal memory for dom0 and convert to KiB
>> +    min_mem = XendOptions.instance().get_dom0_min_mem() * 1024
>> +
>> +    return total_mem - mem_used - min_mem
>> +
>> +def check_for_restore_bail(msg):
>> +    raise VmError("Cannot restore: %s" % msg)
>> +
>> +def check_for_enough_mem(val):
>> +    mem_avail = get_avail_memory() / 1024
>> +    log.debug("Available memory: %s MiB, guest requires: %s MiB" % (mem_avail, val))
>> +    return int(mem_avail)>  int(val)
>> +
>> +def check_for_restore_is_hvm(cfg):
>> +    for item in cfg:
>> +        if (type(item) == list):
>> +           if item[0] == 'image':
>> +               return type(item[1]) == list and item[1][0] == 'hvm'
>>      
> There is no chance that 'hvm' would be in different case? Say 'HVM' ?
>    

According to my testing it's always lowercase here and maybe everything 
on the SXP is lowercase. Nevertheless I am not sure about everything but 
what I'm sure is that the hvm is *always* lowercase.
>> +
>> +def check_for_restore_hvm_have_readonly_ide(cfg):
>> +    """Check the configuration for read-only IDE devices
>> +       Fail if such a device is found."""
>> +    disallow = None
>> +    if type(cfg) == list and cfg[0] in ('tap', 'vbd'):
>> +        for p in cfg:
>> +            if (type(p) != str):
>>      
> Is that right? should it not == list ?
>    

It's been tested and working fine. There are no more options than 'list' 
and 'str' so in fact this is the same to implement it like 'if .. != 
str' and 'if ... == list'.

>> +                if p[0] == 'dev':
>> +                    disallow = ((p[1].find('hd')>= 0) and
>> +                                (p[1].find('cdrom') == -1))
>> +                if p[0] == 'mode' and disallow:
>> +                    if p[1] == 'r':
>> +                        return True
>> +    return False
>> +
>> +def check_for_restore(cfg):
>> +    is_hvm = check_for_restore_is_hvm(cfg)
>> +    name = None
>> +    for item in cfg:
>> +        if (type(item) == list):
>> +            # Check for enough memory to create the guest
>> +            if item[0] == 'memory':
>> +                if not check_for_enough_mem(item[1]):
>> +                    check_for_restore_bail('Host machine doesn\'t have enough memory to create the guest')
>> +            # We disable read-only IDE drives only for HVM guests
>> +            if item[0] == 'device' and is_hvm:
>> +                if check_for_restore_hvm_have_readonly_ide(item[1]):
>> +                    check_for_restore_bail('HVM domains cannot be using read-only IDE drives')
>>
>>   def save(fd, dominfo, network, live, dst, checkpoint=False, node=-1):
>>       from xen.xend import XendDomain
>> @@ -220,6 +292,7 @@ def restore(xd, fd, dominfo = None, paus
>>               othervm = xd.domain_lookup_nr(domconfig["uuid"])
>>           if othervm is not None and othervm.domid is not None:
>>               raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
>> +        check_for_restore(vmconfig)
>>
>>       if dominfo:
>>           dominfo.resume()
>> diff -r a24dbfcbdf69 tools/python/xen/xend/XendDomainInfo.py
>> --- a/tools/python/xen/xend/XendDomainInfo.py	Tue Jun 22 07:19:38 2010 +0100
>> +++ b/tools/python/xen/xend/XendDomainInfo.py	Tue Jun 22 12:52:56 2010 +0200
>> @@ -82,6 +82,32 @@ log = logging.getLogger("xend.XendDomain
>>   log = logging.getLogger("xend.XendDomainInfo")
>>   #log.setLevel(logging.TRACE)
>>
>> +def cfg_is_hvm(cfg):
>> +    for item in cfg:
>> +        if (type(item) == list):
>> +           if item[0] == 'image':
>> +               return type(item[1]) == list and item[1][0] == 'hvm'
>> +
>> +def cfg_hvm_have_readonly_ide_disks(cfg):
>> +    """Check whether the configuration for read-only IDE disks since
>> +       they are not supported according to IDE specifications."""
>> +    if not cfg_is_hvm(cfg):
>> +        return False
>> +
>> +    disallow = None
>> +    for item in cfg:
>> +        if (type(item) == list):
>> +            if item[0] == 'device':
>> +                if type(item[1]) == list and item[1][0] in ('tap', 'vbd'):
>> +                    for p in item[1]:
>> +                        if (type(p) != str):
>> +                            if p[0] == 'dev':
>> +                              disallow = ((p[1].find('hd')>= 0) and
>> +                                          (p[1].find('cdrom') == -1))
>> +                        if p[0] == 'mode' and disallow:
>> +                            if p[1] == 'r':
>> +                                return True
>> +    return False
>>
>>   def create(config):
>>       """Creates and start a VM using the supplied configuration.
>> @@ -100,6 +126,10 @@ def create(config):
>>           othervm = XendDomain.instance().domain_lookup_nr(domconfig["uuid"])
>>       if othervm is not None and othervm.domid is not None:
>>           raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
>> +
>> +    if cfg_hvm_have_readonly_ide_disks(config):
>> +        raise VmError("HVM domains cannot be using read-only IDE drives")
>> +
>>       log.debug("XendDomainInfo.create(%s)", scrub_password(config))
>>       vm = XendDomainInfo(domconfig)
>>       try:
>>      
> Otherwise looks good to me.
>    
Good, thanks for review.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

RE: [PATCH] Fix restore handling checks

[Dan Magenheimer]

Correct me if I am wrong, but I think your patch assumes
that the amount of free memory in the system can be
computed by assuming each guest memory is fixed size.
Due to various features in Xen 4.0, this is no longer
a safe assumption.  Tmem has a libxc call to freeze
and unfreeze its use of memory so dynamic memory use
by tmem can be stopped, and another libxc call to
determine "freeable" memory, and another to free it.
I don't know if the page-sharing functionality added
at 4.0 has anything similar.

But in any case, simple algorithms to add up current
(or max) guest memory will have many false-positive
and false-negative results.

> -----Original Message-----
> From: Michal Novotny [mailto:minovotn@redhat.com]
> Sent: Tuesday, June 22, 2010 6:56 AM
> To: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] Fix restore handling checks
> 
> On 06/22/2010 08:17 AM, Michal Novotny wrote:
> > On 06/22/2010 08:14 AM, Keir Fraser wrote:
> >> Okay, hopefully someone will be able to Ack this patch with better
> >> knwoledge
> >> of xend than me.
> >>
> >>   -- Keir
> >>
> > Ok, good. If it will be accepted and put into xen-staging/xen-
> unstable
> > tree please let me know about it.
> >
> > Thanks,
> > Michal
> >
> 
> Well, this is new version of the patch since I've tried now to both
> migrate from RHEL-5 host (python-2.4) to RHEL-6 host (python-2.6) using
> the RHEL-5 host as a source machine for migrations and also RHEL-6 host
> to restore the the guest locally using `xm restore` to see whether the
> guest memory calculated correctly and it did pass when there was enough
> memory to create the guest and failed when there was not enough memory
> to create the guest. Also, some fixing for comparing the values was
> necessary since it was treated as string comparison on one of the
> hosts.
> 
> Both the host machines were having the latest Xen-4.1 installed.
> 
> Michal
> 
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> 
> --
> Michal Novotny<minovotn@redhat.com>, RHCE
> Virtualization Team (xen userspace), Red Hat
> 

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/22/2010 10:46 PM, Dan Magenheimer wrote:
> Correct me if I am wrong, but I think your patch assumes
> that the amount of free memory in the system can be
> computed by assuming each guest memory is fixed size.
> Due to various features in Xen 4.0, this is no longer
> a safe assumption.  Tmem has a libxc call to freeze
> and unfreeze its use of memory so dynamic memory use
> by tmem can be stopped, and another libxc call to
> determine "freeable" memory, and another to free it.
> I don't know if the page-sharing functionality added
> at 4.0 has anything similar.
>
>    

Honestly I don't know about how tmem features work since I have them 
disabled because I'm unable to start any guest with them enabled because 
it fails on creation.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/22/2010 10:46 PM, Dan Magenheimer wrote:
> Correct me if I am wrong, but I think your patch assumes
> that the amount of free memory in the system can be
> computed by assuming each guest memory is fixed size.
> Due to various features in Xen 4.0, this is no longer
> a safe assumption.  Tmem has a libxc call to freeze
> and unfreeze its use of memory so dynamic memory use
> by tmem can be stopped

Maybe it's stopped but in the domain_getinfo() of libxc we should be 
getting the value of original guest memory although it's being freezed 
but the only difference is that the memory should not be accessible 
since it's locked somehow. Is my understanding of tmem freeze correct?

> and another libxc call to
> determine "freeable" memory, and another to free it.
> I don't know if the page-sharing functionality added
> at 4.0 has anything similar.
>
> But in any case, simple algorithms to add up current
> (or max) guest memory will have many false-positive
> and false-negative results.
>
>    

Why should it give too many false-positives/false-negatives since the 
handling there is to sum the total guest memory and decrease the 
computed size from the total host memory according to physinfo() output 
from libxc. Also there should be the minimal memory for dom0 taken in 
account since. There's the example for my configuration - I'm having 8G 
of RAM in total, if I start up one guest with 2G of RAM allocated, we 
should be having 8 - 2 = 6 G available now (no matter what amount of 
memory is being allocated to the dom0 since the physinfo() is getting 
the total memory information from hypervisor directly, i.e. you could be 
having 4G allocated to dom0 but the host machine could be having 8G of 
RAM in total).

1. total physical memory = 8G
2. dom0_mem = 4G, dom0-min-mem = 1G
3. create the guest A with 2G RAM -> 6G in total are available now
4. create the guest B with 4G RAM -> 4G should be available but guest is 
still on migration/restore
5. In the middle of the guest restore/migrate from step 4 (guest B) we 
start another migration/restore of 2G guest (guest C), since the guest B 
is having already 2G  memory, that way "mem_kb" equals to 2G for guest B 
(instead of 4G) so we have to take "maxmem_kb" instead (i.e. 4G value) 
to compute we don't have enough memory for guest C creation.

If we used "mem_kb" in all the cases (even for migration/restore case) 
we would sum up the value to be: 2 + 2 (there should be 4 since the 
guest is restoring right now) + 2 = 6G which is less than 8G (total 
memory) - 1G (dom0-min-mem) = 7G so it would allow the guest creation 
which would result into failure when the migration/restore of guest C 
fails and therefore the guest C will be destroyed with incomplete memory 
transfer.

That's why I used the computation of "maxmem_kb" instead, since for this 
scenario the value is: 2 + 4 + 2 = 8G which is bigger than 7G (total 
memory - dom0-min-mem) so we disallow the guest restore immediately.

So what should those calculations give many false-positives or 
false-negatives?

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

RE: [PATCH] Fix restore handling checks

[Dan Magenheimer]

> From: Michal Novotny [mailto:minovotn@redhat.com]
>
> Honestly I don't know about how tmem features work since I have them
> disabled because I'm unable to start any guest with them enabled
> because it fails on creation.

If you are booting Xen enabling tmem and NOT setting dom0_mem=
there was a bug just reported and fixed (or patch posted at
least).  If this is another problem with tmem (e.g. you ARE
setting both tmem and dom0_mem), please tell me more.

However, note that just enabling tmem in Xen doesn't do
anything.  Tmem must also be enabled in guests.

> Why should it give too many false-positives/false-negatives...

The issue is that in a tmem system (with self-ballooning enabled
which will be the default for tmem-enabled guests), all the
guests are dynamically changing in size between their maximum
memory and some minimum which may be smaller than the mem=
they were launched with.

And "free" memory as reported by Xen is also changing dynamically
due to guest tmem calls.

The only way to ensure a fixed amount of memory is available is:
1) freeze tmem
2) get free memory and "tmem freeable" memory and if there
   is enough
3) ALLOCATE the memory NOW
4) check to ensure the allocation worked
5) unfreeze tmem

The code in xends currently does this when launching new domains.

Tmem freeze only stops tmem from allocating more free memory
from Xen (by failing all tmem calls from tmem-enabled guests).
It doesn't stop guest ballooning activity.  So even the
"ALLOCATE the memory NOW" may fail... but that is acceptable
when launching a new domain.

Hope that helps,
Dan

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/23/2010 03:51 PM, Dan Magenheimer wrote:
>> From: Michal Novotny [mailto:minovotn@redhat.com]
>>
>> Honestly I don't know about how tmem features work since I have them
>> disabled because I'm unable to start any guest with them enabled
>> because it fails on creation.
>>      
> If you are booting Xen enabling tmem and NOT setting dom0_mem=
> there was a bug just reported and fixed (or patch posted at
> least).  If this is another problem with tmem (e.g. you ARE
> setting both tmem and dom0_mem), please tell me more.
>
> However, note that just enabling tmem in Xen doesn't do
> anything.  Tmem must also be enabled in guests.
>
>    

Well, I double-checked the configuration and I was not using dom0_mem on 
HV command-line so this is the bug I hit. It's a version about week old 
or something like that so I guess it was the issue I met.

>> Why should it give too many false-positives/false-negatives...
>>      
> The issue is that in a tmem system (with self-ballooning enabled
> which will be the default for tmem-enabled guests), all the
> guests are dynamically changing in size between their maximum
> memory and some minimum which may be smaller than the mem=
> they were launched with.
>
> And "free" memory as reported by Xen is also changing dynamically
> due to guest tmem calls.
>
> The only way to ensure a fixed amount of memory is available is:
> 1) freeze tmem
> 2) get free memory and "tmem freeable" memory and if there
>     is enough
> 3) ALLOCATE the memory NOW
> 4) check to ensure the allocation worked
> 5) unfreeze tmem
>
> The code in xends currently does this when launching new domains.
>
> Tmem freeze only stops tmem from allocating more free memory
> from Xen (by failing all tmem calls from tmem-enabled guests).
> It doesn't stop guest ballooning activity.  So even the
> "ALLOCATE the memory NOW" may fail... but that is acceptable
> when launching a new domain.
>
> Hope that helps,
> Dan
>
>    
Oh, ok. Then I guess that the patch could be dropped since there are 
objections against both IDE read-only disk violation and memory check 
parts, i.e. against both parts.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix restore handling checks

[Ian Jackson]

Michal Novotny writes ("Re: [Xen-devel] [PATCH] Fix restore handling checks"):
> Right, there's the exception for CD-ROMs according to the spec. I should 
> implement this as well but read-only IDE disk devices are not supported 
> according to the IDE specs.

I don't think this is a very good argument for not supporting them.

The Xen administrator should be able to present a guest with an IDE
disk and nevertheless prevent the guest from writing to it.

If that means that the guest administrator needs to be told
out-of-band that writes will not work, rather than having their kernel
automatically discover the read-only nature, then that's fine.  And if
the guest administrator doesn't do that and their kernel tries to
write and gets IDE errors then that is the correct consequence.

Ian.

Re: [PATCH] Fix restore handling checks

[Paolo Bonzini]

On 06/22/2010 04:56 PM, Ian Jackson wrote:
> Michal Novotny writes ("Re: [Xen-devel] [PATCH] Fix restore handling checks"):
>> Right, there's the exception for CD-ROMs according to the spec. I should
>> implement this as well but read-only IDE disk devices are not supported
>> according to the IDE specs.
>
> I don't think this is a very good argument for not supporting them.
>
> The Xen administrator should be able to present a guest with an IDE
> disk and nevertheless prevent the guest from writing to it.
>
> If that means that the guest administrator needs to be told
> out-of-band that writes will not work, rather than having their kernel
> automatically discover the read-only nature, then that's fine.  And if
> the guest administrator doesn't do that and their kernel tries to
> write and gets IDE errors then that is the correct consequence.

We tried to get that to work, and we could not really find a device 
error that worked reliably in the guests.  We often got infinite loops 
on mount or little after that, on both Linux and Windows.

If you want to use read-only devices, you should really use SCSI or PV 
devices.

Paolo

Re: [PATCH] Fix restore handling checks

[Ian Jackson]

Paolo Bonzini writes ("Re: [PATCH] Fix restore handling checks"):
> We tried to get that to work, and we could not really find a device 
> error that worked reliably in the guests.  We often got infinite loops 
> on mount or little after that, on both Linux and Windows.

That's fine.  If the admins don't want that then they can use SCSI or
PV or simply not tell the guest to write to the device.

Disallowing the configuration entirely just because the error handling
is suboptimal (but safe!) is not correct.

Ian.

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/23/2010 01:27 PM, Ian Jackson wrote:
> Paolo Bonzini writes ("Re: [PATCH] Fix restore handling checks"):
>    
>> We tried to get that to work, and we could not really find a device
>> error that worked reliably in the guests.  We often got infinite loops
>> on mount or little after that, on both Linux and Windows.
>>      
> That's fine.  If the admins don't want that then they can use SCSI or
> PV or simply not tell the guest to write to the device.
>
> Disallowing the configuration entirely just because the error handling
> is suboptimal (but safe!) is not correct.
>
> Ian.
>    

Why do you think so, Ian? This follows the IDE specifications to 
disallow read-only IDE disks since the specs doesn't support IDE 
read-only disks. The only read-only device being supported by IDE specs 
is the CD-ROM device.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix restore handling checks

[Ian Jackson]

Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
> Why do you think so, Ian? This follows the IDE specifications to 
> disallow read-only IDE disks since the specs doesn't support IDE 
> read-only disks. The only read-only device being supported by IDE specs 
> is the CD-ROM device.

The administrator should be free to violate the IDE specification if
the IDE specification conflicts with their security goals.

Ian.

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/23/2010 01:50 PM, Ian Jackson wrote:
> Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
>    
>> Why do you think so, Ian? This follows the IDE specifications to
>> disallow read-only IDE disks since the specs doesn't support IDE
>> read-only disks. The only read-only device being supported by IDE specs
>> is the CD-ROM device.
>>      
> The administrator should be free to violate the IDE specification if
> the IDE specification conflicts with their security goals.
>
> Ian.
>    

Are you saying that it's OK for administrators to violate the IDE specs 
and do it the way that is should never be done since this way it's not 
working on bare-metal systems ? This is the breach and it shouldn't be 
done this way so why to allow it? Shouldn't we care the code complies 
with the specifications to have it done the right way?

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix restore handling checks

[Ian Jackson]

Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
> Are you saying that it's OK for administrators to violate the IDE specs 
> and do it the way that is should never be done since this way it's not 
> working on bare-metal systems ? This is the breach and it shouldn't be 
> done this way so why to allow it? Shouldn't we care the code complies 
> with the specifications to have it done the right way?

The job of the programmer is to give effect to the wishes of the
users, not to comply with rules from elsewhere.  If the wishes of the
users conflict with rules from elsewhere, including specs, then the
programmer should do what the user wants.

Ian.

Re: [PATCH] Fix restore handling checks

[Paolo Bonzini]

On 06/23/2010 02:04 PM, Ian Jackson wrote:
> Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
>> Are you saying that it's OK for administrators to violate the IDE specs
>> and do it the way that is should never be done since this way it's not
>> working on bare-metal systems ? This is the breach and it shouldn't be
>> done this way so why to allow it? Shouldn't we care the code complies
>> with the specifications to have it done the right way?
>
> The job of the programmer is to give effect to the wishes of the
> users, not to comply with rules from elsewhere.  If the wishes of the
> users conflict with rules from elsewhere, including specs, then the
> programmer should do what the user wants.

Telling him that he wanted something that cannot be _emulated_ 
accurately is also a possibility.  But we can agree to disagree here and 
Michal can remove this part of the patch.

Paolo

Re: Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/23/2010 02:10 PM, Paolo Bonzini wrote:
> On 06/23/2010 02:04 PM, Ian Jackson wrote:
>> Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
>>> Are you saying that it's OK for administrators to violate the IDE specs
>>> and do it the way that is should never be done since this way it's not
>>> working on bare-metal systems ? This is the breach and it shouldn't be
>>> done this way so why to allow it? Shouldn't we care the code complies
>>> with the specifications to have it done the right way?
>>
>> The job of the programmer is to give effect to the wishes of the
>> users, not to comply with rules from elsewhere.  If the wishes of the
>> users conflict with rules from elsewhere, including specs, then the
>> programmer should do what the user wants.
>
> Telling him that he wanted something that cannot be _emulated_ 
> accurately is also a possibility.  But we can agree to disagree here 
> and Michal can remove this part of the patch.
>
> Paolo
>

Well, just one correction to this Paolo. It can be emulated but the 
emulation doesn't comply to the specifications and since it was never 
supported at least for HVM guests prior to my patch to fix read-only 
image handling so it shouldn't break anything so if it was not emulated 
before the patch I sent 13 days ago why not to emulate/implement it the 
right way? I don't see any reason to emulate it the wrong way since we 
know the right way that's been implemented/fixed just 13 days ago - at 
least for HVM guests. If users want to use read-only drives they can use 
SCSI drives so what's the problem here?

Surely, I can remove this part of the patch but I'm just trying to tell 
the readers of this thread that I don't see much point in having the 
wrong implementation. I don't think users want to do it the wrong way 
and I think they could understand that this is not supported and that 
they should use SCSI drive definition for read-only drives instead.

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/23/2010 02:10 PM, Paolo Bonzini wrote:
> On 06/23/2010 02:04 PM, Ian Jackson wrote:
>> Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
>>> Are you saying that it's OK for administrators to violate the IDE specs
>>> and do it the way that is should never be done since this way it's not
>>> working on bare-metal systems ? This is the breach and it shouldn't be
>>> done this way so why to allow it? Shouldn't we care the code complies
>>> with the specifications to have it done the right way?
>>
>> The job of the programmer is to give effect to the wishes of the
>> users, not to comply with rules from elsewhere.  If the wishes of the
>> users conflict with rules from elsewhere, including specs, then the
>> programmer should do what the user wants.
>
> Telling him that he wanted something that cannot be _emulated_ 
> accurately is also a possibility.  But we can agree to disagree here 
> and Michal can remove this part of the patch.
>
> Paolo
>

Well, just one correction to this Paolo. It can be emulated but the 
emulation doesn't comply to the specifications and since it was never 
supported at least for HVM guests prior to my patch to fix read-only 
image handling so it shouldn't break anything so if it was not emulated 
before the patch I sent 13 days ago why not to emulate/implement it the 
right way? I don't see any reason to emulate it the wrong way since we 
know the right way that's been implemented/fixed just 13 days ago - at 
least for HVM guests. If users want to use read-only drives they can use 
SCSI drives so what's the problem here?

Surely, I can remove this part of the patch but I'm just trying to tell 
the readers of this thread that I don't see much point in having the 
wrong implementation. I don't think users want to do it the wrong way 
and I think they could understand that this is not supported and that 
they should use SCSI drive definition for read-only drives instead.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/23/2010 02:04 PM, Ian Jackson wrote:
> Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
>    
>> Are you saying that it's OK for administrators to violate the IDE specs
>> and do it the way that is should never be done since this way it's not
>> working on bare-metal systems ? This is the breach and it shouldn't be
>> done this way so why to allow it? Shouldn't we care the code complies
>> with the specifications to have it done the right way?
>>      
> The job of the programmer is to give effect to the wishes of the
> users, not to comply with rules from elsewhere.  If the wishes of the
> users conflict with rules from elsewhere, including specs, then the
> programmer should do what the user wants.
>
> Ian.
>    
Right but I don't think someone uses the read-only IDE drives or 
read-only drives in general - at least for HVM guests since just few 
days I posted the patch to disallow write to the read-only devices on 
ioemu - maybe you remember I did post the patch at [1]. Before this 
patch applied the read-only disks were treated as read-write always no 
matter what the state of disk device was present in the xenstore so why 
not to teach the users the right things according to the specs. This 
shouldn't break anything if you consider you added my patch to xen-ioemu 
just 13 days ago and I find it rather confusing to let users define 
read-only IDE disks which is not right according to the specs, this 
means that by allowing it we could confuse the users so that they can 
think that read-only IDE disks does exists even according to the specs 
and they may try to think that the bare-metal IDE disks does support it 
as well. Since this shouldn't break anything why shouldn't we do the 
things the right way? They have SCSI disks and PV drivers if they want 
read-only disks after all.

Michal

[1] 
http://xenbits.xensource.com/gitweb?p=qemu-xen-unstable.git;a=commit;h=6392763643311272590ef5c6f75ba11d5b132585

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: Re: [PATCH] Fix restore handling checks

[Alan Cox]

On Wed, 23 Jun 2010 13:04:26 +0100
Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:

> Michal Novotny writes ("Re: [PATCH] Fix restore handling checks"):
> > Are you saying that it's OK for administrators to violate the IDE specs 
> > and do it the way that is should never be done since this way it's not 
> > working on bare-metal systems ? This is the breach and it shouldn't be 
> > done this way so why to allow it? Shouldn't we care the code complies 
> > with the specifications to have it done the right way?
> 
> The job of the programmer is to give effect to the wishes of the
> users, not to comply with rules from elsewhere.  If the wishes of the
> users conflict with rules from elsewhere, including specs, then the
> programmer should do what the user wants.

So why doesn't Xen automatically produce free ice cream when the server
room gets warm ? and why doesn't Xen support storing 1Tb of data on a
40GB disc ? These are what the user wants.

That said there is nothing forbidding read only IDE media. It's unusual
and in the real world you normally only encounter it via hardware plugins
used for forensics work (so you can mirror a device and stand up in court
and say 'Neither I nor my system could have modified the contents of the
disc').

I guess the elegant way would be to present a read only IDE volume as a
magneto optical (ie ATAPI disc) but simply aborting (0x04) any write
commands isn't any different to what some of the drive protection widgets
do.

Alan

Re: Re: [PATCH] Fix restore handling checks

[George Dunlap]

On Wed, Jun 23, 2010 at 1:33 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> So why doesn't Xen automatically produce free ice cream when the server
> room gets warm ? and why doesn't Xen support storing 1Tb of data on a
> 40GB disc ? These are what the user wants.

If there was a ISO server room standard somewhere that specified no
automatically generated free ice cream, but a 200-line patch would
include this feature in Xen (disabled by default), I'd say the right
thing to do is to include the patch, standard be damned.  More to the
point, if generating free ice cream was a natural byproduct of
whatever we were doing already, and someone said, "But the standard
says no automatic free ice cream" and submitted a patch to disable it,
I'd argue against it too.  Same principle applies. :-)

 -George

RE: Re: [PATCH] Fix restore handling checks

[Dan Magenheimer]

> > The job of the programmer is to give effect to the wishes of the
> > users, not to comply with rules from elsewhere.  If the wishes of the
> > users conflict with rules from elsewhere, including specs, then the
> > programmer should do what the user wants.
> 
> So why doesn't Xen automatically produce free ice cream when the server
> room gets warm ? and why doesn't Xen support storing 1Tb of data on a
> 40GB disc ? These are what the user wants.

Mmm.... free ice cream... mmmm.  This user wishes for it.
Ian, could you kindly do what this user wants, even though
it conflicts with rules from my wife?

(sorry couldn't resist :-)
Dan

Re: Re: [PATCH] Fix restore handling checks

[Alan Cox]

On Wed, 23 Jun 2010 13:29:56 +0200
Michal Novotny <minovotn@redhat.com> wrote:

> On 06/23/2010 01:27 PM, Ian Jackson wrote:
> > Paolo Bonzini writes ("Re: [PATCH] Fix restore handling checks"):
> >    
> >> We tried to get that to work, and we could not really find a device
> >> error that worked reliably in the guests.  We often got infinite loops
> >> on mount or little after that, on both Linux and Windows.
> >>      
> > That's fine.  If the admins don't want that then they can use SCSI or
> > PV or simply not tell the guest to write to the device.
> >
> > Disallowing the configuration entirely just because the error handling
> > is suboptimal (but safe!) is not correct.
> >
> > Ian.
> >    
> 
> Why do you think so, Ian? This follows the IDE specifications to 
> disallow read-only IDE disks since the specs doesn't support IDE 
> read-only disks. The only read-only device being supported by IDE specs 
> is the CD-ROM device.

IDE (or more properly ATA) doesn't support CD-ROM either. ATAPI does, and
it also supports other ATAPI devices with write protect including magneto
optical and floppy.

Re: Re: [PATCH] Fix restore handling checks

[Michal Novotny]

On 06/23/2010 02:35 PM, Alan Cox wrote:
> On Wed, 23 Jun 2010 13:29:56 +0200
> Michal Novotny<minovotn@redhat.com>  wrote:
>
>    
>> On 06/23/2010 01:27 PM, Ian Jackson wrote:
>>      
>>> Paolo Bonzini writes ("Re: [PATCH] Fix restore handling checks"):
>>>
>>>        
>>>> We tried to get that to work, and we could not really find a device
>>>> error that worked reliably in the guests.  We often got infinite loops
>>>> on mount or little after that, on both Linux and Windows.
>>>>
>>>>          
>>> That's fine.  If the admins don't want that then they can use SCSI or
>>> PV or simply not tell the guest to write to the device.
>>>
>>> Disallowing the configuration entirely just because the error handling
>>> is suboptimal (but safe!) is not correct.
>>>
>>> Ian.
>>>
>>>        
>> Why do you think so, Ian? This follows the IDE specifications to
>> disallow read-only IDE disks since the specs doesn't support IDE
>> read-only disks. The only read-only device being supported by IDE specs
>> is the CD-ROM device.
>>      
> IDE (or more properly ATA) doesn't support CD-ROM either. ATAPI does, and
> it also supports other ATAPI devices with write protect including magneto
> optical and floppy.
>
>
>    

Agreed Alan. This is right that ATAPI does. Nevertheless in IDE specs 
it's about ATA and ATAPI. The read-only file systems are supported for 
IDE drives only for floppy and tape media according to the linux kernel 
source codes. Also, CD-ROM devices can be read-only but no other IDE 
media, i.e. no IDE disks.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat