On Fri, Mar 05, 2021 at 09:58:26AM -0500, Brian Foster wrote: > On Fri, Mar 05, 2021 at 09:48:48AM +1100, Dave Chinner wrote: > > > > > So not only does this patch expose subsystem internals to > > > > > external layers and create more log forcing interfaces/behaviors to > > > > > > > > Sorry, I don't follow. What "subsystem internals" are being exposed > > > > and what external layer are they being exposed to? > > > > > > > > > > Cleaning up the mess that is the xfs_log_* and xlog_* interfaces and > > > > > > changing things like log force behaviour and implementation is for > > > > > > a future series. > > > > > > > > > > > > > > > > TBH I think this patch is kind of a mess on its own. I think the > > > > > mechanism it wants to provide is sane, but I've not even got to the > > > > > point of reviewing _that_ yet because of the seeming dismissal of higher > > > > > level feedback. I'd rather not go around in circles on this so I'll just > > > > > offer my summarized feedback to this patch... > > > > > > > > I'm not dismissing review nor am I saying the API cannot or should > > > > not be improved. I'm simply telling you that I think the additional > > > > changes you are proposing are outside the scope of the problem I am > > > > addressing here. I already plan to rework the log force API (and > > > > others) but doing so it not something that this patchset needs to > > > > address, or indeed should address. > > > > > > > > > > I'm not proposing additional changes nor to rework the log force API. > > > I'm pointing out that I find this implementation to be extremely and > > > unnecessarily confusing. > > > > And that's just fine - not everyone has to understand all of the > > code all of the time. That's why we have a team.... > > > > > To improve it, I'm suggesting to either coopt > > > the existing async log force API... > > > > And I'm telling you that this is *future work*. As the person > > actually doing the work, that's my decision and you need to accept > > that. I understand your concerns and have a plan to address them - > > you just have to accept that the plan to address them isn't going to > > be exactly to your liking. > > > > As a reviewer, I am pointing out that I object to how this patch is > implemented and offered several fairly simple suggestions on how to > address my concerns. Those suggestions have been rejected pretty much > out of hand on the basis of the existence of some future plans. We're allowed to disagree on the best approach. But to find a way forward means that both the summitter and the reviewer need to compromise. I've fixed up the obvious warts and bugs you've pointed out, and agreed that it needs cleaning up and have committed to cleaning it up in the near future. > Future rework plans do not justify or change my view of this patch > and the mess it adds (for other developers, for historical context > and downstreams, etc.). Therefore, if the only option you're > willing to consider is "make this mess now and clean it up later > in some broader rework," then I'd rather we just defer this patch > until after that rework is available and avoid the mess that way. So you won't review it until I have 100 outstanding patches in this series and it's completely and utterly unreviewable? Then you'll ask me to split it up and re-order it into digestable chunks, and so we'll go back to having to merge this because any of the API rework that depends on the mechanism that this patch introduces. I'm not going to play that "now jump through this hoop" game. We add flags for on-off behaviours in internal functions -all the time-. If this makes the interface so complex and confusing that you don't understand it, then the interface was already too complex and confusing. And fixing that is *not in the scope of this patchset*. Demanding that code be made perfect before it can be merged is really not very helpful. Especially when there are already plans to rework the API but that rework is dependent on a bunch of other changes than need to be done first. iclogs are something that need to be moved behind the CIL, not sit in front of CIL. The CIL abstracts the journal and writing to the journal completely away from the transaction subsystem, yet the log force code punches straight through that abstraction and walks iclogs directly. The whole log force implementation needs to change, and I plan for the API that wraps the log forces to get reworked at that time. For example, if we want to direct map storage for log writes, then iclog-based log force synchronisation needs to go away because we don't need iclogs for buffering journal writes. Hence the log foce code should interface only with the CIL, and only the CIL should manage whatever mechanism it is using to write to stable storage. The code is currently the way it is because the CIL, when first implemented, had to co-exist with the old way of writing to the log. We haven't used that old way for a decade now and we have very different storage performance characteristics these days, so it's about time we got rid of the mechanism designed to be optimal for spinning disks and actually integrated the CIL and the log efficiently. There are a *lot* of steps to do this, and reworking the log force implementation and API is part of that. But reworking that API is premature because we haven't done all the necessary pre-work in place to make such a change yet. This patch is actually part of that pre-work to get the mechanisms that the log force rework will rely on. I have very good reasons for pushing back against your suggestions, Brian. Your suggestions have merit but this patch is not the time to be making the changes you suggest. Code does not need to be perfect to be merged, nor does the entire larger change they will be part of need to be complete and 100% tested and reviewed before preparation and infrastructure patches can be merged. This is how we've done big changes in the past - they've been staged across multiple kernel cycles - and not everything needs to be done in the first patchset of a larger body of work.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
