fetch: Close data race between BOC state and objcore flags

[dridi]

The changes from #4073 increases the likelihood of a data race between a BOC state check followed immediately by an objcore flag check:

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

Once we reach BOS_STREAM, we touch oc->flags to check OC_F_BUSY.

If the backend fetch failed immediately after reaching BOS_STREAM, we may then observe BOS_FAILED when we touch oc->boc->state but still see oc->flags from before the failure in this thread.

Because oc->flags and oc->boc->state are guarded by different locks, we can't safely rely on this construct. One option could be to move the OC_F_BUSY check down, and another, could be to capture the BOC state:

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

This pull request implements the state capture.

[nigoroll]

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

[dridi]

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

[nigoroll]

@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.

[nigoroll]

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

[dridi]

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.

[dridi]

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.

[nigoroll]

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

[dridi]

Bugwash: ok