[PATCH] msdos: add support for large disks

[PATCH] msdos: add support for large disks

[Daniel Taylor]

In order to use disks larger than 2TiB on Windows XP, it is necessary to use
4096-byte logical sectors in an MBR.

Although the kernel storage and functions called from msdos.c used
"sector_t" internally, msdos.c still used u32 variables, which results in
the ability to handle XP-compatible large disks.

This patch changes the internal variables to "sector_t".

Signed-off-by: Daniel Taylor <daniel.taylor@wdc.com>
---
diff --git a/fs/partitions/msdos.c b/fs/partitions/msdos.c
index 0028d2e..fa9c77d 100644
--- a/fs/partitions/msdos.c
+++ b/fs/partitions/msdos.c
@@ -104,12 +104,12 @@ static int aix_magic_present(unsigned char *p, struct
block_device *bdev)
 
 static void
 parse_extended(struct parsed_partitions *state, struct block_device *bdev,
-			u32 first_sector, u32 first_size)
+			sector_t first_sector, sector_t first_size)
 {
 	struct partition *p;
 	Sector sect;
 	unsigned char *data;
-	u32 this_sector, this_size;
+	sector_t this_sector, this_size;
 	int sector_size = bdev_logical_block_size(bdev) / 512;
 	int loopct = 0;		/* number of links followed
 				   without finding a data partition */
@@ -145,15 +145,16 @@ parse_extended(struct parsed_partitions *state, struct
block_device *bdev,
 		 * First process the data partition(s)
 		 */
 		for (i=0; i<4; i++, p++) {
-			u32 offs, size, next;
+			sector_t offs, size, next;
 			if (!NR_SECTS(p) || is_extended_partition(p))
 				continue;
 
 			/* Check the 3rd and 4th entries -
 			   these sometimes contain random garbage */
-			offs = START_SECT(p)*sector_size;
-			size = NR_SECTS(p)*sector_size;
+			offs =
(sector_t)(START_SECT(p))*(sector_t)(sector_size);
+			size =
(sector_t)(NR_SECTS(p))*(sector_t)(sector_size);
 			next = this_sector + offs;
+
 			if (i >= 2) {
 				if (offs + size > this_size)
 					continue;
@@ -184,8 +185,8 @@ parse_extended(struct parsed_partitions *state, struct
block_device *bdev,
 		if (i == 4)
 			goto done;	 /* nothing left to do */
 
-		this_sector = first_sector + START_SECT(p) * sector_size;
-		this_size = NR_SECTS(p) * sector_size;
+		this_sector = first_sector + (sector_t)(START_SECT(p)) *
sector_size;
+		this_size = (sector_t)(NR_SECTS(p)) *
(sector_t)(sector_size);
 		put_dev_sector(sect);
 	}
 done:
@@ -197,7 +198,7 @@ done:
 
 static void
 parse_solaris_x86(struct parsed_partitions *state, struct block_device
*bdev,
-			u32 offset, u32 size, int origin)
+			sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_SOLARIS_X86_PARTITION
 	Sector sect;
@@ -244,7 +245,7 @@ parse_solaris_x86(struct parsed_partitions *state,
struct block_device *bdev,
  */
 static void
 parse_bsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin, char *flavour,
+		sector_t offset, sector_t size, int origin, char *flavour,
 		int max_partitions)
 {
 	Sector sect;
@@ -263,14 +264,14 @@ parse_bsd(struct parsed_partitions *state, struct
block_device *bdev,
 	if (le16_to_cpu(l->d_npartitions) < max_partitions)
 		max_partitions = le16_to_cpu(l->d_npartitions);
 	for (p = l->d_partitions; p - l->d_partitions < max_partitions; p++)
{
-		u32 bsd_start, bsd_size;
+		sector_t bsd_start, bsd_size;
 
 		if (state->next == state->limit)
 			break;
 		if (p->p_fstype == BSD_FS_UNUSED) 
 			continue;
-		bsd_start = le32_to_cpu(p->p_offset);
-		bsd_size = le32_to_cpu(p->p_size);
+		bsd_start = (sector_t)(le32_to_cpu(p->p_offset));
+		bsd_size = (sector_t)(le32_to_cpu(p->p_size));
 		if (offset == bsd_start && size == bsd_size)
 			/* full parent partition, we have it already */
 			continue;
@@ -290,7 +291,7 @@ parse_bsd(struct parsed_partitions *state, struct
block_device *bdev,
 
 static void
 parse_freebsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_BSD_DISKLABEL
 	parse_bsd(state, bdev, offset, size, origin,
@@ -300,7 +301,7 @@ parse_freebsd(struct parsed_partitions *state, struct
block_device *bdev,
 
 static void
 parse_netbsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_BSD_DISKLABEL
 	parse_bsd(state, bdev, offset, size, origin,
@@ -310,7 +311,7 @@ parse_netbsd(struct parsed_partitions *state, struct
block_device *bdev,
 
 static void
 parse_openbsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_BSD_DISKLABEL
 	parse_bsd(state, bdev, offset, size, origin,
@@ -324,7 +325,7 @@ parse_openbsd(struct parsed_partitions *state, struct
block_device *bdev,
  */
 static void
 parse_unixware(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_UNIXWARE_DISKLABEL
 	Sector sect;
@@ -363,7 +364,7 @@ parse_unixware(struct parsed_partitions *state, struct
block_device *bdev,
  */
 static void
 parse_minix(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_MINIX_SUBPARTITION
 	Sector sect;
@@ -401,7 +402,7 @@ parse_minix(struct parsed_partitions *state, struct
block_device *bdev,
 static struct {
 	unsigned char id;
 	void (*parse)(struct parsed_partitions *, struct block_device *,
-			u32, u32, int);
+			sector_t, sector_t, int);
 } subtypes[] = {
 	{FREEBSD_PARTITION, parse_freebsd},
 	{NETBSD_PARTITION, parse_netbsd},
@@ -483,8 +484,8 @@ int msdos_partition(struct parsed_partitions *state,
struct block_device *bdev)
 
 	state->next = 5;
 	for (slot = 1 ; slot <= 4 ; slot++, p++) {
-		u32 start = START_SECT(p)*sector_size;
-		u32 size = NR_SECTS(p)*sector_size;
+		sector_t start =
(sector_t)(START_SECT(p))*(sector_t)(sector_size);
+		sector_t size =
(sector_t)(NR_SECTS(p))*(sector_t)(sector_size);
 		if (!size)
 			continue;
 		if (is_extended_partition(p)) {
@@ -521,8 +522,8 @@ int msdos_partition(struct parsed_partitions *state,
struct block_device *bdev)
 
 		if (!subtypes[n].parse)
 			continue;
-		subtypes[n].parse(state, bdev, START_SECT(p)*sector_size,
-						NR_SECTS(p)*sector_size,
slot);
+		subtypes[n].parse(state, bdev,
(sector_t)(START_SECT(p))*(sector_t)(sector_size),
+
(sector_t)(NR_SECTS(p))*(sector_t)(sector_size), slot);
 	}
 	put_dev_sector(sect);
 	return 1;

Re: [PATCH] msdos: add support for large disks

[Andrew Morton]

On Wed, 24 Feb 2010 19:17:47 -0800
"Daniel Taylor" <Daniel.Taylor@wdc.com> wrote:

> In order to use disks larger than 2TiB on Windows XP, it is necessary to use
> 4096-byte logical sectors in an MBR.
> 
> Although the kernel storage and functions called from msdos.c used
> "sector_t" internally, msdos.c still used u32 variables, which results in
> the ability to handle XP-compatible large disks.
> 
> This patch changes the internal variables to "sector_t".
> 

Please Cc OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> and myself on
this work.

The patch was really badly wordwrapped.  Please fix up your email
client for next time.

> ---
> diff --git a/fs/partitions/msdos.c b/fs/partitions/msdos.c
> index 0028d2e..fa9c77d 100644
> --- a/fs/partitions/msdos.c
> +++ b/fs/partitions/msdos.c
> @@ -104,12 +104,12 @@ static int aix_magic_present(unsigned char *p, struct
> block_device *bdev)
>  
>  static void
>  parse_extended(struct parsed_partitions *state, struct block_device *bdev,
> -			u32 first_sector, u32 first_size)
> +			sector_t first_sector, sector_t first_size)
>  {
>  	struct partition *p;
>  	Sector sect;
>  	unsigned char *data;
> -	u32 this_sector, this_size;
> +	sector_t this_sector, this_size;
>  	int sector_size = bdev_logical_block_size(bdev) / 512;
>  	int loopct = 0;		/* number of links followed
>  				   without finding a data partition */
> @@ -145,15 +145,16 @@ parse_extended(struct parsed_partitions *state, struct
> block_device *bdev,
>  		 * First process the data partition(s)
>  		 */
>  		for (i=0; i<4; i++, p++) {
> -			u32 offs, size, next;
> +			sector_t offs, size, next;
>  			if (!NR_SECTS(p) || is_extended_partition(p))
>  				continue;
>  
>  			/* Check the 3rd and 4th entries -
>  			   these sometimes contain random garbage */
> -			offs = START_SECT(p)*sector_size;
> -			size = NR_SECTS(p)*sector_size;
> +			offs =
> (sector_t)(START_SECT(p))*(sector_t)(sector_size);
> +			size =
> (sector_t)(NR_SECTS(p))*(sector_t)(sector_size);

The patch would be cleaner if START_SECT() and NR_SECTS() were
returning the correct type.  I suggest converting these to regular C
functions, renaming them to start_sect() and nr_sects() (or something
more descriptive if you like) and making them return a sector_t.

Making local variable `sector_size' have type sector_t would eliminate
the other typecast and is a logical enough thing to do, I think.

>  			next = this_sector + offs;
> +
>  			if (i >= 2) {
>  				if (offs + size > this_size)
>  					continue;
>
> ...
>
> @@ -263,14 +264,14 @@ parse_bsd(struct parsed_partitions *state, struct
> block_device *bdev,
>  	if (le16_to_cpu(l->d_npartitions) < max_partitions)
>  		max_partitions = le16_to_cpu(l->d_npartitions);
>  	for (p = l->d_partitions; p - l->d_partitions < max_partitions; p++)
> {
> -		u32 bsd_start, bsd_size;
> +		sector_t bsd_start, bsd_size;
>  
>  		if (state->next == state->limit)
>  			break;
>  		if (p->p_fstype == BSD_FS_UNUSED) 
>  			continue;
> -		bsd_start = le32_to_cpu(p->p_offset);
> -		bsd_size = le32_to_cpu(p->p_size);
> +		bsd_start = (sector_t)(le32_to_cpu(p->p_offset));
> +		bsd_size = (sector_t)(le32_to_cpu(p->p_size));

I don't see why these two casts are needed?  The compiler will take
care of assigning a u32 to a sector_t.  Perhaps this applies to other
places in the patch.

>  		if (offset == bsd_start && size == bsd_size)
>  			/* full parent partition, we have it already */
>  			continue;
>
> ...
>
> @@ -483,8 +484,8 @@ int msdos_partition(struct parsed_partitions *state,
> struct block_device *bdev)
>  
>  	state->next = 5;
>  	for (slot = 1 ; slot <= 4 ; slot++, p++) {
> -		u32 start = START_SECT(p)*sector_size;
> -		u32 size = NR_SECTS(p)*sector_size;
> +		sector_t start =
> (sector_t)(START_SECT(p))*(sector_t)(sector_size);
> +		sector_t size =
> (sector_t)(NR_SECTS(p))*(sector_t)(sector_size);
>  		if (!size)
>  			continue;
>  		if (is_extended_partition(p)) {
> @@ -521,8 +522,8 @@ int msdos_partition(struct parsed_partitions *state,
> struct block_device *bdev)
>  
>  		if (!subtypes[n].parse)
>  			continue;
> -		subtypes[n].parse(state, bdev, START_SECT(p)*sector_size,
> -						NR_SECTS(p)*sector_size,
> slot);
> +		subtypes[n].parse(state, bdev,
> (sector_t)(START_SECT(p))*(sector_t)(sector_size),
> +
> (sector_t)(NR_SECTS(p))*(sector_t)(sector_size), slot);
>  	}
>  	put_dev_sector(sect);
>  	return 1;

Again, we can avoid some/all of this casting by more appropriate
selection of types.

Re: [PATCH] msdos: add support for large disks

[OGAWA Hirofumi]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Wed, 24 Feb 2010 19:17:47 -0800
> "Daniel Taylor" <Daniel.Taylor@wdc.com> wrote:
>
>> In order to use disks larger than 2TiB on Windows XP, it is necessary to use
>> 4096-byte logical sectors in an MBR.

BTW, you can use GPT instead?

And to make sure, "4096-byte logical sectors in an MBR" is meaning, the
storage must be supporting 4096B and is using it?

>> Although the kernel storage and functions called from msdos.c used
>> "sector_t" internally, msdos.c still used u32 variables, which results in
>> the ability to handle XP-compatible large disks.
>> 
>> This patch changes the internal variables to "sector_t".
>> 
>
> Please Cc OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> and myself on
> this work.

BTW, to make sure, this is fs/partitions/, not fs/fat/. So, I'm not
sure, my knowledge is enough or not, and/or you just missed
fs/partitions/.

Well, I'll make time to check. BTW, basically it sounds good to me
except implementation details. But, I'd like to compare and check other
implement a bit if possible. IIRC, this is not a well defined
specification.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

RE: [PATCH] msdos: add support for large disks

[Daniel Taylor]

 

-----Original Message-----
From: OGAWA Hirofumi [mailto:hirofumi@mail.parknet.co.jp] 
Sent: Wednesday, March 03, 2010 6:02 AM
To: Andrew Morton
Cc: Daniel Taylor; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] msdos: add support for large disks

Andrew Morton <akpm@linux-foundation.org> writes:

>> On Wed, 24 Feb 2010 19:17:47 -0800
>> "Daniel Taylor" <Daniel.Taylor@wdc.com> wrote:
>>
>>> In order to use disks larger than 2TiB on Windows XP, it is necessary 
>>> to use 4096-byte logical sectors in an MBR.

> BTW, you can use GPT instead?

Windows XP does not support GPT, although Vista/7 do.

> And to make sure, "4096-byte logical sectors in an MBR" is meaning, the
storage must be supporting 4096B and is using it?

The drives report physical/logical sector sizes of 4096 bytes, which the
kernel correctly detects.  32-bit fields in the MBR are then sufficient for
storage capacites >2TiB (up to 16 TiB), and the storage in struct
parsed_partitions is already sector_t.

>>> Although the kernel storage and functions called from msdos.c used 
>>> "sector_t" internally, msdos.c still used u32 variables, which 
>>> results in the ability to handle XP-compatible large disks.
>>> 
>>> This patch changes the internal variables to "sector_t".
>>> 
>>
>> Please Cc OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> and myself on 
>> this work.

>> BTW, to make sure, this is fs/partitions/, not fs/fat/. So, I'm not sure,
my knowledge is enough or not, and/or you just missed fs/partitions/.

This is definitely a change in fs/partitions (fs/partitions/msdos.c).  I
could not find a maintainer for fs/partitions, but FAT and MBR are vaguely
related.  Andrew Morton asked me to copy you on the change, also.

> Well, I'll make time to check. BTW, basically it sounds good to me except
implementation details. But, I'd like to compare > and check other implement
a bit if possible. IIRC, this is not a well defined specification.

Sanity checking patches is a good idea, particularly from someone who has no
other significant change in place.

> Thanks.
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] msdos: add support for large disks

[H. Peter Anvin]

On 03/03/2010 02:50 PM, Daniel Taylor wrote:
> 
> The drives report physical/logical sector sizes of 4096 bytes, which the
> kernel correctly detects.  32-bit fields in the MBR are then sufficient for
> storage capacites >2TiB (up to 16 TiB), and the storage in struct
> parsed_partitions is already sector_t.
> 

It's probably worth noting that such media:

a) will not work with anything that is based on disk images;
b) will not be bootable on most existing systems.

Both of these are going to be issues for people in real-life use (and
yes, people boot from USB disks -- trust me, when it doesn't work they
send the bug reports to *me* rather than to the hw/bios vendors.)  Of
course, so is XP compatibility.

	-hpa

RE: [PATCH] msdos: add support for large disks

[Daniel Taylor]

 

-----Original Message-----
From: H. Peter Anvin [mailto:hpa@zytor.com] 
Sent: Wednesday, March 03, 2010 3:05 PM
To: Daniel Taylor
Cc: OGAWA Hirofumi; Andrew Morton; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] msdos: add support for large disks

On 03/03/2010 02:50 PM, Daniel Taylor wrote:
>> 
>> The drives report physical/logical sector sizes of 4096 bytes, which 
>> the kernel correctly detects.  32-bit fields in the MBR are then 
>> sufficient for storage capacites >2TiB (up to 16 TiB), and the storage 
>> in struct parsed_partitions is already sector_t.
>> 

> It's probably worth noting that such media:

> a) will not work with anything that is based on disk images;
> b) will not be bootable on most existing systems.

> Both of these are going to be issues for people in real-life use (and yes,
people boot from USB disks -- trust me, when it > doesn't work they send the
bug reports to *me* rather than to the hw/bios vendors.)  Of course, so is
XP compatibility.

> 	-hpa

Those are good points.  We (WD) are trying to get the NDAs, regarding our
specific not-yet-released products, in place to get the userspace utilities
updated, and WD is also in communication with the Windows tool developers.

Fundamentally, though, the way Windows XP, which only supports MBR
partitioning, can use a disk drive capacity larger than 2 TiB is with sector
sizes larger than 512 bytes.  That is far too large a slice of the installed
base for the disk drive makers to ignore, so these drives and partitions are
going to be released, whether by WD, or someone else.  When someone plugs
one of these disks into a system running Linux, we don't need to hear about
"Linux doesn't support it", if there's a reasonable accomodation.  In this
case, both the inputs MBR partitioning, and the data stored as a result, are
fully capable.  It is only the intermediate variables in the module that are
currently hard-coded to 32-bits, and which truncate the values.

dlt

Re: [PATCH] msdos: add support for large disks

[H. Peter Anvin]

On 03/03/2010 03:24 PM, Daniel Taylor wrote:
> 
> Those are good points.  We (WD) are trying to get the NDAs, regarding our
> specific not-yet-released products, in place to get the userspace utilities
> updated, and WD is also in communication with the Windows tool developers.
> 
> Fundamentally, though, the way Windows XP, which only supports MBR
> partitioning, can use a disk drive capacity larger than 2 TiB is with sector
> sizes larger than 512 bytes.  That is far too large a slice of the installed
> base for the disk drive makers to ignore, so these drives and partitions are
> going to be released, whether by WD, or someone else.  When someone plugs
> one of these disks into a system running Linux, we don't need to hear about
> "Linux doesn't support it", if there's a reasonable accomodation.  In this
> case, both the inputs MBR partitioning, and the data stored as a result, are
> fully capable.  It is only the intermediate variables in the module that are
> currently hard-coded to 32-bits, and which truncate the values.
> 

I should probably clarify that I was not, in any way, objecting to the
patch; quite on the contrary.

	-hpa

Re: [PATCH] msdos: add support for large disks

[OGAWA Hirofumi]

"Daniel Taylor" <Daniel.Taylor@wdc.com> writes:

> It is only the intermediate variables in the module that are
> currently hard-coded to 32-bits, and which truncate the values.

BTW, it may not be only 32-bits truncate issue. IIRC, e.g. some
implement is checking the 0xaa mark at the end of sector. I forgot
almost though, IIRC, Windows writes 0xaa mark to the both of "end of
sector" and "end of 512" for compatibility.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] msdos: add support for large disks

[H. Peter Anvin]

On 03/04/2010 01:18 AM, OGAWA Hirofumi wrote:
> "Daniel Taylor"<Daniel.Taylor@wdc.com>  writes:
>
>> It is only the intermediate variables in the module that are
>> currently hard-coded to 32-bits, and which truncate the values.
>
> BTW, it may not be only 32-bits truncate issue. IIRC, e.g. some
> implement is checking the 0xaa mark at the end of sector. I forgot
> almost though, IIRC, Windows writes 0xaa mark to the both of "end of
> sector" and "end of 512" for compatibility.
>
> Thanks.

The spec is that the mark, partition table etc. at the end of 512 bytes; 
of course, a lot of people go that wrong.

	-hpa

Re: [PATCH] msdos: add support for large disks

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> BTW, to make sure, this is fs/partitions/, not fs/fat/. So, I'm not
> sure, my knowledge is enough or not, and/or you just missed
> fs/partitions/.
>
> Well, I'll make time to check. BTW, basically it sounds good to me
> except implementation details. But, I'd like to compare and check other
> implement a bit if possible. IIRC, this is not a well defined
> specification.

I reviewed/tested more or less on [RESUBMIT PATCH] patch. And rediff
with fixed patch is attached.

> Signed-off-by: Daniel Taylor <daniel.taylor@wdc.com> diff --git
> a/fs/partitions/msdos.c b/fs/partitions/msdos.c index 0028d2e..039bb61
> 100644

This patch is broken like this way. I couldn't apply this, so I applied
by hand this time though, it can be easily to miss. Please fix your
mailer (or if not possible to fix, attach is better off than broken, at
least for me).

> @@ -348,7 +355,8 @@ parse_unixware(struct parsed_partitions *state, struct
> block_device *bdev,
>  
>  		if (p->s_label != UNIXWARE_FS_UNUSED)
>  			put_partition(state, state->next++,
> -						START_SECT(p), NR_SECTS(p));
> +				      start_sect((struct partition *)p),
> +				      nr_sects((struct partition *)p));

This is a wrong conversion for macro to function. "p" is not "struct
partition" in here. And it seems we have no reason to use
get_unaligned() for this, so just use le32_to_cpu() instead.


And related to > 1024b sector (this is not new for this patch though),

			/* prevent someone doing mkfs or mkswap on an
			   extended partition, but leave room for LILO */
			put_partition(state, slot, start, size == 1 ? 1 : 2);

This part is broken. If "size" is smaller than minimum blocksize (note,
"size" is number of 512b, not number of blocksize), we can't read any
block after all (it is handled as outside of i_size by bdev).

Peter (or someone) know what do we want to in here? This is on extended
partition itself (e.g. if sda1 sda2 <sda5 sda6>, it's the size of sda2)

[BTW, it seems subtypes stuff is not thinking about sector size > 512b...]

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>



From: "Daniel Taylor" <Daniel.Taylor@wdc.com>

In order to use disks larger than 2TiB on Windows XP, it is necessary
to use 4096-byte logical sectors in an MBR.

Although the kernel storage and functions called from msdos.c used
"sector_t" internally, msdos.c still used u32 variables, which results
in the ability to handle XP-compatible large disks.

This patch changes the internal variables to "sector_t".

Signed-off-by: Daniel Taylor <daniel.taylor@wdc.com>
[tweaks and fix]
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/partitions/msdos.c |   74 +++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 35 deletions(-)

diff -puN fs/partitions/msdos.c~part-support-4k-block fs/partitions/msdos.c
--- linux-2.6/fs/partitions/msdos.c~part-support-4k-block	2010-03-07 04:10:48.000000000 +0900
+++ linux-2.6-hirofumi/fs/partitions/msdos.c	2010-03-07 19:41:56.000000000 +0900
@@ -31,14 +31,17 @@
  */
 #include <asm/unaligned.h>
 
-#define SYS_IND(p)	(get_unaligned(&p->sys_ind))
-#define NR_SECTS(p)	({ __le32 __a =	get_unaligned(&p->nr_sects);	\
-				le32_to_cpu(__a); \
-			})
-
-#define START_SECT(p)	({ __le32 __a =	get_unaligned(&p->start_sect);	\
-				le32_to_cpu(__a); \
-			})
+#define SYS_IND(p)	get_unaligned(&p->sys_ind)
+
+static inline sector_t nr_sects(struct partition *p)
+{
+	return (sector_t)get_unaligned_le32(&p->nr_sects);
+}
+
+static inline sector_t start_sect(struct partition *p)
+{
+	return (sector_t)get_unaligned_le32(&p->start_sect);
+}
 
 static inline int is_extended_partition(struct partition *p)
 {
@@ -104,13 +107,13 @@ static int aix_magic_present(unsigned ch
 
 static void
 parse_extended(struct parsed_partitions *state, struct block_device *bdev,
-			u32 first_sector, u32 first_size)
+			sector_t first_sector, sector_t first_size)
 {
 	struct partition *p;
 	Sector sect;
 	unsigned char *data;
-	u32 this_sector, this_size;
-	int sector_size = bdev_logical_block_size(bdev) / 512;
+	sector_t this_sector, this_size;
+	sector_t sector_size = bdev_logical_block_size(bdev) / 512;
 	int loopct = 0;		/* number of links followed
 				   without finding a data partition */
 	int i;
@@ -145,14 +148,14 @@ parse_extended(struct parsed_partitions
 		 * First process the data partition(s)
 		 */
 		for (i=0; i<4; i++, p++) {
-			u32 offs, size, next;
-			if (!NR_SECTS(p) || is_extended_partition(p))
+			sector_t offs, size, next;
+			if (!nr_sects(p) || is_extended_partition(p))
 				continue;
 
 			/* Check the 3rd and 4th entries -
 			   these sometimes contain random garbage */
-			offs = START_SECT(p)*sector_size;
-			size = NR_SECTS(p)*sector_size;
+			offs = start_sect(p)*sector_size;
+			size = nr_sects(p)*sector_size;
 			next = this_sector + offs;
 			if (i >= 2) {
 				if (offs + size > this_size)
@@ -179,13 +182,13 @@ parse_extended(struct parsed_partitions
 		 */
 		p -= 4;
 		for (i=0; i<4; i++, p++)
-			if (NR_SECTS(p) && is_extended_partition(p))
+			if (nr_sects(p) && is_extended_partition(p))
 				break;
 		if (i == 4)
 			goto done;	 /* nothing left to do */
 
-		this_sector = first_sector + START_SECT(p) * sector_size;
-		this_size = NR_SECTS(p) * sector_size;
+		this_sector = first_sector + start_sect(p) * sector_size;
+		this_size = nr_sects(p) * sector_size;
 		put_dev_sector(sect);
 	}
 done:
@@ -197,7 +200,7 @@ done:
 
 static void
 parse_solaris_x86(struct parsed_partitions *state, struct block_device *bdev,
-			u32 offset, u32 size, int origin)
+			sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_SOLARIS_X86_PARTITION
 	Sector sect;
@@ -244,7 +247,7 @@ parse_solaris_x86(struct parsed_partitio
  */
 static void
 parse_bsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin, char *flavour,
+		sector_t offset, sector_t size, int origin, char *flavour,
 		int max_partitions)
 {
 	Sector sect;
@@ -263,7 +266,7 @@ parse_bsd(struct parsed_partitions *stat
 	if (le16_to_cpu(l->d_npartitions) < max_partitions)
 		max_partitions = le16_to_cpu(l->d_npartitions);
 	for (p = l->d_partitions; p - l->d_partitions < max_partitions; p++) {
-		u32 bsd_start, bsd_size;
+		sector_t bsd_start, bsd_size;
 
 		if (state->next == state->limit)
 			break;
@@ -290,7 +293,7 @@ parse_bsd(struct parsed_partitions *stat
 
 static void
 parse_freebsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_BSD_DISKLABEL
 	parse_bsd(state, bdev, offset, size, origin,
@@ -300,7 +303,7 @@ parse_freebsd(struct parsed_partitions *
 
 static void
 parse_netbsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_BSD_DISKLABEL
 	parse_bsd(state, bdev, offset, size, origin,
@@ -310,7 +313,7 @@ parse_netbsd(struct parsed_partitions *s
 
 static void
 parse_openbsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_BSD_DISKLABEL
 	parse_bsd(state, bdev, offset, size, origin,
@@ -324,7 +327,7 @@ parse_openbsd(struct parsed_partitions *
  */
 static void
 parse_unixware(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_UNIXWARE_DISKLABEL
 	Sector sect;
@@ -348,7 +351,8 @@ parse_unixware(struct parsed_partitions
 
 		if (p->s_label != UNIXWARE_FS_UNUSED)
 			put_partition(state, state->next++,
-						START_SECT(p), NR_SECTS(p));
+				      le32_to_cpu(p->start_sect),
+				      le32_to_cpu(p->nr_sects));
 		p++;
 	}
 	put_dev_sector(sect);
@@ -363,7 +367,7 @@ parse_unixware(struct parsed_partitions
  */
 static void
 parse_minix(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_MINIX_SUBPARTITION
 	Sector sect;
@@ -390,7 +394,7 @@ parse_minix(struct parsed_partitions *st
 			/* add each partition in use */
 			if (SYS_IND(p) == MINIX_PARTITION)
 				put_partition(state, state->next++,
-					      START_SECT(p), NR_SECTS(p));
+					      start_sect(p), nr_sects(p));
 		}
 		printk(" >\n");
 	}
@@ -401,7 +405,7 @@ parse_minix(struct parsed_partitions *st
 static struct {
 	unsigned char id;
 	void (*parse)(struct parsed_partitions *, struct block_device *,
-			u32, u32, int);
+			sector_t, sector_t, int);
 } subtypes[] = {
 	{FREEBSD_PARTITION, parse_freebsd},
 	{NETBSD_PARTITION, parse_netbsd},
@@ -415,7 +419,7 @@ static struct {
  
 int msdos_partition(struct parsed_partitions *state, struct block_device *bdev)
 {
-	int sector_size = bdev_logical_block_size(bdev) / 512;
+	sector_t sector_size = bdev_logical_block_size(bdev) / 512;
 	Sector sect;
 	unsigned char *data;
 	struct partition *p;
@@ -483,8 +487,8 @@ int msdos_partition(struct parsed_partit
 
 	state->next = 5;
 	for (slot = 1 ; slot <= 4 ; slot++, p++) {
-		u32 start = START_SECT(p)*sector_size;
-		u32 size = NR_SECTS(p)*sector_size;
+		sector_t start = start_sect(p)*sector_size;
+		sector_t size = nr_sects(p)*sector_size;
 		if (!size)
 			continue;
 		if (is_extended_partition(p)) {
@@ -513,7 +517,7 @@ int msdos_partition(struct parsed_partit
 		unsigned char id = SYS_IND(p);
 		int n;
 
-		if (!NR_SECTS(p))
+		if (!nr_sects(p))
 			continue;
 
 		for (n = 0; subtypes[n].parse && id != subtypes[n].id; n++)
@@ -521,8 +525,8 @@ int msdos_partition(struct parsed_partit
 
 		if (!subtypes[n].parse)
 			continue;
-		subtypes[n].parse(state, bdev, START_SECT(p)*sector_size,
-						NR_SECTS(p)*sector_size, slot);
+		subtypes[n].parse(state, bdev, start_sect(p)*sector_size,
+						nr_sects(p)*sector_size, slot);
 	}
 	put_dev_sector(sect);
 	return 1;
_

Re: [PATCH] msdos: add support for large disks

[H. Peter Anvin]

On 03/07/2010 02:59 AM, OGAWA Hirofumi wrote:
> This part is broken. If "size" is smaller than minimum blocksize (note,
> "size" is number of 512b, not number of blocksize), we can't read any
> block after all (it is handled as outside of i_size by bdev).
>
> Peter (or someone) know what do we want to in here? This is on extended
 > partition itself (e.g. if sda1 sda2<sda5 sda6>, it's the size of sda2)

We should presumably set it to one logical sector, regardless of size. 
Either that, or the offset of the lowest contained partition.

> [BTW, it seems subtypes stuff is not thinking about sector size>  512b...]

Much else isn't, either.  With the exception of some MO disks, it was 
nearly unheard of for 20 years.

	-hpa

Re: [PATCH] msdos: add support for large disks

[OGAWA Hirofumi]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 03/07/2010 02:59 AM, OGAWA Hirofumi wrote:
>> This part is broken. If "size" is smaller than minimum blocksize (note,
>> "size" is number of 512b, not number of blocksize), we can't read any
>> block after all (it is handled as outside of i_size by bdev).
>>
>> Peter (or someone) know what do we want to in here? This is on extended
>  > partition itself (e.g. if sda1 sda2<sda5 sda6>, it's the size of sda2)
>
> We should presumably set it to one logical sector, regardless of size. 
> Either that, or the offset of the lowest contained partition.

It means,

	min(size, max(logical_sector_size / 512, 2));

or like this? ("size" is "nr_sects(part) * (logical_sector_size/512)")

This should keep old logic, i.e. if partition is bigger than 1kb, it
uses 1kb for (probably) LILO mentioned by comment.

>> [BTW, it seems subtypes stuff is not thinking about sector size>  512b...]
>
> Much else isn't, either.  With the exception of some MO disks, it was 
> nearly unheard of for 20 years.

Yes. So, well, if someone know the spec of those subtypes, let us know.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] msdos: add support for large disks

[H. Peter Anvin]

On 03/07/2010 04:09 PM, OGAWA Hirofumi wrote:
>
> It means,
>
> 	min(size, max(logical_sector_size / 512, 2));
>
> or like this? ("size" is "nr_sects(part) * (logical_sector_size/512)")
>
> This should keep old logic, i.e. if partition is bigger than 1kb, it
> uses 1kb for (probably) LILO mentioned by comment.
>

I don't really know where the 1K cames from, and it isn't necessarily 
safe, either.

	-hpa

Re: [PATCH] msdos: add support for large disks

[OGAWA Hirofumi]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 03/07/2010 04:09 PM, OGAWA Hirofumi wrote:
>>
>> It means,
>>
>> 	min(size, max(logical_sector_size / 512, 2));
>>
>> or like this? ("size" is "nr_sects(part) * (logical_sector_size/512)")
>>
>> This should keep old logic, i.e. if partition is bigger than 1kb, it
>> uses 1kb for (probably) LILO mentioned by comment.
>>
>
> I don't really know where the 1K cames from, and it isn't necessarily 
> safe, either.

Um..., I see. However, it wouldn't also be the reason to remove this 1kb
logic. So, I'll just use the above code with some comments for now,
it'll be by separated incremental patch though.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[PATCH 1/2] fs/partitions/msdos: add support for large disks

[OGAWA Hirofumi]

Hi,

This is rediff with fix of "Daniel Taylor" <Daniel.Taylor@wdc.com>.
Daniel, please check I'm breaking your patch.



From: "Daniel Taylor" <Daniel.Taylor@wdc.com>

In order to use disks larger than 2TiB on Windows XP, it is necessary
to use 4096-byte logical sectors in an MBR.

Although the kernel storage and functions called from msdos.c used
"sector_t" internally, msdos.c still used u32 variables, which results
in the ability to handle XP-compatible large disks.

This patch changes the internal variables to "sector_t".

Signed-off-by: Daniel Taylor <daniel.taylor@wdc.com>
[tweaks and fix]
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/partitions/msdos.c |   74 +++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 35 deletions(-)

diff -puN fs/partitions/msdos.c~part-support-4k-block fs/partitions/msdos.c
--- linux-2.6/fs/partitions/msdos.c~part-support-4k-block	2010-03-07 04:10:48.000000000 +0900
+++ linux-2.6-hirofumi/fs/partitions/msdos.c	2010-03-07 19:41:56.000000000 +0900
@@ -31,14 +31,17 @@
  */
 #include <asm/unaligned.h>
 
-#define SYS_IND(p)	(get_unaligned(&p->sys_ind))
-#define NR_SECTS(p)	({ __le32 __a =	get_unaligned(&p->nr_sects);	\
-				le32_to_cpu(__a); \
-			})
-
-#define START_SECT(p)	({ __le32 __a =	get_unaligned(&p->start_sect);	\
-				le32_to_cpu(__a); \
-			})
+#define SYS_IND(p)	get_unaligned(&p->sys_ind)
+
+static inline sector_t nr_sects(struct partition *p)
+{
+	return (sector_t)get_unaligned_le32(&p->nr_sects);
+}
+
+static inline sector_t start_sect(struct partition *p)
+{
+	return (sector_t)get_unaligned_le32(&p->start_sect);
+}
 
 static inline int is_extended_partition(struct partition *p)
 {
@@ -104,13 +107,13 @@ static int aix_magic_present(unsigned ch
 
 static void
 parse_extended(struct parsed_partitions *state, struct block_device *bdev,
-			u32 first_sector, u32 first_size)
+			sector_t first_sector, sector_t first_size)
 {
 	struct partition *p;
 	Sector sect;
 	unsigned char *data;
-	u32 this_sector, this_size;
-	int sector_size = bdev_logical_block_size(bdev) / 512;
+	sector_t this_sector, this_size;
+	sector_t sector_size = bdev_logical_block_size(bdev) / 512;
 	int loopct = 0;		/* number of links followed
 				   without finding a data partition */
 	int i;
@@ -145,14 +148,14 @@ parse_extended(struct parsed_partitions
 		 * First process the data partition(s)
 		 */
 		for (i=0; i<4; i++, p++) {
-			u32 offs, size, next;
-			if (!NR_SECTS(p) || is_extended_partition(p))
+			sector_t offs, size, next;
+			if (!nr_sects(p) || is_extended_partition(p))
 				continue;
 
 			/* Check the 3rd and 4th entries -
 			   these sometimes contain random garbage */
-			offs = START_SECT(p)*sector_size;
-			size = NR_SECTS(p)*sector_size;
+			offs = start_sect(p)*sector_size;
+			size = nr_sects(p)*sector_size;
 			next = this_sector + offs;
 			if (i >= 2) {
 				if (offs + size > this_size)
@@ -179,13 +182,13 @@ parse_extended(struct parsed_partitions
 		 */
 		p -= 4;
 		for (i=0; i<4; i++, p++)
-			if (NR_SECTS(p) && is_extended_partition(p))
+			if (nr_sects(p) && is_extended_partition(p))
 				break;
 		if (i == 4)
 			goto done;	 /* nothing left to do */
 
-		this_sector = first_sector + START_SECT(p) * sector_size;
-		this_size = NR_SECTS(p) * sector_size;
+		this_sector = first_sector + start_sect(p) * sector_size;
+		this_size = nr_sects(p) * sector_size;
 		put_dev_sector(sect);
 	}
 done:
@@ -197,7 +200,7 @@ done:
 
 static void
 parse_solaris_x86(struct parsed_partitions *state, struct block_device *bdev,
-			u32 offset, u32 size, int origin)
+			sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_SOLARIS_X86_PARTITION
 	Sector sect;
@@ -244,7 +247,7 @@ parse_solaris_x86(struct parsed_partitio
  */
 static void
 parse_bsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin, char *flavour,
+		sector_t offset, sector_t size, int origin, char *flavour,
 		int max_partitions)
 {
 	Sector sect;
@@ -263,7 +266,7 @@ parse_bsd(struct parsed_partitions *stat
 	if (le16_to_cpu(l->d_npartitions) < max_partitions)
 		max_partitions = le16_to_cpu(l->d_npartitions);
 	for (p = l->d_partitions; p - l->d_partitions < max_partitions; p++) {
-		u32 bsd_start, bsd_size;
+		sector_t bsd_start, bsd_size;
 
 		if (state->next == state->limit)
 			break;
@@ -290,7 +293,7 @@ parse_bsd(struct parsed_partitions *stat
 
 static void
 parse_freebsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_BSD_DISKLABEL
 	parse_bsd(state, bdev, offset, size, origin,
@@ -300,7 +303,7 @@ parse_freebsd(struct parsed_partitions *
 
 static void
 parse_netbsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_BSD_DISKLABEL
 	parse_bsd(state, bdev, offset, size, origin,
@@ -310,7 +313,7 @@ parse_netbsd(struct parsed_partitions *s
 
 static void
 parse_openbsd(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_BSD_DISKLABEL
 	parse_bsd(state, bdev, offset, size, origin,
@@ -324,7 +327,7 @@ parse_openbsd(struct parsed_partitions *
  */
 static void
 parse_unixware(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_UNIXWARE_DISKLABEL
 	Sector sect;
@@ -348,7 +351,8 @@ parse_unixware(struct parsed_partitions
 
 		if (p->s_label != UNIXWARE_FS_UNUSED)
 			put_partition(state, state->next++,
-						START_SECT(p), NR_SECTS(p));
+				      le32_to_cpu(p->start_sect),
+				      le32_to_cpu(p->nr_sects));
 		p++;
 	}
 	put_dev_sector(sect);
@@ -363,7 +367,7 @@ parse_unixware(struct parsed_partitions
  */
 static void
 parse_minix(struct parsed_partitions *state, struct block_device *bdev,
-		u32 offset, u32 size, int origin)
+		sector_t offset, sector_t size, int origin)
 {
 #ifdef CONFIG_MINIX_SUBPARTITION
 	Sector sect;
@@ -390,7 +394,7 @@ parse_minix(struct parsed_partitions *st
 			/* add each partition in use */
 			if (SYS_IND(p) == MINIX_PARTITION)
 				put_partition(state, state->next++,
-					      START_SECT(p), NR_SECTS(p));
+					      start_sect(p), nr_sects(p));
 		}
 		printk(" >\n");
 	}
@@ -401,7 +405,7 @@ parse_minix(struct parsed_partitions *st
 static struct {
 	unsigned char id;
 	void (*parse)(struct parsed_partitions *, struct block_device *,
-			u32, u32, int);
+			sector_t, sector_t, int);
 } subtypes[] = {
 	{FREEBSD_PARTITION, parse_freebsd},
 	{NETBSD_PARTITION, parse_netbsd},
@@ -415,7 +419,7 @@ static struct {
  
 int msdos_partition(struct parsed_partitions *state, struct block_device *bdev)
 {
-	int sector_size = bdev_logical_block_size(bdev) / 512;
+	sector_t sector_size = bdev_logical_block_size(bdev) / 512;
 	Sector sect;
 	unsigned char *data;
 	struct partition *p;
@@ -483,8 +487,8 @@ int msdos_partition(struct parsed_partit
 
 	state->next = 5;
 	for (slot = 1 ; slot <= 4 ; slot++, p++) {
-		u32 start = START_SECT(p)*sector_size;
-		u32 size = NR_SECTS(p)*sector_size;
+		sector_t start = start_sect(p)*sector_size;
+		sector_t size = nr_sects(p)*sector_size;
 		if (!size)
 			continue;
 		if (is_extended_partition(p)) {
@@ -513,7 +517,7 @@ int msdos_partition(struct parsed_partit
 		unsigned char id = SYS_IND(p);
 		int n;
 
-		if (!NR_SECTS(p))
+		if (!nr_sects(p))
 			continue;
 
 		for (n = 0; subtypes[n].parse && id != subtypes[n].id; n++)
@@ -521,8 +525,8 @@ int msdos_partition(struct parsed_partit
 
 		if (!subtypes[n].parse)
 			continue;
-		subtypes[n].parse(state, bdev, START_SECT(p)*sector_size,
-						NR_SECTS(p)*sector_size, slot);
+		subtypes[n].parse(state, bdev, start_sect(p)*sector_size,
+						nr_sects(p)*sector_size, slot);
 	}
 	put_dev_sector(sect);
 	return 1;
_

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[OGAWA Hirofumi]

We can drop this patch, if we want to delay until someone notice this
and have some real requests to this.




Smaller size than a minimum blocksize can't be used, after all it's
handled like 0 size.

For extended partition itself, this makes sure to use bigger size than
one logical sector size at least.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/partitions/msdos.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff -puN fs/partitions/msdos.c~part-support-4k-block-fix fs/partitions/msdos.c
--- linux-2.6/fs/partitions/msdos.c~part-support-4k-block-fix	2010-03-08 20:11:39.000000000 +0900
+++ linux-2.6-hirofumi/fs/partitions/msdos.c	2010-03-08 20:32:19.000000000 +0900
@@ -492,9 +492,16 @@ int msdos_partition(struct parsed_partit
 		if (!size)
 			continue;
 		if (is_extended_partition(p)) {
-			/* prevent someone doing mkfs or mkswap on an
-			   extended partition, but leave room for LILO */
-			put_partition(state, slot, start, size == 1 ? 1 : 2);
+			/*
+			 * prevent someone doing mkfs or mkswap on an
+			 * extended partition, but leave room for LILO
+			 * FIXME: this uses one logical sector for > 512b
+			 * sector, although it may not be enough/proper.
+			 */
+			sector_t n = 2;
+			n = min(size, max(sector_size, n));
+			put_partition(state, slot, start, n);
+
 			printk(" <");
 			parse_extended(state, bdev, start, size);
 			printk(" >");
_

--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

RE: [PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[Daniel Taylor]

In the near future, WD will be releasing products that need this patch.

Wouldn't it be a better Linux user experience to never have the problem,
rather than wait for a bug-fix cycle on the kernel?

OTOH, it would be reasonable to wait until someone else had a chance to
test the change.  We are awaiting NDAs from RedHat, Canonical, and
Novell/SUSE to send them the affected products for library/application
development.

Regards,

Dan 

> -----Original Message-----
> From: OGAWA Hirofumi [mailto:hirofumi@mail.parknet.co.jp] 
> Sent: Wednesday, March 10, 2010 7:52 AM
> To: Andrew Morton
> Cc: H. Peter Anvin; Daniel Taylor; linux-kernel@vger.kernel.org
> Subject: [PATCH 2/2] fs/partition/msdos: Fix unusable 
> extended partition for > 512B sector
> 
> We can drop this patch, if we want to delay until someone notice this
> and have some real requests to this.
> 
> 
> 
> 
> Smaller size than a minimum blocksize can't be used, after all it's
> handled like 0 size.
> 
> For extended partition itself, this makes sure to use bigger size than
> one logical sector size at least.
> 
> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> ---
> 
>  fs/partitions/msdos.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff -puN fs/partitions/msdos.c~part-support-4k-block-fix 
> fs/partitions/msdos.c
> --- linux-2.6/fs/partitions/msdos.c~part-support-4k-block-fix	
> 2010-03-08 20:11:39.000000000 +0900
> +++ linux-2.6-hirofumi/fs/partitions/msdos.c	2010-03-08 
> 20:32:19.000000000 +0900
> @@ -492,9 +492,16 @@ int msdos_partition(struct parsed_partit
>  		if (!size)
>  			continue;
>  		if (is_extended_partition(p)) {
> -			/* prevent someone doing mkfs or mkswap on an
> -			   extended partition, but leave room 
> for LILO */
> -			put_partition(state, slot, start, size 
> == 1 ? 1 : 2);
> +			/*
> +			 * prevent someone doing mkfs or mkswap on an
> +			 * extended partition, but leave room for LILO
> +			 * FIXME: this uses one logical sector 
> for > 512b
> +			 * sector, although it may not be enough/proper.
> +			 */
> +			sector_t n = 2;
> +			n = min(size, max(sector_size, n));
> +			put_partition(state, slot, start, n);
> +
>  			printk(" <");
>  			parse_extended(state, bdev, start, size);
>  			printk(" >");
> _
> 
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> 

Re: [PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[OGAWA Hirofumi]

"Daniel Taylor" <Daniel.Taylor@wdc.com> writes:

> In the near future, WD will be releasing products that need this patch.
>
> Wouldn't it be a better Linux user experience to never have the problem,
> rather than wait for a bug-fix cycle on the kernel?

Of course, if we can fix, it's better.

However, probably, users of this patch would be only boot loader,
because this is a first sector on extended-partition itself, not
logical-partitions in extended-partition.

So, maybe this wouldn't be so major problem for normal users, and what
is needed actually is not sure to me for now (I guess it might be
depending to BIOS, if boot loader is using BIOS call.).

So, this patch provides one logical sector, but it's just guess.

> OTOH, it would be reasonable to wait until someone else had a chance to
> test the change.  We are awaiting NDAs from RedHat, Canonical, and
> Novell/SUSE to send them the affected products for library/application
> development.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

RE: [PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[Daniel Taylor]

> -----Original Message-----
> From: OGAWA Hirofumi [mailto:hirofumi@mail.parknet.co.jp] 
> Sent: Thursday, March 11, 2010 3:58 AM
> To: Daniel Taylor
> Cc: Andrew Morton; H. Peter Anvin; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] fs/partition/msdos: Fix unusable 
> extended partition for > 512B sector
> 
> "Daniel Taylor" <Daniel.Taylor@wdc.com> writes:
> 
> > In the near future, WD will be releasing products that need 
> this patch.
> >
> > Wouldn't it be a better Linux user experience to never have 
> the problem,
> > rather than wait for a bug-fix cycle on the kernel?
> 
> Of course, if we can fix, it's better.
> 
> However, probably, users of this patch would be only boot loader,
> because this is a first sector on extended-partition itself, not
> logical-partitions in extended-partition.

I have not yet tried booting from one of these disks.

They are in USB-attached enclosures, attached well after boot, so the
bootloader has never seen them.  They simply refuse to mount to a running
Linux system because, when the storage for partition size and start was
expanded to 64-bit, no one bothered to fix the intermediate storage in
msdos.c, so the kernel cannot locate the start nor figure the size of
the partitions.

Logically, this patch is not complicated.  The data types in msdos.c
are flat-out wrong, given that the real stored data is of type sector_t.
The intermediate variables should not be u32.

For users of small disks, that are not shared with Windows XP, the patch
is totally innocuous.  It does not diminish any existing working behavior,
for anyone, nor change any API, so I do not understand the resistance to
using it.

> 
> So, maybe this wouldn't be so major problem for normal users, and what
> is needed actually is not sure to me for now (I guess it might be
> depending to BIOS, if boot loader is using BIOS call.).
> 
> So, this patch provides one logical sector, but it's just guess.
> 
> > OTOH, it would be reasonable to wait until someone else had 
> a chance to
> > test the change.  We are awaiting NDAs from RedHat, Canonical, and
> > Novell/SUSE to send them the affected products for 
> library/application
> > development.
> 
> Thanks.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> 

Re: [PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[H. Peter Anvin]

On 03/11/2010 01:25 PM, Daniel Taylor wrote:
>
> I have not yet tried booting from one of these disks.
>
> They are in USB-attached enclosures,

Right...

> attached well after boot, so the
> bootloader has never seen them.

Wrong.  A lot of BIOSes will attempt to boot from USB storage.  Worse, a 
fair number of BIOSes will hang during startup if a USB storage device 
that confuses them -- even if not the primary boot device.

> They simply refuse to mount to a running
> Linux system because, when the storage for partition size and start was
> expanded to 64-bit, no one bothered to fix the intermediate storage in
> msdos.c, so the kernel cannot locate the start nor figure the size of
> the partitions.
>
> Logically, this patch is not complicated.  The data types in msdos.c
> are flat-out wrong, given that the real stored data is of type sector_t.
> The intermediate variables should not be u32.

I would consider this a bugfix.  As such, it should be pushed outside 
the merge window.

	-hpa

RE: [PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[Daniel Taylor]

 

> -----Original Message-----
> From: H. Peter Anvin [mailto:hpa@zytor.com] 
> Sent: Thursday, March 11, 2010 1:58 PM
> To: Daniel Taylor
> Cc: OGAWA Hirofumi; Andrew Morton; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] fs/partition/msdos: Fix unusable 
> extended partition for > 512B sector
> 
> On 03/11/2010 01:25 PM, Daniel Taylor wrote:
> >
> > I have not yet tried booting from one of these disks.
> >
> > They are in USB-attached enclosures,
> 
> Right...
> 
> > attached well after boot, so the
> > bootloader has never seen them.
> 
> Wrong.  A lot of BIOSes will attempt to boot from USB 
> storage.  Worse, a 
> fair number of BIOSes will hang during startup if a USB 
> storage device 
> that confuses them -- even if not the primary boot device.

BIOS failures prior to loading the kernel are not Linux'
responsibility.  Confused BIOS will still fail, even if the
patch is not implemented,  so how does that effect the
acceptance of a patch that fixes post-boot behavior?
 
>> They simply refuse to mount to a running
>> Linux system because, when the storage for partition size and start was
>> expanded to 64-bit, no one bothered to fix the intermediate storage in
>> msdos.c, so the kernel cannot locate the start nor figure the size of
>> the partitions.
>>
>> Logically, this patch is not complicated.  The data types in msdos.c
>> are flat-out wrong, given that the real stored data is of type sector_t.
>> The intermediate variables should not be u32.
> 
> I would consider this a bugfix.  As such, it should be pushed outside 
> the merge window.
> 
> 	-hpa
> 

Re: [PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[H. Peter Anvin]

On 03/11/2010 02:06 PM, Daniel Taylor wrote:
>
> BIOS failures prior to loading the kernel are not Linux'
> responsibility.  Confused BIOS will still fail, even if the
> patch is not implemented,  so how does that effect the
> acceptance of a patch that fixes post-boot behavior?
>

I didn't say it does.

>>
>> I would consider this a bugfix.  As such, it should be pushed outside
>> the merge window.
>>

Just in case I wasn't clear enough -- this patch is a bugfix and should 
be pushed upstream as long as it is technically ready.

	-hpa

Re: [PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[Andrew Morton]

On Thu, 11 Mar 2010 14:09:09 -0800
"H. Peter Anvin" <hpa@zytor.com> wrote:

> On 03/11/2010 02:06 PM, Daniel Taylor wrote:
> >
> > BIOS failures prior to loading the kernel are not Linux'
> > responsibility.  Confused BIOS will still fail, even if the
> > patch is not implemented,  so how does that effect the
> > acceptance of a patch that fixes post-boot behavior?
> >
> 
> I didn't say it does.
> 
> >>
> >> I would consider this a bugfix.  As such, it should be pushed outside
> >> the merge window.
> >>
> 
> Just in case I wasn't clear enough -- this patch is a bugfix and should 
> be pushed upstream as long as it is technically ready.
> 

yeah.  In fact I tagged it for -stable backporting.  Because as Daniel
says, "In the near future, WD will be releasing products that need this
patch".  People will surely want to use 2.6.33.x-based kernels with
those devices, so packagers of 2.6.33.x-based kernels will need to hunt
down and apply this patch.  So let's do it for them.

Re: [PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[OGAWA Hirofumi]

"Daniel Taylor" <Daniel.Taylor@wdc.com> writes:

>> Of course, if we can fix, it's better.
>> 
>> However, probably, users of this patch would be only boot loader,
>> because this is a first sector on extended-partition itself, not
>> logical-partitions in extended-partition.
>
> I have not yet tried booting from one of these disks.
>
> They are in USB-attached enclosures, attached well after boot, so the
> bootloader has never seen them.  They simply refuse to mount to a running
> Linux system because, when the storage for partition size and start was
> expanded to 64-bit, no one bothered to fix the intermediate storage in
> msdos.c, so the kernel cannot locate the start nor figure the size of
> the partitions.
>
> Logically, this patch is not complicated.  The data types in msdos.c
> are flat-out wrong, given that the real stored data is of type sector_t.
> The intermediate variables should not be u32.
>
> For users of small disks, that are not shared with Windows XP, the patch
> is totally innocuous.  It does not diminish any existing working behavior,
> for anyone, nor change any API, so I do not understand the resistance to
> using it.

Those are all about the first (1/2) patch, not this second patch.
Personally, I'm thinking we should apply the first patch as bugfix.

I'm talking about only second (2/2) patch in here.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[H. Peter Anvin]

On 03/11/2010 02:17 PM, OGAWA Hirofumi wrote:
>
> Those are all about the first (1/2) patch, not this second patch.
> Personally, I'm thinking we should apply the first patch as bugfix.
>
> I'm talking about only second (2/2) patch in here.
>

The 2/2 patch seems to be a bug fix too; it implements the intent of the 
original code as far as I can see.

	-hpa

Re: [PATCH 2/2] fs/partition/msdos: Fix unusable extended partition for > 512B sector

[OGAWA Hirofumi]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 03/11/2010 02:17 PM, OGAWA Hirofumi wrote:
>>
>> Those are all about the first (1/2) patch, not this second patch.
>> Personally, I'm thinking we should apply the first patch as bugfix.
>>
>> I'm talking about only second (2/2) patch in here.
>>
>
> The 2/2 patch seems to be a bug fix too; it implements the intent of the 
> original code as far as I can see.

Yes, however the state is not so sure if compared to first patch. So I
said, if we want, we can drop it (2/2), because it's not so sure to fix
the real problem (bootloader's requirement).

Even if we apply this patch too, it shouldn't introduce new problems
though.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

RE: [PATCH 1/2] fs/partitions/msdos: add support for large disks

[Daniel Taylor]

Looks good to me.

Thank you for the review/fixes.

Regards,

Dan

> -----Original Message-----
> From: OGAWA Hirofumi [mailto:hirofumi@mail.parknet.co.jp] 
> Sent: Wednesday, March 10, 2010 7:48 AM
> To: Andrew Morton
> Cc: H. Peter Anvin; Daniel Taylor; linux-kernel@vger.kernel.org
> Subject: [PATCH 1/2] fs/partitions/msdos: add support for large disks
> 
> Hi,
> 
> This is rediff with fix of "Daniel Taylor" <Daniel.Taylor@wdc.com>.
> Daniel, please check I'm breaking your patch.
> 
> 
> 
> From: "Daniel Taylor" <Daniel.Taylor@wdc.com>
> 
> In order to use disks larger than 2TiB on Windows XP, it is necessary
> to use 4096-byte logical sectors in an MBR.
> 
> Although the kernel storage and functions called from msdos.c used
> "sector_t" internally, msdos.c still used u32 variables, which results
> in the ability to handle XP-compatible large disks.
> 
> This patch changes the internal variables to "sector_t".
> 
> Signed-off-by: Daniel Taylor <daniel.taylor@wdc.com>
> [tweaks and fix]
> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> ---
> 
>  fs/partitions/msdos.c |   74 
> +++++++++++++++++++++++++------------------------
>  1 file changed, 39 insertions(+), 35 deletions(-)
> 
> diff -puN fs/partitions/msdos.c~part-support-4k-block 
> fs/partitions/msdos.c
> --- linux-2.6/fs/partitions/msdos.c~part-support-4k-block	
> 2010-03-07 04:10:48.000000000 +0900
> +++ linux-2.6-hirofumi/fs/partitions/msdos.c	2010-03-07 
> 19:41:56.000000000 +0900
> @@ -31,14 +31,17 @@
>   */
>  #include <asm/unaligned.h>
>  
> -#define SYS_IND(p)	(get_unaligned(&p->sys_ind))
> -#define NR_SECTS(p)	({ __le32 __a =	
> get_unaligned(&p->nr_sects);	\
> -				le32_to_cpu(__a); \
> -			})
> -
> -#define START_SECT(p)	({ __le32 __a =	
> get_unaligned(&p->start_sect);	\
> -				le32_to_cpu(__a); \
> -			})
> +#define SYS_IND(p)	get_unaligned(&p->sys_ind)
> +
> +static inline sector_t nr_sects(struct partition *p)
> +{
> +	return (sector_t)get_unaligned_le32(&p->nr_sects);
> +}
> +
> +static inline sector_t start_sect(struct partition *p)
> +{
> +	return (sector_t)get_unaligned_le32(&p->start_sect);
> +}
>  
>  static inline int is_extended_partition(struct partition *p)
>  {
> @@ -104,13 +107,13 @@ static int aix_magic_present(unsigned ch
>  
>  static void
>  parse_extended(struct parsed_partitions *state, struct 
> block_device *bdev,
> -			u32 first_sector, u32 first_size)
> +			sector_t first_sector, sector_t first_size)
>  {
>  	struct partition *p;
>  	Sector sect;
>  	unsigned char *data;
> -	u32 this_sector, this_size;
> -	int sector_size = bdev_logical_block_size(bdev) / 512;
> +	sector_t this_sector, this_size;
> +	sector_t sector_size = bdev_logical_block_size(bdev) / 512;
>  	int loopct = 0;		/* number of links followed
>  				   without finding a data partition */
>  	int i;
> @@ -145,14 +148,14 @@ parse_extended(struct parsed_partitions
>  		 * First process the data partition(s)
>  		 */
>  		for (i=0; i<4; i++, p++) {
> -			u32 offs, size, next;
> -			if (!NR_SECTS(p) || is_extended_partition(p))
> +			sector_t offs, size, next;
> +			if (!nr_sects(p) || is_extended_partition(p))
>  				continue;
>  
>  			/* Check the 3rd and 4th entries -
>  			   these sometimes contain random garbage */
> -			offs = START_SECT(p)*sector_size;
> -			size = NR_SECTS(p)*sector_size;
> +			offs = start_sect(p)*sector_size;
> +			size = nr_sects(p)*sector_size;
>  			next = this_sector + offs;
>  			if (i >= 2) {
>  				if (offs + size > this_size)
> @@ -179,13 +182,13 @@ parse_extended(struct parsed_partitions
>  		 */
>  		p -= 4;
>  		for (i=0; i<4; i++, p++)
> -			if (NR_SECTS(p) && is_extended_partition(p))
> +			if (nr_sects(p) && is_extended_partition(p))
>  				break;
>  		if (i == 4)
>  			goto done;	 /* nothing left to do */
>  
> -		this_sector = first_sector + START_SECT(p) * 
> sector_size;
> -		this_size = NR_SECTS(p) * sector_size;
> +		this_sector = first_sector + start_sect(p) * 
> sector_size;
> +		this_size = nr_sects(p) * sector_size;
>  		put_dev_sector(sect);
>  	}
>  done:
> @@ -197,7 +200,7 @@ done:
>  
>  static void
>  parse_solaris_x86(struct parsed_partitions *state, struct 
> block_device *bdev,
> -			u32 offset, u32 size, int origin)
> +			sector_t offset, sector_t size, int origin)
>  {
>  #ifdef CONFIG_SOLARIS_X86_PARTITION
>  	Sector sect;
> @@ -244,7 +247,7 @@ parse_solaris_x86(struct parsed_partitio
>   */
>  static void
>  parse_bsd(struct parsed_partitions *state, struct block_device *bdev,
> -		u32 offset, u32 size, int origin, char *flavour,
> +		sector_t offset, sector_t size, int origin, 
> char *flavour,
>  		int max_partitions)
>  {
>  	Sector sect;
> @@ -263,7 +266,7 @@ parse_bsd(struct parsed_partitions *stat
>  	if (le16_to_cpu(l->d_npartitions) < max_partitions)
>  		max_partitions = le16_to_cpu(l->d_npartitions);
>  	for (p = l->d_partitions; p - l->d_partitions < 
> max_partitions; p++) {
> -		u32 bsd_start, bsd_size;
> +		sector_t bsd_start, bsd_size;
>  
>  		if (state->next == state->limit)
>  			break;
> @@ -290,7 +293,7 @@ parse_bsd(struct parsed_partitions *stat
>  
>  static void
>  parse_freebsd(struct parsed_partitions *state, struct 
> block_device *bdev,
> -		u32 offset, u32 size, int origin)
> +		sector_t offset, sector_t size, int origin)
>  {
>  #ifdef CONFIG_BSD_DISKLABEL
>  	parse_bsd(state, bdev, offset, size, origin,
> @@ -300,7 +303,7 @@ parse_freebsd(struct parsed_partitions *
>  
>  static void
>  parse_netbsd(struct parsed_partitions *state, struct 
> block_device *bdev,
> -		u32 offset, u32 size, int origin)
> +		sector_t offset, sector_t size, int origin)
>  {
>  #ifdef CONFIG_BSD_DISKLABEL
>  	parse_bsd(state, bdev, offset, size, origin,
> @@ -310,7 +313,7 @@ parse_netbsd(struct parsed_partitions *s
>  
>  static void
>  parse_openbsd(struct parsed_partitions *state, struct 
> block_device *bdev,
> -		u32 offset, u32 size, int origin)
> +		sector_t offset, sector_t size, int origin)
>  {
>  #ifdef CONFIG_BSD_DISKLABEL
>  	parse_bsd(state, bdev, offset, size, origin,
> @@ -324,7 +327,7 @@ parse_openbsd(struct parsed_partitions *
>   */
>  static void
>  parse_unixware(struct parsed_partitions *state, struct 
> block_device *bdev,
> -		u32 offset, u32 size, int origin)
> +		sector_t offset, sector_t size, int origin)
>  {
>  #ifdef CONFIG_UNIXWARE_DISKLABEL
>  	Sector sect;
> @@ -348,7 +351,8 @@ parse_unixware(struct parsed_partitions
>  
>  		if (p->s_label != UNIXWARE_FS_UNUSED)
>  			put_partition(state, state->next++,
> -						START_SECT(p), 
> NR_SECTS(p));
> +				      le32_to_cpu(p->start_sect),
> +				      le32_to_cpu(p->nr_sects));
>  		p++;
>  	}
>  	put_dev_sector(sect);
> @@ -363,7 +367,7 @@ parse_unixware(struct parsed_partitions
>   */
>  static void
>  parse_minix(struct parsed_partitions *state, struct 
> block_device *bdev,
> -		u32 offset, u32 size, int origin)
> +		sector_t offset, sector_t size, int origin)
>  {
>  #ifdef CONFIG_MINIX_SUBPARTITION
>  	Sector sect;
> @@ -390,7 +394,7 @@ parse_minix(struct parsed_partitions *st
>  			/* add each partition in use */
>  			if (SYS_IND(p) == MINIX_PARTITION)
>  				put_partition(state, state->next++,
> -					      START_SECT(p), 
> NR_SECTS(p));
> +					      start_sect(p), 
> nr_sects(p));
>  		}
>  		printk(" >\n");
>  	}
> @@ -401,7 +405,7 @@ parse_minix(struct parsed_partitions *st
>  static struct {
>  	unsigned char id;
>  	void (*parse)(struct parsed_partitions *, struct block_device *,
> -			u32, u32, int);
> +			sector_t, sector_t, int);
>  } subtypes[] = {
>  	{FREEBSD_PARTITION, parse_freebsd},
>  	{NETBSD_PARTITION, parse_netbsd},
> @@ -415,7 +419,7 @@ static struct {
>   
>  int msdos_partition(struct parsed_partitions *state, struct 
> block_device *bdev)
>  {
> -	int sector_size = bdev_logical_block_size(bdev) / 512;
> +	sector_t sector_size = bdev_logical_block_size(bdev) / 512;
>  	Sector sect;
>  	unsigned char *data;
>  	struct partition *p;
> @@ -483,8 +487,8 @@ int msdos_partition(struct parsed_partit
>  
>  	state->next = 5;
>  	for (slot = 1 ; slot <= 4 ; slot++, p++) {
> -		u32 start = START_SECT(p)*sector_size;
> -		u32 size = NR_SECTS(p)*sector_size;
> +		sector_t start = start_sect(p)*sector_size;
> +		sector_t size = nr_sects(p)*sector_size;
>  		if (!size)
>  			continue;
>  		if (is_extended_partition(p)) {
> @@ -513,7 +517,7 @@ int msdos_partition(struct parsed_partit
>  		unsigned char id = SYS_IND(p);
>  		int n;
>  
> -		if (!NR_SECTS(p))
> +		if (!nr_sects(p))
>  			continue;
>  
>  		for (n = 0; subtypes[n].parse && id != 
> subtypes[n].id; n++)
> @@ -521,8 +525,8 @@ int msdos_partition(struct parsed_partit
>  
>  		if (!subtypes[n].parse)
>  			continue;
> -		subtypes[n].parse(state, bdev, 
> START_SECT(p)*sector_size,
> -						
> NR_SECTS(p)*sector_size, slot);
> +		subtypes[n].parse(state, bdev, 
> start_sect(p)*sector_size,
> +						
> nr_sects(p)*sector_size, slot);
>  	}
>  	put_dev_sector(sect);
>  	return 1;
> _
> 
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>