NetBSD Problem Report #45990
From tsutsui@ceres.dti.ne.jp Sun Feb 12 03:50:05 2012
Return-Path: <tsutsui@ceres.dti.ne.jp>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
by www.NetBSD.org (Postfix) with ESMTP id 8B07663B86B
for <gnats-bugs@gnats.NetBSD.org>; Sun, 12 Feb 2012 03:50:05 +0000 (UTC)
Message-Id: <201202120350.q1C3o1p7005702@mirage.localdomain>
Date: Sun, 12 Feb 2012 12:50:01 +0900 (JST)
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Reply-To: tsutsui@ceres.dti.ne.jp
To: gnats-bugs@gnats.NetBSD.org
Cc: tsutsui@ceres.dti.ne.jp
Subject: sysinst uses traditional 63 sector MBR offset
X-Send-Pr-Version: 3.95
>Number: 45990
>Category: install
>Synopsis: sysinst uses traditional 63 sector MBR offset
>Confidential: no
>Severity: non-critical
>Priority: high
>Responsible: install-manager
>State: closed
>Class: change-request
>Submitter-Id: net
>Arrival-Date: Sun Feb 12 03:55:00 +0000 2012
>Closed-Date: Sun Oct 13 20:13:43 +0000 2013
>Last-Modified: Sun Oct 20 13:45:06 +0000 2013
>Originator: Izumi Tsutsui
>Release: NetBSD 5.99.65
>Organization:
>Environment:
System: NetBSD i386 5.99.65 on QEMU
Architecture: i386
Machine: i386
>Description:
sysinst(8) creates MBR partition with traditional offset 63 sectors
if "Use the entire disk" is selected.
It's too bad for modern 4KB/sector drives and flash memory based drives.
Note fdisk(8) has been fixed to use 1MB boundary for MBR partitions.
>How-To-Repeat:
boot NetBSD/i386 5.99.65 installcd
% qemu -m 512m -hda hdd.img -cdrom NetBSD-5.99.65-i386.iso -boot d
a: Installation messages in English
a: unchanged
a: Install NetBSD to hard disk
b: Yes
Hit enter to continue
a: Full installation
a: This is the correct geometry
b: Use the entire disk
a: Yes
a: Set sizes of NetBSD partitions
Accept partition sizes.
g: Change input units (sectors/cylinders/MB)
c: Sectors
Then the root partition starts at 63 sector offset.
>Fix:
Fix src/utils/sysinst/mbr.c to use proper boundary?
>Release-Note:
>Audit-Trail:
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: gnats-bugs@NetBSD.org
Cc: netbsd-bugs@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: install/45990: sysinst uses traditional 63 sector MBR offset
Date: Sun, 4 Mar 2012 12:37:43 +0900
> >Synopsis: sysinst uses traditional 63 sector MBR offset
:
> >Fix:
> Fix src/utils/sysinst/mbr.c to use proper boundary?
The attached patch fixes mbr.c to use ths same strategy as fdisk(8):
>> the alignment of the first partition is
>> inspected. If it ends on a 2048 sector boundary, then the align-
>> ment is set to 2048, if the start is a power of 2 less than, or
>> equal to 2048 then the offset is set to the start sector. If the
>> first partition isn't defined then the alignment and offset for
>> disks larger than 128GB is set to 2048 (1MB). In all other cases
>> the alignment default to a cylinder and the offset to a track
>> (both using the BIOS geometry). The 1MB alignment is the same as
>> that used by recent windows versions.
I wonder if using 1MB boundary for smaller disks is still worth
for modern SSD or other flash media, but it's a different problem.
---
Index: mbr.c
===================================================================
RCS file: /cvsroot/src/distrib/utils/sysinst/mbr.c,v
retrieving revision 1.89
diff -u -p -r1.89 mbr.c
--- mbr.c 5 Jan 2012 21:29:24 -0000 1.89
+++ mbr.c 4 Mar 2012 03:20:54 -0000
@@ -113,6 +113,10 @@ static int get_mapping(struct mbr_partit
daddr_t *);
static void convert_mbr_chs(int, int, int, uint8_t *, uint8_t *,
uint8_t *, uint32_t);
+static void get_ptn_0_offset(struct mbr_partition *);
+
+static unsigned int ptn_alignment;
+static unsigned int ptn_0_offset;
/*
* Notes on the extended partition editor.
@@ -462,8 +466,9 @@ set_mbr_type(menudesc *m, void *arg)
return 0;
mbri->extended = ext;
ext->sector = mbrp->mbrp_start;
- ext->mbr.mbr_parts[0].mbrp_start = bsec;
- ext->mbr.mbr_parts[0].mbrp_size = mbrp->mbrp_size - bsec;
+ ext->mbr.mbr_parts[0].mbrp_start = ptn_0_offset;
+ ext->mbr.mbr_parts[0].mbrp_size =
+ mbrp->mbrp_size - ptn_0_offset;
}
mbrp->mbrp_type = type;
@@ -742,14 +747,14 @@ edit_mbr_size(menudesc *m, void *arg)
/* If unchanged, don't re-round size */
new = dflt;
else {
- /* Round end to cylinder boundary */
+ /* Round end to the partition alignment */
if (sizemult != 1) {
new *= sizemult;
- new += rounddown(start, current_cylsize);
- new = roundup(new, current_cylsize);
+ new += rounddown(start, ptn_alignment);
+ new = roundup(new, ptn_alignment);
new -= start;
while (new <= 0)
- new += current_cylsize;
+ new += ptn_alignment;
}
}
if (new > max)
@@ -1237,12 +1242,12 @@ mbr_use_wholedisk(mbr_info_t *mbri)
memset(&mbri->mbrb, 0, sizeof mbri->mbrb);
#endif
part[0].mbrp_type = MBR_PTYPE_NETBSD;
- part[0].mbrp_size = dlsize - bsec;
- part[0].mbrp_start = bsec;
+ part[0].mbrp_size = dlsize - ptn_0_offset;
+ part[0].mbrp_start = ptn_0_offset;
part[0].mbrp_flag = MBR_PFLAG_ACTIVE;
- ptstart = bsec;
- ptsize = dlsize - bsec;
+ ptstart = ptn_0_offset;
+ ptsize = dlsize - ptn_0_offset;
return 1;
}
@@ -1450,6 +1455,10 @@ read_mbr(const char *disk, mbr_info_t *m
*/
if (bsec == 0)
bsec = dlsec;
+ ptn_0_offset = bsec;
+ /* use 1MB default offset on large disks as fdisk(8) */
+ if (dlsize > 2048 * 1024 * 128)
+ ptn_0_offset = 2048;
memset(mbri, 0, sizeof *mbri);
@@ -1467,7 +1476,9 @@ read_mbr(const char *disk, mbr_info_t *m
break;
mbrp = &mbrs->mbr_parts[0];
- if (ext_base != 0) {
+ if (ext_base == 0) {
+ get_ptn_0_offset(mbrp);
+ } else {
/* sanity check extended chain */
if (MBR_IS_EXTENDED(mbrp[0].mbrp_type))
break;
@@ -1547,9 +1558,9 @@ read_mbr(const char *disk, mbr_info_t *m
ext->sector = base;
ext->mbr.mbr_magic = htole16(MBR_MAGIC);
ext->mbr.mbr_parts[1] = mbrp[1];
- ext->mbr.mbr_parts[0].mbrp_start = bsec;
+ ext->mbr.mbr_parts[0].mbrp_start = ptn_0_offset;
ext->mbr.mbr_parts[0].mbrp_size =
- ext_base + limit - base - bsec;
+ ext_base + limit - base - ptn_0_offset;
mbrp[1].mbrp_type = MBR_PTYPE_EXT;
mbrp[1].mbrp_start = base - ext_base;
mbrp[1].mbrp_size = limit - mbrp[1].mbrp_start;
@@ -1850,3 +1861,36 @@ get_mapping(struct mbr_partition *parts,
return 0;
}
+
+/*
+ * Determine first partition header offset value as fdisk(8) does.
+ */
+static void
+get_ptn_0_offset(struct mbr_partition *mbrp0)
+{
+ uint32_t ptn_0_base, ptn_0_limit;
+
+ /* Default to using 'traditional' cylinder alignment */
+ ptn_alignment = bhead * bsec;
+ ptn_0_offset = bsec;
+
+ if (mbrp0->mbrp_type != 0) {
+ /* Try to copy offset of first partition */
+ ptn_0_base = le32toh(mbrp0->mbrp_start);
+ ptn_0_limit = ptn_0_base + le32toh(mbrp0->mbrp_size);
+ if (!(ptn_0_limit & 2047)) {
+ /* Partition ends on a 1MB boundary, align to 1MB */
+ ptn_alignment = 2048;
+ if (ptn_0_base <= 2048
+ && !(ptn_0_base & (ptn_0_base - 1))) {
+ /* ptn_base is a power of 2, use it */
+ ptn_0_offset = ptn_0_base;
+ }
+ }
+ } else {
+ /* Use 1MB offset for large (>128GB) disks */
+ if (dlsize > 2048 * 1024 * 128)
+ ptn_alignment = 2048;
+ ptn_0_offset = 2048;
+ }
+}
---
Izumi Tsutsui
State-Changed-From-To: open->analyzed
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Sun, 04 Mar 2012 19:20:57 +0900
State-Changed-Why:
Patch proposed. Probably we need "awaiting review" state in gnats...
From: "Izumi Tsutsui" <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/45990 CVS commit: src/distrib/utils/sysinst
Date: Sat, 7 Apr 2012 03:08:33 +0000
Module Name: src
Committed By: tsutsui
Date: Sat Apr 7 03:08:33 UTC 2012
Modified Files:
src/distrib/utils/sysinst: mbr.c
Log Message:
Use 1MB alignment rather than the default 63 sectors for
fdisk partition boundary for >128GB disks, as fdisk(8) does.
No particular comments for my PR install/45990.
To generate a diff of this commit:
cvs rdiff -u -r1.89 -r1.90 src/distrib/utils/sysinst/mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: Matthew Mondor <mm_lists@pulsar-zone.net>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: PR/45990: sysinst uses traditional 63 sector MBR offset
Date: Sat, 7 Apr 2012 09:30:24 -0400
On Sat, 7 Apr 2012 03:10:06 +0000 (UTC)
"Izumi Tsutsui" <tsutsui@netbsd.org> wrote:
> Log Message:
> Use 1MB alignment rather than the default 63 sectors for
> fdisk partition boundary for >128GB disks, as fdisk(8) does.
> No particular comments for my PR install/45990.
Is this something that should be pulled-up to netbsd-6 considering
it'll increase performance of new installations, and netbsd-6 isn't yet
officially released?
Thanks,
--
Matt
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: gnats-bugs@NetBSD.org
Cc: mm_lists@pulsar-zone.net, tsutsui@ceres.dti.ne.jp
Subject: Re: PR/45990: sysinst uses traditional 63 sector MBR offset
Date: Sun, 8 Apr 2012 00:00:46 +0900
> Is this something that should be pulled-up to netbsd-6 considering
> it'll increase performance of new installations, and netbsd-6 isn't yet
> officially released?
I'm not an expert of PC partitioning and modern storage
devices, and there is no review comment on this PR.
If someone[TM] who has enough knowledge about this issue
thinks it's worth, he will send a pullup request.
I also wonder if 128GB is a good threshold for modern SSD etc.
---
Izumi Tsutsui
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: PR/45990: sysinst uses traditional 63 sector MBR offset
Date: Mon, 9 Apr 2012 06:38:35 +0000
On Sat, Apr 07, 2012 at 03:05:03PM +0000, Izumi Tsutsui wrote:
> > Is this something that should be pulled-up to netbsd-6 considering
> > it'll increase performance of new installations, and netbsd-6 isn't yet
> > officially released?
>
> I'm not an expert of PC partitioning and modern storage
> devices, and there is no review comment on this PR.
> If someone[TM] who has enough knowledge about this issue
> thinks it's worth, he will send a pullup request.
Yes, it should be pulled up.
> I also wonder if 128GB is a good threshold for modern SSD etc.
It's better than nothing.
--
David A. Holland
dholland@netbsd.org
From: "Izumi Tsutsui" <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/45990 CVS commit: src/distrib/utils/sysinst
Date: Sat, 26 May 2012 05:10:00 +0000
Module Name: src
Committed By: tsutsui
Date: Sat May 26 05:09:59 UTC 2012
Modified Files:
src/distrib/utils/sysinst: mbr.c
Log Message:
Fix regression by my dumb patch in PR/45990.
sysinst fails with "floating exception" after
changing MBR partition size in MBR editor menu
if the target disk doesn't have valid MBR partition
or has a valid partition 0 whose offset is not 1MB aligned
(like 63 sectors).
read_mbr() (which calls get_ptn_alignment()) is called
before set_bios_geom(), so bhead is not initialized there
and ptn_alignment could be zero.
To workaround, explicitly call get_ptn_alignment() again
in edit_mbr() to update ptn_alignemnt per BIOS geom values.
To generate a diff of this commit:
cvs rdiff -u -r1.90 -r1.91 src/distrib/utils/sysinst/mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Jeff Rizzo" <riz@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/45990 CVS commit: [netbsd-6] src/distrib/utils/sysinst
Date: Tue, 12 Jun 2012 19:19:22 +0000
Module Name: src
Committed By: riz
Date: Tue Jun 12 19:19:21 UTC 2012
Modified Files:
src/distrib/utils/sysinst [netbsd-6]: mbr.c
Log Message:
Pull up following revision(s) (requested by tsutsui in ticket #317):
distrib/utils/sysinst/mbr.c: revision 1.91
Fix regression by my dumb patch in PR/45990.
sysinst fails with "floating exception" after
changing MBR partition size in MBR editor menu
if the target disk doesn't have valid MBR partition
or has a valid partition 0 whose offset is not 1MB aligned
(like 63 sectors).
read_mbr() (which calls get_ptn_alignment()) is called
before set_bios_geom(), so bhead is not initialized there
and ptn_alignment could be zero.
To workaround, explicitly call get_ptn_alignment() again
in edit_mbr() to update ptn_alignemnt per BIOS geom values.
To generate a diff of this commit:
cvs rdiff -u -r1.89.2.1 -r1.89.2.2 src/distrib/utils/sysinst/mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: analyzed->closed
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Wed, 04 Jul 2012 00:03:08 +0900
State-Changed-Why:
pullup-6 #247 sync'ed sysinst with HEAD and a regression was fixed by #317.
From: "Izumi Tsutsui" <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/45990 CVS commit: src/distrib/utils/sysinst
Date: Sun, 13 Oct 2013 15:32:14 +0000
Module Name: src
Committed By: tsutsui
Date: Sun Oct 13 15:32:14 UTC 2013
Modified Files:
src/distrib/utils/sysinst: mbr.c
Log Message:
Fix another botch of my dumb patch in PR/45990; add missing braces.
The offset of MBR partition 0 was unintentionally set to 2048 even on
small (<=128GB) disks. Probably we should rethink the threshold,
but anyway sysinst(8) should follow fdisk(8) default.
http://nxr.NetBSD.org/xref/src/sbin/fdisk/fdisk.c?r=1.145#1199
http://cvsweb.NetBSD.org/bsdweb.cgi/src/sbin/fdisk/fdisk.c#rev1.129
The problem is pointed out and analyzed by Simon Nicolussi in PR/48304.
Should be pulled up to all netbsd-6* branches.
To generate a diff of this commit:
cvs rdiff -u -r1.91 -r1.92 src/distrib/utils/sysinst/mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: closed->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 13 Oct 2013 20:08:29 +0000
State-Changed-Why:
pullup to -6 needed for the recent fix
State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 13 Oct 2013 20:13:43 +0000
State-Changed-Why:
never mind, the recent fix has its own PR (48304)
From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/45990 CVS commit: [netbsd-6] src/distrib/utils/sysinst
Date: Sun, 20 Oct 2013 13:43:17 +0000
Module Name: src
Committed By: bouyer
Date: Sun Oct 20 13:43:17 UTC 2013
Modified Files:
src/distrib/utils/sysinst [netbsd-6]: mbr.c
Log Message:
Pull up following revision(s) (requested by tsutsui in ticket #972):
distrib/utils/sysinst/mbr.c: revision 1.92
Fix another botch of my dumb patch in PR/45990; add missing braces.
The offset of MBR partition 0 was unintentionally set to 2048 even on
small (<=128GB) disks. Probably we should rethink the threshold,
but anyway sysinst(8) should follow fdisk(8) default.
http://nxr.NetBSD.org/xref/src/sbin/fdisk/fdisk.c?r=1.145#1199
http://cvsweb.NetBSD.org/bsdweb.cgi/src/sbin/fdisk/fdisk.c#rev1.129
The problem is pointed out and analyzed by Simon Nicolussi in PR/48304.
Should be pulled up to all netbsd-6* branches.
To generate a diff of this commit:
cvs rdiff -u -r1.89.2.2 -r1.89.2.3 src/distrib/utils/sysinst/mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/45990 CVS commit: [netbsd-6-1] src/distrib/utils/sysinst
Date: Sun, 20 Oct 2013 13:43:23 +0000
Module Name: src
Committed By: bouyer
Date: Sun Oct 20 13:43:23 UTC 2013
Modified Files:
src/distrib/utils/sysinst [netbsd-6-1]: mbr.c
Log Message:
Pull up following revision(s) (requested by tsutsui in ticket #972):
distrib/utils/sysinst/mbr.c: revision 1.92
Fix another botch of my dumb patch in PR/45990; add missing braces.
The offset of MBR partition 0 was unintentionally set to 2048 even on
small (<=128GB) disks. Probably we should rethink the threshold,
but anyway sysinst(8) should follow fdisk(8) default.
http://nxr.NetBSD.org/xref/src/sbin/fdisk/fdisk.c?r=1.145#1199
http://cvsweb.NetBSD.org/bsdweb.cgi/src/sbin/fdisk/fdisk.c#rev1.129
The problem is pointed out and analyzed by Simon Nicolussi in PR/48304.
Should be pulled up to all netbsd-6* branches.
To generate a diff of this commit:
cvs rdiff -u -r1.89.2.2 -r1.89.2.2.6.1 src/distrib/utils/sysinst/mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/45990 CVS commit: [netbsd-6-0] src/distrib/utils/sysinst
Date: Sun, 20 Oct 2013 13:43:26 +0000
Module Name: src
Committed By: bouyer
Date: Sun Oct 20 13:43:26 UTC 2013
Modified Files:
src/distrib/utils/sysinst [netbsd-6-0]: mbr.c
Log Message:
Pull up following revision(s) (requested by tsutsui in ticket #972):
distrib/utils/sysinst/mbr.c: revision 1.92
Fix another botch of my dumb patch in PR/45990; add missing braces.
The offset of MBR partition 0 was unintentionally set to 2048 even on
small (<=128GB) disks. Probably we should rethink the threshold,
but anyway sysinst(8) should follow fdisk(8) default.
http://nxr.NetBSD.org/xref/src/sbin/fdisk/fdisk.c?r=1.145#1199
http://cvsweb.NetBSD.org/bsdweb.cgi/src/sbin/fdisk/fdisk.c#rev1.129
The problem is pointed out and analyzed by Simon Nicolussi in PR/48304.
Should be pulled up to all netbsd-6* branches.
To generate a diff of this commit:
cvs rdiff -u -r1.89.2.2 -r1.89.2.2.4.1 src/distrib/utils/sysinst/mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home