xfs: fixes for 3.10-rc6

From: Dave Chinner <[email protected]>

Unfortunately, we cannot guarantee that items logged multiple times and replayed by log recovery do not take objects back in time. When theya re taken back in time, the go into an intermediate state which is corrupt, and hence verification that occurs on this intermediate state causes log recovery to abort with a corruption shutdown.

Instead of causing a shutdown and unmountable filesystem, don't verify post-recovery items before they are written to disk. This is less than optimal, but there is no way to detect this issue for non-CRC filesystems If log recovery successfully completes, this will be undone and the object will be consistent by subsequent transactions that are replayed, so in most cases we don't need to take drastic action.

For CRC enabled filesystems, leave the verifiers in place - we need to call them to recalculate the CRCs on the objects anyway. This recovery problem canbe solved for such filesystems - we have a LSN stamped in all metadata at writeback time that we can to determine whether the item should be replayed or not. This is a separate piece of work, so is not addressed by this patch.

Hey Dave,

Quoted 16 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Unfortunately, we cannot guarantee that items logged multiple times and replayed by log recovery do not take objects back in time. When theya re taken back in time, the go into an intermediate state which is corrupt, and hence verification that occurs on this intermediate state causes log recovery to abort with a corruption shutdown.

Instead of causing a shutdown and unmountable filesystem, don't verify post-recovery items before they are written to disk. This is less than optimal, but there is no way to detect this issue for non-CRC filesystems If log recovery successfully completes, this will be undone and the object will be consistent by subsequent transactions that are replayed, so in most cases we don't need to take drastic action.

Quoted 6 lines

For CRC enabled filesystems, leave the verifiers in place - we need to call them to recalculate the CRCs on the objects anyway. This recovery problem canbe solved for such filesystems - we have a LSN stamped in all metadata at writeback time that we can to determine whether the item should be replayed or not. This is a separate piece of work, so is not addressed by this patch.

Is there a test case for this one? How are you reproducing this?

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Unfortunately, we cannot guarantee that items logged multiple times and replayed by log recovery do not take objects back in time. When theya re taken back in time, the go into an intermediate state which is corrupt, and hence verification that occurs on this intermediate state causes log recovery to abort with a corruption shutdown.

Instead of causing a shutdown and unmountable filesystem, don't verify post-recovery items before they are written to disk. This is less than optimal, but there is no way to detect this issue for non-CRC filesystems If log recovery successfully completes, this will be undone and the object will be consistent by subsequent transactions that are replayed, so in most cases we don't need to take drastic action.

Quoted 8 lines

Quoted 6 lines

For CRC enabled filesystems, leave the verifiers in place - we need to call them to recalculate the CRCs on the objects anyway. This recovery problem canbe solved for such filesystems - we have a LSN stamped in all metadata at writeback time that we can to determine whether the item should be replayed or not. This is a separate piece of work, so is not addressed by this patch.

Is there a test case for this one? How are you reproducing this?

The test case was Dave Jones running sysrq-b on a hung test machine. The machine would occasionally end up with a corrupt home directory.

http://oss.sgi.com/pipermail/xfs/2013-May/026759.html

Analysis from a metdadump provided by Dave:

http://oss.sgi.com/pipermail/xfs/2013-June/026965.html

And Cai also appeared to be hitting this after a crash on 3.10-rc4, as it's giving exactly the same "verifier failed during log recovery" stack trace:

http://oss.sgi.com/pipermail/xfs/2013-June/026889.html

Hi Dave,

Quoted 40 lines · Dave Chinner

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Unfortunately, we cannot guarantee that items logged multiple times and replayed by log recovery do not take objects back in time. When theya re taken back in time, the go into an intermediate state which is corrupt, and hence verification that occurs on this intermediate state causes log recovery to abort with a corruption shutdown.

Instead of causing a shutdown and unmountable filesystem, don't verify post-recovery items before they are written to disk. This is less than optimal, but there is no way to detect this issue for non-CRC filesystems If log recovery successfully completes, this will be undone and the object will be consistent by subsequent transactions that are replayed, so in most cases we don't need to take drastic action.

Quoted 8 lines

Quoted 6 lines

For CRC enabled filesystems, leave the verifiers in place - we need to call them to recalculate the CRCs on the objects anyway. This recovery problem canbe solved for such filesystems - we have a LSN stamped in all metadata at writeback time that we can to determine whether the item should be replayed or not. This is a separate piece of work, so is not addressed by this patch.

Is there a test case for this one? How are you reproducing this?

The test case was Dave Jones running sysrq-b on a hung test machine. The machine would occasionally end up with a corrupt home directory.

http://oss.sgi.com/pipermail/xfs/2013-May/026759.html

Analysis from a metdadump provided by Dave:

http://oss.sgi.com/pipermail/xfs/2013-June/026965.html

And Cai also appeared to be hitting this after a crash on 3.10-rc4, as it's giving exactly the same "verifier failed during log recovery" stack trace:

Quoted 1 line

http://oss.sgi.com/pipermail/xfs/2013-June/026889.html

Thanks. It appears that the verifiers have found corruption due to a flaw in log recovery, and the fix you are proposing is to stop using them. If we do that, we'll have no way of detecting the corruption and will end up hanging users of older kernels out to dry.

I think your suggestion that non-debug systems could warn instead of fail is a good one, but removing the verifier altogether is inappropriate.

Can you make the metadump available? I need to understand this better before I can sign off. Also: Any idea how far back this one goes?

Quoted 43 lines · Ben Myers

Hi Dave,

Quoted 40 lines · Dave Chinner

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Unfortunately, we cannot guarantee that items logged multiple times and replayed by log recovery do not take objects back in time. When theya re taken back in time, the go into an intermediate state which is corrupt, and hence verification that occurs on this intermediate state causes log recovery to abort with a corruption shutdown.

Instead of causing a shutdown and unmountable filesystem, don't verify post-recovery items before they are written to disk. This is less than optimal, but there is no way to detect this issue for non-CRC filesystems If log recovery successfully completes, this will be undone and the object will be consistent by subsequent transactions that are replayed, so in most cases we don't need to take drastic action.

Quoted 8 lines

Quoted 6 lines

For CRC enabled filesystems, leave the verifiers in place - we need to call them to recalculate the CRCs on the objects anyway. This recovery problem canbe solved for such filesystems - we have a LSN stamped in all metadata at writeback time that we can to determine whether the item should be replayed or not. This is a separate piece of work, so is not addressed by this patch.

Is there a test case for this one? How are you reproducing this?

The test case was Dave Jones running sysrq-b on a hung test machine. The machine would occasionally end up with a corrupt home directory.

http://oss.sgi.com/pipermail/xfs/2013-May/026759.html

Analysis from a metdadump provided by Dave:

http://oss.sgi.com/pipermail/xfs/2013-June/026965.html

And Cai also appeared to be hitting this after a crash on 3.10-rc4, as it's giving exactly the same "verifier failed during log recovery" stack trace:

Quoted 6 lines

Quoted 1 line

http://oss.sgi.com/pipermail/xfs/2013-June/026889.html

Thanks. It appears that the verifiers have found corruption due to a flaw in log recovery, and the fix you are proposing is to stop using them. If we do that, we'll have no way of detecting the corruption and will end up hanging users of older kernels out to dry.

We've never detected it before, and it's causing regressions for multiple people. We *can't fix it* because we can't detect the situation sanely, and we are not leaving people with old kernels hanging out to dry. The opposite is true: we are fucking over current users by preventing log recovery on filesystems that will recovery perfectly OK and have almost always recovered just fine in the past.

Quoted 3 lines

I think your suggestion that non-debug systems could warn instead of fail is a good one, but removing the verifier altogether is inappropriate.

Changing every single verifier in a non-trivial way is not something I'm about to do for a -rc6 kernel. Removing the verifiers from log recovery just reverts to the pre-3.8 situation, so is perfectly acceptable short term solution while we do the more invasive verify changes.

Quoted 2 lines

Can you make the metadump available? I need to understand this better before I can sign off. Also: Any idea how far back this one goes?

No, I can't make the metadump available to you - it was provided privately and not obfuscated and so you'd have to ask Dave for it.

As to how long this problem has existed? It's a zero-day bug. Like I said, I've suspected for years that this can happen, and only now do we have proof of it...

Cheers,

Dave.

Quoted 43 lines · Ben Myers

Hi Dave,

Quoted 40 lines · Dave Chinner

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Unfortunately, we cannot guarantee that items logged multiple times and replayed by log recovery do not take objects back in time. When theya re taken back in time, the go into an intermediate state which is corrupt, and hence verification that occurs on this intermediate state causes log recovery to abort with a corruption shutdown.

Instead of causing a shutdown and unmountable filesystem, don't verify post-recovery items before they are written to disk. This is less than optimal, but there is no way to detect this issue for non-CRC filesystems If log recovery successfully completes, this will be undone and the object will be consistent by subsequent transactions that are replayed, so in most cases we don't need to take drastic action.

Quoted 8 lines

Quoted 6 lines

For CRC enabled filesystems, leave the verifiers in place - we need to call them to recalculate the CRCs on the objects anyway. This recovery problem canbe solved for such filesystems - we have a LSN stamped in all metadata at writeback time that we can to determine whether the item should be replayed or not. This is a separate piece of work, so is not addressed by this patch.

Is there a test case for this one? How are you reproducing this?

The test case was Dave Jones running sysrq-b on a hung test machine. The machine would occasionally end up with a corrupt home directory.

http://oss.sgi.com/pipermail/xfs/2013-May/026759.html

Analysis from a metdadump provided by Dave:

http://oss.sgi.com/pipermail/xfs/2013-June/026965.html

And Cai also appeared to be hitting this after a crash on 3.10-rc4, as it's giving exactly the same "verifier failed during log recovery" stack trace:

Quoted 6 lines

Quoted 1 line

http://oss.sgi.com/pipermail/xfs/2013-June/026889.html

Thanks. It appears that the verifiers have found corruption due to a flaw in log recovery, and the fix you are proposing is to stop using them. If we do that, we'll have no way of detecting the corruption and will end up hanging users of older kernels out to dry.

We've never detected it before, and it's causing regressions for multiple people. We *can't fix it* because we can't detect the situation sanely, and we are not leaving people with old kernels hanging out to dry. The opposite is true: we are fucking over current users by preventing log recovery on filesystems that will recovery perfectly OK and have almost always recovered just fine in the past.

Quoted 3 lines

I think your suggestion that non-debug systems could warn instead of fail is a good one, but removing the verifier altogether is inappropriate.

Changing every single verifier in a non-trivial way is not something I'm about to do for a -rc6 kernel. Removing the verifiers from log recovery just reverts to the pre-3.8 situation, so is perfectly acceptable short term solution while we do the more invasive verify changes.

Quoted 2 lines

Can you make the metadump available? I need to understand this better before I can sign off. Also: Any idea how far back this one goes?

No, I can't make the metadump available to you - it was provided privately and not obfuscated and so you'd have to ask Dave for it.

Hey,

Quoted 72 lines · Dave Chinner

Quoted 43 lines · Ben Myers

Hi Dave,

Quoted 40 lines · Dave Chinner

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Unfortunately, we cannot guarantee that items logged multiple times and replayed by log recovery do not take objects back in time. When theya re taken back in time, the go into an intermediate state which is corrupt, and hence verification that occurs on this intermediate state causes log recovery to abort with a corruption shutdown.

Instead of causing a shutdown and unmountable filesystem, don't verify post-recovery items before they are written to disk. This is less than optimal, but there is no way to detect this issue for non-CRC filesystems If log recovery successfully completes, this will be undone and the object will be consistent by subsequent transactions that are replayed, so in most cases we don't need to take drastic action.

Quoted 8 lines

Quoted 6 lines

For CRC enabled filesystems, leave the verifiers in place - we need to call them to recalculate the CRCs on the objects anyway. This recovery problem canbe solved for such filesystems - we have a LSN stamped in all metadata at writeback time that we can to determine whether the item should be replayed or not. This is a separate piece of work, so is not addressed by this patch.

Is there a test case for this one? How are you reproducing this?

The test case was Dave Jones running sysrq-b on a hung test machine. The machine would occasionally end up with a corrupt home directory.

http://oss.sgi.com/pipermail/xfs/2013-May/026759.html

Analysis from a metdadump provided by Dave:

http://oss.sgi.com/pipermail/xfs/2013-June/026965.html

And Cai also appeared to be hitting this after a crash on 3.10-rc4, as it's giving exactly the same "verifier failed during log recovery" stack trace:

Quoted 6 lines

Quoted 1 line

http://oss.sgi.com/pipermail/xfs/2013-June/026889.html

Thanks. It appears that the verifiers have found corruption due to a flaw in log recovery, and the fix you are proposing is to stop using them. If we do that, we'll have no way of detecting the corruption and will end up hanging users of older kernels out to dry.

We've never detected it before, and it's causing regressions for multiple people. We *can't fix it* because we can't detect the situation sanely, and we are not leaving people with old kernels hanging out to dry. The opposite is true: we are fucking over current users by preventing log recovery on filesystems that will recovery perfectly OK and have almost always recovered just fine in the past.

Quoted 3 lines

I think your suggestion that non-debug systems could warn instead of fail is a good one, but removing the verifier altogether is inappropriate.

Changing every single verifier in a non-trivial way is not something I'm about to do for a -rc6 kernel. Removing the verifiers from log recovery just reverts to the pre-3.8 situation, so is perfectly acceptable short term solution while we do the more invasive verify changes.

Quoted 2 lines

Can you make the metadump available? I need to understand this better before I can sign off. Also: Any idea how far back this one goes?

Quoted 2 lines

No, I can't make the metadump available to you - it was provided privately and not obfuscated and so you'd have to ask Dave for it.

Dave (Jones), could you make the metadump available to me? I'd like to understand this a little bit better. I'm a bit uncomfortable with the proposition that we should corrupt silently in this case...

Thanks,
Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 71 lines · Ben Myers

Quoted 70 lines · Dave Chinner

Quoted 41 lines · Ben Myers

Quoted 40 lines · Dave Chinner

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Unfortunately, we cannot guarantee that items logged multiple times and replayed by log recovery do not take objects back in time. When theya re taken back in time, the go into an intermediate state which is corrupt, and hence verification that occurs on this intermediate state causes log recovery to abort with a corruption shutdown.

Instead of causing a shutdown and unmountable filesystem, don't verify post-recovery items before they are written to disk. This is less than optimal, but there is no way to detect this issue for non-CRC filesystems If log recovery successfully completes, this will be undone and the object will be consistent by subsequent transactions that are replayed, so in most cases we don't need to take drastic action.

Quoted 8 lines

Quoted 6 lines

For CRC enabled filesystems, leave the verifiers in place - we need to call them to recalculate the CRCs on the objects anyway. This recovery problem canbe solved for such filesystems - we have a LSN stamped in all metadata at writeback time that we can to determine whether the item should be replayed or not. This is a separate piece of work, so is not addressed by this patch.

Is there a test case for this one? How are you reproducing this?

The test case was Dave Jones running sysrq-b on a hung test machine. The machine would occasionally end up with a corrupt home directory.

http://oss.sgi.com/pipermail/xfs/2013-May/026759.html

Analysis from a metdadump provided by Dave:

http://oss.sgi.com/pipermail/xfs/2013-June/026965.html

And Cai also appeared to be hitting this after a crash on 3.10-rc4, as it's giving exactly the same "verifier failed during log recovery" stack trace:

Quoted 6 lines

Quoted 1 line

http://oss.sgi.com/pipermail/xfs/2013-June/026889.html

Thanks. It appears that the verifiers have found corruption due to a flaw in log recovery, and the fix you are proposing is to stop using them. If we do that, we'll have no way of detecting the corruption and will end up hanging users of older kernels out to dry.

We've never detected it before, and it's causing regressions for multiple people. We *can't fix it* because we can't detect the situation sanely, and we are not leaving people with old kernels hanging out to dry. The opposite is true: we are fucking over current users by preventing log recovery on filesystems that will recovery perfectly OK and have almost always recovered just fine in the past.

Quoted 3 lines

I think your suggestion that non-debug systems could warn instead of fail is a good one, but removing the verifier altogether is inappropriate.

Changing every single verifier in a non-trivial way is not something I'm about to do for a -rc6 kernel. Removing the verifiers from log recovery just reverts to the pre-3.8 situation, so is perfectly acceptable short term solution while we do the more invasive verify changes.

Quoted 2 lines

Can you make the metadump available? I need to understand this better before I can sign off. Also: Any idea how far back this one goes?

Quoted 6 lines

Quoted 2 lines

No, I can't make the metadump available to you - it was provided privately and not obfuscated and so you'd have to ask Dave for it.

Dave (Jones), could you make the metadump available to me? I'd like to understand this a little bit better. I'm a bit uncomfortable with the proposition that we should corrupt silently in this case...

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?

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.

As we're at -rc6, it seems quite reasonable to me as a quick fix to just short-circuit it for now.

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.

Whenever you're ready, you can also add:

Reviewed-by Eric Sandeen <[email protected]>

(And dchinner, should this cc: stable for 3.9?)

Hey Eric,

Quoted 79 lines · Eric Sandeen

Quoted 77 lines · Ben Myers

Quoted 67 lines · Dave Chinner

Quoted 41 lines · Ben Myers

Quoted 40 lines · Dave Chinner

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Unfortunately, we cannot guarantee that items logged multiple times and replayed by log recovery do not take objects back in time. When theya re taken back in time, the go into an intermediate state which is corrupt, and hence verification that occurs on this intermediate state causes log recovery to abort with a corruption shutdown.

Instead of causing a shutdown and unmountable filesystem, don't verify post-recovery items before they are written to disk. This is less than optimal, but there is no way to detect this issue for non-CRC filesystems If log recovery successfully completes, this will be undone and the object will be consistent by subsequent transactions that are replayed, so in most cases we don't need to take drastic action.

Quoted 8 lines

Quoted 6 lines

For CRC enabled filesystems, leave the verifiers in place - we need to call them to recalculate the CRCs on the objects anyway. This recovery problem canbe solved for such filesystems - we have a LSN stamped in all metadata at writeback time that we can to determine whether the item should be replayed or not. This is a separate piece of work, so is not addressed by this patch.

Is there a test case for this one? How are you reproducing this?

The test case was Dave Jones running sysrq-b on a hung test machine. The machine would occasionally end up with a corrupt home directory.

http://oss.sgi.com/pipermail/xfs/2013-May/026759.html

Analysis from a metdadump provided by Dave:

http://oss.sgi.com/pipermail/xfs/2013-June/026965.html

And Cai also appeared to be hitting this after a crash on 3.10-rc4, as it's giving exactly the same "verifier failed during log recovery" stack trace:

Quoted 6 lines

Quoted 1 line

http://oss.sgi.com/pipermail/xfs/2013-June/026889.html

Thanks. It appears that the verifiers have found corruption due to a flaw in log recovery, and the fix you are proposing is to stop using them. If we do that, we'll have no way of detecting the corruption and will end up hanging users of older kernels out to dry.

We've never detected it before, and it's causing regressions for multiple people. We *can't fix it* because we can't detect the situation sanely, and we are not leaving people with old kernels hanging out to dry. The opposite is true: we are fucking over current users by preventing log recovery on filesystems that will recovery perfectly OK and have almost always recovered just fine in the past.

Quoted 3 lines

I think your suggestion that non-debug systems could warn instead of fail is a good one, but removing the verifier altogether is inappropriate.

Changing every single verifier in a non-trivial way is not something I'm about to do for a -rc6 kernel. Removing the verifiers from log recovery just reverts to the pre-3.8 situation, so is perfectly acceptable short term solution while we do the more invasive verify changes.

Quoted 5 lines

Quoted 2 lines

Can you make the metadump available? I need to understand this better before I can sign off. Also: Any idea how far back this one goes?

No, I can't make the metadump available to you - it was provided privately and not obfuscated and so you'd have to ask Dave for it.

Dave (Jones), could you make the metadump available to me? I'd like to understand this a little bit better. I'm a bit uncomfortable with the proposition that we should corrupt silently in this case...

Quoted 5 lines

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

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 3 lines

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? ;)

Quoted 3 lines

Whenever you're ready, you can also add:

Reviewed-by Eric Sandeen <[email protected]>

Sure.

Quoted 1 line

(And dchinner, should this cc: stable for 3.9?)

Looks like verifiers were added in 3.7. We should Cc stable.

Thanks,
Ben

Quoted 9 lines · Ben Myers

Quoted 5 lines · Eric Sandeen

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.

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Hey Eric,

Quoted 65 lines · Eric Sandeen

Quoted 9 lines · Ben Myers

Quoted 5 lines · Eric Sandeen

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.

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

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.

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,

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.

Are you guys arguing that the log replay bug should not be fixed?

-Ben

Quoted 77 lines · Ben Myers

Hey Eric,

Quoted 73 lines · Eric Sandeen

Quoted 9 lines · Ben Myers

Quoted 5 lines · Eric Sandeen

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.

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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)

Quoted 4 lines

Quoted 1 line

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)

...until log replay is fixed. Hell. I'll just pull it in as-is.

Reviewed-by Ben Myers <[email protected]>

Quoted 2 lines · Ben Myers

Quoted 2 lines

Quoted 1 line

Are you guys arguing that the log replay bug should not be fixed?

Quoted 6 lines

Quoted 2 lines

Speaking for myself, I'm not arguing that, not at all. (not that I know how to fix it, either)

...until log replay is fixed. Hell. I'll just pull it in as-is.

Reviewed-by Ben Myers <[email protected]>

Is this on its way to Linus ? Given it's a regression over 3.9, it probably should be.

Hi Dave,

Quoted 11 lines · Dave Jones

Quoted 8 lines · Ben Myers

Quoted 4 lines

Quoted 1 line

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)

...until log replay is fixed. Hell. I'll just pull it in as-is.

Reviewed-by Ben Myers <[email protected]>

Quoted 1 line

Is this on its way to Linus ? Given it's a regression over 3.9, it probably should be.

This one was in my pull request for -rc6. Looks like Linus merged it here:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d58c6ff0b779c5adae2a8596fde69cb45f2a5d68

We still need to propose this one for -stable, IIRC.

Quoted 16 lines · Ben Myers

Hi Dave,

Quoted 12 lines · Dave Jones

Quoted 8 lines · Ben Myers

Quoted 4 lines

Quoted 1 line

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)

...until log replay is fixed. Hell. I'll just pull it in as-is.

Reviewed-by Ben Myers <[email protected]>

Is this on its way to Linus ? Given it's a regression over 3.9, it probably should be.

Quoted 5 lines

This one was in my pull request for -rc6. Looks like Linus merged it here:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d58c6ff0b779c5adae2a8596fde69cb45f2a5d68

We still need to propose this one for -stable, IIRC.

Bah, no Reported-by:

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.

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

Are you guys arguing that the log replay bug should not be fixed?

Quoted 11 lines · Ben Myers

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.

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

Quoted 1 line

Are you guys arguing that the log replay bug should not be fixed?

I'm not arguing that it should not be fixed,

I'm *stating* that it
*can't be fixed* for non-CRC filesystems. That is, the best we can do is work around this deficiency in log recovery with some kind of self-defusing warning....

To fix it properly, you need to know the age of the object being overwritten relative to the age of overwrite data. For non-CRC filesystems we don't have that information in the metadata object being overwritten.

We can't even correctly identify the object being
overwritten.

So, it's simply not fixable in log recovery for non-CRC filesystems, and the LSN stamped in every piece of metadata at writeback time for CRC enabled filesystems is designed precisely to avoid this problem.

Indeed, the LSN stamp is a far more effective method than what used to be in the inode core to try to avoid the problem for unlogged inode size updates to try to prevent log recovery from replaying inode core updates over more recently written inodes - the di_flushiter field. Note the comment in xfs_flush_int():

/*
* bump the flush iteration count, used to detect flushes which
* postdate a log record during recovery. This is redundant as we now
* log every change and hence this can't happen. Still, it doesn't hurt. */
ip->i_d.di_flushiter++;

And this in xlog_recover_inode_pass2():

        /* Skip replay when the on disk inode is newer than the log one */
        if (dicp->di_flushiter < be16_to_cpu(dip->di_flushiter)) {

This recovery problem has been around forever, and it we cannot fix log recovery with age information in the metadata on disk that recovery is going overwrite. CRC enabled filesystems have that information on disk, existing filesystems don't. Therefore, we can only solve the recovery problem for CRC enabled filesystems...

We could probably also avoid the problem by modifying the way we do writeback from the AIL to limit it only to objects at the tail LSN, but that has a horrific performance penalty associated with it for many common workloads because of the way relogging works. And for a problem that I've suspected has occurred maybe 5 times in 10 years, modifying metadata writeback to avoid this problem is a bad tradeoff to make for just about everyone...

Hey Dave,

Quoted 12 lines · Dave Chinner

Quoted 11 lines · Ben Myers

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.

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

Quoted 3 lines

Quoted 1 line

Are you guys arguing that the log replay bug should not be fixed?

I'm not arguing that it should not be fixed,

Good.

Quoted 4 lines

I'm *stating* that it
*can't be fixed* for non-CRC filesystems. That is, the best we can do is work around this deficiency in log recovery with some kind of self-defusing warning....

I think you're mistaken here. In order to prevent this from happening on non-crc filesystems we need only ensure that all of the buffer modifications in the log for a given buffer are played back onto the buffer before we write it out to disk. My impression is that the final state of the buffer is always the correct one, so it's a matter of preventing any intermediate states from being written.

Quoted 4 lines

To fix it properly, you need to know the age of the object being overwritten relative to the age of overwrite data. For non-CRC filesystems we don't have that information in the metadata object being overwritten.

Only if you take the tack that you are not going to write intermediate state into the buffer. I agree this is one way to fix the problem, but I'm not sure that it's the only option.

Quoted 2 lines

We can't even correctly identify the object being
overwritten.

Can you be more specific about what you mean there?

Quoted 13 lines

Indeed, the LSN stamp is a far more effective method than what used to be in the inode core to try to avoid the problem for unlogged inode size updates to try to prevent log recovery from replaying inode core updates over more recently written inodes - the di_flushiter field. Note the comment in xfs_flush_int():

/*
* bump the flush iteration count, used to detect flushes which
* postdate a log record during recovery. This is redundant as we now
* log every change and hence this can't happen. Still, it doesn't hurt. */
ip->i_d.di_flushiter++;

Quoted 4 lines

And this in xlog_recover_inode_pass2():

        /* Skip replay when the on disk inode is newer than the log one */
        if (dicp->di_flushiter < be16_to_cpu(dip->di_flushiter)) {

Why is using the lsn more effective than the counter? Seems like the counter would be adequate as is.

Quoted 7 lines

We could probably also avoid the problem by modifying the way we do writeback from the AIL to limit it only to objects at the tail LSN, but that has a horrific performance penalty associated with it for many common workloads because of the way relogging works. And for a problem that I've suspected has occurred maybe 5 times in 10 years, modifying metadata writeback to avoid this problem is a bad tradeoff to make for just about everyone...

How would limiting writeback from the AIL to only structures at the tail resolve the issue of playback of intermediate states onto newer buffers during recovery? I don't understand what you're getting at here.

I think there may be some creative ways to resolve this problem for older filesystems which are worth looking into.

Quoted 13 lines

Quoted 3 lines

I think your suggestion that non-debug systems could warn instead of fail is a good one, but removing the verifier altogether is inappropriate.

Changing every single verifier in a non-trivial way is not something I'm about to do for a -rc6 kernel. Removing the verifiers from log recovery just reverts to the pre-3.8 situation, so is perfectly acceptable short term solution while we do the more invasive verify changes.

Quoted 2 lines

Can you make the metadump available? I need to understand this better before I can sign off. Also: Any idea how far back this one goes?

Quoted 2 lines

No, I can't make the metadump available to you - it was provided privately and not obfuscated and so you'd have to ask Dave for it.

Dave (Jones), could you make the metadump available to me? I'd like to understand this a little bit better. I'm a bit uncomfortable with the proposition that we should corrupt silently in this case...

Quoted 16 lines · Ben Myers

Quoted 15 lines

Quoted 3 lines

I think your suggestion that non-debug systems could warn instead of fail is a good one, but removing the verifier altogether is inappropriate.

Changing every single verifier in a non-trivial way is not something I'm about to do for a -rc6 kernel. Removing the verifiers from log recovery just reverts to the pre-3.8 situation, so is perfectly acceptable short term solution while we do the more invasive verify changes.

Quoted 2 lines

Can you make the metadump available? I need to understand this better before I can sign off. Also: Any idea how far back this one goes?

No, I can't make the metadump available to you - it was provided privately and not obfuscated and so you'd have to ask Dave for it.

Quoted 3 lines

Dave (Jones), could you make the metadump available to me? I'd like to understand this a little bit better. I'm a bit uncomfortable with the proposition that we should corrupt silently in this case...

Sorry, I don't have it any more. I'll see if I can recreate the problem
next week and prepare another dump.

From: Dave Chinner <[email protected]>

Michael L. Semon has been testing CRC patches ona 32 bit system and

been seeing assert failures in the directory code from xfs/080. Thanks to Michael's heroic efforts with printk debugging, we found that the problem was that the last free space being left in the directory structure was too small to fit a unused tag structure and it was being corrupted and attempting to log a region out of bounds. Hence the assert failure looked something like:

.....
#5 calling xfs_dir2_data_log_unused() 36 32
#1 4092 4095 4096
#2 8182 8183 4096

XFS: Assertion failed: first <= last && last < BBTOB(bp->b_length), file: fs/xfs/xfs_trans_buf.c, line: 568

Where #1 showed the first region of the dup being logged (i.e. the last 4 bytes of a directory buffer) and #2 shows the corrupt values being calculated from the length of the dup entry which overflowed the size of the buffer.

It turns out that the problem was not in the logging code, nor in the freespace handling code. It is an initial condition bug that only shows up on 32 bit systems. When a new buffer is initialised, where's the freespace that is set up:

[  172.316249] calling xfs_dir2_leaf_addname() from xfs_dir_createname()
[  172.316346] #9 calling xfs_dir2_data_log_unused()
[  172.316351] #1 calling xfs_trans_log_buf() 60 63 4096
[  172.316353] #2 calling xfs_trans_log_buf() 4094 4095 4096

Note the offset of the first region being logged? It's 60 bytes into the buffer. Once I saw that, I pretty much knew what the bug was going to be caused by this.

Essentially, all direct entries are rounded to 8 bytes in length, and all entries start with an 8 byte alignment. This means that we can decode inplace as variables are naturally aligned. With the directory data supposedly starting on a 8 byte boundary, and all entries padded to 8 bytes, the minimum freespace in a directory block is supposed to be 8 bytes, which is large enough to fit a unused data entry structure (6 bytes in size). The fact we only have 4 bytes of free space indicates a directory data block alignment problem.

And what do you know - there's an implicit hole in the directory data block header for the CRC format, which means the header is 60 byte on 32 bit intel systems and 64 bytes on 64 bit systems. Needs padding. And while looking at the structures, I found the same problem in the attr leaf header. Fix them both.

Note that this only affects 32 bit systems with CRCs enabled. Everything else is just fine. Note that filesystems created before

this fix on such systems will not be readable with this fix applied.

Reported-by Michael L. Semon <[email protected]>

Debugged-by Michael L. Semon <[email protected]>

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_attr_leaf.h   |    1 +
 fs/xfs/xfs_dir2_format.h |    5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

fs/xfs/xfs_attr_leaf.h +1 -0

index f9d7846..444a770 100644
@@ -128,6 +128,7 @@ struct xfs_attr3_leaf_hdr {
 	__u8			holes;
 	__u8			pad1;
 	struct xfs_attr_leaf_map freemap[XFS_ATTR_LEAF_MAPSIZE];
+	__be32			pad2;		/* 64 bit alignment */
 };
 
 #define XFS_ATTR3_LEAF_CRC_OFF	(offsetof(struct xfs_attr3_leaf_hdr, info.crc))

fs/xfs/xfs_dir2_format.h +0 -0

index 995f1f5..7826782 100644

Quoted 3 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Michael L. Semon has been testing CRC patches ona 32 bit system and

on a

Quoted 7 lines

been seeing assert failures in the directory code from xfs/080. Thanks to Michael's heroic efforts with printk debugging, we found that the problem was that the last free space being left in the directory structure was too small to fit a unused tag structure and it was being corrupted and attempting to log a region out of bounds. Hence the assert failure looked something like:

Quoted 4 lines

.....
#5 calling xfs_dir2_data_log_unused() 36 32
#1 4092 4095 4096
#2 8182 8183 4096

     first? 
          last?
               bp->b_length?

Quoted 2 lines

#1 4092 4095 4096
#2 8182 8183 4096

Quoted 1 line

#5 calling xfs_dir2_data_log_unused() 36 32

Quoted 3 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Michael L. Semon has been testing CRC patches ona 32 bit system and

on a

Quoted 7 lines

been seeing assert failures in the directory code from xfs/080. Thanks to Michael's heroic efforts with printk debugging, we found that the problem was that the last free space being left in the directory structure was too small to fit a unused tag structure and it was being corrupted and attempting to log a region out of bounds. Hence the assert failure looked something like:

Quoted 4 lines

.....
#5 calling xfs_dir2_data_log_unused() 36 32
#1 4092 4095 4096
#2 8182 8183 4096

     first? 
          last?
               bp->b_length?

Quoted 3 lines

Note that this only affects 32 bit systems with CRCs enabled. Everything else is just fine. Note that filesystems created before

CRC enabled filesystems

I suggest this be added to head off any confusion.

fs/xfs/xfs_dir2_format.h struct xfs_dir3_blk_hdr { · line 269

Quoted 8 lines

index 995f1f5..7826782 100644

fs/xfs/xfs_dir2_format.h +1 -0

@@ -266,6 +266,7 @@ struct xfs_dir3_blk_hdr {
 struct xfs_dir3_data_hdr {
 	struct xfs_dir3_blk_hdr	hdr;
 	xfs_dir2_data_free_t	best_free[XFS_DIR2_DATA_FD_COUNT];
+	__be32			pad;	/* 64 bit alignment */

I counted these up and it looks fine. Nice work gents.

Quoted 13 lines · Ben Myers

Quoted 3 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Michael L. Semon has been testing CRC patches ona 32 bit system and

on a

Quoted 7 lines

been seeing assert failures in the directory code from xfs/080. Thanks to Michael's heroic efforts with printk debugging, we found that the problem was that the last free space being left in the directory structure was too small to fit a unused tag structure and it was being corrupted and attempting to log a region out of bounds. Hence the assert failure looked something like:

Quoted 7 lines

Quoted 4 lines

.....
#5 calling xfs_dir2_data_log_unused() 36 32
#1 4092 4095 4096
#2 8182 8183 4096

     first? 
          last?
               bp->b_length?

Yup.

Quoted 1 line

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

Note that this only affects 32 bit systems with CRCs enabled. Everything else is just fine. Note that filesystems created before

CRC enabled filesystems

I suggest this be added to head off any confusion.

Sure. Do I need to resubmit this, or are you going to just modify the commit message yourself before applying it?

Quoted 10 lines

fs/xfs/xfs_dir2_format.h struct xfs_dir3_blk_hdr { · line 269

Quoted 8 lines

index 995f1f5..7826782 100644

fs/xfs/xfs_dir2_format.h +1 -0

@@ -266,6 +266,7 @@ struct xfs_dir3_blk_hdr {
 struct xfs_dir3_data_hdr {
 	struct xfs_dir3_blk_hdr	hdr;
 	xfs_dir2_data_free_t	best_free[XFS_DIR2_DATA_FD_COUNT];
+	__be32			pad;	/* 64 bit alignment */

I counted these up and it looks fine. Nice work gents.

pahole is a much better way of determining structure size - it tells you exactly what the compiler did, rather than having to assume what the compiler is going to do...

$ pahole fs/xfs/xfs_dir2_data.o |less

From: Dave Chinner <[email protected]>

For CRC enabled filesystems, the BMBT is rooted in an inode, so it passes through a difference code path on root splits to the

freespace and inode btrees. This is much less traversed by xfstests than the other trees. When testing on a 1k block size filesystem, I've been seeing ASSERT failures in generic/234 like:

XFS: Assertion failed: cur->bc_btnum != XFS_BTNUM_BMAP || cur->bc_private.b.allocated == 0, file: fs/xfs/xfs_btree.c, line: 317

which are generally preceded by a lblock check failure. I noticed this in the bmbt stats:

$ pminfo -f xfs.btree.block_map

xfs.btree.block_map.lookup

    value 39135

xfs.btree.block_map.compare

    value 268432

xfs.btree.block_map.insrec

    value 15786

xfs.btree.block_map.delrec

    value 13884

xfs.btree.block_map.newroot
value 2

xfs.btree.block_map.killroot
value 0
.....

Very little coverage of root splits and merges. Indeed, on a 4k filesystem, block_map.newroot and block_map.killroot are both zero. i.e the code is not exercised at all, and it's the only generic

btree infrastruct operation that is not exercised by a default run

of xfstests.

Turns out that on a 1k filesystem, generic/234 accounts for one of those two root splits, and that is somewhat of a smoking gun. In fact, it's the same problem we saw in the directory/attr code where headers are memcpy()d from one block to another without updating the self describing metadata.

Simple fix - when copying the header out of the root block, make sure the block number is updated correctly.

Signed-off-by Dave Chinner <[email protected]>

Quoted 5 lines · Dave Chinner

Turns out that on a 1k filesystem, generic/234 accounts for one of those two root splits, and that is somewhat of a smoking gun. In fact, it's the same problem we saw in the directory/attr code where headers are memcpy()d from one block to another without updating the self describing metadata.

It is very interesting that this area of code is exercised so infrequently.

I
remember seeing a paper that described a tool that would list codepaths that are exercised during a test run. Does that ring a bell? It seems like this might be worth looking into more generally.