On Thu, Aug 31, 2017 at 11:10 PM, Brian Foster <bfoster@xxxxxxxxxx> wrote: > On Thu, Aug 31, 2017 at 10:20:19PM +0300, Amir Goldstein wrote: >> On Thu, Aug 31, 2017 at 7:39 PM, Brian Foster <bfoster@xxxxxxxxxx> wrote: ... >> > If we do something >> > like the above, I wonder if that means we could wait for the submit == >> > complete if we observe submit was bumped since it was initially sampled >> > above (rather than issue another flush, which would be necessary if a >> > submit hadn't occurred))..? >> > >> > If we do end up with something like this, I think it's a bit cleaner to >> > stuff the counter(s) in the xfs_buftarg structure and update them from >> > the generic buffer submit/completion code based on XBF_FLUSH. FWIW, I >> > suspect we could also update said counter(s) from >> > xfs_blkdev_issue_flush(). >> > >> >> I think what you are suggesting is to optimize more cases which are >> not optimized now. That is probably possible, but also more complicated >> to get right and not sure if the workloads that gain from this are important >> enough. >> > > Not necessarily. I'm just suggesting that the code could be factored > more generically/elegantly such that the logic is easier to follow. That > may facilitate optimizing more cases, but that's a secondary benefit. In > practice, the log buffer code is the only place we actually set > XBF_FLUSH, for example. > I guess that makes sense. Although it is going to end up with more code, so if we are not going for optimization for more cases (i.e. subsequent fdatasync) we should consider if the extra code is worth it. > >> If I am not mistaken the way to fix the current optimization is to record >> the last SYNC_DONE lsn (which is sort of what Christoph suggested) >> and the last WANY_SYNC|ACTIVE lsn. >> After file_write_and_wait() need to save pre_sync_lsn and before >> return need to make sure that post_sync_lsn >= pre_sync_lsn or >> issue a flush. >> > > Perhaps, but I'm not quite following what you mean by pre/post LSNs. > Note that I believe log buffers can complete out of order, if that is > relevant here. Either way, this still seems like underhanded logic > IMO... > > If the requirement is a simple "issue a flush if we can't detect that > one has submitted+completed on this device since our writeback > completed" rule, why intentionally obfuscate that with internal log > buffer state such as log buffer header LSN and log state machine values? > Just track flush submission/completions as you suggested earlier and the > fsync logic is much easier to follow. Then we don't need to work > backwards from the XFS logging infrastructure just to try and verify > whether a flush has occurred in all cases. :) > Your argument makes a lot of sense. I'm just trying to be extra cautious and looking for a small step solution. As Darrick wrote.. "safety first" :) Amir.