Re: [PATCH 3/5] userns: Don't read extents twice in m_start

[Peter Zijlstra]

On Wed, Nov 01, 2017 at 12:20:52PM -0500, Eric W. Biederman wrote:

> Well the way I was hearing the conversation was that there was a patch
> that fixed a real bug, but it was wrong because checkpatch complained
> about it.
> 
> So I don't even know if the warning is a problem.  But blocking bug
> fixes because there is a warning certainly is.
> 
> If someone wants to change coding style in practice so that every
> smp_rmb and every smp_wmb has detailed comments that everyone must
> include they need to follow the usual rule and update the entire kernel
> when making an interface change.  As that did not happen I don't see
> any problems with incremental updates in the style the code is already
> in.

Reverse engineering all the memory barriers in the code is a massive
undertaking. We are looking at it, but its slow progress. Mostly an
update when touched approach.

Although some searching for dodgy patterns; see for example commit:

  a7af0af32171 ("blk-mq: attempt to fix atomic flag memory ordering")

which was the result of people looking at creative
smp_mb__{before,after}_atomic() usage -- in specific use of those
barriers not immediately adjacent to the respective atomic operation.

But to use the lack of completion of that work to not write comments on
code you understand is complete bollocks though.

> Not that I will mind a patch that updates the code, but I am not going
> to hold up a perfectly good bug fix waiting for one either.

If both you and the submitter actually understand the ordering, asking
them to write the comment isn't onerous and should not hold up anything
much at all.

In fact, if you cannot write that comment you cannot claim it to be a
good fix. And I'm saying the lack of the comment means its not a
perfectly good fix at all, since a perfect fix would indeed explain
memory ordering.