Re: [PATCH v2] xfs: fix incorrect log_flushed on fsync

[Brian Foster <bfoster@redhat.com>]

On Tue, Sep 05, 2017 at 04:54:42PM +0200, Christoph Hellwig wrote:
> On Tue, Sep 05, 2017 at 10:40:07AM -0400, Brian Foster wrote:
> > Thanks.. I agree that a rework of the optimization can come later now
> > that the bug is fixed.
> 
> I agree in principle, but I don't think the patch in for-next is enough,
> given that it doesn't take ic_refcnt into account.
> 

Hmm.. Ok, I might be missing something here. In xfs_log_force() at least
it looks like we only set log_flushed if the refcnt has reached zero.

In xfs_log_force_lsn(), the iclog has to be ACTIVE. We switch it to
WANT_SYNC, bump the refcnt and call release_iclog(). The latter may or
may not do anything depending on the refcount. We set log_flushed
immediately after the release before we know whether an xlog_sync() has
actually occurred (is that your concern?). If XFS_LOG_SYNC is set, we
wait on the iclog before _force_lsn() returns, which means the log
buffer I/O has to have completed before we return either way. If the
iclog is still WANT_SYNC, the release didn't do anything, but we wait on
ic_force_wait which is triggered during I/O completion handling. If the
iclog is already DIRTY/ACTIVE, then the I/O has already submitted and
completed.

fsync looks like the only the user of log_flushed and it also uses
XFS_LOG_SYNC. ISTM that the code is probably still bogus in principle,
but in practice the fsync code may be safe. Thoughts? Am I missing
something else?

> > Christoph, are you planning to continue with your flushseq based patch?
> 
> I think it is fundamentally the right thing to do, but given how
> much I've got on my plate I wouldn't mind someone else looking into it :)

Ok, I may be able to take a crack at it...

Brian

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html