I understand that we should read the boc state consistently, but don't we have an outdated read left on oc->flags here?

I'm not sure what "here" is referring to, the link you shared is highlighting if (state == BOS_FAILED).

Around this line there are two accesses to oc->flags:

state = ObjWaitState(oc, BOS_STREAM);
AZ(oc->flags & OC_F_BUSY);
if (state == BOS_FAILED)
        AN(oc->flags & OC_F_FAILED);

The first one is always true so it's not a problem, and the second is safe too as long as the observed state is BOS_FAILED. We first fail the oc before failing the boc, so capturing BOS_FAILED is our warranty.

Whereas before, the sequence could have been:

• client resumes upon BOS_STREAM
• client observes no OC_F_BUSY
• backend fetch fails
• client observes BOS_FAILED
• client cannot observe OC_F_FAILED

@dridi I clarified my earlier comment.
My question was regarding outdated reads of OC_F_*. We set the oc flags before taking the boc lock so, while common architectures are likely to have a read of the oc flags updated, there is no guarantee, IIUC.

But anyway, irrespective of a possible outdated read, this patch is an improvement, so we should merge it.

Oh, we may still observe an outdated oc->flags, but because we observed BOS_FAILED at resumption time it must contain the OC_F_FAILED bit. That bit is raised prior to reaching BOS_FAILED.

while common architectures are likely to have a read of the oc flags updated, there is no guarantee, IIUC.

I guess the alternative is to either drop the check altogether or check while holding the objhead lock. I would expect that the boc critical section on the client side is enough to invalidate the oc pointer and warrant a refresh.

I would expect that the boc critical section on the client side is enough to invalidate the oc pointer and warrant a refresh.

That's what I guessed with similarly structured code and had to learn that the answer is no. But we can still see if it happens here