After the first GET_EVENT_STATUS_NOTIFICATION command, any new media
notification is reset by the device. The following is then noticed:
1. insert a CD of a particular size
2. mount it
3. note /sys/block/sr0/size
4. unmount cd
5. replace cd with a size greater than previous one
6. mount it
7. /sys/block/sr0/size isn't updated
8. copy all files from cd to somewhere; IO errors will pop up where the
files lie beyond previous CD's geometry
The cause is:
cdrom_open()
open_for_data()
cdo->drive_status() = sr_drive_status()
cdrom_get_media_event()
--> GPCMD_GET_EVENT_STATUS_NOTIFICATION
--> med.media_present is true, return CDS_DISK_OK
(success)
check_disk_change()
... -> 2nd call to GPCMD_GET_EVENT_STATUS_NOTIFICATION
at this point the device has already reset the new media event and the
call to revalidate_disk() in check_disk_change() is never made.
All of this is noticed in a qemu-kvm virtual machine where two CD images
are created from two files different in size.
CC: Jens Axboe <[email protected]>
CC: "James E.J. Bottomley" <[email protected]>
CC: [email protected]
CC: Markus Armbruster <[email protected]>
CC: Stefan Hajnoczi <[email protected]>
Signed-off-by: Amit Shah <[email protected]>
---
Needless to say this is based on my limited research of the flow of
data and events. I think sr_ioctl is the best place to handle this
revalidation as that's where the information gets lost. If there's a
better or a more generic place where this needs to be done, please
point me to it.
Also, qemu upstream doesn't yet support GET_EVENT_STATUS_NOTIFICATION,
I have a scratch implementation that I'm testing against.
drivers/scsi/sr_ioctl.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index 8be3055..0651448 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -316,12 +316,19 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)
return CDS_DRIVE_NOT_READY;
if (!cdrom_get_media_event(cdi, &med)) {
- if (med.media_present)
+ if (med.media_present) {
+ /*
+ * New media was inserted; ensure disk data is
+ * revalidated.
+ */
+ if (cdi->disk->fops->revalidate_disk)
+ cdi->disk->fops->revalidate_disk(cdi->disk);
return CDS_DISC_OK;
- else if (med.door_open)
+ } else if (med.door_open) {
return CDS_TRAY_OPEN;
- else
+ } else {
return CDS_NO_DISC;
+ }
}
/*
--
1.7.4
Hello,
On Thu, Mar 31, 2011 at 11:50:04PM +0530, Amit Shah wrote:
> After the first GET_EVENT_STATUS_NOTIFICATION command, any new media
> notification is reset by the device. The following is then noticed:
>
> 1. insert a CD of a particular size
> 2. mount it
> 3. note /sys/block/sr0/size
> 4. unmount cd
> 5. replace cd with a size greater than previous one
> 6. mount it
> 7. /sys/block/sr0/size isn't updated
> 8. copy all files from cd to somewhere; IO errors will pop up where the
> files lie beyond previous CD's geometry
>
> The cause is:
>
> cdrom_open()
> open_for_data()
> cdo->drive_status() = sr_drive_status()
> cdrom_get_media_event()
> --> GPCMD_GET_EVENT_STATUS_NOTIFICATION
> --> med.media_present is true, return CDS_DISK_OK
> (success)
> check_disk_change()
> ... -> 2nd call to GPCMD_GET_EVENT_STATUS_NOTIFICATION
>
> at this point the device has already reset the new media event and the
> call to revalidate_disk() in check_disk_change() is never made.
Hmm... I see. That's something I didn't expect.
> All of this is noticed in a qemu-kvm virtual machine where two CD images
> are created from two files different in size.
...
> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> index 8be3055..0651448 100644
> --- a/drivers/scsi/sr_ioctl.c
> +++ b/drivers/scsi/sr_ioctl.c
> @@ -316,12 +316,19 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)
> return CDS_DRIVE_NOT_READY;
>
> if (!cdrom_get_media_event(cdi, &med)) {
> - if (med.media_present)
> + if (med.media_present) {
> + /*
> + * New media was inserted; ensure disk data is
> + * revalidated.
> + */
> + if (cdi->disk->fops->revalidate_disk)
> + cdi->disk->fops->revalidate_disk(cdi->disk);
> return CDS_DISC_OK;
> - else if (med.door_open)
> + } else if (med.door_open) {
> return CDS_TRAY_OPEN;
> - else
> + } else {
> return CDS_NO_DISC;
> + }
> }
But I don't think this is the correct place to do it. The problem
happens because block layer consumes the event but doesn't remember it
when the time for revalidation comes. It should be done by block
layer, not sr. Hmmm... looking at the code, the new disk event code
should handle this correctly. Was 2.6.38 showing the problem too?
Thanks.
--
tejun
On (Fri) 01 Apr 2011 [17:43:27], Tejun Heo wrote:
> Hello,
>
> On Thu, Mar 31, 2011 at 11:50:04PM +0530, Amit Shah wrote:
> > After the first GET_EVENT_STATUS_NOTIFICATION command, any new media
> > notification is reset by the device. The following is then noticed:
> >
> > 1. insert a CD of a particular size
> > 2. mount it
> > 3. note /sys/block/sr0/size
> > 4. unmount cd
> > 5. replace cd with a size greater than previous one
> > 6. mount it
> > 7. /sys/block/sr0/size isn't updated
> > 8. copy all files from cd to somewhere; IO errors will pop up where the
> > files lie beyond previous CD's geometry
> >
> > The cause is:
> >
> > cdrom_open()
> > open_for_data()
> > cdo->drive_status() = sr_drive_status()
> > cdrom_get_media_event()
> > --> GPCMD_GET_EVENT_STATUS_NOTIFICATION
> > --> med.media_present is true, return CDS_DISK_OK
> > (success)
> > check_disk_change()
> > ... -> 2nd call to GPCMD_GET_EVENT_STATUS_NOTIFICATION
> >
> > at this point the device has already reset the new media event and the
> > call to revalidate_disk() in check_disk_change() is never made.
>
> Hmm... I see. That's something I didn't expect.
>
> > All of this is noticed in a qemu-kvm virtual machine where two CD images
> > are created from two files different in size.
> ...
> > diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> > index 8be3055..0651448 100644
> > --- a/drivers/scsi/sr_ioctl.c
> > +++ b/drivers/scsi/sr_ioctl.c
> > @@ -316,12 +316,19 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)
> > return CDS_DRIVE_NOT_READY;
> >
> > if (!cdrom_get_media_event(cdi, &med)) {
> > - if (med.media_present)
> > + if (med.media_present) {
> > + /*
> > + * New media was inserted; ensure disk data is
> > + * revalidated.
> > + */
> > + if (cdi->disk->fops->revalidate_disk)
> > + cdi->disk->fops->revalidate_disk(cdi->disk);
> > return CDS_DISC_OK;
> > - else if (med.door_open)
> > + } else if (med.door_open) {
> > return CDS_TRAY_OPEN;
> > - else
> > + } else {
> > return CDS_NO_DISC;
> > + }
> > }
>
> But I don't think this is the correct place to do it. The problem
> happens because block layer consumes the event but doesn't remember it
> when the time for revalidation comes. It should be done by block
> layer, not sr. Hmmm... looking at the code, the new disk event code
> should handle this correctly. Was 2.6.38 showing the problem too?
Yes, 2.6.38 shows the same problem. I went back to ancient kernels
(2.6.31 on Fedora 11-alpha) which had the previous media_changed
infrastructure and those places too show the same behaviour (with the
TEST_UNIT_READY way of detecting media changes).
Amit
On (Fri) 01 Apr 2011 [17:43:27], Tejun Heo wrote:
> Hello,
>
> On Thu, Mar 31, 2011 at 11:50:04PM +0530, Amit Shah wrote:
> > After the first GET_EVENT_STATUS_NOTIFICATION command, any new media
> > notification is reset by the device. The following is then noticed:
> >
> > 1. insert a CD of a particular size
> > 2. mount it
> > 3. note /sys/block/sr0/size
> > 4. unmount cd
> > 5. replace cd with a size greater than previous one
> > 6. mount it
> > 7. /sys/block/sr0/size isn't updated
> > 8. copy all files from cd to somewhere; IO errors will pop up where the
> > files lie beyond previous CD's geometry
> >
> > The cause is:
> >
> > cdrom_open()
> > open_for_data()
> > cdo->drive_status() = sr_drive_status()
> > cdrom_get_media_event()
> > --> GPCMD_GET_EVENT_STATUS_NOTIFICATION
> > --> med.media_present is true, return CDS_DISK_OK
> > (success)
> > check_disk_change()
> > ... -> 2nd call to GPCMD_GET_EVENT_STATUS_NOTIFICATION
> >
> > at this point the device has already reset the new media event and the
> > call to revalidate_disk() in check_disk_change() is never made.
>
> Hmm... I see. That's something I didn't expect.
...
> But I don't think this is the correct place to do it. The problem
> happens because block layer consumes the event but doesn't remember it
> when the time for revalidation comes. It should be done by block
> layer, not sr.
Hm, only the open call at the start of the call sequence is involved
from the block layer. How to pass on such a pending event back? Some
kind of an event array with helpers to modify it?
Amit
On Tue, Apr 05, 2011 at 12:21:30PM +0530, Amit Shah wrote:
> > But I don't think this is the correct place to do it. The problem
> > happens because block layer consumes the event but doesn't remember it
> > when the time for revalidation comes. It should be done by block
> > layer, not sr. Hmmm... looking at the code, the new disk event code
> > should handle this correctly. Was 2.6.38 showing the problem too?
>
> Yes, 2.6.38 shows the same problem. I went back to ancient kernels
> (2.6.31 on Fedora 11-alpha) which had the previous media_changed
> infrastructure and those places too show the same behaviour (with the
> TEST_UNIT_READY way of detecting media changes).
Can you please the patch attached in the following bz and see whether
it makes any difference?
https://bugzilla.kernel.org/show_bug.cgi?id=13029
Thanks.
--
tejun
On (Wed) 06 Apr 2011 [03:06:20], Tejun Heo wrote:
> On Tue, Apr 05, 2011 at 12:21:30PM +0530, Amit Shah wrote:
> > > But I don't think this is the correct place to do it. The problem
> > > happens because block layer consumes the event but doesn't remember it
> > > when the time for revalidation comes. It should be done by block
> > > layer, not sr. Hmmm... looking at the code, the new disk event code
> > > should handle this correctly. Was 2.6.38 showing the problem too?
> >
> > Yes, 2.6.38 shows the same problem. I went back to ancient kernels
> > (2.6.31 on Fedora 11-alpha) which had the previous media_changed
> > infrastructure and those places too show the same behaviour (with the
> > TEST_UNIT_READY way of detecting media changes).
>
> Can you please the patch attached in the following bz and see whether
> it makes any difference?
>
> https://bugzilla.kernel.org/show_bug.cgi?id=13029
Yes, this works.
You can add Tested-by: Amit Shah <[email protected]>
Thanks!
Amit
On Wed, Apr 6, 2011 at 11:06 AM, Tejun Heo <[email protected]> wrote:
> On Tue, Apr 05, 2011 at 12:21:30PM +0530, Amit Shah wrote:
>> > But I don't think this is the correct place to do it. ?The problem
>> > happens because block layer consumes the event but doesn't remember it
>> > when the time for revalidation comes. ?It should be done by block
>> > layer, not sr. ?Hmmm... looking at the code, the new disk event code
>> > should handle this correctly. ?Was 2.6.38 showing the problem too?
>>
>> Yes, 2.6.38 shows the same problem. ?I went back to ancient kernels
>> (2.6.31 on Fedora 11-alpha) which had the previous media_changed
>> infrastructure and those places too show the same behaviour (with the
>> TEST_UNIT_READY way of detecting media changes).
>
> Can you please the patch attached in the following bz and see whether
> it makes any difference?
>
> ?https://bugzilla.kernel.org/show_bug.cgi?id=13029
Hi Tejun,
There is a related issue I have been discussing with Amit:
https://lkml.org/lkml/2011/3/23/156
On media change the inode size is not updated by the sr driver or the
universal cdrom driver. A userspace process that holds a /dev/sr0
file descriptor open across media change causes all processes on the
system to see the old medium size when they do lseek(fd, 0, SEEK_END).
I think it would make sense to refresh the inode size on media change
so that even open file descriptors see the new size and a single
process cannot force a stale value for all other userspace processes
on the system.
Thoughts?
Stefan
Hello,
On Fri, Apr 08, 2011 at 12:37:56PM +0100, Stefan Hajnoczi wrote:
> There is a related issue I have been discussing with Amit:
> https://lkml.org/lkml/2011/3/23/156
>
> On media change the inode size is not updated by the sr driver or the
> universal cdrom driver. A userspace process that holds a /dev/sr0
> file descriptor open across media change causes all processes on the
> system to see the old medium size when they do lseek(fd, 0, SEEK_END).
>
> I think it would make sense to refresh the inode size on media change
> so that even open file descriptors see the new size and a single
> process cannot force a stale value for all other userspace processes
> on the system.
Hmmm... I don't know. Maybe we can but I'm not sure whether there's a
good reason for it. cdrom is locked while opened after all. Are
there actual problems?
Thanks.
--
tejun
On Fri, Apr 8, 2011 at 5:20 PM, Tejun Heo <[email protected]> wrote:
> Hello,
>
> On Fri, Apr 08, 2011 at 12:37:56PM +0100, Stefan Hajnoczi wrote:
>> There is a related issue I have been discussing with Amit:
>> https://lkml.org/lkml/2011/3/23/156
>>
>> On media change the inode size is not updated by the sr driver or the
>> universal cdrom driver. ?A userspace process that holds a /dev/sr0
>> file descriptor open across media change causes all processes on the
>> system to see the old medium size when they do lseek(fd, 0, SEEK_END).
>>
>> I think it would make sense to refresh the inode size on media change
>> so that even open file descriptors see the new size and a single
>> process cannot force a stale value for all other userspace processes
>> on the system.
>
> Hmmm... I don't know. ?Maybe we can but I'm not sure whether there's a
> good reason for it. ?cdrom is locked while opened after all. ?Are
> there actual problems?
Yeah, sorry I didn't explain what the use case was. With QEMU you can
pass through the physical CD-ROM into the virtual machine.
QEMU opens /dev/cdrom with O_NONBLOCK | O_RDONLY. The guest can test
if the medium is present and QEMU will do ioctl(fd,
CDROM_DRIVE_STATUS, CDSL_CURRENT). The guest can also lock the tray
and eject, again using the respective ioctls. Read operations are
serviced by performing a read on the file descriptor in QEMU. And
finally the medium size is queried by QEMU using lseek(fd, 0,
SEEK_END).
Today QEMU cannot keep /dev/cdrom open across media change because it
will have an outdated inode size returned from lseek(fd, 0, SEEK_END).
But if the cdrom driver (or sr) refresh the inode size on media
change then there is no need to work around this from userspace.
Stefan
Hello,
On Fri, Apr 08, 2011 at 05:43:16PM +0100, Stefan Hajnoczi wrote:
> >> I think it would make sense to refresh the inode size on media change
> >> so that even open file descriptors see the new size and a single
> >> process cannot force a stale value for all other userspace processes
> >> on the system.
> >
> > Hmmm... I don't know. ?Maybe we can but I'm not sure whether there's a
> > good reason for it. ?cdrom is locked while opened after all. ?Are
> > there actual problems?
>
> Yeah, sorry I didn't explain what the use case was. With QEMU you can
> pass through the physical CD-ROM into the virtual machine.
>
> QEMU opens /dev/cdrom with O_NONBLOCK | O_RDONLY. The guest can test
> if the medium is present and QEMU will do ioctl(fd,
> CDROM_DRIVE_STATUS, CDSL_CURRENT). The guest can also lock the tray
> and eject, again using the respective ioctls. Read operations are
> serviced by performing a read on the file descriptor in QEMU. And
> finally the medium size is queried by QEMU using lseek(fd, 0,
> SEEK_END).
>
> Today QEMU cannot keep /dev/cdrom open across media change because it
> will have an outdated inode size returned from lseek(fd, 0, SEEK_END).
> But if the cdrom driver (or sr) refresh the inode size on media
> change then there is no need to work around this from userspace.
Hmmm... ISTR there was some discussion about changing inode size on
the fly quite a while ago. I didn't follow the discussion but it
seemed to have rather nasty/delicate implications.
Jens, any ideas?
Thanks.
--
tejun
On (Fri) 08 Apr 2011 [09:52:07], Tejun Heo wrote:
> Hello,
>
> On Fri, Apr 08, 2011 at 05:43:16PM +0100, Stefan Hajnoczi wrote:
> > >> I think it would make sense to refresh the inode size on media change
> > >> so that even open file descriptors see the new size and a single
> > >> process cannot force a stale value for all other userspace processes
> > >> on the system.
> > >
> > > Hmmm... I don't know. ?Maybe we can but I'm not sure whether there's a
> > > good reason for it. ?cdrom is locked while opened after all. ?Are
> > > there actual problems?
> >
> > Yeah, sorry I didn't explain what the use case was. With QEMU you can
> > pass through the physical CD-ROM into the virtual machine.
> >
> > QEMU opens /dev/cdrom with O_NONBLOCK | O_RDONLY. The guest can test
> > if the medium is present and QEMU will do ioctl(fd,
> > CDROM_DRIVE_STATUS, CDSL_CURRENT). The guest can also lock the tray
> > and eject, again using the respective ioctls. Read operations are
> > serviced by performing a read on the file descriptor in QEMU. And
> > finally the medium size is queried by QEMU using lseek(fd, 0,
> > SEEK_END).
> >
> > Today QEMU cannot keep /dev/cdrom open across media change because it
> > will have an outdated inode size returned from lseek(fd, 0, SEEK_END).
> > But if the cdrom driver (or sr) refresh the inode size on media
> > change then there is no need to work around this from userspace.
>
> Hmmm... ISTR there was some discussion about changing inode size on
> the fly quite a while ago. I didn't follow the discussion but it
> seemed to have rather nasty/delicate implications.
I don't necessarily agree with having to modify inode sizes on the
fly, but the main bug here is that the inode doesn't get invalidated
if a CDROM is ejected while a process has an fd to the CDROM device
opened. So as in the original case, if a CD is swapped with one
having more data, lseek continues to report the original media's size
in the process that keeps the fd open across eject/insert.
I haven't tried this on physical systems, so can't count out it being
a qemu bug as well.
Amit
On Tue, Apr 12, 2011 at 5:51 AM, Amit Shah <[email protected]> wrote:
> On (Fri) 08 Apr 2011 [09:52:07], Tejun Heo wrote:
>> Hello,
>>
>> On Fri, Apr 08, 2011 at 05:43:16PM +0100, Stefan Hajnoczi wrote:
>> > >> I think it would make sense to refresh the inode size on media change
>> > >> so that even open file descriptors see the new size and a single
>> > >> process cannot force a stale value for all other userspace processes
>> > >> on the system.
>> > >
>> > > Hmmm... I don't know. ?Maybe we can but I'm not sure whether there's a
>> > > good reason for it. ?cdrom is locked while opened after all. ?Are
>> > > there actual problems?
>> >
>> > Yeah, sorry I didn't explain what the use case was. ?With QEMU you can
>> > pass through the physical CD-ROM into the virtual machine.
>> >
>> > QEMU opens /dev/cdrom with O_NONBLOCK | O_RDONLY. ?The guest can test
>> > if the medium is present and QEMU will do ioctl(fd,
>> > CDROM_DRIVE_STATUS, CDSL_CURRENT). ?The guest can also lock the tray
>> > and eject, again using the respective ioctls. ?Read operations are
>> > serviced by performing a read on the file descriptor in QEMU. ?And
>> > finally the medium size is queried by QEMU using lseek(fd, 0,
>> > SEEK_END).
>> >
>> > Today QEMU cannot keep /dev/cdrom open across media change because it
>> > will have an outdated inode size returned from lseek(fd, 0, SEEK_END).
>> > ?But if the cdrom driver (or sr) refresh the inode size on media
>> > change then there is no need to work around this from userspace.
>>
>> Hmmm... ISTR there was some discussion about changing inode size on
>> the fly quite a while ago. ?I didn't follow the discussion but it
>> seemed to have rather nasty/delicate implications.
>
> I don't necessarily agree with having to modify inode sizes on the
> fly, but the main bug here is that the inode doesn't get invalidated
> if a CDROM is ejected while a process has an fd to the CDROM device
> opened. ?So as in the original case, if a CD is swapped with one
> having more data, lseek continues to report the original media's size
> in the process that keeps the fd open across eject/insert.
>
> I haven't tried this on physical systems, so can't count out it being
> a qemu bug as well.
Amit,
This sounds exactly like the bug that I described and it happens on
bare metal without virtualization.
Stefan
On (Tue) 12 Apr 2011 [09:16:41], Stefan Hajnoczi wrote:
> On Tue, Apr 12, 2011 at 5:51 AM, Amit Shah <[email protected]> wrote:
> > On (Fri) 08 Apr 2011 [09:52:07], Tejun Heo wrote:
> >> Hello,
> >>
> >> On Fri, Apr 08, 2011 at 05:43:16PM +0100, Stefan Hajnoczi wrote:
> >> > >> I think it would make sense to refresh the inode size on media change
> >> > >> so that even open file descriptors see the new size and a single
> >> > >> process cannot force a stale value for all other userspace processes
> >> > >> on the system.
> >> > >
> >> > > Hmmm... I don't know. ?Maybe we can but I'm not sure whether there's a
> >> > > good reason for it. ?cdrom is locked while opened after all. ?Are
> >> > > there actual problems?
> >> >
> >> > Yeah, sorry I didn't explain what the use case was. ?With QEMU you can
> >> > pass through the physical CD-ROM into the virtual machine.
> >> >
> >> > QEMU opens /dev/cdrom with O_NONBLOCK | O_RDONLY. ?The guest can test
> >> > if the medium is present and QEMU will do ioctl(fd,
> >> > CDROM_DRIVE_STATUS, CDSL_CURRENT). ?The guest can also lock the tray
> >> > and eject, again using the respective ioctls. ?Read operations are
> >> > serviced by performing a read on the file descriptor in QEMU. ?And
> >> > finally the medium size is queried by QEMU using lseek(fd, 0,
> >> > SEEK_END).
> >> >
> >> > Today QEMU cannot keep /dev/cdrom open across media change because it
> >> > will have an outdated inode size returned from lseek(fd, 0, SEEK_END).
> >> > ?But if the cdrom driver (or sr) refresh the inode size on media
> >> > change then there is no need to work around this from userspace.
> >>
> >> Hmmm... ISTR there was some discussion about changing inode size on
> >> the fly quite a while ago. ?I didn't follow the discussion but it
> >> seemed to have rather nasty/delicate implications.
> >
> > I don't necessarily agree with having to modify inode sizes on the
> > fly, but the main bug here is that the inode doesn't get invalidated
> > if a CDROM is ejected while a process has an fd to the CDROM device
> > opened. ?So as in the original case, if a CD is swapped with one
> > having more data, lseek continues to report the original media's size
> > in the process that keeps the fd open across eject/insert.
> >
> > I haven't tried this on physical systems, so can't count out it being
> > a qemu bug as well.
>
> Amit,
> This sounds exactly like the bug that I described and it happens on
> bare metal without virtualization.
Oh OK -- just wanted to get the qemu passthrough out of the equation.
Thanks.
Amit