On Tue, Jul 25, 2017 at 01:32:03PM +1000, Dave Chinner wrote: > On Mon, Jul 24, 2017 at 05:14:33PM -0700, Darrick J. Wong wrote: > > On Tue, Jul 25, 2017 at 09:38:13AM +1000, Dave Chinner wrote: > > > On Mon, Jul 24, 2017 at 03:36:54PM -0700, Darrick J. Wong wrote: > > > > On Mon, Jul 24, 2017 at 11:43:27AM +1000, Dave Chinner wrote: > > > > > On Thu, Jul 20, 2017 at 09:39:00PM -0700, Darrick J. Wong wrote: > > > > > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > > > > > > > > > > > Call the verifier function for all in-memory metadata buffers, looking > > > > > > for memory corruption either due to bad memory or coding bugs. > > > > > > > > > > How does this fit into the bigger picture? We can't do an exhaustive > > > > > search of the in memory buffer cache, because access is racy w.r.t. > > > > > the life cycle of in memory buffers. > > > > > > > > > > Also, if we are doing a full scrub, we're going to hit and then > > > > > check the cached in-memory buffers anyway, so I'm missing the > > > > > context that explains why this code is necessary. > > > > > > > > Before we start scanning the filesystem (which could lead to clean > > > > buffers being pushed out of memory and later reread), we want to check > > > > the buffers that have been sitting around in memory to see if they've > > > > mutated since the last time the verifiers ran. > > > > > > I'm not sure we need a special cache walk to do this. > > > > > > My thinking is that if the buffers get pushed out of memory, the > > > verifier will be run at that time, so we don't need to run the > > > verifier before a scrub to avoid problems here. > > > > Agreed. > > > > > Further, if we read the buffer as part of the scrub and it's found > > > in cache, then if the scrub finds a corruption we'll know it > > > happened between the last verifier invocation and the scrub. > > > > Hm. Prior to the introduction of the metabufs scanner a few weeks ago, > > I had thought it sufficient to assume that memory won't get corrupt, so > > as long as the read verifier ran at /some/ point in the past we didn't > > need to recheck now. > > > > What if we scrap the metabufs scanner and adapt the read verifier > > function pointer to allow scrub to bypass the crc check and return the > > _THIS_IP_ from any failing structural test? Then scrubbers can call the > > read verifier directly and extract failure info directly. > > Yeah, that would work - rather than adapting the .read_verify op > we currently have, maybe a new op .read_verify_nocrc could be added? > THat would mostly just be a different wrapper around the existing > verify functions that are shared between the read and write > verifiers... I was going to call them ->verify_structure() or something like that. > > > If the buffer is not in cache and scrub reads the metadata from > > > disk, then the verifier should fire on read if the item is corrupt > > > coming off disk. If the verifier doesn't find corruption in this > > > case but scrub does, then we've got to think about whether the > > > verifier has sufficient coverage. > > > > Scrub has more comprehensive checks (or it will when xref comes along) > > so this is likely to happen, fyi. > > Yup, I expect it will. :) I also expect this to point out where the > verifiers can be improved, because I'm sure we haven't caught all > the "obviously wrong" cases in the verifiers yet... Well yes. :) I have been running the "dangerous_fuzzers dangerous_scrub dangerous_repair" tests again, as they fuzz everything to see what scrub complains about vs. what repair actually tries to fix. Current wtf list: - do we need to check all the padding areas? does repair? - problems with repair not resetting di_nlink when it moves stuff to l+f? (x/380) - repair doesn't correct remote symlink blocks - scrub doesn't barf on free block problems?? (x/396) - repair doesn't notice attr remote value block problems? (x/404) - XFS_WANT_CORRUPTED_ are verifiers, but should not trigger asserts? - xfs_repair segfaults when freetag is too big (Actually, I already sent a fix a couple of days ago for that last one, but nobody has reviewed it.) --D > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
