On Fri, May 01, 2020 at 09:53:57AM -0700, Darrick J. Wong wrote: > > - Setting XFS_LI_RECOVERED could also move to common code, basically > > set it whenever iop_recover returns. Also we can remove the > > XFS_LI_RECOVERED asserts in ->iop_recovery when the caller checks > > it just before. > > I've noticed two weird things about the xfs_*_recover functions: > > 1. We'll set LI_RECOVERED if the intent is corrupt or if the final > commit succeeds (or fails), but we won't set it for other error bailouts > during recovery (e.g. xfs_trans_alloc fails). > > 2. If the intent is corrupt, iop_recovery also release the intent item, > but we don't do that for any of the other error returns from the > ->iop_recovery function. AFAICT those items (including the one that > failed recovery) are still on the AIL list and get released when we call > cancel_intents, which means that iop_recovery should /not/ be releasing > the item, right? LI_RECOVERED just prevents entering ->iop_recover again. Given that we give up after any failed recovery I don't think it matters if we set it or not. That being said, we should be consistent, and taking the setting into the caller will force them to be consistent. Well, releasing them will remove them from the AIL. So I think the manual release is pointless, but not actively harmful. But again, removing them is probably and improvements, as that means all the releasing from the AIL is driven from the common code.
