On 6/14/13 2:44 PM, Ben Myers wrote: > Hey Eric, > > On Fri, Jun 14, 2013 at 02:18:20PM -0500, Eric Sandeen wrote: >> On 6/14/13 2:08 PM, Ben Myers wrote: >>> On Fri, Jun 14, 2013 at 11:15:41AM -0500, Eric Sandeen wrote: >>>> Ben, isn't it the case that the corruption would only happen if >>>> log replay failed for some reason (as has always been the case, >>>> verifier or not), but with the verifier in place, it kills replay >>>> even w/o other problems due to a logical problem with the >>>> (recently added) verifiers? >>> >>> It seems like the verifier prevented corruption from hitting disk during >>> log replay. >> >> It detected a an inconsistent *interim* state during replay, which is >> always made correct by log replay completion. But it *stopped* that log >> replay completion. And caused log replay to fail. And mount to fail. >> This is *new* behavior, and bad. >> >> As I understand it. >> >>> It is enforcing a partial replay up to the point where the >>> corruption occurred. Now you should be able to zero the log and the >>> filesystem is not corrupted. >>> >>>> IOW - this seems like an actual functional regression due to the >>>> addition of the verifier, and dchinner's patch gets us back >>>> to the almost-always-fine state we were in prior to the change. >>> >>> Oh, the spin doctor is *in*! >> >> This is not spin. >> >>> This isn't a logical problem with the verifier, it's a logical problem >>> with log replay. We need to find a way for recovery to know whether a >>> given transaction should be replayed. Fixing that is nontrivial. >> >> Right. >> >> And it's been around for years. The verifier now detects that >> interim state, and makes things *worse* than they would be had log >> replay been allowed to continue. >> >> Fixing the interim state may be nontrivial; allowing log replay >> to continue to a consistent state as it always has *is* trivial, >> it's what's done in Dave's small patch. >> >>>> As we're at -rc6, it seems quite reasonable to me as a quick >>>> fix to just short-circuit it for now. >>> >>> If we're talking about a short term fix, that's fine. This should be >>> conditional on CONFIG_XFS_DEBUG and marked as such. >>> >>> Long term, removing the verifiers is the wrong thing to do here. We >>> need to fix the recovery bug and then remove this temporary workaround. >>> >>>> If you have time to analyze dave's metadump that's cool, but >>>> this seems like something that really needs to be addressed >>>> before 3.10 gets out the door. >>> >>> If this really is a day one bug then it's been out the door almost >>> twenty years. And you want to hurry now? ;) >> >> We seem to be talking past each other. >> >> The corrupted interim state has been around for years. Up until >> now, log replay completion left things in perfect state. >> >> The verifier now *breaks replay* at that interim point. >> Were it allowed to continue, everything would be fine. >> >> As things stand, it is not fine, and this is a recent change >> which Dave is trying to correct. >> >> Leaving it in place will cause filesystems which were replaying >> logs just fine until recently to now fail with no good way out. > > That is consistent with my understanding of the problem... > > Unfortunately log replay is broken. The verifier has detected this and stopped > replay. Ideally the solution would be to fix log replay, but that is going to > take some time. So, in the near term we're just going to disable the verifier > to allow replay to complete. Right, that's what we're hoping - for 3.10 right? Maybe the talking-past-each-other was only that part. I thought you didn't want to disable it for now. > I'm suggesting that this disabling be done conditionally on CONFIG_XFS_DEBUG so > that developers still have a chance at hitting the log replay problem, so that real-world users will still feel the pain ...? Or did you say that backwards (really only *disabling* it under debug?) Ok, confirmed on IRC you mean to disable it if *NOT* debug, enable it under debug. > and a > comment should be added explaining that we've disabled the verifier due to a > specific bug as a temporary workaround and we'll re-enable the verifier once > it's fixed. I'll update the patch and repost. Maybe if the verifiers were *on* under debug that'd make sense. I think putting it under the config is overkill, since anyone who wants to fix it is surely capable of re-enabling it in the code. But if that avoids an impasse, I don't much care. > Are you guys arguing that the log replay bug should not be fixed? Speaking for myself, I'm not arguing that, not at all. (not that I know how to fix it, either) -Eric > -Ben > _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
