Re: [PATCH] xfs: don't reuse busy extents on extent trim

[Amir Goldstein <amir73il@gmail.com>]

> > Hi Brian,
> >
> > This patch was one of my selected fixes to backport for v5.10.y.
> > It has a very scary looking commit message and the change seems
> > to be independent of any infrastructure changes(?).
> >
> > The problem is that applying this patch to v5.10.y reliably reproduces
> > this buffer corruption assertion [*] with test xfs/076.
> >
> > This happens on the kdevops system that is using loop devices over
> > sparse files inside qemu images. It does not reproduce on my small
> > VM at home.
> >
> > Normally, I would just drop this patch from the stable candidates queue
> > and move on, but I thought you might be interested to investigate this
> > reliable reproducer, because maybe this system exercises an error
> > that is otherwise rare to hit.
> >
> > It seemed weird to me that NOT reusing the extent would result in
> > data corruption, but it could indicate that reusing the extent was masking
> > the assertion and hiding another bug(?).
> >
>
> Indeed, this does seem like an odd failure. The shutdown on transaction
> cancel implies cancellation of a dirty transaction. This is not
> necessarily corruption as much as just being the generic
> naming/messaging related to shutdowns due to unexpected in-core state.
> The patch in question removes some modifications to in-core busy extent
> state during extent allocation that are fundamentally unsafe in
> combination with how allocation works. This change doesn't appear to
> affect any transaction directly, so the correlation may be indirect.
>
> xfs/076 looks like it's a sparse inode allocation test, which certainly
> seems relevant in that it is stressing the ability to allocate inode
> chunks under free space fragmentation. If this patch further restricts
> extent allocation by removing availability of some set of (recently
> freed, busy) extents, then perhaps there is some allocation failure
> sequence that was previously unlikely enough to mask some poor error
> handling logic or transaction handling (like an agfl fixup dirtying a
> transaction followed by an allocation failure, for example) that we're
> now running into.
>
> > Can you think of another reason to explain the regression this fix
> > introduces to 5.10.y?
> >
>
> Not off the top of my head. Something along the lines of the above seems
> plausible, but that's just speculation at this point.
>
> > Do you care to investigate this failure or shall I just move on?
> >
>
> I think it would be good to understand whether there's a regression
> introduced by this patch, a bug somewhere else or just some impedence
> mismatch in logic between the combination of this change and whatever
> else happens to be in v5.10.y. Unfortunately I'm not able to reproduce
> if I pull just this commit back into latest 5.10.y (5.10.118). I've
> tried with a traditional bdev as well as a preallocated and sparse
> loopback scratch dev.

I also failed to reproduce it on another VM, but it reproduces reliably
on this system. That's why I thought we'd better use this opportunity.
This system has lots of RAM and disk to spare so I have no problem
running this test in a VM in parallel to my work.

It is not actually my system, it's a system that Luis has setup for
stable XFS testing and gave me access to, so if the need arises
you could get direct access to the system, but for now, I have no
problem running the test for you.

> Have you tested this patch (backport) in isolation
> in your reproducer env or only in combination with other pending
> backports?
>

I tested it on top of 5.10.109 + these 5 patches:
https://github.com/amir73il/linux/commits/xfs-5.10.y-1

I can test it in isolation if you like. Let me know if there are
other forensics that you would like me to collect.

Thanks,
Amir.