[REGRESSION] cdrom drive doesn't detect removal

[REGRESSION] cdrom drive doesn't detect removal

[Maxim Levitsky]

Hi,

After a switch between hal to devkit stack, a different strategy of
detecting a cdrom removal was applied.
Instead of polling it, userspace just tells the kernel not to lock the
dour (/proc/sys/dev/cdrom/lock) and as soon as user removes the disk,
udev notifies the userspace and it unmounts it. Since CDROMs are
readonly this is perfectly safe and fits the same procedure used for all
other removable disks. (usb, flash cards, etc..)

(Well, most cdroms aren't really read-only these days, but state of
packet writing is so sad these days that is doesn't matter).

However 2.6.36 doesn't detect that removal.
According to udevadm monitor --property no uevents are send on removal.

My feeling is that this is BKL fallout.
I of course use new libata stack, so my cdrom driver is sr.



Best regards,
Maxim Levitsky

Re: [REGRESSION] cdrom drive doesn't detect removal

[Maxim Levitsky]

On Sun, 2010-09-12 at 11:49 +0200, Maxim Levitsky wrote: 
> Hi,
> 
> After a switch between hal to devkit stack, a different strategy of
> detecting a cdrom removal was applied.
> Instead of polling it, userspace just tells the kernel not to lock the
> dour (/proc/sys/dev/cdrom/lock) and as soon as user removes the disk,
> udev notifies the userspace and it unmounts it. Since CDROMs are
> readonly this is perfectly safe and fits the same procedure used for all
> other removable disks. (usb, flash cards, etc..)
> 
> (Well, most cdroms aren't really read-only these days, but state of
> packet writing is so sad these days that is doesn't matter).
> 
> However 2.6.36 doesn't detect that removal.
> According to udevadm monitor --property no uevents are send on removal.
Correction, this is regression between 2.6.34 and 2.6.35. This shows how
much I use cd these days...


I bisected it down to this:

6b4517a7913a09d3259bb1d21c9cb300f12294bd is the first bad commit
commit 6b4517a7913a09d3259bb1d21c9cb300f12294bd
Author: Tejun Heo <tj@kernel.org>
Date:   Wed Apr 7 18:53:59 2010 +0900

    block: implement bd_claiming and claiming block
    
    Currently, device claiming for exclusive open is done after low level
    open - disk->fops->open() - has completed successfully.  This means
    that exclusive open attempts while a device is already exclusively
    open will fail only after disk->fops->open() is called.
    
    cdrom driver issues commands during open() which means that O_EXCL
    open attempt can unintentionally inject commands to in-progress
    command stream for burning thus disturbing burning process.  In most
    cases, this doesn't cause problems because the first command to be
    issued is TUR which most devices can process in the middle of burning.
    However, depending on how a device replies to TUR during burning,
    cdrom driver may end up issuing further commands.
    
    This can't be resolved trivially by moving bd_claim() before doing
    actual open() because that means an open attempt which will end up
    failing could interfere other legit O_EXCL open attempts.
    ie. unconfirmed open attempts can fail others.
    
    This patch resolves the problem by introducing claiming block which is
    started by bd_start_claiming() and terminated either by bd_claim() or
    bd_abort_claiming().  bd_claim() from inside a claiming block is
    guaranteed to succeed and once a claiming block is started, other
    bd_start_claiming() or bd_claim() attempts block till the current
    claiming block is terminated.
    
    bd_claim() can still be used standalone although now it always
    synchronizes against claiming blocks, so the existing users will keep
    working without any change.
    
    blkdev_open() and open_bdev_exclusive() are converted to use claiming
    blocks so that exclusive open attempts from these functions don't
    interfere with the existing exclusive open.
    
    This problem was discovered while investigating bko#15403.
    
      https://bugzilla.kernel.org/show_bug.cgi?id=15403
    
    The burning problem itself can be resolved by updating userspace
    probing tools to always open w/ O_EXCL.
    
    Signed-off-by: Tejun Heo <tj@kernel.org>
    Reported-by: Matthias-Christian Ott <ott@mirix.org>
    Cc: Kay Sievers <kay.sievers@vrfy.org>
    Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

The bisect log:


maxim@maxim-laptop:~/software/kernel/linux-2.6$ git bisect log
git bisect start
# bad: [9fe6206f400646a2322096b56c59891d530e8d51] Linux 2.6.35
git bisect bad 9fe6206f400646a2322096b56c59891d530e8d51
# good: [e40152ee1e1c7a63f4777791863215e3faa37a86] Linus 2.6.34
git bisect good e40152ee1e1c7a63f4777791863215e3faa37a86
# good: [e0bc5d4a54938eedcde14005210e6c08aa9727e4] Merge branch 'i2c-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging
git bisect good e0bc5d4a54938eedcde14005210e6c08aa9727e4
# bad: [d30fda355188272430d3865db2ff9e24b4135ae3] posix-cpu-timers: avoid "task->signal != NULL" checks
git bisect bad d30fda355188272430d3865db2ff9e24b4135ae3
# bad: [d79df0b1eda0099a22cbcece01ce5e7d222450de] Merge git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6
git bisect bad d79df0b1eda0099a22cbcece01ce5e7d222450de
# good: [6969a434737dd82f7343e3fcd529bc320508d9fc] Merge branch 'upstream' of git://ftp.linux-mips.org/pub/scm/upstream-linus
git bisect good 6969a434737dd82f7343e3fcd529bc320508d9fc
# good: [e34d2c5fa2254197b0a01925cc6f77e12552f9b9] staging:iio: ABI documentation (partial)
git bisect good e34d2c5fa2254197b0a01925cc6f77e12552f9b9
# good: [7fb794b32cbd7e97e37628cb67279b86c1436e84] Staging: vt6655: remove unused SUCCESS definition
git bisect good 7fb794b32cbd7e97e37628cb67279b86c1436e84
# bad: [fc8ce1941d668c70e57a07f13f5a63e73e5dbff3] drbd: Fix: Do not detach, if a bio with a barrier fails
git bisect bad fc8ce1941d668c70e57a07f13f5a63e73e5dbff3
# bad: [8d4ce82b3ccd755c8ba401469ced5286b1e02284] drbd: don't start a resync without access to up-to-date Data
git bisect bad 8d4ce82b3ccd755c8ba401469ced5286b1e02284
# good: [1a3cbbc5a5e8a66934aa0947896a4aca6fd77298] block: factor out bd_may_claim()
git bisect good 1a3cbbc5a5e8a66934aa0947896a4aca6fd77298
# bad: [0f3942a39ed768c967cb71ea0e9be7fc94112713] block: kill some useless goto's in blk-cgroup.c
git bisect bad 0f3942a39ed768c967cb71ea0e9be7fc94112713
# bad: [3f14d792f9a8fede64ce918dbb517f934497a4f8] blkdev: add blkdev_issue_zeroout helper function
git bisect bad 3f14d792f9a8fede64ce918dbb517f934497a4f8
# bad: [fbd9b09a177a481eda256447c881f014f29034fe] blkdev: generalize flags for blkdev_issue_fn functions
git bisect bad fbd9b09a177a481eda256447c881f014f29034fe
# bad: [6b4517a7913a09d3259bb1d21c9cb300f12294bd] block: implement bd_claiming and claiming block
git bisect bad 6b4517a7913a09d3259bb1d21c9cb300f12294bd


Best regards,
	Maxim Levitsky

Re: [REGRESSION] cdrom drive doesn't detect removal

[Tejun Heo]

Hello,

On 09/14/2010 03:27 AM, Maxim Levitsky wrote:
>> However 2.6.36 doesn't detect that removal.
>> According to udevadm monitor --property no uevents are send on removal.
> Correction, this is regression between 2.6.34 and 2.6.35. This shows how
> much I use cd these days...
> 
> I bisected it down to this:
> 
> 6b4517a7913a09d3259bb1d21c9cb300f12294bd is the first bad commit
> commit 6b4517a7913a09d3259bb1d21c9cb300f12294bd
> Author: Tejun Heo <tj@kernel.org>
> Date:   Wed Apr 7 18:53:59 2010 +0900
> 
>     block: implement bd_claiming and claiming block

Hmmm... weird.  This commit does change the open behavior but
shouldn't change the end result.  Can someone please enlighten me how
udevadm is interacting with the device at system call level?

Thanks.

-- 
tejun

Re: [REGRESSION] cdrom drive doesn't detect removal

[Kay Sievers]

On Tue, Sep 14, 2010 at 09:39, Tejun Heo <tj@kernel.org> wrote:
> On 09/14/2010 03:27 AM, Maxim Levitsky wrote:
>>> However 2.6.36 doesn't detect that removal.
>>> According to udevadm monitor --property no uevents are send on removal.
>> Correction, this is regression between 2.6.34 and 2.6.35. This shows how
>> much I use cd these days...
>>
>> I bisected it down to this:
>>
>> 6b4517a7913a09d3259bb1d21c9cb300f12294bd is the first bad commit
>> commit 6b4517a7913a09d3259bb1d21c9cb300f12294bd
>> Author: Tejun Heo <tj@kernel.org>
>> Date:   Wed Apr 7 18:53:59 2010 +0900
>>
>>     block: implement bd_claiming and claiming block
>
> Hmmm... weird.  This commit does change the open behavior but
> shouldn't change the end result.  Can someone please enlighten me how
> udevadm is interacting with the device at system call level?

Not at all. Udev does not really touch it.

The cdrom drive isn't unlocked on usual systems, udisks polls the
cdrom drive periodically just like HAL did. The only difference is
that with the polling, the sr driver detects a media change and sends
a uevent to udev, instead of HAL looking at the result of the open().

Are we sure, that there is something that still polls the drive?
Udisks is only auto-started, when the desktop calls into some D-Bus
methods, unlike HAL where it was an init script.

Kay

Re: [REGRESSION] cdrom drive doesn't detect removal

[Maxim Levitsky]

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

On Tue, 2010-09-14 at 10:07 +0200, Kay Sievers wrote: 
> On Tue, Sep 14, 2010 at 09:39, Tejun Heo <tj@kernel.org> wrote:
> > On 09/14/2010 03:27 AM, Maxim Levitsky wrote:
> >>> However 2.6.36 doesn't detect that removal.
> >>> According to udevadm monitor --property no uevents are send on removal.
> >> Correction, this is regression between 2.6.34 and 2.6.35. This shows how
> >> much I use cd these days...
> >>
> >> I bisected it down to this:
> >>
> >> 6b4517a7913a09d3259bb1d21c9cb300f12294bd is the first bad commit
> >> commit 6b4517a7913a09d3259bb1d21c9cb300f12294bd
> >> Author: Tejun Heo <tj@kernel.org>
> >> Date:   Wed Apr 7 18:53:59 2010 +0900
> >>
> >>     block: implement bd_claiming and claiming block
> >
> > Hmmm... weird.  This commit does change the open behavior but
> > shouldn't change the end result.  Can someone please enlighten me how
> > udevadm is interacting with the device at system call level?
> 
> Not at all. Udev does not really touch it.
> 
> The cdrom drive isn't unlocked on usual systems, udisks polls the
> cdrom drive periodically just like HAL did. The only difference is
> that with the polling, the sr driver detects a media change and sends
> a uevent to udev, instead of HAL looking at the result of the open().
> 
> Are we sure, that there is something that still polls the drive?
> Udisks is only auto-started, when the desktop calls into some D-Bus
> methods, unlike HAL where it was an init script.


Note that on current vanilla head, I applied the attached manual revert
patch, and problem disappears (and probably introduces the same bug that
patch supposed to fix).


Best regards,
Maxim Levitsky

[-- Attachment #2: test.patch --]
[-- Type: text/x-patch, Size: 2661 bytes --]

commit 6256435b81d05c98be64fbeea57776520961a10c
Author: Maxim Levitsky <maximlevitsky@gmail.com>
Date:   Tue Sep 14 23:47:14 2010 +0200

    block_dev: revert cdrom breakage

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 50e8c85..f1095f3 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -839,14 +839,15 @@ static void bd_finish_claiming(struct block_device *bdev,
 int bd_claim(struct block_device *bdev, void *holder)
 {
 	struct block_device *whole = bdev->bd_contains;
-	int res;
+	int res = -EBUSY;
 
 	might_sleep();
 
 	spin_lock(&bdev_lock);
-	res = bd_prepare_to_claim(bdev, whole, holder);
-	if (res == 0)
+	if (bd_may_claim(bdev, whole, holder)) {
+		res = 0;
 		__bd_claim(bdev, whole, holder);
+	}
 	spin_unlock(&bdev_lock);
 
 	return res;
@@ -1462,7 +1463,6 @@ EXPORT_SYMBOL(blkdev_get);
 
 static int blkdev_open(struct inode * inode, struct file * filp)
 {
-	struct block_device *whole = NULL;
 	struct block_device *bdev;
 	int res;
 
@@ -1485,24 +1485,24 @@ static int blkdev_open(struct inode * inode, struct file * filp)
 	if (bdev == NULL)
 		return -ENOMEM;
 
+	filp->f_mapping = bdev->bd_inode->i_mapping;
+
+	res = blkdev_get(bdev, filp->f_mode);
+	if (res)
+	        return res;
+ 
 	if (filp->f_mode & FMODE_EXCL) {
-		whole = bd_start_claiming(bdev, filp);
-		if (IS_ERR(whole)) {
-			bdput(bdev);
-			return PTR_ERR(whole);
-		}
+	        res = bd_claim(bdev, filp);
+	        if (res)
+	                goto out_blkdev_put;
 	}
 
-	filp->f_mapping = bdev->bd_inode->i_mapping;
+	return 0;
 
-	res = blkdev_get(bdev, filp->f_mode);
+ out_blkdev_put:
+	blkdev_put(bdev, filp->f_mode);
+        return res;
 
-	if (whole) {
-		if (res == 0)
-			bd_finish_claiming(bdev, whole, filp);
-		else
-			bd_abort_claiming(whole, filp);
-	}
 
 	return res;
 }
@@ -1712,34 +1712,30 @@ EXPORT_SYMBOL(lookup_bdev);
  */
 struct block_device *open_bdev_exclusive(const char *path, fmode_t mode, void *holder)
 {
-	struct block_device *bdev, *whole;
-	int error;
+	struct block_device *bdev;
+	int error = 0;
 
 	bdev = lookup_bdev(path);
 	if (IS_ERR(bdev))
 		return bdev;
 
-	whole = bd_start_claiming(bdev, holder);
-	if (IS_ERR(whole)) {
-		bdput(bdev);
-		return whole;
-	}
-
 	error = blkdev_get(bdev, mode);
 	if (error)
-		goto out_abort_claiming;
+		return ERR_PTR(error);
 
 	error = -EACCES;
 	if ((mode & FMODE_WRITE) && bdev_read_only(bdev))
 		goto out_blkdev_put;
+	error = bd_claim(bdev, holder);
+	if (error)
+		goto blkdev_put;
+
 
 	bd_finish_claiming(bdev, whole, holder);
 	return bdev;
 
 out_blkdev_put:
 	blkdev_put(bdev, mode);
-out_abort_claiming:
-	bd_abort_claiming(whole, holder);
 	return ERR_PTR(error);
 }
 

Re: [REGRESSION] cdrom drive doesn't detect removal

[Kay Sievers]

On Wed, Sep 15, 2010 at 01:38, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Tue, 2010-09-14 at 10:07 +0200, Kay Sievers wrote:

>> Are we sure, that there is something that still polls the drive?
>> Udisks is only auto-started, when the desktop calls into some D-Bus
>> methods, unlike HAL where it was an init script.
>
> Note that on current vanilla head, I applied the attached manual revert
> patch, and problem disappears (and probably introduces the same bug that
> patch supposed to fix).

Can you please check which process polls the device, and attach an
strace to it, to see what's going on?

If there is no such process, does "touch /dev/sr0" create the missing event?

Kay

Re: [REGRESSION] cdrom drive doesn't detect removal

[Maxim Levitsky]

On Wed, 2010-09-15 at 01:49 +0200, Kay Sievers wrote: 
> On Wed, Sep 15, 2010 at 01:38, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > On Tue, 2010-09-14 at 10:07 +0200, Kay Sievers wrote:
> 
> >> Are we sure, that there is something that still polls the drive?
> >> Udisks is only auto-started, when the desktop calls into some D-Bus
> >> methods, unlike HAL where it was an init script.
> >
> > Note that on current vanilla head, I applied the attached manual revert
> > patch, and problem disappears (and probably introduces the same bug that
> > patch supposed to fix).
> 
> Can you please check which process polls the device, and attach an
> strace to it, to see what's going on?
As far as I can tell there is no such process.
(at least sudo fuser /dev/sr0 doesn't show up anything with or without
cd in the drive)


> 
> If there is no such process, does "touch /dev/sr0" create the missing event?
Yes.

Best regards,
Maxim Levitsky

Re: [REGRESSION] cdrom drive doesn't detect removal

[Kay Sievers]

On Wed, Sep 15, 2010 at 02:37, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Wed, 2010-09-15 at 01:49 +0200, Kay Sievers wrote:
>> On Wed, Sep 15, 2010 at 01:38, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
>> > On Tue, 2010-09-14 at 10:07 +0200, Kay Sievers wrote:
>>
>> >> Are we sure, that there is something that still polls the drive?
>> >> Udisks is only auto-started, when the desktop calls into some D-Bus
>> >> methods, unlike HAL where it was an init script.
>> >
>> > Note that on current vanilla head, I applied the attached manual revert
>> > patch, and problem disappears (and probably introduces the same bug that
>> > patch supposed to fix).
>>
>> Can you please check which process polls the device, and attach an
>> strace to it, to see what's going on?
> As far as I can tell there is no such process.
> (at least sudo fuser /dev/sr0 doesn't show up anything with or without
> cd in the drive)
>>
>> If there is no such process, does "touch /dev/sr0" create the missing event?
> Yes.

Hmm, strange, I really have no idea why the old kernel works
differently for you.

The behavior you describe for the current kernel seems correct and is
the expected one. If there is nothing in userspace that polls
periodically, than we can't expect any event for a media change.

It's only the polling that should be able to trigger these events for
the device. the device itself can't report any changes to the host,
unless you have a SATA drive which can send AN (async notification)
events. But we disabled the AN support recently, because it was
running in loops on some boxes.

Kay

Re: [REGRESSION] cdrom drive doesn't detect removal

[Henrique de Moraes Holschuh]

On Wed, 15 Sep 2010, Kay Sievers wrote:
> It's only the polling that should be able to trigger these events for
> the device. the device itself can't report any changes to the host,
> unless you have a SATA drive which can send AN (async notification)
> events. But we disabled the AN support recently, because it was
> running in loops on some boxes.

Is it so widespread or extremely difficult to detect, that it justifies
breaking AN for all users?

Anyway, if userspace thinks AN is available, it won't pool or touch
/dev/sr*, which would explain the problem (but one still would have to find
the root cause: why userspace thinks it doesn't need to pool?).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [REGRESSION] cdrom drive doesn't detect removal

[Kay Sievers]

On Wed, Sep 15, 2010 at 15:27, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Wed, 15 Sep 2010, Kay Sievers wrote:
>> It's only the polling that should be able to trigger these events for
>> the device. the device itself can't report any changes to the host,
>> unless you have a SATA drive which can send AN (async notification)
>> events. But we disabled the AN support recently, because it was
>> running in loops on some boxes.
>
> Is it so widespread or extremely difficult to detect, that it justifies
> breaking AN for all users?

Yes. It's not properly implemented, and many drives just always send
an event on access, regardless if the media is changed or not. To work
properly, it will need in-kernel media presence polling, which is
being worked on. The current AN events, the stuff the drives send, and
the way it is handled in the kernel, is just not usable.

> Anyway, if userspace thinks AN is available, it won't pool or touch
> /dev/sr*, which would explain the problem (but one still would have to find
> the root cause: why userspace thinks it doesn't need to pool?).

There is no userspace checking for AN support right now. It wouldn't
work anyway. And the only thing that makes sense is to move the
polling in the kernel regardless of AN support or not, and make
userspace check for that and skip polling when the kernel takes care.

Kay

Re: [REGRESSION] cdrom drive doesn't detect removal

[Maxim Levitsky]

On Wed, 2010-09-15 at 15:44 +0200, Kay Sievers wrote: 
> On Wed, Sep 15, 2010 at 15:27, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
> > On Wed, 15 Sep 2010, Kay Sievers wrote:
> >> It's only the polling that should be able to trigger these events for
> >> the device. the device itself can't report any changes to the host,
> >> unless you have a SATA drive which can send AN (async notification)
> >> events. But we disabled the AN support recently, because it was
> >> running in loops on some boxes.
> >
> > Is it so widespread or extremely difficult to detect, that it justifies
> > breaking AN for all users?
> 
> Yes. It's not properly implemented, and many drives just always send
> an event on access, regardless if the media is changed or not. To work
> properly, it will need in-kernel media presence polling, which is
> being worked on. The current AN events, the stuff the drives send, and
> the way it is handled in the kernel, is just not usable.
> 
> > Anyway, if userspace thinks AN is available, it won't pool or touch
> > /dev/sr*, which would explain the problem (but one still would have to find
> > the root cause: why userspace thinks it doesn't need to pool?).
> 
> There is no userspace checking for AN support right now. It wouldn't
> work anyway. And the only thing that makes sense is to move the
> polling in the kernel regardless of AN support or not, and make
> userspace check for that and skip polling when the kernel takes care.

So it makes the sense.
It turns out that hal is still running on ubuntu systems, and yes, it
still polls the drives, and yes I disabled that feature....
So restoring that polling feature indeed fixes that problem for now.
Yet, why without that commit detection did work?

Best regards,
Maxim Levitsky

Re: [REGRESSION] cdrom drive doesn't detect removal

[Kay Sievers]

On Thu, Sep 16, 2010 at 00:20, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> So it makes the sense.
> It turns out that hal is still running on ubuntu systems, and yes, it
> still polls the drives, and yes I disabled that feature....
> So restoring that polling feature indeed fixes that problem for now.
> Yet, why without that commit detection did work?

Really, I have no idea how this can happen. You could only find out
with blocktrace, if something else is trying to open the device. The
state change in the drive should not be able to get known to the host
unless something is causing i/o with open().

Kay

Re: [REGRESSION] cdrom drive doesn't detect removal

[Maxim Levitsky]

On Thu, 2010-09-16 at 08:51 +0200, Kay Sievers wrote: 
> On Thu, Sep 16, 2010 at 00:20, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > So it makes the sense.
> > It turns out that hal is still running on ubuntu systems, and yes, it
> > still polls the drives, and yes I disabled that feature....
> > So restoring that polling feature indeed fixes that problem for now.
> > Yet, why without that commit detection did work?
> 
> Really, I have no idea how this can happen. You could only find out
> with blocktrace, if something else is trying to open the device. The
> state change in the drive should not be able to get known to the host
> unless something is causing i/o with open().
> 
> Kay

Due to some unexplained laziness, I didn't put a printk to cdrom_open to
figure out if drive is polled or not without hal.
Today I finally found why kgdb didn't work (it was conflict with nmi
watchdog), and just for fun I have put a breakpoint to cdrom_open.
Well udisks *does* poll the drive, every few seconds, and therefore this
is a regression.
I will soon look at udisk source to see how it polls the drive.
(Maybe it uses exclusive open and hal doesn't or something like that.)

Best regards,
Maxim Levitsky

Re: [REGRESSION] cdrom drive doesn't detect removal

[Maxim Levitsky]

On Tue, 2010-09-21 at 13:42 +0200, Maxim Levitsky wrote: 
> On Thu, 2010-09-16 at 08:51 +0200, Kay Sievers wrote: 
> > On Thu, Sep 16, 2010 at 00:20, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > > So it makes the sense.
> > > It turns out that hal is still running on ubuntu systems, and yes, it
> > > still polls the drives, and yes I disabled that feature....
> > > So restoring that polling feature indeed fixes that problem for now.
> > > Yet, why without that commit detection did work?
> > 
> > Really, I have no idea how this can happen. You could only find out
> > with blocktrace, if something else is trying to open the device. The
> > state change in the drive should not be able to get known to the host
> > unless something is causing i/o with open().
> > 
> > Kay
> 
> Due to some unexplained laziness, I didn't put a printk to cdrom_open to
> figure out if drive is polled or not without hal.
> Today I finally found why kgdb didn't work (it was conflict with nmi
> watchdog), and just for fun I have put a breakpoint to cdrom_open.
> Well udisks *does* poll the drive, every few seconds, and therefore this
> is a regression.
> I will soon look at udisk source to see how it polls the drive.
> (Maybe it uses exclusive open and hal doesn't or something like that.)

I just did a strace on udisks, and it is pretty much self explanatory.
(While CD is mounted).


open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)

Sure, a filesystem is mounted, so exclusive access fails...

So, we end up with impossible to solve problem.
We want on one hand to guard the burner against polling programs that disturb it, but one the other hand
we must do polling to check CD status.

Unless the kernel does the polling, but then we also must stop it when burning is done.
I think that we need new ioctl in the CD driver that would give absolute access to the burning application, and lock it fully.

What to do?

Best regards,
	Maxim Levitsky

Re: [REGRESSION] cdrom drive doesn't detect removal

[Tejun Heo]

Hello,

On 09/22/2010 01:09 AM, Maxim Levitsky wrote:
> I just did a strace on udisks, and it is pretty much self explanatory.
> (While CD is mounted).
> 
> open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
> open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
> open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
> open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> 
> Sure, a filesystem is mounted, so exclusive access fails...
> 
> So, we end up with impossible to solve problem.  We want on one hand
> to guard the burner against polling programs that disturb it, but
> one the other hand we must do polling to check CD status.
>
> Unless the kernel does the polling, but then we also must stop it
> when burning is done.  I think that we need new ioctl in the CD
> driver that would give absolute access to the burning application,
> and lock it fully.

One thing I don't get is why the behavior changed after the claiming
block patch.  Can you please trace udisks from a previous working
kernel?

Thanks.

-- 
tejun

Re: [REGRESSION] cdrom drive doesn't detect removal

[Maxim Levitsky]

On Wed, 2010-09-22 at 09:38 +0200, Tejun Heo wrote: 
> Hello,
> 
> On 09/22/2010 01:09 AM, Maxim Levitsky wrote:
> > I just did a strace on udisks, and it is pretty much self explanatory.
> > (While CD is mounted).
> > 
> > open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> > poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
> > open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> > poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
> > open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> > poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
> > open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> > 
> > Sure, a filesystem is mounted, so exclusive access fails...
> > 
> > So, we end up with impossible to solve problem.  We want on one hand
> > to guard the burner against polling programs that disturb it, but
> > one the other hand we must do polling to check CD status.
> >
> > Unless the kernel does the polling, but then we also must stop it
> > when burning is done.  I think that we need new ioctl in the CD
> > driver that would give absolute access to the burning application,
> > and lock it fully.
> 
> One thing I don't get is why the behavior changed after the claiming
> block patch.  Can you please trace udisks from a previous working
> kernel?

It shows pretty much same output.
The difference here is, even though open fails, it still has a side
effect of polling the driver, a bug that bisected commit fixed.

So problem is clear, but how to fix it, I don't know...

Also about the exclusive mode, how exactly is it defined?
open manpage is somewhat sparse about it.

Regardless of the above a complete solution is:

1. Make exclusive opens really exclusive.
That is if someone opens a device with exclusive access, no more opens
will succeed.

2. Add in-kernel polling, and stop it as as soon as an exclusive open is
done.

3. Make sure that userspace doesn't touch the cdrom device anymore.
Few patches to udisks and hal will do.

Best regards,
Maxim Levitsky

Re: [REGRESSION] cdrom drive doesn't detect removal

[Maxim Levitsky]

On Wed, 2010-09-22 at 15:41 +0200, Maxim Levitsky wrote: 
> On Wed, 2010-09-22 at 09:38 +0200, Tejun Heo wrote: 
> > Hello,
> > 
> > On 09/22/2010 01:09 AM, Maxim Levitsky wrote:
> > > I just did a strace on udisks, and it is pretty much self explanatory.
> > > (While CD is mounted).
> > > 
> > > open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> > > poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
> > > open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> > > poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
> > > open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> > > poll([{fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 2, 1997) = 0 (Timeout)
> > > open("/dev/sr0", O_RDONLY|O_EXCL|O_NONBLOCK) = -1 EBUSY (Device or resource busy)
> > > 
> > > Sure, a filesystem is mounted, so exclusive access fails...
> > > 
> > > So, we end up with impossible to solve problem.  We want on one hand
> > > to guard the burner against polling programs that disturb it, but
> > > one the other hand we must do polling to check CD status.
> > >
> > > Unless the kernel does the polling, but then we also must stop it
> > > when burning is done.  I think that we need new ioctl in the CD
> > > driver that would give absolute access to the burning application,
> > > and lock it fully.
> > 
> > One thing I don't get is why the behavior changed after the claiming
> > block patch.  Can you please trace udisks from a previous working
> > kernel?
> 
> It shows pretty much same output.
> The difference here is, even though open fails, it still has a side
> effect of polling the driver, a bug that bisected commit fixed.
> 
> So problem is clear, but how to fix it, I don't know...
> 
> Also about the exclusive mode, how exactly is it defined?
> open manpage is somewhat sparse about it.
> 
> Regardless of the above a complete solution is:
> 
> 1. Make exclusive opens really exclusive.
> That is if someone opens a device with exclusive access, no more opens
> will succeed.
And as a follow-up, indeed hal first tries exclusive open, and if it
fails, it retries with non-exclusive open, and it succeeds.
And that somewhat makes me think that exclusive open is pretty much
useless.
Look if it fails. sure the device is open, but if doesn't fail, nothing
prevents a bit less honest clients (that don't use exclusive open) to
open the device. How exclusive such an open is then?

So I mean exclusive open should really block _all_ following opens of
the device, exclusive or not.
Btw I had few failed dual layer disk burns that failed just after write
of few MBs. I wouldn't be surprised if this was the cause.


> 
> 2. Add in-kernel polling, and stop it as as soon as an exclusive open is
> done.
> 
> 3. Make sure that userspace doesn't touch the cdrom device anymore.
> Few patches to udisks and hal will do.
> 

Best regards,
	Maxim Levitsky

Re: [REGRESSION] cdrom drive doesn't detect removal

[Tejun Heo]

Hello,

On 09/22/2010 03:58 PM, Maxim Levitsky wrote:
>> 1. Make exclusive opens really exclusive.
>> That is if someone opens a device with exclusive access, no more opens
>> will succeed.
> And as a follow-up, indeed hal first tries exclusive open, and if it
> fails, it retries with non-exclusive open, and it succeeds.
> And that somewhat makes me think that exclusive open is pretty much
> useless.

Yeah, what I'm curious about is why hal behaves differently with
claiming block patch.  Exclusive open still fails with EBUSY with or
without the patch, right?  So, why does hal behave differently?

> Look if it fails. sure the device is open, but if doesn't fail, nothing
> prevents a bit less honest clients (that don't use exclusive open) to
> open the device. How exclusive such an open is then?

It's cooperative exclusion.  It doesn't assume the presence of hostile
programs having access to the device.

> So I mean exclusive open should really block _all_ following opens of
> the device, exclusive or not.

That will probably break a lot of stuff.

> Btw I had few failed dual layer disk burns that failed just after write
> of few MBs. I wouldn't be surprised if this was the cause.

Usually open sequence just inserts TEST UNIT READY which usually is
safe but yeah it's possible that some device might react badly.

I'm currently working on in-kernel media presence polling to handle
the open and polling command sequence issues.  That said, it's not
entirely clear how the mount case should be handled.  If a media is
mounted, the device is exclusively open and media presence polling
shouldn't be inserting commands in the middle but then how can it
detect the media has been ejected by the user?  Kay, can you please
enlighten me on how it's supposed to work?

Thanks.

-- 
tejun

Re: [REGRESSION] cdrom drive doesn't detect removal

[Kay Sievers]

On Thu, Sep 23, 2010 at 10:47, Tejun Heo <tj@kernel.org> wrote:

> Yeah, what I'm curious about is why hal behaves differently with
> claiming block patch.  Exclusive open still fails with EBUSY with or
> without the patch, right?  So, why does hal behave differently?

We don't support unlocked cd doors. Currently eject/umount of optical
media has to be initiated by the user.

HAL checked if the device was mounted, and if it was, it dropped the
O_EXCL. This was to support polling of the eject-button state, which
worked on a few drives. That's no longer cecked with udisks, it does
O_EXCL only for optical media.

>> Look if it fails. sure the device is open, but if doesn't fail, nothing
>> prevents a bit less honest clients (that don't use exclusive open) to
>> open the device. How exclusive such an open is then?

>> So I mean exclusive open should really block _all_ following opens of
>> the device, exclusive or not.
>
> That will probably break a lot of stuff.

That would surely need a new flag like O_REALLYEXCL. :)

> I'm currently working on in-kernel media presence polling to handle
> the open and polling command sequence issues.  That said, it's not
> entirely clear how the mount case should be handled.  If a media is
> mounted, the device is exclusively open and media presence polling
> shouldn't be inserting commands in the middle but then how can it
> detect the media has been ejected by the user?  Kay, can you please
> enlighten me on how it's supposed to work?

Non-optical devices should not be a problem, and can be always polled,
as it seems. We do this without O_EXCL since forever.

For optical drives I would never ever bypass O_EXCL, like udisks is
doing it. There are far too many problems with burning, which never
got really solved.

Force-removed media (not recommended unlocked doors) might not be
detected until the filesystem is cleaned-up/umounted, but that's
probably the better compromise than fiddling with the broken drives
during burning sessions.

Kay

Re: [REGRESSION] cdrom drive doesn't detect removal

[Florian Mickler]

On Thu, 23 Sep 2010 11:21:08 +0200
Kay Sievers <kay.sievers@vrfy.org> wrote:

> On Thu, Sep 23, 2010 at 10:47, Tejun Heo <tj@kernel.org> wrote:
> 
> > Yeah, what I'm curious about is why hal behaves differently with
> > claiming block patch.  Exclusive open still fails with EBUSY with or
> > without the patch, right?  So, why does hal behave differently?
> 
> We don't support unlocked cd doors. Currently eject/umount of optical
> media has to be initiated by the user.
> 
> HAL checked if the device was mounted, and if it was, it dropped the
> O_EXCL. This was to support polling of the eject-button state, which
> worked on a few drives. That's no longer cecked with udisks, it does
> O_EXCL only for optical media.
> 
> >> Look if it fails. sure the device is open, but if doesn't fail, nothing
> >> prevents a bit less honest clients (that don't use exclusive open) to
> >> open the device. How exclusive such an open is then?
> 
> >> So I mean exclusive open should really block _all_ following opens of
> >> the device, exclusive or not.
> >
> > That will probably break a lot of stuff.
> 
> That would surely need a new flag like O_REALLYEXCL. :)
> 
> > I'm currently working on in-kernel media presence polling to handle
> > the open and polling command sequence issues.  That said, it's not
> > entirely clear how the mount case should be handled.  If a media is
> > mounted, the device is exclusively open and media presence polling
> > shouldn't be inserting commands in the middle but then how can it
> > detect the media has been ejected by the user?  Kay, can you please
> > enlighten me on how it's supposed to work?
> 
> Non-optical devices should not be a problem, and can be always polled,
> as it seems. We do this without O_EXCL since forever.
> 
> For optical drives I would never ever bypass O_EXCL, like udisks is
> doing it. There are far too many problems with burning, which never
> got really solved.
> 
> Force-removed media (not recommended unlocked doors) might not be
> detected until the filesystem is cleaned-up/umounted, but that's
> probably the better compromise than fiddling with the broken drives
> during burning sessions.
> 
> Kay

So, is the $subject problem solved now? Normally, we shouldn't break
stuff with new kernels... If this is only a temporary breakage on
the other hand, we should keep track of it... 
I ask, because this is listed as https://bugzilla.kernel.org/show_bug.cgi?id=18522.
If it should stay listed, we may need an ETA for the fix... 

Regards,
Flo

Re: [REGRESSION] cdrom drive doesn't detect removal

[Kay Sievers]

On Thu, Sep 30, 2010 at 08:30, Florian Mickler <florian@mickler.org> wrote:
> On Thu, 23 Sep 2010 11:21:08 +0200
> Kay Sievers <kay.sievers@vrfy.org> wrote:
>> On Thu, Sep 23, 2010 at 10:47, Tejun Heo <tj@kernel.org> wrote:
>>
>> > Yeah, what I'm curious about is why hal behaves differently with
>> > claiming block patch.  Exclusive open still fails with EBUSY with or
>> > without the patch, right?  So, why does hal behave differently?
>>
>> We don't support unlocked cd doors. Currently eject/umount of optical
>> media has to be initiated by the user.
>>
>> HAL checked if the device was mounted, and if it was, it dropped the
>> O_EXCL. This was to support polling of the eject-button state, which
>> worked on a few drives. That's no longer cecked with udisks, it does
>> O_EXCL only for optical media.
>>
>> >> Look if it fails. sure the device is open, but if doesn't fail, nothing
>> >> prevents a bit less honest clients (that don't use exclusive open) to
>> >> open the device. How exclusive such an open is then?
>>
>> >> So I mean exclusive open should really block _all_ following opens of
>> >> the device, exclusive or not.
>> >
>> > That will probably break a lot of stuff.
>>
>> That would surely need a new flag like O_REALLYEXCL. :)
>>
>> > I'm currently working on in-kernel media presence polling to handle
>> > the open and polling command sequence issues.  That said, it's not
>> > entirely clear how the mount case should be handled.  If a media is
>> > mounted, the device is exclusively open and media presence polling
>> > shouldn't be inserting commands in the middle but then how can it
>> > detect the media has been ejected by the user?  Kay, can you please
>> > enlighten me on how it's supposed to work?
>>
>> Non-optical devices should not be a problem, and can be always polled,
>> as it seems. We do this without O_EXCL since forever.
>>
>> For optical drives I would never ever bypass O_EXCL, like udisks is
>> doing it. There are far too many problems with burning, which never
>> got really solved.
>>
>> Force-removed media (not recommended unlocked doors) might not be
>> detected until the filesystem is cleaned-up/umounted, but that's
>> probably the better compromise than fiddling with the broken drives
>> during burning sessions.
>
> So, is the $subject problem solved now? Normally, we shouldn't break
> stuff with new kernels... If this is only a temporary breakage on
> the other hand, we should keep track of it...
> I ask, because this is listed as https://bugzilla.kernel.org/show_bug.cgi?id=18522.
> If it should stay listed, we may need an ETA for the fix...

Hmm, I still don't think that's a bug or regression.

Optical drives are not supposed to report media changes without
constantly being polled. Why Tejun's seems to have an influence in
Maxim's setup, is likely more a timing-related issue, or some other
thing, we never really got an idea why it could change anything.

The current behavior is the expected and correct behavior, and for me
also the older kernels behave like this.

Kay

Re: [REGRESSION] cdrom drive doesn't detect removal

[Florian Mickler]

On Thu, 30 Sep 2010 09:48:50 +0200
Kay Sievers <kay.sievers@vrfy.org> wrote:
> >
> > So, is the $subject problem solved now? Normally, we shouldn't break
> > stuff with new kernels... If this is only a temporary breakage on
> > the other hand, we should keep track of it...
> > I ask, because this is listed as https://bugzilla.kernel.org/show_bug.cgi?id=18522.
> > If it should stay listed, we may need an ETA for the fix...
> 
> Hmm, I still don't think that's a bug or regression.
> 
> Optical drives are not supposed to report media changes without
> constantly being polled. Why Tejun's seems to have an influence in
> Maxim's setup, is likely more a timing-related issue, or some other
> thing, we never really got an idea why it could change anything.
> 
> The current behavior is the expected and correct behavior, and for me
> also the older kernels behave like this.
> 
> Kay

So I'm gonna close this as invalid then. Please shout, if that's
not ok.

Regards,
Flo

Re: [REGRESSION] cdrom drive doesn't detect removal

[Maxim Levitsky]

On Thu, 2010-09-30 at 13:38 +0200, Florian Mickler wrote:
> On Thu, 30 Sep 2010 09:48:50 +0200
> Kay Sievers <kay.sievers@vrfy.org> wrote:
> > >
> > > So, is the $subject problem solved now? Normally, we shouldn't break
> > > stuff with new kernels... If this is only a temporary breakage on
> > > the other hand, we should keep track of it...
> > > I ask, because this is listed as https://bugzilla.kernel.org/show_bug.cgi?id=18522.
> > > If it should stay listed, we may need an ETA for the fix...
> > 
> > Hmm, I still don't think that's a bug or regression.
> > 
> > Optical drives are not supposed to report media changes without
> > constantly being polled. Why Tejun's seems to have an influence in
> > Maxim's setup, is likely more a timing-related issue, or some other
> > thing, we never really got an idea why it could change anything.
> > 
> > The current behavior is the expected and correct behavior, and for me
> > also the older kernels behave like this.
> > 
> > Kay
> 
> So I'm gonna close this as invalid then. Please shout, if that's
> not ok.
Its not invalid at all.

Let me explain again the problem:

Indeed cdrom drivers need to be polled.
And yes, both udisks and hal _do_ poll the drive.

However, udisks uses O_EXCL, when it opens the drive, and that fails s
soon as filesystem is mounted, therefore effectively, as soon as disk is
inserted, polling stops.

Without the bisected commit, O_EXCL would still fail, however due to the
bug that commit fixed, the drive will still be polled.

Hal also polls the drive, but it has a workaround, that is if open with
O_EXCL fails, it retries the open without it.

This is pretty much all, there are no unknowns left.


However the reason behind O_EXCL is that we don't want polling while a
disk is being written (burned), but we do want polling while disk is
mounted.

So just 'fixing' udisk to not take O_EXCL will bring us to square one,
as well as reverting the commit.

Therefore this problem doesn't have a simple solution.
I think that the best solution is to move polling to the kernel, and
introduce a way for burning application to take absolute exclusive
access to the device and as a part of that disable that in-kernel
polling.
A new ioctl will suffice, or the meaning of O_EXCL can be changed for
cdrom driver only (if somebody has it open with O_EXCL, fail all opens,
including the ones that don't have O_EXCL).


Best regards,
	Maxim Levitsky

Re: [REGRESSION] cdrom drive doesn't detect removal

[Florian Mickler]

On Thu, 30 Sep 2010 16:17:32 +0200
Maxim Levitsky <maximlevitsky@gmail.com> wrote:

> On Thu, 2010-09-30 at 13:38 +0200, Florian Mickler wrote:
> > On Thu, 30 Sep 2010 09:48:50 +0200
> > Kay Sievers <kay.sievers@vrfy.org> wrote:
> > > >
> > > > So, is the $subject problem solved now? Normally, we shouldn't break
> > > > stuff with new kernels... If this is only a temporary breakage on
> > > > the other hand, we should keep track of it...
> > > > I ask, because this is listed as https://bugzilla.kernel.org/show_bug.cgi?id=18522.
> > > > If it should stay listed, we may need an ETA for the fix...
> > > 
> > > Hmm, I still don't think that's a bug or regression.
> > > 
> > > Optical drives are not supposed to report media changes without
> > > constantly being polled. Why Tejun's seems to have an influence in
> > > Maxim's setup, is likely more a timing-related issue, or some other
> > > thing, we never really got an idea why it could change anything.
> > > 
> > > The current behavior is the expected and correct behavior, and for me
> > > also the older kernels behave like this.
> > > 
> > > Kay
> > 
> > So I'm gonna close this as invalid then. Please shout, if that's
> > not ok.
> Its not invalid at all.
> 
> Let me explain again the problem:
> 
> Indeed cdrom drivers need to be polled.
> And yes, both udisks and hal _do_ poll the drive.
> 
> However, udisks uses O_EXCL, when it opens the drive, and that fails s
> soon as filesystem is mounted, therefore effectively, as soon as disk is
> inserted, polling stops.
> 
> Without the bisected commit, O_EXCL would still fail, however due to the
> bug that commit fixed, the drive will still be polled.
> 
> Hal also polls the drive, but it has a workaround, that is if open with
> O_EXCL fails, it retries the open without it.
> 
> This is pretty much all, there are no unknowns left.
> 
> 
> However the reason behind O_EXCL is that we don't want polling while a
> disk is being written (burned), but we do want polling while disk is
> mounted.
> 
> So just 'fixing' udisk to not take O_EXCL will bring us to square one,
> as well as reverting the commit.
> 
> Therefore this problem doesn't have a simple solution.
> I think that the best solution is to move polling to the kernel, and
> introduce a way for burning application to take absolute exclusive
> access to the device and as a part of that disable that in-kernel
> polling.
> A new ioctl will suffice, or the meaning of O_EXCL can be changed for
> cdrom driver only (if somebody has it open with O_EXCL, fail all opens,
> including the ones that don't have O_EXCL).
> 
> 
> Best regards,
> 	Maxim Levitsky
> 

Ok. I will reopen then. Is someone working on fixing this issue? 

If we leave it open, we wanna have an ETA for the fix so that people
trolling the bugzilla know for how long they have to let this report
simmer.

Regards,
Flo

Re: [REGRESSION] cdrom drive doesn't detect removal

[Kay Sievers]

On Thu, Sep 30, 2010 at 16:49, Florian Mickler <florian@mickler.org> wrote:
> On Thu, 30 Sep 2010 16:17:32 +0200
> Maxim Levitsky <maximlevitsky@gmail.com> wrote:
>> On Thu, 2010-09-30 at 13:38 +0200, Florian Mickler wrote:
>> > On Thu, 30 Sep 2010 09:48:50 +0200
>> > Kay Sievers <kay.sievers@vrfy.org> wrote:
>> > > >
>> > > > So, is the $subject problem solved now? Normally, we shouldn't break
>> > > > stuff with new kernels... If this is only a temporary breakage on
>> > > > the other hand, we should keep track of it...
>> > > > I ask, because this is listed as https://bugzilla.kernel.org/show_bug.cgi?id=18522.
>> > > > If it should stay listed, we may need an ETA for the fix...
>> > >
>> > > Hmm, I still don't think that's a bug or regression.
>> > >
>> > > Optical drives are not supposed to report media changes without
>> > > constantly being polled. Why Tejun's seems to have an influence in
>> > > Maxim's setup, is likely more a timing-related issue, or some other
>> > > thing, we never really got an idea why it could change anything.
>> > >
>> > > The current behavior is the expected and correct behavior, and for me
>> > > also the older kernels behave like this.
>> >
>> > So I'm gonna close this as invalid then. Please shout, if that's
>> > not ok.
>> Its not invalid at all.
>>
>> Let me explain again the problem:
>>
>> Indeed cdrom drivers need to be polled.
>> And yes, both udisks and hal _do_ poll the drive.
>>
>> However, udisks uses O_EXCL, when it opens the drive, and that fails s
>> soon as filesystem is mounted, therefore effectively, as soon as disk is
>> inserted, polling stops.
>>
>> Without the bisected commit, O_EXCL would still fail, however due to the
>> bug that commit fixed, the drive will still be polled.
>>
>> Hal also polls the drive, but it has a workaround, that is if open with
>> O_EXCL fails, it retries the open without it.
>>
>> This is pretty much all, there are no unknowns left.
>>
>>
>> However the reason behind O_EXCL is that we don't want polling while a
>> disk is being written (burned), but we do want polling while disk is
>> mounted.
>>
>> So just 'fixing' udisk to not take O_EXCL will bring us to square one,
>> as well as reverting the commit.
>>
>> Therefore this problem doesn't have a simple solution.
>> I think that the best solution is to move polling to the kernel, and
>> introduce a way for burning application to take absolute exclusive
>> access to the device and as a part of that disable that in-kernel
>> polling.
>> A new ioctl will suffice, or the meaning of O_EXCL can be changed for
>> cdrom driver only (if somebody has it open with O_EXCL, fail all opens,
>> including the ones that don't have O_EXCL).

> Ok. I will reopen then. Is someone working on fixing this issue?
>
> If we leave it open, we wanna have an ETA for the fix so that people
> trolling the bugzilla know for how long they have to let this report
> simmer.

I don't think anything needs to be or can really be fixed in the
kernel. We need a working O_EXCL, and before Tejun's patch it was
broken. That the broken behavior seemed to have some wanted effects on
some boxes can't the reason to leave O_EXCL broken.

The current state is the expected behavior. It all seems more like an
issue with current udisks which should be discussed at the freedesktop
bugzilla. Udisk's behavior is intended, but there might be no specific
reason not to change it to what HAL did.

Kay

Re: [REGRESSION] cdrom drive doesn't detect removal

[Florian Mickler]

On Thu, 30 Sep 2010 21:27:52 +0200
Kay Sievers <kay.sievers@vrfy.org> wrote:

> On Thu, Sep 30, 2010 at 16:49, Florian Mickler <florian@mickler.org> wrote:
> > On Thu, 30 Sep 2010 16:17:32 +0200
> > Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> >> On Thu, 2010-09-30 at 13:38 +0200, Florian Mickler wrote:
> >> > On Thu, 30 Sep 2010 09:48:50 +0200
> >> > Kay Sievers <kay.sievers@vrfy.org> wrote:
> >> > > >
> >> > > > So, is the $subject problem solved now? Normally, we shouldn't break
> >> > > > stuff with new kernels... If this is only a temporary breakage on
> >> > > > the other hand, we should keep track of it...
> >> > > > I ask, because this is listed as https://bugzilla.kernel.org/show_bug.cgi?id=18522.
> >> > > > If it should stay listed, we may need an ETA for the fix...
> >> > >
> >> > > Hmm, I still don't think that's a bug or regression.
> >> > >
> >> > > Optical drives are not supposed to report media changes without
> >> > > constantly being polled. Why Tejun's seems to have an influence in
> >> > > Maxim's setup, is likely more a timing-related issue, or some other
> >> > > thing, we never really got an idea why it could change anything.
> >> > >
> >> > > The current behavior is the expected and correct behavior, and for me
> >> > > also the older kernels behave like this.
> >> >
> >> > So I'm gonna close this as invalid then. Please shout, if that's
> >> > not ok.
> >> Its not invalid at all.
> >>
> >> Let me explain again the problem:
> >>
> >> Indeed cdrom drivers need to be polled.
> >> And yes, both udisks and hal _do_ poll the drive.
> >>
> >> However, udisks uses O_EXCL, when it opens the drive, and that fails s
> >> soon as filesystem is mounted, therefore effectively, as soon as disk is
> >> inserted, polling stops.
> >>
> >> Without the bisected commit, O_EXCL would still fail, however due to the
> >> bug that commit fixed, the drive will still be polled.
> >>
> >> Hal also polls the drive, but it has a workaround, that is if open with
> >> O_EXCL fails, it retries the open without it.
> >>
> >> This is pretty much all, there are no unknowns left.
> >>
> >>
> >> However the reason behind O_EXCL is that we don't want polling while a
> >> disk is being written (burned), but we do want polling while disk is
> >> mounted.
> >>
> >> So just 'fixing' udisk to not take O_EXCL will bring us to square one,
> >> as well as reverting the commit.
> >>
> >> Therefore this problem doesn't have a simple solution.
> >> I think that the best solution is to move polling to the kernel, and
> >> introduce a way for burning application to take absolute exclusive
> >> access to the device and as a part of that disable that in-kernel
> >> polling.
> >> A new ioctl will suffice, or the meaning of O_EXCL can be changed for
> >> cdrom driver only (if somebody has it open with O_EXCL, fail all opens,
> >> including the ones that don't have O_EXCL).
> 
> > Ok. I will reopen then. Is someone working on fixing this issue?
> >
> > If we leave it open, we wanna have an ETA for the fix so that people
> > trolling the bugzilla know for how long they have to let this report
> > simmer.
> 
> I don't think anything needs to be or can really be fixed in the
> kernel. We need a working O_EXCL, and before Tejun's patch it was
> broken. That the broken behavior seemed to have some wanted effects on
> some boxes can't the reason to leave O_EXCL broken.

Ack on this on the technical side.

But we have to look from the user side at this, as bug tracking is
mostly a tool to get feedback from the users. So if we close this bug,
we loose valuable feedback on the functioning of the kernel.

So, I can't really close the report if it is not resolved in any way.

The issue is that the cdrom drive does not detect removal while it is
mounted. 

Do you see any way, how userspace can change this without kernel help?

> 
> The current state is the expected behavior. 

Expected for a kernel developer who understands the inner workings of
his system. For a user, I'd argue, this is not expected. 


> It all seems more like an
> issue with current udisks which should be discussed at the freedesktop
> bugzilla. Udisk's behavior is intended, but there might be no specific
> reason not to change it to what HAL did.
> 
> Kay

I think we can close this, as soon as we have an acknowledged bug by
the udisks maintainer. But not earlier. 

At the moment, I don't see this issue as resolved. 

Regards,
Flo

Re: [REGRESSION] cdrom drive doesn't detect removal

[Kay Sievers]

On Thu, Sep 30, 2010 at 22:14, Florian Mickler <florian@mickler.org> wrote:
> On Thu, 30 Sep 2010 21:27:52 +0200 Kay Sievers <kay.sievers@vrfy.org> wrote:

>> I don't think anything needs to be or can really be fixed in the
>> kernel. We need a working O_EXCL, and before Tejun's patch it was
>> broken. That the broken behavior seemed to have some wanted effects on
>> some boxes can't the reason to leave O_EXCL broken.
>
> Ack on this on the technical side.
>
> But we have to look from the user side at this, as bug tracking is
> mostly a tool to get feedback from the users. So if we close this bug,
> we loose valuable feedback on the functioning of the kernel.
>
> So, I can't really close the report if it is not resolved in any way.

As said, it can't be resolved. That it seemed to work was a buggy
O_EXCL, which let stuff through the O_EXCL barrier, which was never
supposed to reach the drive. I can't even reproduce that on older
kernels. It seems like a specific timing problem.

> The issue is that the cdrom drive does not detect removal while it is
> mounted.
>
> Do you see any way, how userspace can change this without kernel help?

Sure, it can open the device without O_EXCL like HAL. But successors
of HAL stopped doing that because it broke too many burning sessions.

>> The current state is the expected behavior.
>
> Expected for a kernel developer who understands the inner workings of
> his system. For a user, I'd argue, this is not expected.

Yeah, might be. There is still nothing I think we can do at the moment
at the kernel side.

>> It all seems more like an
>> issue with current udisks which should be discussed at the freedesktop
>> bugzilla. Udisk's behavior is intended, but there might be no specific
>> reason not to change it to what HAL did.
>
> I think we can close this, as soon as we have an acknowledged bug by
> the udisks maintainer. But not earlier.

It's not a bug of udisks, it's decided to do it that way. It can be
discussed though.

> At the moment, I don't see this issue as resolved.

Sure, no problem. I'm just saying that nobody can fix the problem in
the kernel without adding something completely new like in-kernel
polling. And that might take a longer time to get actually enabled by
default. :)

Kay

Re: [REGRESSION] cdrom drive doesn't detect removal

[Florian Mickler]

On Thu, 30 Sep 2010 22:32:33 +0200
Kay Sievers <kay.sievers@vrfy.org> wrote:

> On Thu, Sep 30, 2010 at 22:14, Florian Mickler <florian@mickler.org> wrote:
> > On Thu, 30 Sep 2010 21:27:52 +0200 Kay Sievers <kay.sievers@vrfy.org> wrote:
> 
> >> I don't think anything needs to be or can really be fixed in the
> >> kernel. We need a working O_EXCL, and before Tejun's patch it was
> >> broken. That the broken behavior seemed to have some wanted effects on
> >> some boxes can't the reason to leave O_EXCL broken.
> >
> > Ack on this on the technical side.
> >
> > But we have to look from the user side at this, as bug tracking is
> > mostly a tool to get feedback from the users. So if we close this bug,
> > we loose valuable feedback on the functioning of the kernel.
> >
> > So, I can't really close the report if it is not resolved in any way.
> 
> As said, it can't be resolved. That it seemed to work was a buggy
> O_EXCL, which let stuff through the O_EXCL barrier, which was never
> supposed to reach the drive. I can't even reproduce that on older
> kernels. It seems like a specific timing problem.

Yes I see this from the changelog of Tejuns commit. It shure looks like
it all works just fine... 

Aren't cdrom devices physically locked while mounted? How could this be
a problem then? Is it about force-opening the device?

And what are the symptoms when the cd is removed while mounted? Are
there horrible timeouts when accessing the file system?


> > The issue is that the cdrom drive does not detect removal while it is
> > mounted.
> >
> > Do you see any way, how userspace can change this without kernel help?
> 
> Sure, it can open the device without O_EXCL like HAL. But successors
> of HAL stopped doing that because it broke too many burning sessions.

...seems sensible...

> >> The current state is the expected behavior.
> >
> > Expected for a kernel developer who understands the inner workings of
> > his system. For a user, I'd argue, this is not expected.
> 
> Yeah, might be. There is still nothing I think we can do at the moment
> at the kernel side.

I'm shure, there are ways to make this happen. And I don't think there
is any solution that doesn't involve the kernel.
 
> >> It all seems more like an
> >> issue with current udisks which should be discussed at the freedesktop
> >> bugzilla. Udisk's behavior is intended, but there might be no specific
> >> reason not to change it to what HAL did.
> >
> > I think we can close this, as soon as we have an acknowledged bug by
> > the udisks maintainer. But not earlier.
> 
> It's not a bug of udisks, it's decided to do it that way. It can be
> discussed though.
> 
> > At the moment, I don't see this issue as resolved.
> 
> Sure, no problem. I'm just saying that nobody can fix the problem in
> the kernel without adding something completely new like in-kernel
> polling. And that might take a longer time to get actually enabled by
> default. :)
> 
> Kay

I don't think we pay rent in the bugzilla yet... So this can just stay
open, until the necessary infrastructure is in place.. 
But I'm wondering if it is futile or not.

Regards,
Flo

Re: [REGRESSION] cdrom drive doesn't detect removal

[Kay Sievers]

On Thu, Sep 30, 2010 at 22:47, Florian Mickler <florian@mickler.org> wrote:
> On Thu, 30 Sep 2010 22:32:33 +0200
> Kay Sievers <kay.sievers@vrfy.org> wrote:
>
>> On Thu, Sep 30, 2010 at 22:14, Florian Mickler <florian@mickler.org> wrote:
>> > On Thu, 30 Sep 2010 21:27:52 +0200 Kay Sievers <kay.sievers@vrfy.org> wrote:
>>
>> >> I don't think anything needs to be or can really be fixed in the
>> >> kernel. We need a working O_EXCL, and before Tejun's patch it was
>> >> broken. That the broken behavior seemed to have some wanted effects on
>> >> some boxes can't the reason to leave O_EXCL broken.
>> >
>> > Ack on this on the technical side.
>> >
>> > But we have to look from the user side at this, as bug tracking is
>> > mostly a tool to get feedback from the users. So if we close this bug,
>> > we loose valuable feedback on the functioning of the kernel.
>> >
>> > So, I can't really close the report if it is not resolved in any way.
>>
>> As said, it can't be resolved. That it seemed to work was a buggy
>> O_EXCL, which let stuff through the O_EXCL barrier, which was never
>> supposed to reach the drive. I can't even reproduce that on older
>> kernels. It seems like a specific timing problem.
>
> Yes I see this from the changelog of Tejuns commit. It shure looks like
> it all works just fine...
>
> Aren't cdrom devices physically locked while mounted? How could this be
> a problem then? Is it about force-opening the device?

Yes, udisks doesn't support force-unlocked cdrom doors. This all can
not happen otherwise.

> And what are the symptoms when the cd is removed while mounted? Are
> there horrible timeouts when accessing the file system?

It did even crash the entire kernel sometimes in the past, I didn't
try the last 2 years.

> I don't think we pay rent in the bugzilla yet... So this can just stay
> open, until the necessary infrastructure is in place..
> But I'm wondering if it is futile or not.

Sure, keep it open if you like. I just say that it's not a regression
in the sense that it can be fixed by rolling back to an earlier state.
Such functionality will need a new feature to be implemented. It was
only a bug that let O_EXCL pass the barrier, and that same bug broke
physical media, and we need to fix it.

Kay

Re: [REGRESSION] cdrom drive doesn't detect removal

[Tejun Heo]

Hello,

On 09/30/2010 04:49 PM, Florian Mickler wrote:
> Ok. I will reopen then. Is someone working on fixing this issue? 
> 
> If we leave it open, we wanna have an ETA for the fix so that people
> trolling the bugzilla know for how long they have to let this report
> simmer.

I just started working on in-kernel media presence polling and it
shouldn't be too hard to distinguish ro-mount and burning cases and it
should be possible to keep polling for ro mounts.  I think it's
already a bit too late for the next merge window but the one after
that would be a reasonable target.

Thanks.

-- 
tejun

Re: [REGRESSION] cdrom drive doesn't detect removal

[Florian Mickler]

On Fri, 01 Oct 2010 07:55:44 +0200
Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> On 09/30/2010 04:49 PM, Florian Mickler wrote:
> > Ok. I will reopen then. Is someone working on fixing this issue? 
> > 
> > If we leave it open, we wanna have an ETA for the fix so that people
> > trolling the bugzilla know for how long they have to let this report
> > simmer.
> 
> I just started working on in-kernel media presence polling and it
> shouldn't be too hard to distinguish ro-mount and burning cases and it
> should be possible to keep polling for ro mounts.  I think it's
> already a bit too late for the next merge window but the one after
> that would be a reasonable target.
> 
> Thanks.
> 

Ok, cool. 

Flo