Date:	Fri, 12 Sep 2008 17:14:04 -0300
From:	Herton Ronaldo Krzesinski <herton@...driva.com.br>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	bogdano@...driva.com.br,
	"Luiz Fernando N. Capitulino" <lcapitulino@...driva.com.br>,
	Abdel Benamrouche <draconux@...il.com>,
	Damien Lallement <dlallement@...driva.com>,
	"pterjan@...driva.com" <pterjan@...driva.com>,
	Jens Axboe <axboe@...nel.dk>
Subject: Re: Partition check considered as error is breaking mounting in 2.6.27

On Friday 12 September 2008 15:40:41 Alan Stern wrote:
> On Fri, 12 Sep 2008, Herton Ronaldo Krzesinski wrote:
> > > > After formatting the camera is this you get from fdisk (display units
> > > > in sectors):
> > > >
> > > > Disk /dev/sdb: 21 MB, 21544448 bytes
> > > > 6 heads, 16 sectors/track, 438 cylinders, total 42079 sectors
> > > > Units = sectors of 1 * 512 = 512 bytes
> > > > Disk identifier: 0x00000000
> > > >
> > > >    Device Boot      Start         End      Blocks   Id  System
> > > > /dev/sdb1   *           1       42079       21039+   1  FAT12
> > > > Partition 1 has different physical/logical endings:
> > > >      phys=(328, 5, 16) logical=(438, 1, 16)
> > > >
> > > > (after zeroing out the partition table I only run fdisk on the
> > > > device, created the a new partition with maximum size allowed and
> > > > change the type to FAT12)
> > >
> > > That is not a valid procedure.  You have to go into Expert mode and set
> > > the number of sectors, heads, and tracks first.  Most likely you'll
> > > want to set them to the same values used by the firmware; it looks like
> > > the firmware thinks there are 16 sectors/track, 8 heads, and 329
> > > tracks.
> >
> > 8 heads? it reports 6 from the fdisk output:
> > phys=(328, 5, 16) logical=(438, 1, 16)
>
> No.  phys=(328, 5, 16) doesn't mean that the device has 329 tracks, 6
> heads, and 16 sectors.  It means that the last sector of the partition
> (as recorded in the table) is track 328, head 5, sector 16.
> Note that partitions don't have to end on cylinder boundaries.
>
> Guessing that the firmware intends there to be 16 sectors per track,
> and also guessing that the 42079 value is too low by one, we get a
> total of 42080 / 16 = 2630 as the product of heads and tracks.  Since
> 2630 = 8 * 328.75 this seems to say that the firmware thinks there are
> approximately 329 tracks and 8 heads.  That explains the "328" above.
>
> Continuing this reasoning, the physical end (328,5,16) corresponds to
> sector (328*8*16) + (5*16) + 15 = 42079 (the 15 is because sector
> numbers start at 1 instead of at 0).  Which would be valid if the first
> sector of the partition was sector 1, or phys=(0,0,2), and if the
> capacity was 42080.  However you didn't tell us what value was stored
> in the table for the start of the partition.

Ok, thanks for the explanation, indeed you are right, it's about the physical 
end, I confused things, I'm not used to CHS... The start sector is 1, 
(0,0,2), so what you said is valid. I attach the dump of first sector of the 
memory here just for reference that shows this.

>
> > > Adding quirks to alter partition sizes sounds very dangerous.  Your
> > > best bet is simply to write a valid partition table.
> >
> > Yes, but this is breaking other devices, not only this olympus camera.
> > Bogdano reported what could be the same case with his cel. phone (Bogdano
> > can you give more details?), also on IRC today Damien Lallement
> > complained about what looks the same issue of "p* exceeds device
> > capacity" (Damien can you give more info too?). But I'm afraid of how
> > much devices this partition error fatal check now can affect, after all
> > it's not "user friendly" to instruct the user to use hexedit or fdisk to
> > fix the device's partitions.
>
> You should complain to the people who wrote and accepted the
> troublesome commit.  Tell them it caused a regression; that always gets
> people's attention!

Ok, here goes a patch that revert the relevant part of the commit and 
goes back to behaviour of previous kernels, and fixes the regression here.
If considered a good solution, can be applied.

---

fs/partition/check.c: revert part of commit 04ebd4aee52b06a2c38127d9208546e5b96f3a19

Fix regression introduced by commit 04ebd4aee52b06a2c38127d9208546e5b96f3a19,
where kernel changed behaviour making fatal the error when some partition
exceeds the limit of the device size. Some buggy devices become inacessible
because of errors in their partition table if the error is fatal.

This closes http://bugzilla.kernel.org/show_bug.cgi?id=11554

diff --git a/fs/partitions/check.c b/fs/partitions/check.c
index 7d6b34e..15c70df 100644
--- a/fs/partitions/check.c
+++ b/fs/partitions/check.c
@@ -499,9 +499,9 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 		if (!size)
 			continue;
 		if (from + size > get_capacity(disk)) {
-			printk(KERN_ERR " %s: p%d exceeds device capacity\n",
+			printk(KERN_WARNING
+				" %s: p%d exceeds device capacity\n",
 				disk->disk_name, p);
-			continue;
 		}
 		res = add_partition(disk, p, from, size, state->parts[p].flags);
 		if (res) {


>
> Alan Stern

-- 
[]'s
Herton

Download attachment "firstsector" of type "application/octet-stream" (512 bytes)

Powered by blists - more mailing lists