On Thu, May 26, 2022 at 06:28:23PM +0300, Amir Goldstein 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. > Hm. Still no luck if I move to .109 and pull in those few patches. I assume there's nothing else potentially interesting about the test env other than the sparse file scratch dev (i.e., default mkfs options, etc.)? If so and you can reliably reproduce, I suppose it couldn't hurt to try and grab a tracepoint dump of the test when it fails (feel free to send directly or upload somewhere as the list may punt it, and please also include the dmesg output that goes along with it) and I can see if that shows anything helpful. I think what we want to know initially is what error code we're producing (-ENOSPC?) and where it originates, and from there we can probably work out how the transaction might be dirty. I'm not sure a trace dump will express that conclusively. If you wanted to increase the odds of getting some useful information it might be helpful to stick a few trace_printk() calls in the various trans cancel error paths out of xfs_create() to determine whether it's the inode allocation attempt that fails or the subsequent attempt to create the directory entry.. Brian > Thanks, > Amir. >
