On Fri, May 17, 2019 at 01:49:16PM -0400, Brian Foster wrote: > Refactoring aside, I see that the majority of this patch is focused on > intent log item implementations. I don't think an item leak is possible > here because we intentionally dirty transactions when either an intent > or done item is logged. See xfs_extent_free_log_item() and > xfs_trans_free_extent() for examples associated with the EFI/EFD items. That's why I said theoretical. > On one hand this logic supports the current item reference counting > logic (for example, so we know whether to drop the log's reference to an > EFI on transaction abort or wait until physical log commit time). On the > other hand, the items themselves must be logged to disk so we have to > mark them dirty (along with the transaction on behalf of the item) for > that reason as well. FWIW, I do think the current approach of adding the > intent item and dirtying it separately is slightly confusing, > particularly since I'm not sure we have any valid use case to have a > clean intent/done item in a transaction. Indeed. I think there is plenty of opportunity for futher wok here. > I suppose this kind of refactoring might still make sense on its own if > the resulting code is more clear or streamlined. I.e., perhaps there's > no need for the separate ->iop_committing() and ->iop_unlock() callbacks > invoked one after the other. That said, I think the commit log should > probably be updated to focus on that (unless I'm missing something about > the potential leak). Hm? The streamlining was the the point. I just noticed that if we were every to about a clean intent item we'd leak it while doing that.
