On Thu, May 26, 2022 at 6:28 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > 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. >. FWIW, the test fails with just that one patch over 5.10.109. Thanks, Amir.
