Re: [PATCH v2] xfs: test agfl reset on bad list wrapping

Eryu Guan
2018-03-21T14:45:43Z

On Tue, Mar 20, 2018 at 08:17:46PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> From the kernel patch that this test examines ("xfs: detect agfl count
> corruption and reset agfl"):
>
> "The struct xfs_agfl v5 header was originally introduced with
> unexpected padding that caused the AGFL to operate with one less
> slot than intended. The header has since been packed, but the fix
> left an incompatibility for users who upgrade from an old kernel
> with the unpacked header to a newer kernel with the packed header
> while the AGFL happens to wrap around the end. The newer kernel
> recognizes one extra slot at the physical end of the AGFL that the
> previous kernel did not. The new kernel will eventually attempt to
> allocate a block from that slot, which contains invalid data, and
> cause a crash.
>
> "This condition can be detected by comparing the active range of the
> AGFL to the count. While this detects a padding mismatch, it can
> also trigger false positives for unrelated flcount corruption. Since
> we cannot distinguish a size mismatch due to padding from unrelated
> corruption, we can't trust the AGFL enough to simply repopulate the
> empty slot.
>
> "Instead, avoid unnecessarily complex detection logic and and use a
> solution that can handle any form of flcount corruption that slips
> through read verifiers: distrust the entire AGFL and reset it to an
> empty state. Any valid blocks within the AGFL are intentionally
> leaked. This requires xfs_repair to rectify (which was already
> necessary based on the state the AGFL was found in). The reset
> mitigates the side effect of the padding mismatch problem from a
> filesystem crash to a free space accounting inconsistency."
>
> This test exercises the reset code by mutating a fresh filesystem to
> contain an agfl with various list configurations of correctly wrapped,
> incorrectly wrapped, not wrapped, and actually corrupt free lists; then
> checks the success of the reset operation by fragmenting the free space
> btrees to exercise the agfl. Kernels without this reset fix will shut
> down the filesystem with corruption errors.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> common/rc | 6 +
> tests/xfs/709 | 254 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> tests/xfs/709.out | 13 +++
> tests/xfs/group | 1
> 4 files changed, 274 insertions(+)
> create mode 100755 tests/xfs/709
> create mode 100644 tests/xfs/709.out
>
> diff --git a/common/rc b/common/rc
> index 2c29d55..8f048f1 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3440,6 +3440,12 @@ _get_device_size()
> grep `_short_dev $1` /proc/partitions | awk '{print $3}'
> }
>
> +# check dmesg log for a specific string
> +_check_dmesg_for() {
> + dmesg | tac | sed -ne "0,\#run fstests $seqnum at $date_time#p" | \
> + tac | egrep -q "$1"

Hmm, searching dmesg log for a specific test this way requires a
writable /dev/kmsg, we have checked it in 'check', otherwise we won't
write such logs to dmesg. Need a _require_check_dmesg or something?

And it seems this "dmesg | tac ... | tac" sequence can be factored out
to a helper and reused in _check_dmesg too.

Thanks,
Eryu