Hi,

Thank you for review and comments.

On 02/16/12 02:26, Tejun Heo wrote:

> On Wed, Feb 15, 2012 at 11:56:19AM +0900, Jun'ichi Nomura wrote:

>> +int invalidate_partitions(struct gendisk *disk, struct block_device *bdev)

>> +{

>> +	int res;

>> +

>> +	res = drop_partitions(disk, bdev);

>> +	if (res)

>> +		return res;

>> +

> 

> Hmmm... shouldn't we have set_capacity(disk, 0) here?

Added.

I wasn't sure whether I should leave it to drivers.

But it seems capacity 0 for ENOMEDIUM device is reasonable.

>> +	check_disk_size_change(disk, bdev);

>> +	bdev->bd_invalidated = 0;

>> +	/* tell userspace that the media / partition table may have changed */

>> +	kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);

> 

> Also, we really shouldn't be generating KOBJ_CHANGE after every

> -ENOMEDIUM open.  This can easily lead to infinite loop.  We should

> generate this iff we actually dropped partitions && modified the size.

invalidate_partitions() is called only when bd_invalidated is set.

So KOBJ_CHANGE is not raised for every ENOMEDIUM open.

I put it explicit in the function to make it safer for

possible misuse.

How about this?

---------------------------------------------------------

Do not call drivers when invalidating partitions for -ENOMEDIUM

When a scsi driver returns -ENOMEDIUM for open(),

__blkdev_get() calls rescan_partitions(), which ends up calling

sd_revalidate_disk() without getting a refcount of scsi_device.

That could lead to oops like this:

  process A                  process B

  ----------------------------------------------

  sys_open

    __blkdev_get

      sd_open

        returns -ENOMEDIUM

                             scsi_remove_device

                               <scsi_device torn down>

      rescan_partitions

        sd_revalidate_disk

          <oops>

Oopses are reported here:

http://marc.info/?l=linux-scsi&m=132388619710052

This patch separates the partition invalidation from rescan_partitions()

and use it for -ENOMEDIUM case. 

Index: linux-3.3/block/partition-generic.c

===================================================================

--- linux-3.3.orig/block/partition-generic.c	2012-02-15 09:00:25.147293790 +0900

+++ linux-3.3/block/partition-generic.c	2012-02-16 10:48:22.257680685 +0900

@@ -389,17 +389,11 @@ static bool disk_unlock_native_capacity(

 	}

 }

-int rescan_partitions(struct gendisk *disk, struct block_device *bdev)

+static int drop_partitions(struct gendisk *disk, struct block_device *bdev)

 {

-	struct parsed_partitions *state = NULL;

 	struct disk_part_iter piter;

 	struct hd_struct *part;

-	int p, highest, res;

-rescan:

-	if (state && !IS_ERR(state)) {

-		kfree(state);

-		state = NULL;

-	}

+	int res;

 	if (bdev->bd_part_count)

 		return -EBUSY;

@@ -412,6 +406,24 @@ rescan:

 		delete_partition(disk, part->partno);

 	disk_part_iter_exit(&piter);

+	return 0;

+}

+

+int rescan_partitions(struct gendisk *disk, struct block_device *bdev)

+{

+	struct parsed_partitions *state = NULL;

+	struct hd_struct *part;

+	int p, highest, res;

+rescan:

+	if (state && !IS_ERR(state)) {

+		kfree(state);

+		state = NULL;

+	}

+

+	res = drop_partitions(disk, bdev);

+	if (res)

+		return res;

+

 	if (disk->fops->revalidate_disk)

 		disk->fops->revalidate_disk(disk);

 	check_disk_size_change(disk, bdev);

@@ -515,6 +527,26 @@ rescan:

 	return 0;

 }

+int invalidate_partitions(struct gendisk *disk, struct block_device *bdev)

+{

+	int res;

+

+	if (!bdev->bd_invalidated)

+		return 0;

+

+	res = drop_partitions(disk, bdev);

+	if (res)

+		return res;

+

+	set_capacity(disk, 0);

+	check_disk_size_change(disk, bdev);

+	bdev->bd_invalidated = 0;

+	/* tell userspace that the media / partition table may have changed */

+	kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);

+

+	return 0;

+}

+

 unsigned char *read_dev_sector(struct block_device *bdev, sector_t n, Sector *p)

 {

 	struct address_space *mapping = bdev->bd_inode->i_mapping;

Index: linux-3.3/include/linux/genhd.h

===================================================================

--- linux-3.3.orig/include/linux/genhd.h	2012-02-09 12:21:53.000000000 +0900

+++ linux-3.3/include/linux/genhd.h	2012-02-16 10:47:43.783681813 +0900

@@ -596,6 +596,7 @@ extern char *disk_name (struct gendisk *

 extern int disk_expand_part_tbl(struct gendisk *disk, int target);

 extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev);

+extern int invalidate_partitions(struct gendisk *disk, struct block_device *bdev);

 extern struct hd_struct * __must_check add_partition(struct gendisk *disk,

 						     int partno, sector_t start,

 						     sector_t len, int flags,

Index: linux-3.3/fs/block_dev.c

===================================================================

--- linux-3.3.orig/fs/block_dev.c	2012-02-09 12:21:53.000000000 +0900

+++ linux-3.3/fs/block_dev.c	2012-02-16 10:47:52.602681441 +0900

@@ -1183,8 +1183,12 @@ static int __blkdev_get(struct block_dev

 			 * The latter is necessary to prevent ghost

 			 * partitions on a removed medium.

 			 */

-			if (bdev->bd_invalidated && (!ret || ret == -ENOMEDIUM))

-				rescan_partitions(disk, bdev);

+			if (bdev->bd_invalidated) {

+				if (!ret)

+					rescan_partitions(disk, bdev);

+				else if (ret == -ENOMEDIUM)

+					invalidate_partitions(disk, bdev);

+			}

 			if (ret)

 				goto out_clear;

 		} else {

@@ -1214,8 +1218,12 @@ static int __blkdev_get(struct block_dev

 			if (bdev->bd_disk->fops->open)

 				ret = bdev->bd_disk->fops->open(bdev, mode);

 			/* the same as first opener case, read comment there */

-			if (bdev->bd_invalidated && (!ret || ret == -ENOMEDIUM))

-				rescan_partitions(bdev->bd_disk, bdev);

+			if (bdev->bd_invalidated) {

+				if (!ret)

+					rescan_partitions(bdev->bd_disk, bdev);

+				else if (ret == -ENOMEDIUM)

+					invalidate_partitions(bdev->bd_disk, bdev);

+			}

 			if (ret)

 				goto out_unlock_bdev;

 		}