On Fri, May 01, 2020 at 07:51:32AM -0400, Brian Foster wrote: > On Fri, May 01, 2020 at 12:42:45PM +0200, Christoph Hellwig wrote: > > On Fri, May 01, 2020 at 06:24:03AM -0400, Brian Foster wrote: > > > I recall looking at this when it was first posted and my first reaction > > > was that I didn't really like the interface. I decided to think about it > > > to see if it grew on me and then just lost track (sorry). It's not so > > > much passing a flag to commit as opposed to the flags not directly > > > controlling behavior (i.e., one flag means sync if <something> is true, > > > another flag means sync if <something else> is true, etc.) tends to > > > confuse me. I don't feel terribly strongly about it if others prefer > > > this pattern, but I still find the existing code more readable. > > > > > > I vaguely recall thinking it might be nice if we could dump this into > > > transaction state to avoid the aforementioned logic warts, but IIRC that > > > might not have been possible for all users of this functionality.. > > > > Moving the flag out of the transaction structure was the main motivation > > for this series - the fact that we need different arguments to > > xfs_trans_commit is just a fallout from that. The rationale is that > > I found it highly confusing to figure out how and where we set the sync > > flag vs having it obvious in the one place where we commit the > > transaction. > > > > Sorry, I was referring to moving your new [W|DIR]SYNC variants to > somewhere like xfs_trans_res->tr_logflags in the comment above, not the > existing XFS_TRANS_SYNC flag (which I would keep). Regardless, I didn't > think that would work across the board from looking at it before. > Perhaps it would work in some cases.. > > I agree that the current approach is confusing in that it's not always > clear when to set the sync flag. I disagree that this patch makes it > obvious and in one place because when I see this: > > error = xfs_trans_commit(args->trans, XFS_TRANS_COMMIT_WSYNC); > > ... it makes me think the flag has an immediate effect (like COMMIT_SYNC > does) and subsequently raises the same questions around the existing > code of when or when not to use which flag in the context of the > individual transaction. *shrug* Just my .02. Similarly, this fell off my radar screen once the three of us started lobbing log refactoring patchsets at each other. :) AFAICT the goal of the WSYNC/DIRSYNC logic is basically to annotate the transaction to say "sysadmin wants all (write|namespace) operations to commit synchronously", so at first I was a little surprised that this added a flags argument to xfs_trans_commit instead of adding a couple of flags to capture the "I'm a write op" or "I'm a namespace op" to xfs_trans_alloc. /Then/ I realized that xfs_trans_alloc lets you set t_flags directly with almost no validation and died a little inside. Then it occurred to me that maybe this is deliberately done just prior to the final commit so that the intermediate transaction rolls are not committed synchronously, just one big log force at the end. /And then/ I noticed that in the places where we set SYNC just prior to xfs_trans_commit, each deferred op that gets run via xfs_trans_commit's call to xfs_defer_finish_noroll blocks waiting for xfs_log_force_lsn, when we could probably get away with only forcing the log at the very end of the transaction chain. So, uh, my question is, would it make more sense to (a) create a separate trans_alloc flags namespace so that (b) we can add the wsync/dirsync annotations from the outset, while keeping in mind that (c) we could possibly let the intermediate transaction rolls commit in the usual asynchronous fashion so long as the last one forces the log? --D > Brian >
