Re: Panics in ffs_clusteracct with todays -current
David O'Brien wrote: > > If this fixes a problem, then please go ahead and commit it. :-) > > Many reasons to not rush this: > > Maybe because we are in the middle of a discussion. > Maybe to educate others why the change was bad so others will not attempt > the same change later. > Maybe the person that made the change does not agree with the analysis and > wants to fix things a different way. I have to agree with David; there had to be some reason involved in the change in the first place. I can think of a couple of reasons, haviong to do with sign extended math, which might prompt the change. It may be that Alfred's fix is the best one, despite the risks Bruce has identified. -- Terry To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Panics in ffs_clusteracct with todays -current
On Mon, Feb 04, 2002 at 03:56:45PM +, Mark Murray wrote: > If this fixes a problem, then please go ahead and commit it. :-) Many reasons to not rush this: Maybe because we are in the middle of a discussion. Maybe to educate others why the change was bad so others will not attempt the same change later. Maybe the person that made the change does not agree with the analysis and wants to fix things a different way. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Panics in ffs_clusteracct with todays -current
> The kernel from today's current (CVSupped 3 Feb 2002 around 17:40 > PST) can't stay up for more than a few minutes without getting a > page-not-present panic at line 1815 of ufs/ffs/ffs_alloc.c revision > 1.86. It is in this code: > > /* > * Find the size of the cluster going backward. > */ > start = blkno - 1; > end = start - fs->fs_contigsumsize; > if (end < 0) > end = -1; > mapp = &freemapp[start / NBBY]; > map = *mapp--; > ^ > BANG! I just reverted the "upgrade" of NBBY (to unsigned). M -- o Mark Murray \_ FreeBSD Services Limited O.\_Warning: this .sig is umop ap!sdn To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Panics in ffs_clusteracct with todays -current
On Mon, 4 Feb 2002, Alfred Perlstein wrote:
> * Bruce Evans <[EMAIL PROTECTED]> [020204 05:26] wrote:
> > This patch just backs out the change to NBBY.
>
> Wouldn't this make more sense:
>
> Index: ffs/ffs_alloc.c
> ===
> RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_alloc.c,v
> retrieving revision 1.86
> diff -u -r1.86 ffs_alloc.c
> --- ffs/ffs_alloc.c 2 Feb 2002 01:42:44 - 1.86
> +++ ffs/ffs_alloc.c 4 Feb 2002 16:08:34 -
> @@ -1790,7 +1790,7 @@
> end = start + fs->fs_contigsumsize;
> if (end >= cgp->cg_nclusterblks)
> end = cgp->cg_nclusterblks;
> - mapp = &freemapp[start / NBBY];
> + mapp = &freemapp[start < 0 ? 0 : start / NBBY];
> map = *mapp++;
> bit = 1 << (start % NBBY);
> for (i = start; i < end; i++) {
>
Except it doesn't fix the potentially millions of other lines that are
affected by the fix, and leaves at best confusing code: mapp, map and
bit are unitialized to bogus values. This seems to be harmless because
they are not used (the loop is iterated 0 times if start < 0).
Bruce
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Panics in ffs_clusteracct with todays -current
In article <[EMAIL PROTECTED]>,
Alfred Perlstein <[EMAIL PROTECTED]> wrote:
> * Bruce Evans <[EMAIL PROTECTED]> [020204 05:26] wrote:
> > This was broken by a recent change to the type of NBBY.
> >
> > `start' is apparently negative (it would be for blkno == 0). Small
> > negative values of `start' used to give an index of 0 in freemapp (not
> > even small negative, since integer division bogusly rounds negative
> > values towards 0). They now give an index of about 0x1fff on
> > 32-bit machines.
> >
> > I also got panics in vm. The vm code apparently trapped while trying
> > to map the preposterous address generated by the above.
> >
> > This patch just backs out the change to NBBY.
>
> Wouldn't this make more sense:
>
> Index: ffs/ffs_alloc.c
> ===
> RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_alloc.c,v
> retrieving revision 1.86
> diff -u -r1.86 ffs_alloc.c
> --- ffs/ffs_alloc.c 2 Feb 2002 01:42:44 - 1.86
> +++ ffs/ffs_alloc.c 4 Feb 2002 16:08:34 -
> @@ -1790,7 +1790,7 @@
> end = start + fs->fs_contigsumsize;
> if (end >= cgp->cg_nclusterblks)
> end = cgp->cg_nclusterblks;
> - mapp = &freemapp[start / NBBY];
> + mapp = &freemapp[start < 0 ? 0 : start / NBBY];
> map = *mapp++;
> bit = 1 << (start % NBBY);
> for (i = start; i < end; i++) {
I think your change makes sense here, but I also think the change
to NBBY should be backed out. In general it is very dangerous to
change constants to explicitly unsigned. There's no reason why NBBY
shouldn't be used in signed contexts, but making it unsigned promotes
all of the other ints in containing expressions to unsigned as well
(on the i386). NBBY is used in lots of code, including 3rd party
applications. It has a long tradition, and now isn't the time to
change its type. There's no reason for it to be explicitly unsigned.
The constant 8 is every bit as positive as the constant 8U. :-)
John
--
John Polstra
John D. Polstra & Co., Inc.Seattle, Washington USA
"Disappointment is a good sign of basic intelligence." -- Chögyam Trungpa
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Panics in ffs_clusteracct with todays -current
* Bruce Evans <[EMAIL PROTECTED]> [020204 05:26] wrote:
> On Sun, 3 Feb 2002, John Polstra wrote:
>
> > The kernel from today's current (CVSupped 3 Feb 2002 around 17:40
> > PST) can't stay up for more than a few minutes without getting a
> > page-not-present panic at line 1815 of ufs/ffs/ffs_alloc.c revision
> > 1.86. It is in this code:
> >
> > /*
> > * Find the size of the cluster going backward.
> > */
> > start = blkno - 1;
> > end = start - fs->fs_contigsumsize;
> > if (end < 0)
> > end = -1;
> > mapp = &freemapp[start / NBBY];
> > map = *mapp--;
> > ^
> > BANG!
>
> This was broken by a recent change to the type of NBBY.
>
> `start' is apparently negative (it would be for blkno == 0). Small
> negative values of `start' used to give an index of 0 in freemapp (not
> even small negative, since integer division bogusly rounds negative
> values towards 0). They now give an index of about 0x1fff on
> 32-bit machines.
>
> I also got panics in vm. The vm code apparently trapped while trying
> to map the preposterous address generated by the above.
>
> This patch just backs out the change to NBBY.
Wouldn't this make more sense:
Index: ffs/ffs_alloc.c
===
RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_alloc.c,v
retrieving revision 1.86
diff -u -r1.86 ffs_alloc.c
--- ffs/ffs_alloc.c 2 Feb 2002 01:42:44 - 1.86
+++ ffs/ffs_alloc.c 4 Feb 2002 16:08:34 -
@@ -1790,7 +1790,7 @@
end = start + fs->fs_contigsumsize;
if (end >= cgp->cg_nclusterblks)
end = cgp->cg_nclusterblks;
- mapp = &freemapp[start / NBBY];
+ mapp = &freemapp[start < 0 ? 0 : start / NBBY];
map = *mapp++;
bit = 1 << (start % NBBY);
for (i = start; i < end; i++) {
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Panics in ffs_clusteracct with todays -current
If this fixes a problem, then please go ahead and commit it. :-) M > On Sun, 3 Feb 2002, John Polstra wrote: > > > The kernel from today's current (CVSupped 3 Feb 2002 around 17:40 > > PST) can't stay up for more than a few minutes without getting a > > page-not-present panic at line 1815 of ufs/ffs/ffs_alloc.c revision > > 1.86. It is in this code: > > > > /* > > * Find the size of the cluster going backward. > > */ > > start = blkno - 1; > > end = start - fs->fs_contigsumsize; > > if (end < 0) > > end = -1; > > mapp = &freemapp[start / NBBY]; > > map = *mapp--; > > ^ > > BANG! > > This was broken by a recent change to the type of NBBY. > > `start' is apparently negative (it would be for blkno == 0). Small > negative values of `start' used to give an index of 0 in freemapp (not > even small negative, since integer division bogusly rounds negative > values towards 0). They now give an index of about 0x1fff on > 32-bit machines. > > I also got panics in vm. The vm code apparently trapped while trying > to map the preposterous address generated by the above. > > This patch just backs out the change to NBBY. > > %%% > Index: types.h > === > RCS file: /home/ncvs/src/sys/sys/types.h,v > retrieving revision 1.50 > diff -u -2 -r1.50 types.h > --- types.h 3 Feb 2002 11:36:59 - 1.50 > +++ types.h 4 Feb 2002 13:08:05 - > @@ -172,5 +172,5 @@ > > #ifndef _POSIX_SOURCE > -#define NBBY8U /* number of bits in a byte */ > +#define NBBY8 /* number of bits in a byte */ > > /* > %%% > > Bruce > -- o Mark Murray \_ FreeBSD Services Limited O.\_Warning: this .sig is umop ap!sdn To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Panics in ffs_clusteracct with todays -current
On Sun, 3 Feb 2002, John Polstra wrote: > The kernel from today's current (CVSupped 3 Feb 2002 around 17:40 > PST) can't stay up for more than a few minutes without getting a > page-not-present panic at line 1815 of ufs/ffs/ffs_alloc.c revision > 1.86. It is in this code: > > /* > * Find the size of the cluster going backward. > */ > start = blkno - 1; > end = start - fs->fs_contigsumsize; > if (end < 0) > end = -1; > mapp = &freemapp[start / NBBY]; > map = *mapp--; > ^ > BANG! This was broken by a recent change to the type of NBBY. `start' is apparently negative (it would be for blkno == 0). Small negative values of `start' used to give an index of 0 in freemapp (not even small negative, since integer division bogusly rounds negative values towards 0). They now give an index of about 0x1fff on 32-bit machines. I also got panics in vm. The vm code apparently trapped while trying to map the preposterous address generated by the above. This patch just backs out the change to NBBY. %%% Index: types.h === RCS file: /home/ncvs/src/sys/sys/types.h,v retrieving revision 1.50 diff -u -2 -r1.50 types.h --- types.h 3 Feb 2002 11:36:59 - 1.50 +++ types.h 4 Feb 2002 13:08:05 - @@ -172,5 +172,5 @@ #ifndef _POSIX_SOURCE -#defineNBBY8U /* number of bits in a byte */ +#defineNBBY8 /* number of bits in a byte */ /* %%% Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Panics in ffs_clusteracct with todays -current
The kernel from today's current (CVSupped 3 Feb 2002 around 17:40 PST) can't stay up for more than a few minutes without getting a page-not-present panic at line 1815 of ufs/ffs/ffs_alloc.c revision 1.86. It is in this code: /* * Find the size of the cluster going backward. */ start = blkno - 1; end = start - fs->fs_contigsumsize; if (end < 0) end = -1; mapp = &freemapp[start / NBBY]; map = *mapp--; ^ BANG! The faulting address is 0xe987dcaf, for what it's worth. After displaying the panic message and register dump the system is locked up hard, so I haven't been able to get a stack trace. I wasn't seeing this with the kernel from around 21 January. There were several commits in ffs between then and now: blake$ cvs -nq upd -D 1/21/2002 U ffs_alloc.c U ffs_balloc.c U ffs_extern.h U ffs_inode.c U ffs_snapshot.c U ffs_softdep.c U ffs_softdep_stub.c The dmesg output (from the good kernel) and config file are attached. John Copyright (c) 1992-2002 The FreeBSD Project. Copyright (c) 1979, 1980, 1983, 1986, 1988, 1989, 1991, 1992, 1993, 1994 The Regents of the University of California. All rights reserved. FreeBSD 5.0-CURRENT #16: Tue Jan 22 15:55:01 PST 2002 [EMAIL PROTECTED]:/a/src/sys/i386/compile/BLAKE Preloaded elf kernel "/boot/kernel/kernel" at 0xc03d8000. Preloaded elf module "/boot/kernel/acpi.ko" at 0xc03d80a8. Timecounter "i8254" frequency 1193160 Hz Timecounter "TSC" frequency 400901723 Hz CPU: Pentium II/Pentium II Xeon/Celeron (400.90-MHz 686-class CPU) Origin = "GenuineIntel" Id = 0x653 Stepping = 3 Features=0x183f9ff real memory = 402640896 (393204K bytes) avail memory = 387747840 (378660K bytes) Pentium Pro MTRR support enabled Using $PIR table, 7 entries at 0xc00f0d10 npx0: on motherboard npx0: INT 16 interface acpi0: on motherboard acpi0: power button is handled as a fixed feature programming model. Timecounter "ACPI" frequency 3579545 Hz acpi_timer0: <24-bit timer at 3.579545MHz> port 0xe408-0xe40b on acpi0 acpi_cpu0: on acpi0 acpi_button0: on acpi0 acpi_pcib0: port 0xcf8-0xcff on acpi0 pci0: on acpi_pcib0 pcib1: at device 1.0 on pci0 pci1: on pcib1 pci1: at device 0.0 (no driver attached) isab0: at device 4.0 on pci0 isa0: on isab0 pci0: at device 4.1 (no driver attached) pci0: at device 4.2 (no driver attached) intpm0: port 0xe800-0xe80f irq 9 at device 4.3 on pci0 intpm0: I/O mapped e800 intpm0: intr IRQ 9 enabled revision 0 smbus0: on intsmb0 smb0: on smbus0 intpm0: PM I/O mapped e400 ahc0: port 0xd000-0xd0ff mem 0xe000 -0xefff irq 5 at device 6.0 on pci0 aic7890/91: Ultra2 Wide Channel A, SCSI Id=7, 32/255 SCBs fxp0: port 0xb800-0xb83f mem 0xdf00-0xdf01 ,0xdf80-0xdf800fff irq 10 at device 10.0 on pci0 fxp0: Ethernet address 00:02:b3:63:f9:a2 inphy0: on miibus0 inphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, auto pci0: at device 12.0 (no driver attached) fdc0: port 0x3f7,0x3f2-0 x3f5 irq 6 on acpi0 fdc0: FIFO enabled, 8 bytes threshold fd0: <1440-KB 3.5" drive> on fdc0 drive 0 sio0 port 0x3f8-0x3ff irq 4 on acpi0 sio0: type 16550A sio1 port 0x2f8-0x2ff irq 3 on acpi0 sio1: type 16550A atkbdc0: port 0x64,0x60 irq 1 on acpi0 atkbd0: flags 0x1 irq 1 on atkbdc0 psm0: irq 12 on atkbdc0 psm0: model Generic PS/2 mouse, device ID 0 atkbdc: atkbdc0 already exists; skipping it fdc: fdc0 already exists; skipping it sio: sio0 already exists; skipping it sio: sio1 already exists; skipping it sc: sc0 already exists; skipping it vga: vga0 already exists; skipping it orm0: at iomem 0xc8000-0xc97ff,0xc-0xc7fff on isa0 sc0: at flags 0x100 on isa0 sc0: VGA <16 virtual consoles, flags=0x300> vga0: at port 0x3c0-0x3df iomem 0xa-0xb on isa0 IPsec: Initialized Security Association Processing. Waiting 2 seconds for SCSI devices to settle Mounting root from ufs:/dev/da0s1a cd0 at ahc0 bus 0 target 5 lun 0 cd0: Removable CD-ROM SCSI-2 device cd0: 10.000MB/s transfers (10.000MHz, offset 16) cd0: Attempt to query device size failed: NOT READY, Medium not present da0 at ahc0 bus 0 target 0 lun 0 da0: Fixed Direct Access SCSI-3 device da0: 40.000MB/s transfers (20.000MHz, offset 31, 16bit), Tagged Queueing Enabled da0: 8748MB (17916240 512 byte sectors: 255H 63S/T 1115C) da1 at ahc0 bus 0 target 1 lun 0 da1: Fixed Direct Access SCSI-2 device da1: 40.000MB/s transfers (20.000MHz, offset 15, 16bit), Tagged Queueing Enabled da1: 4357MB (8925000 512 byte sectors: 255H 63S/T 555C) options NETGRAPH options NETGRAPH_KSOCKET options NETGRAPH_SOCKET # # BLAKE # machine "i386" cpu "I686_CPU" ident BLAKE maxusers32 options SOFTUPDATES options DDB options INVARIANTS options INVARIANT_SUPPORT options NMBCLUSTERS=4096 options "CLK_USE_I8254_CALIBRATION" options
