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.
>> 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
> 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
> 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
> 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)