diff mbox

[v3] Error on O_DIRECT for physical CDROM/DVD drives

Message ID 1279698319-31158-1-git-send-email-Jes.Sorensen@redhat.com
State New
Headers show

Commit Message

Jes Sorensen July 21, 2010, 7:45 a.m. UTC 
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.

Return -ENOTSUP from cdrom_open() in this case.

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

Comments

Markus Armbruster July 21, 2010, 9:20 a.m. UTC | #1 
Jes.Sorensen@redhat.com writes:

> 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.
>
> Return -ENOTSUP from cdrom_open() in this case.

Good short-term fix.
Christoph Hellwig July 21, 2010, 1:15 p.m. UTC | #2 
On Wed, Jul 21, 2010 at 09:45:19AM +0200, 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.
> 
> Return -ENOTSUP from cdrom_open() in this case.

The patch is not quite correct.  There are CDROMs with 512 byte sectors,
just as there are disks with larger sector sizes.  And of course these
limitations also apply when running ontop of filesystems.

So we really need to handle these things better and need to query
the sector size of the device and handle it properly.
Markus Armbruster July 21, 2010, 2:13 p.m. UTC | #3 
Christoph Hellwig <hch@lst.de> writes:

> On Wed, Jul 21, 2010 at 09:45:19AM +0200, 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.
>> 
>> Return -ENOTSUP from cdrom_open() in this case.
>
> The patch is not quite correct.  There are CDROMs with 512 byte sectors,
> just as there are disks with larger sector sizes.  And of course these
> limitations also apply when running ontop of filesystems.
>
> So we really need to handle these things better and need to query
> the sector size of the device and handle it properly.

Agreed, but I figure Jes's fix is the best we can do for .13, and worth
doing for .13.
Christoph Hellwig July 21, 2010, 2:25 p.m. UTC | #4 
On Wed, Jul 21, 2010 at 04:13:10PM +0200, Markus Armbruster wrote:
> Agreed, but I figure Jes's fix is the best we can do for .13, and worth
> doing for .13.

As said in the previous mail it's incorrect in many ways.  If at all
you could reject devices and files on filesysystems on device with
sector size > 512 bytes.  But once that infrastructure is there fixing
it for real should be rather trivial.  I'm looking into it now.
diff mbox

Patch

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 291699f..4b84770 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, "O_DIRECT (cache=none) for CDROM/DVD device (%s) "
+                "is unsupported\n", filename);
+        return -ENOTSUP;
+    }
 
     /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
     return raw_open_common(bs, filename, flags, O_NONBLOCK);