Add support of 4Kn kernel dumps.
ClosedPublic
Actions

Authored by cem on Apr 6 2016, 3:38 AM.

Details

Reviewers

ambrisko
rpokala

Commits

rS298076: Add 4Kn kernel dump support

Summary

Make sure all I/O is of sector size of the dump dev. The kernel dump header
is 512 bytes and assumed that would fit in sector but will be a partial
sector on a 4Kn disk. Introducing a new function dump_write_pad that
will write out a sector with the contents of the header padded with zeros
to the size of a sector. For now assume the max size is 4096. Using
malloc to allocate memory causes issues with:
Dump map grown while dumping. Retrying...

Update savecore to deal with 4Kn dumps.

Fix mrsas(4) so dumping works and temporarily fix mrsas(4) to boot
by removing the tsleep and replace it with DELAY in mrsas_complete_cmd
so booting doesn't hang.

Test Plan

Tested on amd64 with and without minidumps to 4Kn disks and 512 disks.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3274
Build 3308: arc lint + arc unit

Event Timeline

ambrisko updated this revision to Diff 14908.Apr 6 2016, 3:38 AM

ambrisko retitled this revision from to Add support of 4Kn kernel dumps..

ambrisko updated this object.

ambrisko edited the test plan for this revision. (Show Details)

Herald added subscribers: Contributor Reviews (src), imp. · View Herald TranscriptApr 6 2016, 3:38 AM

cem added reviewers: cem, rpokala.Apr 6 2016, 4:12 PM

We talked about a CTASSERT for some assumption over email. Was that not needed?

sys/kern/kern_dump.c
62–63	This is fine for now. I think we could also malloc a buffer (at runtime, not during panic/dump) based on the observed di->blocksize when a dumper is registered.
126–127	We can move this out of the loop.
169	sz and size is potentially confusing :).
202	Does savecore really support page sizes that require padding relative to the native block size? I would just leave this as `dump_write()` for now.
sys/kern/kern_shutdown.c
895	I don't like having a large buf like this on the kernel stack, especially during panic/dump. For now, adding `static` would be fine. We can switch to using `malloc` memory, described above, in a later revision (if we care).
897	`*size = roundup(length, di->blocksize);`
904	As a minor optimization, we only need to zero `(temp + length, sizeof(buf) - length)`.

I agree with all Conrad's comments. In addition, I have a few of my own:

sbin/savecore/savecore.c
496	"sizeof(kdhl)" style(9): "sizeof's are written with parenthesis always." Same issue in other places too.
sys/amd64/amd64/minidump_machdep.c
59	Shouldn't 'di' be passed in as an argument to DEV_ALIGN()?
sys/kern/kern_dump.c
59	These defines are duplicated in sys/amd64/amd64/minidump_machdep.c (and i386). They should probably be consolidated in a header somewhere; perhaps 'sys/x86/include/dump.h'?

Ping :-). I'm happy to make these changes, run it by Rpokala again, and get this committed in time for 11.0 if you're busy, Doug. Thanks!

This revision now requires changes to proceed.Apr 14 2016, 7:08 PM

cem commandeered this revision.Apr 14 2016, 9:50 PM

cem edited reviewers, added: ambrisko; removed: cem.

cem added inline comments.

sys/amd64/amd64/minidump_machdep.c
59	Neither of these macros is actually used in minidump_machdep; I just removed them.
sys/kern/kern_dump.c
59	They aren't used in minidump — I just removed them. They both seem to be a duplicate of `roundup2()` though. DEV_ALIGN(x) = roundup2(x, di->blocksize); MD_ALIGN(x) = roundup2(x, PAGE_SIZE); // PAGE_MASK = PAGE_SIZE-1

Apply my and Rpokala's suggestions.

Replace static 4096 char 'buffer' with a malloc(di->blocksize), kept in a new
dumperinfo .blockbuf member (allocated at runtime, not dump time).

Apply a few minor cleanups in areas I was touching (removing unused macros from
minidump_machdep, some bzero/bcopy to memset/memcpy conversion, making
whitespace consistent in conf.h, some style(9) nits). Got rid of DEV_ALIGN
entirely (used only once) from kern_dump.c.

One thing I forgot to note— I removed the mrsas(4) changes from this diff. I think they should be committed separately.

I've tested the new patch on a normal 512-byte hdd. savecore(8) works for minidumps and full dumps; KGDB seems to understand both kinds of core just fine.

I just performed the following procedure:

% dumpon ${DUMPDEV}
sysctl debug.minidump=${MINIDUMP}
break into debugger
db> call doadump
db> continue
% savecore . ${DUMPDEV}
% kgdb -q /boot/kernel/kernel vmcore.${N}
(kgdb) bt

There are four cases:

DUMPDEV is 512n; MINIDUMP=1
DUMPDEV is 512n; MINIDUMP=0
DUMPDEV is 4Kn; MINIDUMP=1
DUMPDEV is 4Kn; MINIDUMP=0

In almost all cases, `kgdb' was able to extract a reasonable backtrace from the vmcore. The exception was 512n/MINIDUMP=0, which was untested due to insufficient space for a full dump.

Commit it! :-)

This revision is now accepted and ready to land.Apr 15 2016, 5:08 PM

Closed by commit rS298076: Add 4Kn kernel dump support (authored by cem). · Explain WhyApr 15 2016, 5:45 PM

This revision was automatically updated to reflect the committed changes.