Re: [dm-devel] [PATCH] blk-lib: fix error reporting

Mikulas Patocka
2014-07-16T10:26:29Z

On Tue, 8 Jul 2014, James Bottomley wrote:

> On Tue, 2014-07-08 at 09:05 -0400, Mikulas Patocka wrote:
> >
> > On Tue, 8 Jul 2014, Christoph Hellwig wrote:
> >
> > > > + if (unlikely(err))
> > > > + ACCESS_ONCE(bb->error) = err;
> > >
> > > I can't see a reason for the ACCESS_ONCE here.
> >
> > Multiple bios can be completed concurrently, so they write bb->error at
> > the same time. The compiler may do store tearing (see "store tearing" in
> > Documentation/memory-barriers.txt) - it may split one 4-byte write into
> > several smaller writes - and it could result in setting bb->error to
> > invalid value. We need ACCESS_ONCE to make sure that store tearing doesn't
> > happen.
>
> That's not correct, because it's not applicable in this case. Tearing
> may occur on misalignment (which ACCESS_ONCE() cannot rectify because
> it's architectural), short constant loads (again, usually architectural)
> and structure copies, none of which applies here.

Suppose this scenario:
CPU1 writes low byte of the first error code
CPU2 writes low byte of the second error code
CPU2 writes 3 high bytes of the second error code
CPU1 writes 3 high bytes of the first error code

- now, bb->error contains garbage - a mix of the first and second error
code. That's why we need ACCESS_ONCE.

It may happen even if the variable is aligned. The compiler is allowed to
split larger memory access to several smaller accesses. The compiler
usually doesn't do this split (that's why omitting ACCESS_ONCE usually
doesn't result in any observable misbehavior), but it is still a bug to
omit it - you don't really know that for all 29 architectures gcc won't
split the memory write...

> We can rely on a properly aligned 32 bit write being atomic.
>
> James

... only if you use ACCESS_ONCE ...

Mikulas