[Qemu-devel] [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Qemu-devel] [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

O_DIRECT (cache=none) requires sector alignment, however the physical
sector size of CDROM/DVD drives is 2048, as opposed to most disk
devices which use 512. QEMU is hard coding 512 all over the place, so
allowing O_DIRECT for CDROM/DVD devices does not work.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block/raw-posix.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 291699f..0ea79b6 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
     BDRVRawState *s = bs->opaque;
 
     s->type = FTYPE_CD;
+    if (flags & BDRV_O_NOCACHE) {
+        fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for "
+                "CDROM/DVD device (%s)\n", filename);
+        flags &= ~BDRV_O_NOCACHE;
+    }
 
     /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
     return raw_open_common(bs, filename, flags, O_NONBLOCK);
-- 
1.7.1.1

Re: [Qemu-devel] [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Kevin Wolf]

Am 20.07.2010 17:17, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> O_DIRECT (cache=none) requires sector alignment, however the physical
> sector size of CDROM/DVD drives is 2048, as opposed to most disk
> devices which use 512. QEMU is hard coding 512 all over the place, so
> allowing O_DIRECT for CDROM/DVD devices does not work.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  block/raw-posix.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 291699f..0ea79b6 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
>      BDRVRawState *s = bs->opaque;
>  
>      s->type = FTYPE_CD;
> +    if (flags & BDRV_O_NOCACHE) {
> +        fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for "
> +                "CDROM/DVD device (%s)\n", filename);
> +        flags &= ~BDRV_O_NOCACHE;
> +    }

Good point. Just one detail: We should probably change bs->open_flags,
too, to keep things consistent.

Kevin

Re: [Qemu-devel] [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Jes Sorensen]

On 07/20/10 17:40, Kevin Wolf wrote:
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 291699f..0ea79b6 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
>>      BDRVRawState *s = bs->opaque;
>>  
>>      s->type = FTYPE_CD;
>> +    if (flags & BDRV_O_NOCACHE) {
>> +        fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for "
>> +                "CDROM/DVD device (%s)\n", filename);
>> +        flags &= ~BDRV_O_NOCACHE;
>> +    }
> 
> Good point. Just one detail: We should probably change bs->open_flags,
> too, to keep things consistent.

Thats effectively what my patch does. cdrom_open() calls
raw_open_common() which has this part:

    /* Use O_DSYNC for write-through caching, no flags for write-back
caching,
     * and O_DIRECT for no caching. */
    if ((bdrv_flags & BDRV_O_NOCACHE))
        s->open_flags |= O_DIRECT;

Cheers,
Jes

Re: [Qemu-devel] [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Kevin Wolf]

Am 20.07.2010 18:04, schrieb Jes Sorensen:
> On 07/20/10 17:40, Kevin Wolf wrote:
>>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>>> index 291699f..0ea79b6 100644
>>> --- a/block/raw-posix.c
>>> +++ b/block/raw-posix.c
>>> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
>>>      BDRVRawState *s = bs->opaque;
>>>  
>>>      s->type = FTYPE_CD;
>>> +    if (flags & BDRV_O_NOCACHE) {
>>> +        fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for "
>>> +                "CDROM/DVD device (%s)\n", filename);
>>> +        flags &= ~BDRV_O_NOCACHE;
>>> +    }
>>
>> Good point. Just one detail: We should probably change bs->open_flags,
>> too, to keep things consistent.
> 
> Thats effectively what my patch does. cdrom_open() calls
> raw_open_common() which has this part:
> 
>     /* Use O_DSYNC for write-through caching, no flags for write-back
> caching,
>      * and O_DIRECT for no caching. */
>     if ((bdrv_flags & BDRV_O_NOCACHE))
>         s->open_flags |= O_DIRECT;

s and bs both have a field open_flags (and I think they share some more
field names, which has caused confusion more than once). I meant the bs
one here.

Kevin

Re: [Qemu-devel] [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Jes Sorensen]

On 07/20/10 18:12, Kevin Wolf wrote:
> Am 20.07.2010 18:04, schrieb Jes Sorensen:
>> On 07/20/10 17:40, Kevin Wolf wrote:
>> Thats effectively what my patch does. cdrom_open() calls
>> raw_open_common() which has this part:
>>
>>     /* Use O_DSYNC for write-through caching, no flags for write-back
>> caching,
>>      * and O_DIRECT for no caching. */
>>     if ((bdrv_flags & BDRV_O_NOCACHE))
>>         s->open_flags |= O_DIRECT;
> 
> s and bs both have a field open_flags (and I think they share some more
> field names, which has caused confusion more than once). I meant the bs
> one here.
> 
> Kevin
Hi,

It shows up even that case is handled as one of the first things
bdrv_open_common() does is to set bs->open_flags = flags. Anyway
throwing an error as Anthony suggested seems safer.

Cheers,
Jes

[Qemu-devel] Re: [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Anthony Liguori]

On 07/20/2010 10:17 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> O_DIRECT (cache=none) requires sector alignment, however the physical
> sector size of CDROM/DVD drives is 2048, as opposed to most disk
> devices which use 512. QEMU is hard coding 512 all over the place, so
> allowing O_DIRECT for CDROM/DVD devices does not work.
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>    

Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that 
did the appropriate bouncing?

Silently disabling something a user explicitly asked for is not a good 
option.  In the very least, it should error out entirely.

Regards,

Anthony Liguori

> ---
>   block/raw-posix.c |    5 +++++
>   1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 291699f..0ea79b6 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
>       BDRVRawState *s = bs->opaque;
>
>       s->type = FTYPE_CD;
> +    if (flags&  BDRV_O_NOCACHE) {
> +        fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for "
> +                "CDROM/DVD device (%s)\n", filename);
> +        flags&= ~BDRV_O_NOCACHE;
> +    }
>
>       /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
>       return raw_open_common(bs, filename, flags, O_NONBLOCK);
>    

[Qemu-devel] Re: [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Jes Sorensen]

On 07/20/10 17:40, Anthony Liguori wrote:
> On 07/20/2010 10:17 AM, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>
>> O_DIRECT (cache=none) requires sector alignment, however the physical
>> sector size of CDROM/DVD drives is 2048, as opposed to most disk
>> devices which use 512. QEMU is hard coding 512 all over the place, so
>> allowing O_DIRECT for CDROM/DVD devices does not work.
>>
>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>>    
> 
> Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that
> did the appropriate bouncing?
> 
> Silently disabling something a user explicitly asked for is not a good
> option.  In the very least, it should error out entirely.

I thought about this, but it would require basically fixing up or
copying all of the pread/pwrite code to use the right block size. This
is really more of a band-aid but it should be pretty safe.

Cheers,
Jes

[Qemu-devel] Re: [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Anthony Liguori]

On 07/20/2010 11:02 AM, Jes Sorensen wrote:
> On 07/20/10 17:40, Anthony Liguori wrote:
>    
>> On 07/20/2010 10:17 AM, Jes.Sorensen@redhat.com wrote:
>>      
>>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>>
>>> O_DIRECT (cache=none) requires sector alignment, however the physical
>>> sector size of CDROM/DVD drives is 2048, as opposed to most disk
>>> devices which use 512. QEMU is hard coding 512 all over the place, so
>>> allowing O_DIRECT for CDROM/DVD devices does not work.
>>>
>>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>>>
>>>        
>> Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that
>> did the appropriate bouncing?
>>
>> Silently disabling something a user explicitly asked for is not a good
>> option.  In the very least, it should error out entirely.
>>      
> I thought about this, but it would require basically fixing up or
> copying all of the pread/pwrite code to use the right block size. This
> is really more of a band-aid but it should be pretty safe.
>    

Please throw an error.  If a user explicitly asks for something, and we 
can provide it, we should not continue.  Changing it to something else 
is a bug.

That doesn't apply when we're changing a default value, but if the user 
asks for something, we should give it to them or fail.

Regards,

Anthony Liguori

> Cheers,
> Jes
>    

[Qemu-devel] Re: [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Jes Sorensen]

On 07/20/10 18:03, Anthony Liguori wrote:
> On 07/20/2010 11:02 AM, Jes Sorensen wrote:
>> On 07/20/10 17:40, Anthony Liguori wrote:   
>>> Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that
>>> did the appropriate bouncing?
>>>
>>> Silently disabling something a user explicitly asked for is not a good
>>> option.  In the very least, it should error out entirely.
>>>      
>> I thought about this, but it would require basically fixing up or
>> copying all of the pread/pwrite code to use the right block size. This
>> is really more of a band-aid but it should be pretty safe.
> 
> Please throw an error.  If a user explicitly asks for something, and we
> can provide it, we should not continue.  Changing it to something else
> is a bug.
> 
> That doesn't apply when we're changing a default value, but if the user
> asks for something, we should give it to them or fail.

Ok that seems fair enough! I'll post an updated patch in a minute.

Cheers,
Jes

Re: [Qemu-devel] Re: [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[David S. Ahern]

On 07/20/10 10:02, Jes Sorensen wrote:
> On 07/20/10 17:40, Anthony Liguori wrote:
>> On 07/20/2010 10:17 AM, Jes.Sorensen@redhat.com wrote:
>>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>>
>>> O_DIRECT (cache=none) requires sector alignment, however the physical
>>> sector size of CDROM/DVD drives is 2048, as opposed to most disk
>>> devices which use 512. QEMU is hard coding 512 all over the place, so
>>> allowing O_DIRECT for CDROM/DVD devices does not work.
>>>
>>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>>>    
>>
>> Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that
>> did the appropriate bouncing?
>>
>> Silently disabling something a user explicitly asked for is not a good
>> option.  In the very least, it should error out entirely.
> 
> I thought about this, but it would require basically fixing up or
> copying all of the pread/pwrite code to use the right block size. This
> is really more of a band-aid but it should be pretty safe.
> 
> Cheers,
> Jes
> 
> 

What about setting the logical and physical block size properties? Is
the intent of these to handle varying sizes amongst block devices?

David

Re: [Qemu-devel] Re: [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Jes Sorensen]

On 07/20/10 18:16, David S. Ahern wrote:
> What about setting the logical and physical block size properties? Is
> the intent of these to handle varying sizes amongst block devices?
> 
> David

Well the problem is that we need to know the sector size of the host
device, and then start using that within QEMU for anything using
O_DIRECT. The way the QEMU code handles it right now is by hard coding
it, and cleaning that up could be a bit messy, but it should go on the
todo list.

Cheers,
Jes

Re: [Qemu-devel] Re: [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Kevin Wolf]

Am 20.07.2010 17:40, schrieb Anthony Liguori:
> On 07/20/2010 10:17 AM, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>
>> O_DIRECT (cache=none) requires sector alignment, however the physical
>> sector size of CDROM/DVD drives is 2048, as opposed to most disk
>> devices which use 512. QEMU is hard coding 512 all over the place, so
>> allowing O_DIRECT for CDROM/DVD devices does not work.
>>
>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>>    
> 
> Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that 
> did the appropriate bouncing?

We already have code that does some bouncing, we'd just need to teach it
to use different sizes than 512. Duplicating everything wouldn't be a
nice solution.

> Silently disabling something a user explicitly asked for is not a good 
> option.  In the very least, it should error out entirely.

Yeah, that's probably better.

Kevin

Re: [Qemu-devel] Re: [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Jes Sorensen]

On 07/20/10 18:09, Kevin Wolf wrote:
> Am 20.07.2010 17:40, schrieb Anthony Liguori:
>> Wouldn't a better solution be to have a cdrom_read/cdrom_write hook that 
>> did the appropriate bouncing?
> 
> We already have code that does some bouncing, we'd just need to teach it
> to use different sizes than 512. Duplicating everything wouldn't be a
> nice solution.

The problem is that it is quite hard to detect properly and there are
cases where we can get false positives, plus it would require cleaning
up the code to handle variable sector sizes. The latter is probably a
good thing, and if/when it happens, we can remove this.

Cheers,
Jes

Re: [Qemu-devel] [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Natalia Portillo]

El 20/07/2010, a las 16:17, Jes.Sorensen@redhat.com escribió:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> O_DIRECT (cache=none) requires sector alignment, however the physical
> sector size of CDROM/DVD drives is 2048, as opposed to most disk
> devices which use 512. QEMU is hard coding 512 all over the place, so
> allowing O_DIRECT for CDROM/DVD devices does not work.

What about if the device is a 4096 byte/sector hard disk, a 512 byte/sector CD-ROM (IRIX ones), a 2048 byte/sector magnetooptical?

We should get rid of that hard codes and use real values.

> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
> block/raw-posix.c |    5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 291699f..0ea79b6 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
>     BDRVRawState *s = bs->opaque;
> 
>     s->type = FTYPE_CD;
> +    if (flags & BDRV_O_NOCACHE) {
> +        fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for "
> +                "CDROM/DVD device (%s)\n", filename);
> +        flags &= ~BDRV_O_NOCACHE;
> +    }
> 
>     /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
>     return raw_open_common(bs, filename, flags, O_NONBLOCK);
> -- 
> 1.7.1.1
> 
> 

Re: [Qemu-devel] [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Jes Sorensen]

On 07/20/10 18:45, Natalia Portillo wrote:
> El 20/07/2010, a las 16:17, Jes.Sorensen@redhat.com escribió:
> 
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> O_DIRECT (cache=none) requires sector alignment, however the physical
>> sector size of CDROM/DVD drives is 2048, as opposed to most disk
>> devices which use 512. QEMU is hard coding 512 all over the place, so
>> allowing O_DIRECT for CDROM/DVD devices does not work.
> 
> What about if the device is a 4096 byte/sector hard disk, a 512 byte/sector CD-ROM (IRIX ones), a 2048 byte/sector magnetooptical?
> 
> We should get rid of that hard codes and use real values.

I totally agree! However for now my patch throws an error for the CDROM
case, which is better than the old situation. We need to fix it in
general, but it isn't trivial. The 4096 sector size hard disk going to
be the main issue.

Jes

Re: [Qemu-devel] [PATCH] Disable O_DIRECT for physical CDROM/DVD drives

[Anthony Liguori]

On 07/20/2010 11:45 AM, Natalia Portillo wrote:
> El 20/07/2010, a las 16:17, Jes.Sorensen@redhat.com escribió:
>
>    
>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>
>> O_DIRECT (cache=none) requires sector alignment, however the physical
>> sector size of CDROM/DVD drives is 2048, as opposed to most disk
>> devices which use 512. QEMU is hard coding 512 all over the place, so
>> allowing O_DIRECT for CDROM/DVD devices does not work.
>>      
> What about if the device is a 4096 byte/sector hard disk, a 512 byte/sector CD-ROM (IRIX ones), a 2048 byte/sector magnetooptical?
>    

BlockDriverStates need to handle non-aligned reads/writes.  As I 
mentioned earlier, we need cdrom_pread/cdrom_pwrite functions that do 
RMWs as necessary.

Regards,

Anthony Liguori

> We should get rid of that hard codes and use real values.
>
>    
>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>> ---
>> block/raw-posix.c |    5 +++++
>> 1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 291699f..0ea79b6 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -1139,6 +1139,11 @@ static int cdrom_open(BlockDriverState *bs, const char *filename, int flags)
>>      BDRVRawState *s = bs->opaque;
>>
>>      s->type = FTYPE_CD;
>> +    if (flags&  BDRV_O_NOCACHE) {
>> +        fprintf(stderr, "Disabling unsupported O_DIRECT (cache=none) for "
>> +                "CDROM/DVD device (%s)\n", filename);
>> +        flags&= ~BDRV_O_NOCACHE;
>> +    }
>>
>>      /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
>>      return raw_open_common(bs, filename, flags, O_NONBLOCK);
>> -- 
>> 1.7.1.1
>>
>>
>>      
>
>