[Qemu-devel] [PATCH 0/3] block: Correct size across CD-ROM media change

[Qemu-devel] [PATCH 0/3] block: Correct size across CD-ROM media change

[Stefan Hajnoczi]

This patch series fixes two Linux host CD-ROM pass-through bugs in QEMU.

After applying these patches it is possible to pass-through a Linux host CD-ROM
completely.  The guest can eject from software or the physical eject button can
be pressed on the drive.  The guest can detect this and newly inserted media
are noticed.  There is no need to issue any QEMU monitor 'eject' or 'change'
commands because the host CD-ROM is completely "passed through".

Patch details:

The first is that the device size is cached even for removable devices and we
never update it.  If a host CD-ROM is changed, then the size will be stale and
reflect that of the last medium.

The second is that Linux host CD-ROM pass-through requires that we re-open the
device to refresh its size.  This is because the Linux CD-ROM driver only
refreshes the size when the device is opened and furthermore has a bug that
leads to stale sizes if the file descriptor is held across media change.

I have also included a trace event for bdrv_set_locked() because it is useful
information when debugging issues like these in the future.

[Qemu-devel] [PATCH 1/3] trace: Trace bdrv_set_locked()

[Stefan Hajnoczi]

It can be handy to know when the guest locks/unlocks the CD-ROM tray.
This trace event makes that possible.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c      |    2 ++
 trace-events |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index b476479..8f224b4 100644
--- a/block.c
+++ b/block.c
@@ -2696,6 +2696,8 @@ void bdrv_set_locked(BlockDriverState *bs, int locked)
 {
     BlockDriver *drv = bs->drv;
 
+    trace_bdrv_set_locked(bs, locked);
+
     bs->locked = locked;
     if (drv && drv->bdrv_set_locked) {
         drv->bdrv_set_locked(bs, locked);
diff --git a/trace-events b/trace-events
index e6138ea..f87f683 100644
--- a/trace-events
+++ b/trace-events
@@ -53,6 +53,7 @@ disable bdrv_aio_multiwrite_earlyfail(void *mcb) "mcb %p"
 disable bdrv_aio_multiwrite_latefail(void *mcb, int i) "mcb %p i %d"
 disable bdrv_aio_readv(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
 disable bdrv_aio_writev(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
+disable bdrv_set_locked(void *bs, int locked) "bs %p locked %d"
 
 # hw/virtio-blk.c
 disable virtio_blk_req_complete(void *req, int status) "req %p status %d"
-- 
1.7.4.1

[Qemu-devel] [PATCH 2/3] block: Do not cache device size for removable media

[Stefan Hajnoczi]

The block layer caches the device size to avoid doing lseek(fd, 0,
SEEK_END) every time this value is needed.  For removable media the
device size becomes stale if a new medium is inserted.  This patch
simply prevents device size caching for removable media.

A smarter solution is to update the cached device size when a new medium
is inserted.  Given that there are currently bugs with CD-ROM media
change I do not want to implement that approach until we've gotten
things correct first.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 8f224b4..89f6ded 100644
--- a/block.c
+++ b/block.c
@@ -1153,14 +1153,12 @@ int64_t bdrv_getlength(BlockDriverState *bs)
     if (!drv)
         return -ENOMEDIUM;
 
-    /* Fixed size devices use the total_sectors value for speed instead of
-       issuing a length query (like lseek) on each call.  Also, legacy block
-       drivers don't provide a bdrv_getlength function and must use
-       total_sectors. */
-    if (!bs->growable || !drv->bdrv_getlength) {
-        return bs->total_sectors * BDRV_SECTOR_SIZE;
-    }
-    return drv->bdrv_getlength(bs);
+    if (bs->growable || bs->removable) {
+        if (drv->bdrv_getlength) {
+            return drv->bdrv_getlength(bs);
+        }
+    }
+    return bs->total_sectors * BDRV_SECTOR_SIZE;
 }
 
 /* return 0 as number of sectors if no device present or error */
-- 
1.7.4.1

[Qemu-devel] [PATCH 3/3] raw-posix: Re-open host CD-ROM after media change

[Stefan Hajnoczi]

Piggy-back on the guest CD-ROM polling to poll on the host.  Open and
close the host CD-ROM file descriptor to ensure we read the new size and
not a stale size.

Two things are going on here:

1. If hald/udisks is not already polling CD-ROMs on the host then
   re-opening the CD-ROM causes the host to read the new medium's size.

2. There is a bug in Linux which means the CD-ROM file descriptor must
   be re-opened in order for lseek(2) to see the new size.  The
   inode size gets out of sync with the underlying device (which you can
   confirm by checking that /sys/block/sr0/size and lseek(2) do not
   match after media change).  I have raised this with the
   maintainers but we need a workaround for the foreseeable future.

Note that these changes are all in a #ifdef __linux__ section.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/raw-posix.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index a95c8d4..ac95467 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1240,10 +1240,27 @@ static int cdrom_is_inserted(BlockDriverState *bs)
     BDRVRawState *s = bs->opaque;
     int ret;
 
-    ret = ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
-    if (ret == CDS_DISC_OK)
-        return 1;
-    return 0;
+    /*
+     * Opening and closing on each drive status check ensures the medium size
+     * is refreshed.  If the file descriptor is kept open the size can become
+     * stale.  This is essentially replicating CD-ROM polling but is driven by
+     * the guest.  As the guest polls, we poll the host.
+     */
+
+    if (s->fd == -1) {
+        s->fd = qemu_open(bs->filename, s->open_flags, 0644);
+        if (s->fd < 0) {
+            return 0;
+        }
+    }
+
+    ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK);
+
+    if (!ret) {
+        close(s->fd);
+        s->fd = -1;
+    }
+    return ret;
 }
 
 static int cdrom_eject(BlockDriverState *bs, int eject_flag)
-- 
1.7.4.1

[Qemu-devel] Re: [PATCH 2/3] block: Do not cache device size for removable media

[Juan Quintela]

Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
> The block layer caches the device size to avoid doing lseek(fd, 0,
> SEEK_END) every time this value is needed.  For removable media the
> device size becomes stale if a new medium is inserted.  This patch
> simply prevents device size caching for removable media.
>
> A smarter solution is to update the cached device size when a new medium
> is inserted.  Given that there are currently bugs with CD-ROM media
> change I do not want to implement that approach until we've gotten
> things correct first.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block.c |   12 +++++-------
>  1 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/block.c b/block.c
> index 8f224b4..89f6ded 100644
> --- a/block.c
> +++ b/block.c
> @@ -1153,14 +1153,12 @@ int64_t bdrv_getlength(BlockDriverState *bs)
>      if (!drv)
>          return -ENOMEDIUM;
>  
> -    /* Fixed size devices use the total_sectors value for speed instead of
> -       issuing a length query (like lseek) on each call.  Also, legacy block
> -       drivers don't provide a bdrv_getlength function and must use
> -       total_sectors. */
> -    if (!bs->growable || !drv->bdrv_getlength) {

       if (!bs->growable || !bs->removable|| !drv->bdrv_getlength) {

changing just the test don't give exactly the same result?

> -        return bs->total_sectors * BDRV_SECTOR_SIZE;
> -    }
> -    return drv->bdrv_getlength(bs);
> +    if (bs->growable || bs->removable) {
> +        if (drv->bdrv_getlength) {
> +            return drv->bdrv_getlength(bs);
> +        }
> +    }
> +    return bs->total_sectors * BDRV_SECTOR_SIZE;
>  }
>  
>  /* return 0 as number of sectors if no device present or error */

[Qemu-devel] Re: [PATCH 3/3] raw-posix: Re-open host CD-ROM after media change

[Juan Quintela]

Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
> Piggy-back on the guest CD-ROM polling to poll on the host.  Open and
> close the host CD-ROM file descriptor to ensure we read the new size and
> not a stale size.
>
> Two things are going on here:
>
> 1. If hald/udisks is not already polling CD-ROMs on the host then
>    re-opening the CD-ROM causes the host to read the new medium's size.
>
> 2. There is a bug in Linux which means the CD-ROM file descriptor must
>    be re-opened in order for lseek(2) to see the new size.  The
>    inode size gets out of sync with the underlying device (which you can
>    confirm by checking that /sys/block/sr0/size and lseek(2) do not
>    match after media change).  I have raised this with the
>    maintainers but we need a workaround for the foreseeable future.
>
> Note that these changes are all in a #ifdef __linux__ section.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block/raw-posix.c |   25 +++++++++++++++++++++----
>  1 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index a95c8d4..ac95467 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1240,10 +1240,27 @@ static int cdrom_is_inserted(BlockDriverState *bs)
>      BDRVRawState *s = bs->opaque;
>      int ret;
>  
> -    ret = ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
> -    if (ret == CDS_DISC_OK)
> -        return 1;
> -    return 0;
> +    /*
> +     * Opening and closing on each drive status check ensures the medium size
> +     * is refreshed.  If the file descriptor is kept open the size can become
> +     * stale.  This is essentially replicating CD-ROM polling but is driven by
> +     * the guest.  As the guest polls, we poll the host.
> +     */

Comment confused me :p

we are not opening and closing at each status check.
We are opening if the cdrom is _not_ opened.  And we are closing if
there is one error.  If comment 2 above is right, you need to do
insteand something like:

	if (s->fd != -1) {
           close(s->fd);
	}

        s->fd = open( ....);

That is really reopening all the times.

> +
> +    if (s->fd == -1) {
> +        s->fd = qemu_open(bs->filename, s->open_flags, 0644);

Everything else on that file uses plain "open" not "qemu_open".
diference is basically that qemu_open() adds flag O_CLOEXEC.

I don't know if this one should be vanilla open or the other ones
qemu_open().

What do you think?


> +        if (s->fd < 0) {
> +            return 0;
> +        }
> +    }
> +
> +    ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK);

parens are not needed around ==.

> +
> +    if (!ret) {
> +        close(s->fd);
> +        s->fd = -1;
> +    }
> +    return ret;
>  }
>  
>  static int cdrom_eject(BlockDriverState *bs, int eject_flag)

Later, Juan.

Re: [Qemu-devel] Re: [PATCH 2/3] block: Do not cache device size for removable media

[Stefan Hajnoczi]

On Wed, Mar 23, 2011 at 8:18 PM, Juan Quintela <quintela@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
>> diff --git a/block.c b/block.c
>> index 8f224b4..89f6ded 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1153,14 +1153,12 @@ int64_t bdrv_getlength(BlockDriverState *bs)
>>      if (!drv)
>>          return -ENOMEDIUM;
>>
>> -    /* Fixed size devices use the total_sectors value for speed instead of
>> -       issuing a length query (like lseek) on each call.  Also, legacy block
>> -       drivers don't provide a bdrv_getlength function and must use
>> -       total_sectors. */
>> -    if (!bs->growable || !drv->bdrv_getlength) {
>
>       if (!bs->growable || !bs->removable|| !drv->bdrv_getlength) {
>
> changing just the test don't give exactly the same result?

I didn't like the inverted logic.  I think it's clearer this way.

Stefan

Re: [Qemu-devel] Re: [PATCH 3/3] raw-posix: Re-open host CD-ROM after media change

[Stefan Hajnoczi]

On Wed, Mar 23, 2011 at 8:27 PM, Juan Quintela <quintela@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
>> +    /*
>> +     * Opening and closing on each drive status check ensures the medium size
>> +     * is refreshed.  If the file descriptor is kept open the size can become
>> +     * stale.  This is essentially replicating CD-ROM polling but is driven by
>> +     * the guest.  As the guest polls, we poll the host.
>> +     */
>
> Comment confused me :p
>
> we are not opening and closing at each status check.
> We are opening if the cdrom is _not_ opened.  And we are closing if
> there is one error.  If comment 2 above is right, you need to do
> insteand something like:
>
>        if (s->fd != -1) {
>           close(s->fd);
>        }
>
>        s->fd = open( ....);
>
> That is really reopening all the times.

You're right, I'll fix the comment.  It should only close/reopen if
there is no medium present.  If there is already a medium then we're
good.

>> +
>> +    if (s->fd == -1) {
>> +        s->fd = qemu_open(bs->filename, s->open_flags, 0644);
>
> Everything else on that file uses plain "open" not "qemu_open".
> diference is basically that qemu_open() adds flag O_CLOEXEC.
>
> I don't know if this one should be vanilla open or the other ones
> qemu_open().
>
> What do you think?

raw_open_common() uses qemu_open().  That's why I used it.

>> +        if (s->fd < 0) {
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK);
>
> parens are not needed around ==.

Yes, if you want I'll remove them.  I just did it for readability.

Stefan

Re: [Qemu-devel] Re: [PATCH 3/3] raw-posix: Re-open host CD-ROM after media change

[Kevin Wolf]

Am 23.03.2011 21:50, schrieb Stefan Hajnoczi:
> On Wed, Mar 23, 2011 at 8:27 PM, Juan Quintela <quintela@redhat.com> wrote:
>> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
>>> +
>>> +    if (s->fd == -1) {
>>> +        s->fd = qemu_open(bs->filename, s->open_flags, 0644);
>>
>> Everything else on that file uses plain "open" not "qemu_open".
>> diference is basically that qemu_open() adds flag O_CLOEXEC.
>>
>> I don't know if this one should be vanilla open or the other ones
>> qemu_open().
>>
>> What do you think?
> 
> raw_open_common() uses qemu_open().  That's why I used it.

And I think it's correct. There's no reason not to set O_CLOEXEC here.
Maybe some of the open() users need to be fixed.

>>> +        if (s->fd < 0) {
>>> +            return 0;
>>> +        }
>>> +    }
>>> +
>>> +    ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK);
>>
>> parens are not needed around ==.
> 
> Yes, if you want I'll remove them.  I just did it for readability.

I like them.

Kevin

Re: [Qemu-devel] Re: [PATCH 3/3] raw-posix: Re-open host CD-ROM after media change

[Stefan Hajnoczi]

On Thu, Mar 24, 2011 at 12:42 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 23.03.2011 21:50, schrieb Stefan Hajnoczi:
>> On Wed, Mar 23, 2011 at 8:27 PM, Juan Quintela <quintela@redhat.com> wrote:
>>> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
>>>> +
>>>> +    if (s->fd == -1) {
>>>> +        s->fd = qemu_open(bs->filename, s->open_flags, 0644);
>>>
>>> Everything else on that file uses plain "open" not "qemu_open".
>>> diference is basically that qemu_open() adds flag O_CLOEXEC.
>>>
>>> I don't know if this one should be vanilla open or the other ones
>>> qemu_open().
>>>
>>> What do you think?
>>
>> raw_open_common() uses qemu_open().  That's why I used it.
>
> And I think it's correct. There's no reason not to set O_CLOEXEC here.
> Maybe some of the open() users need to be fixed.
>
>>>> +        if (s->fd < 0) {
>>>> +            return 0;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK);
>>>
>>> parens are not needed around ==.
>>
>> Yes, if you want I'll remove them.  I just did it for readability.
>
> I like them.

I will have to #ifdef QUINTELA and #ifdef KWOLF :).  Seriously, I'll
leave the braces unless anyone feels really strongly either way.  This
passes checkpatch.pl BTW.

Stefan

[Qemu-devel] Re: [PATCH 3/3] raw-posix: Re-open host CD-ROM after media change

[Juan Quintela]

Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Mar 24, 2011 at 12:42 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 23.03.2011 21:50, schrieb Stefan Hajnoczi:
>>> On Wed, Mar 23, 2011 at 8:27 PM, Juan Quintela <quintela@redhat.com> wrote:
>>>> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
>>>>> +
>>>>> +    if (s->fd == -1) {
>>>>> +        s->fd = qemu_open(bs->filename, s->open_flags, 0644);
>>>>
>>>> Everything else on that file uses plain "open" not "qemu_open".
>>>> diference is basically that qemu_open() adds flag O_CLOEXEC.
>>>>
>>>> I don't know if this one should be vanilla open or the other ones
>>>> qemu_open().
>>>>
>>>> What do you think?
>>>
>>> raw_open_common() uses qemu_open().  That's why I used it.
>>
>> And I think it's correct. There's no reason not to set O_CLOEXEC here.
>> Maybe some of the open() users need to be fixed.

I didn't doubt that.  What I tried to point is that there are three
opens for cdrom/floppy on that file.  It makes sense that all are the
same.  I guessed that proper fix was to change all others to
qemu_open(), but just wanted to point that it was inconsistent, and
should be done one or other way.

>>>>> +        if (s->fd < 0) {
>>>>> +            return 0;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK);
>>>>
>>>> parens are not needed around ==.
>>>
>>> Yes, if you want I'll remove them.  I just did it for readability.
>>
>> I like them.
>
> I will have to #ifdef QUINTELA and #ifdef KWOLF :).  Seriously, I'll
> leave the braces unless anyone feels really strongly either way.  This
> passes checkpatch.pl BTW.

In

x = (a == b);

braces are useless.  But my preference is not 'strong' enough to try to
force it on other people.

It appears that other people feel strong about it, so let it be.

Later, Juan.