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

[Maxim Levitsky <maximlevitsky@gmail.com>]

[-- 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);
 }