On Mon, Jan 11, 2021 at 10:33:42AM +0200, Nikolay Borisov wrote:
> On 8.01.21 г. 18:01 ч., David Sterba wrote:
> > On Fri, Dec 18, 2020 at 02:24:20PM -0500, Josef Bacik wrote:
> >> @@ -2043,23 +2043,22 @@ int btrfs_commit_transaction(struct
> >> btrfs_trans_handle *trans)
> >> btrfs_trans_release_metadata(trans);
> >> trans->block_rsv = NULL;
> >>
> >> - /* make a pass through all the delayed refs we have so far
> >> - * any runnings procs may add more while we are here
> >> - */
> >> - ret = btrfs_run_delayed_refs(trans, 0);
> >> - if (ret) {
> >> - btrfs_end_transaction(trans);
> >> - return ret;
> >> - }
> >> -
> >> - cur_trans = trans->transaction;
> >> -
> >> /*
> >> - * set the flushing flag so procs in this transaction have to
> >> - * start sending their work down.
> >> + * We only want one transaction commit doing the flushing so we do not
> >> + * waste a bunch of time on lock contention on the extent root node.
> >> */
> >> - cur_trans->delayed_refs.flushing = 1;
> >> - smp_wmb();
> >
> > This barrier obviously separates the flushing = 1 and the rest of the
> > code, now implemented as test_and_set_bit, which implies full barrier.
> >
> > However, hunk in btrfs_should_end_transaction removes the barrier and
> > I'm not sure whether this is correct:
> >
> > - smp_mb();
> > if (cur_trans->state >= TRANS_STATE_COMMIT_START ||
> > - cur_trans->delayed_refs.flushing)
> > + test_bit(BTRFS_DELAYED_REFS_FLUSHING,
> > + &cur_trans->delayed_refs.flags))
> > return true;
> >
> > This is never called under locks so we don't have complete
> > synchronization of neither the transaction state nor the flushing bit.
> > btrfs_should_end_transaction is merely a hint and not called in critical
> > places so we could probably afford to keep it without a barrier, or keep
> > it with comment(s).
>
> I think the point is moot in this case, because the test_bit either sees
> the flag or it doesn't. It's not possible for the flag to be set AND
> should_end_transaction return false that would be gross violation of
> program correctness.
So that's for the flushing part, but what about cur_trans->state?
