[2.6.13-rc6-latest] SCSI disk registration msgs repeat themselves

[2.6.13-rc6-latest] SCSI disk registration msgs repeat themselves

[Chuck Ebbert]

  I just added some usb-storage devices to my system and got the below.
Why do the first four lines repeat for each device?  (Not sure if
this is a SCSI or USB problem.)

[   23.433725] SCSI device sda: 63424 512-byte hdwr sectors (32 MB)
[   23.560564] sda: Write Protect is off
[   23.560581] sda: Mode Sense: 02 00 00 00
[   23.560593] sda: assuming drive cache: write through
[   23.572525] SCSI device sda: 63424 512-byte hdwr sectors (32 MB)
[   23.576504] sda: Write Protect is off
[   23.576519] sda: Mode Sense: 02 00 00 00
[   23.576530] sda: assuming drive cache: write through
[   23.576545]  sda: sda1
[   23.583752] Attached scsi removable disk sda at scsi1, channel 0, id 0, lun 0
[   23.583847] Attached scsi generic sg1 at scsi1, channel 0, id 0, lun 0,  type 0
[   23.584701] usb-storage: device scan complete
[   32.116248] SCSI device sdb: 196608 512-byte hdwr sectors (101 MB)
[   32.141310] sdb: Write Protect is off
[   32.153477] sdb: Mode Sense: 45 00 00 08
[   32.153490] sdb: assuming drive cache: write through
[   32.178386] SCSI device sdb: 196608 512-byte hdwr sectors (101 MB)
[   32.204473] sdb: Write Protect is off
[   32.216654] sdb: Mode Sense: 45 00 00 08
[   32.216735] sdb: assuming drive cache: write through
[   32.233259]  sdb: sdb4
[   32.246595] Attached scsi removable disk sdb at scsi2, channel 0, id 0, lun 0
[   32.270348] Attached scsi generic sg2 at scsi2, channel 0, id 0, lun 0,  type 0
[   32.295843] usb-storage: device scan complete


__
Chuck

Re: [2.6.13-rc6-latest] SCSI disk registration msgs repeat themselves

[Patrick Mansfield]

On Tue, Aug 16, 2005 at 11:01:30PM -0400, Chuck Ebbert wrote:
>   I just added some usb-storage devices to my system and got the below.
> Why do the first four lines repeat for each device?  (Not sure if
> this is a SCSI or USB problem.)

It is in the partition code. I posted twice before about this with no
response.

The changelog said it was a workaround for a borken device, but not what
device it was or any other details.

There is a patch (against 2.6.11) in the below, I haven't tried it with
recent kernels:

http://marc.theaimsgroup.com/?l=linux-scsi&m=110719123625593&w=2
http://marc.theaimsgroup.com/?l=linux-scsi&m=110848617107098&w=2

-- Patrick Mansfield

Re: [2.6.13-rc6-latest] SCSI disk registration msgs repeat themselves

[Pete Zaitcev]

On Tue, 16 Aug 2005 21:39:33 -0700, Patrick Mansfield <patmans@us.ibm.com> wrote:
> On Tue, Aug 16, 2005 at 11:01:30PM -0400, Chuck Ebbert wrote:

> >   I just added some usb-storage devices to my system and got the below.

> > Why do the first four lines repeat for each device?  (Not sure if
> > this is a SCSI or USB problem.)
> 
> It is in the partition code. I posted twice before about this with no
> response.

It's not an important problem, presumably. I observe dual revalidations
as well, but they are not that bothersome. Add to it that your patch
appears wrong (see below). If you offered an acceptable solution, I would
expect a warmer welcome... But even then getting a reply from linux-scsi
folks is like pulling a tooth (if my own little CD-ROM sizing patch is
any indication). So, steel yourself for challenges of this life, Patrick!

> The changelog said it was a workaround for a borken device, but not what
> device it was or any other details.

Here's what it was in 2.6.9, as documented in drivers/block/ub.c:

+	/*
+	 * This is a workaround for a specific problem in our block layer.
+	 * In 2.6.9, register_disk duplicates the code from rescan_partitions.
+	 * However, if we do add_disk with a device which persistently reports
+	 * a changed media, add_disk calls register_disk, which does do_open,
+	 * which will call rescan_paritions for changed media. After that,
+	 * register_disk attempts to do it all again and causes double kobject
+	 * registration and a eventually an oops on module removal.
+	 *
+	 * The bottom line is, Al Viro says that we should not allow
+	 * bdev->bd_invalidated to be set when doing add_disk no matter what.
+	 */
+	if (sc->first_open) {
+		if (sc->changed) {
+			sc->first_open = 0;
+			rc = -ENOMEDIUM;
+			goto err_open;
+		}
+	}

Users were hitting it with oopses like these:
http://www.ussg.iu.edu/hypermail/linux/kernel/0409.2/0011.html

The ub alone was not suffient to motivate Al for the fix, so I added
this silly "first_open" thingie, which papered over it. It was thought
that sd was miraclously immune.

However, over time users hit it with usb-storage and sd, like this:
 http://lkml.org/lkml/2004/2/21/19
This prompted Al's action. He simply dropped all the extra code like
this:

--- linux-2.6.9-11.5.EL/fs/partitions/check.c	2004-10-18 14:55:07.000000000 -0700
+++ linux-2.6.12/fs/partitions/check.c	2005-06-17 12:48:29.000000000 -0700
@@ -358,24 +357,9 @@ void register_disk(struct gendisk *disk)
 	if (!bdev)
 		return;
 
+	bdev->bd_invalidated = 1;
 	if (blkdev_get(bdev, FMODE_READ, 0) < 0)
 		return;
-	state = check_partition(disk, bdev);
-	if (state) {
-		for (j = 1; j < state->limit; j++) {
-			sector_t size = state->parts[j].size;
-			sector_t from = state->parts[j].from;
-			if (!size)
-				continue;
-			add_partition(disk, j, from, size);
-#ifdef CONFIG_BLK_DEV_MD
-			if (!state->parts[j].flags)
-				continue;
-			md_autodetect_dev(bdev->bd_dev+j);
-#endif
-		}
-		kfree(state);
-	}
 	blkdev_put(bdev);
 }
 

I was just about to remove "first_open" from ub, because it's unnecessary
with Al's fix and it was getting on my nerves.

> --- linux-2.6.11-rc1/fs/partitions/check.c	Fri Dec 24 13:35:28 2004
> +++ no-double-sd-linux-2.6.11-rc1/fs/partitions/check.c	Fri Jan 21 11:19:00 2005
> @@ -375,8 +375,6 @@ int rescan_partitions(struct gendisk *di
>  	bdev->bd_invalidated = 0;
>  	for (p = 1; p < disk->minors; p++)
>  		delete_partition(disk, p);
> -	if (disk->fops->revalidate_disk)
> -		disk->fops->revalidate_disk(disk);

As for your proposed fix, it may be problematic. The ->revalidate
method has to be called at least once for a new device, because
that's when drivers fetch the capacities. But ->open only calls
check_disk_change() for removable devices. Who is going to call
->revalidate inside add_disk() for non-removable devices?

-- Pete

Re: [2.6.13-rc6-latest] SCSI disk registration msgs repeat themselves

[Patrick Mansfield]

On Tue, Aug 16, 2005 at 10:51:13PM -0700, Pete Zaitcev wrote:
> On Tue, 16 Aug 2005 21:39:33 -0700, Patrick Mansfield <patmans@us.ibm.com> wrote:
> > On Tue, Aug 16, 2005 at 11:01:30PM -0400, Chuck Ebbert wrote:
> 
> > >   I just added some usb-storage devices to my system and got the below.
> 
> > > Why do the first four lines repeat for each device?  (Not sure if
> > > this is a SCSI or USB problem.)
> > 
> > It is in the partition code. I posted twice before about this with no
> > response.
> 
> It's not an important problem, presumably. I observe dual revalidations
> as well, but they are not that bothersome. Add to it that your patch

Yes.

Does this *only* happens for sd (scsi) devices?

> appears wrong (see below). If you offered an acceptable solution, I would
> expect a warmer welcome... But even then getting a reply from linux-scsi
> folks is like pulling a tooth (if my own little CD-ROM sizing patch is
> any indication). So, steel yourself for challenges of this life, Patrick!

;-)

> Here's what it was in 2.6.9, as documented in drivers/block/ub.c:
> 
> +	/*
> +	 * This is a workaround for a specific problem in our block layer.
> +	 * In 2.6.9, register_disk duplicates the code from rescan_partitions.
> +	 * However, if we do add_disk with a device which persistently reports
> +	 * a changed media, add_disk calls register_disk, which does do_open,
> +	 * which will call rescan_paritions for changed media. After that,
> +	 * register_disk attempts to do it all again and causes double kobject
> +	 * registration and a eventually an oops on module removal.
> +	 *
> +	 * The bottom line is, Al Viro says that we should not allow
> +	 * bdev->bd_invalidated to be set when doing add_disk no matter what.
> +	 */

> +	if (sc->first_open) {
> +		if (sc->changed) {
> +			sc->first_open = 0;
> +			rc = -ENOMEDIUM;
> +			goto err_open;
> +		}
> +	}
> 
> Users were hitting it with oopses like these:
> http://www.ussg.iu.edu/hypermail/linux/kernel/0409.2/0011.html
> 
> The ub alone was not suffient to motivate Al for the fix, so I added
> this silly "first_open" thingie, which papered over it. It was thought
> that sd was miraclously immune.
> 
> However, over time users hit it with usb-storage and sd, like this:
>  http://lkml.org/lkml/2004/2/21/19
> This prompted Al's action. He simply dropped all the extra code like
> this:
> 
> --- linux-2.6.9-11.5.EL/fs/partitions/check.c	2004-10-18 14:55:07.000000000 -0700
> +++ linux-2.6.12/fs/partitions/check.c	2005-06-17 12:48:29.000000000 -0700
> @@ -358,24 +357,9 @@ void register_disk(struct gendisk *disk)
>  	if (!bdev)
>  		return;
>  
> +	bdev->bd_invalidated = 1;
>  	if (blkdev_get(bdev, FMODE_READ, 0) < 0)
>  		return;
> -	state = check_partition(disk, bdev);
> -	if (state) {
> -		for (j = 1; j < state->limit; j++) {
> -			sector_t size = state->parts[j].size;
> -			sector_t from = state->parts[j].from;
> -			if (!size)
> -				continue;
> -			add_partition(disk, j, from, size);
> -#ifdef CONFIG_BLK_DEV_MD
> -			if (!state->parts[j].flags)
> -				continue;
> -			md_autodetect_dev(bdev->bd_dev+j);
> -#endif
> -		}
> -		kfree(state);
> -	}
>  	blkdev_put(bdev);
>  }

OK, thanks for posting those links and information.

> > --- linux-2.6.11-rc1/fs/partitions/check.c	Fri Dec 24 13:35:28 2004
> > +++ no-double-sd-linux-2.6.11-rc1/fs/partitions/check.c	Fri Jan 21 11:19:00 2005
> > @@ -375,8 +375,6 @@ int rescan_partitions(struct gendisk *di
> >  	bdev->bd_invalidated = 0;
> >  	for (p = 1; p < disk->minors; p++)
> >  		delete_partition(disk, p);
> > -	if (disk->fops->revalidate_disk)
> > -		disk->fops->revalidate_disk(disk);
> 
> As for your proposed fix, it may be problematic. The ->revalidate
> method has to be called at least once for a new device, because
> that's when drivers fetch the capacities. But ->open only calls
> check_disk_change() for removable devices. Who is going to call
> ->revalidate inside add_disk() for non-removable devices?

sd.c always calls its revalidate_disk method (sd_revalidate_disk) when the
device is attached, so for scsi, we definitely do not miss anything.

I thought revalidate_disk was not called prior to Al's patch, so why do we
need to call it on the first open now?

You already have to call set_capacity() before add_disk(), else
register_disk thinks there is no media present, and won't set
bd_invalidated. So drivers must already get the capacity (or fake it)
prior to calling add_disk.

-- Patrick Mansfield