Re: block: be more careful about status in __bio_chain_endio

From: John Dorminy
Date: Fri Feb 22 2019 - 22:10:55 EST


I'm also worried about the other two versions, though:

memory-barriers.txt#1724:

1724 (*) The compiler is within its rights to invent stores to a variable,

i.e. the compiler is free to decide __bio_chain_endio looks like this:

static struct bio *__bio_chain_endio(struct bio *bio)
{
struct bio *parent = bio->bi_private;
blk_status_t tmp = parent->bi_status;
parent->bi_status = bio->bi_status;
if (!bio->bi_status)
parent->bi_status = tmp;
bio_put(bio);
return parent;
}

In which case, the read and later store on the two different threads
may overlap in such a way that bio_endio sometimes sees success, even
if one child had an error.

As a result, I believe the setting of parent->bi_status needs to be a
WRITE_ONCE() and the later reading needs to be a READ_ONCE()
[although, since the later reading happens in many different
functions, perhaps some other barrier to make sure all readers get the
correct value is in order.]