Re: [PATCH v3 4/5] xfs_db: sanitize geometry on load
From: Eric Sandeen <hidden>
Date: 2017-01-14 02:35:04
On 1/13/17 7:35 AM, Brian Foster wrote:
quoted
+ fprintf(stderr, +_("%s: device %s AG count is insane, but could be %u. Limiting reads to AG 0.\n"), + progname, fsdevice, dblocks / sbp->sb_agblocks); + } else { + fprintf(stderr, +_("%s: device %s AG count is insane. Limiting reads to AG 0.\n"), + progname, fsdevice); + }For reasons like the above, I think xfs_db shouldn't be in the business of repair like validation (xfs_check notwithstanding). That said, dropping into a fixed single AG mode seems less risky than trying to surmise a valid geometry. I'd get rid of the "this might be your agcount" messaging entirely though and just replace it with something that explicitly states the filesystem is corrupted, the runtime geometry is invalid and that the user should probably run xfs_repair before doing anything.
So keep in mind that xfs_db is for people with super xfs powers. (*) I wouldn't suggest repair, I'd start with 1 ag to avoid the OOM, state that clearly, and punt the problem to the admin with no other specific suggestions.
I still like the idea of the single AG mode thing as a command line flag rather than default behavior because it requires user acknowledgement, but this is a debug tool after all, so I'll defer to Eric on that. I do think that if we create this kind of invalid runtime mode, this should be split into two patches. First, a bugfix patch for the core OOM problem (i.e., detect a wacky superblock and exit). Second, replace the exit with the single AG runtime mode thing.
Well, the problem with a flag, I think, is that you might have already unwittingly OOMed your box to find out that you need it. Rebooting to try again with a flag sucks. (*) unless you are invoking it via xfs_admin.sh, dammit. We sure wouldn't want xfs_admin to exit happily, having updated only one AG. Dammit! Perhaps it should set exitcode, and then xfs_admin could do something like: xfs_db -c quit $DEV first, and check that db is able to initialize sanely before using it again to perform normal admin functions. -Eric