| Message ID | 20180508161353.GA18224@deer-run.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 5/8/18 11:13 AM, hal@deer-run.com wrote: <snip totally acceptable & legit reasons for needing this> ;) >> Otherwise I might suggest adding a switch specific to the blockget_f >> function, which would just skip the sb_logcheck() altogether. That would >> be more targeted, and wouldn't be some global switch which affects >> every current and future caller of sb_logcheck. A warning about how >> the log is being ignored and results may be inconsistent could then >> be added specifically to blockget_f. > Which sort of brings us back to the conversation that Darrick and I > have been having. Adding a command-line switch seems wrong-- I'm fully > convinced of that. But I'd still like to be able to blockget to try > to work if the file system is dirty but "-r" is used. Right, but my concern with "-r" or anything implying "readonly" as a modifier is that check & blockget are /always/ readonly. A readonly modifier to a readonly command makes little sense IMHO. > Darrick came up with one fix, which is the first patch option below. > After staring at Darrick's suggestion a while, I came up with a > different fix (second patch) that requires more code changes but I > think more accurately addresses the problem statement. See previous > messages in this thread for more detail. Yup, I have read them. > Let me know which you prefer and I'll submit the patch in the proper > format to the list. Well, I don't really like either patch for the reasons stated above. xfs_db /can/ write to the filesystem, and -r disables that... but -r as a modifier to affect the behavior of a read-only command is unintuitive. "It is only necessary to omit this flag if a command that changes data (write, blocktrash, crc) is to be used." What are the objections to a blockget modifier option? That seemed like the most direct & obvious solution to me. "blockget/check -L : attempt to perform the blockget and check functions even if the log contains unreplayed metadata," or something like that? Thanks, -Eric > Cheers! > > --Hal > > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Well, I don't really like either patch for the reasons stated above. > xfs_db /can/ write to the filesystem, and -r disables that... but -r > as a modifier to affect the behavior of a read-only command is > unintuitive. > > "It is only necessary to omit this flag if a command that changes data > (write, blocktrash, crc) is to be used." > > What are the objections to a blockget modifier option? That seemed like > the most direct & obvious solution to me. > > "blockget/check -L : attempt to perform the blockget and check functions > even if the log contains unreplayed metadata," or something like that? Ah, OK. I'm getting what you're saying now. Let me poke around with the code some and produce a patch that does what you're suggesting. --Hal -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 08, 2018 at 11:43:40AM -0500, hal@deer-run.com wrote: > > Well, I don't really like either patch for the reasons stated above. > > xfs_db /can/ write to the filesystem, and -r disables that... but -r > > as a modifier to affect the behavior of a read-only command is > > unintuitive. > > > > "It is only necessary to omit this flag if a command that changes data > > (write, blocktrash, crc) is to be used." > > > > What are the objections to a blockget modifier option? That seemed like > > the most direct & obvious solution to me. > > > > "blockget/check -L : attempt to perform the blockget and check functions > > even if the log contains unreplayed metadata," or something like that? That also seems fine to me. :) > Ah, OK. I'm getting what you're saying now. Let me poke around with > the code some and produce a patch that does what you're suggesting. <nod> --D > --Hal > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 08, 2018 at 11:13:53AM -0500, hal@deer-run.com wrote: > > Hm, so this probably makes it possible that blockget will wander off > > into inconsistencies and fail; there's a /reason/ for the check. So > > if anything this is probably best-effort. I doubt you can rely on > > blockget even completing without error, let alone being correct. In > > that case, is it still useful to you? > > I've been testing a modified version of xfs_db on live file systems > and dirty image files, and I have yet to have blockget fail. I agree > that it's certainly possible for blockget to blow up under these > circumstances, but it's working often enough to be useful. That's a slippery slope. IMO, the only thing worse than not having a forensic tool for a specific job is having a forensic tool provided by a trusted toolkit whose results are unreliable and cannot be trusted.... > > What sort of information do you hope to gather after running > > blockget without a replay? > > Suppose I can match a string or magic number for a file of interest > at a specific byte offset in the file system image. A little arithmetic > and I have a daddr value. xfs_db let's me convert that to an fsblock > and then call blockuse -n to get the inode and file path. I've tried > it, and it works with my modifed xfs_db. There are so many ways that can go wrong on a live filesystem. Regardless of whether you've personally seen it go wrong, it's still not a reliable method of information extraction. And we know that there's every chance that xfs_db will stumble on something unexpected in a live filesystem traversal and just crash. FWIW, blockuse -n also requires a full filesystem scan to find the file path (i.e. via blockget -n). That can take a long time, affect anything that is running on the machine at the time. And by the time it's all done, there's no guarantee the path it comes up will be valid or correct.... What you are trying to do - offset/block to owner path lookups on live filesystems - is something that the FSMAP ioctl and the upcoming parent pointer functionality will solve..... > Until we get XFS support into libsleuthkit (which I'm working towards, > but it's going to be a while), having this functionality lets me > use xfs_db as a forensic tool. And it also lets me use xfs_db to > validate other forensic tools. I'd suggest, in that case, that you limit it's use to off-line or read-only snapshots of online filesystems? This would mean that the results of xfs_db operations (while eceedingly slow) will be reliable. Cheers, Dave.
diff --git a/db/sb.c b/db/sb.c index c7fbfd6..fca048c 100644 --- a/db/sb.c +++ b/db/sb.c @@ -259,7 +259,10 @@ sb_logcheck(void) "the xfs_repair -L option to destroy the log and attempt a repair.\n" "Note that destroying the log may cause corruption -- please attempt a mount\n" "of the filesystem before doing this.\n"), progname); - return 0; + /* Dirty log would normally result in "return 0". + Return 1 if "-r" (read-only) option is used so + blockget can proceed. */ + return (x.isreadonly & LIBXFS_ISREADONLY) ? 1 : 0; } /* Log is clean */ return 1; -- 1.8.3.1 [PATCH OPTION #2] --- db/check.c | 5 ++++- db/sb.c | 8 ++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/db/check.c b/db/check.c index 2f8dee5..885e84a 100644 --- a/db/check.c +++ b/db/check.c @@ -1849,6 +1849,7 @@ init( xfs_fsblock_t bno; int c; xfs_ino_t ino; + int lc; int rt; serious_error = 0; @@ -1858,7 +1859,9 @@ init( serious_error = 1; return 0; } - if (!sb_logcheck()) + lc = sb_logcheck(); + /* abort on error or if log is dirty and "-r" flag not used */ + if (lc < 0 || (lc == 0 && !(x.isreadonly & LIBXFS_ISREADONLY))) return 0; rt = mp->m_sb.sb_rextents != 0; dbmap = xmalloc((mp->m_sb.sb_agcount + rt) * sizeof(*dbmap)); diff --git a/db/sb.c b/db/sb.c index c7fbfd6..88e5a94 100644 --- a/db/sb.c +++ b/db/sb.c @@ -235,13 +235,13 @@ sb_logcheck(void) if (x.logdev && x.logdev != x.ddev) { dbprintf(_("aborting - external log specified for FS " "with an internal log\n")); - return 0; + return -1; } } else { if (!x.logdev || (x.logdev == x.ddev)) { dbprintf(_("aborting - no external log specified for FS " "with an external log\n")); - return 0; + return -1; } } @@ -250,7 +250,7 @@ sb_logcheck(void) dirty = xlog_is_dirty(mp, mp->m_log, &x, 0); if (dirty == -1) { dbprintf(_("ERROR: cannot find log head/tail, run xfs_repair\n")); - return 0; + return -1; } else if (dirty == 1) { dbprintf(_( "ERROR: The filesystem has valuable metadata changes in a log which needs to\n" @@ -271,7 +271,7 @@ sb_logzero(uuid_t *uuidp) int cycle = XLOG_INIT_CYCLE; int error; - if (!sb_logcheck()) + if (sb_logcheck() < 1) return 0; /*