Better Write some comment, about we clearing the BM_LOCKED from stage directly and need not to call UnlockBufHdr explicitly.
otherwise its confusing.
Few more comments..
*** 828,837 ****
*/
do
{
! LockBufHdr(bufHdr);
! Assert(bufHdr->flags & BM_VALID);
! bufHdr->flags &= ~BM_VALID;
! UnlockBufHdr(bufHdr);
} while (!StartBufferIO(bufHdr, true));
}
}
--- 826,834 ----
*/
do
{
! uint32 state = LockBufHdr(bufHdr);
! state &= ~(BM_VALID | BM_LOCKED);
! pg_atomic_write_u32(&bufHdr->state, state);
} while (!StartBufferIO(bufHdr, true));
1. Previously there was a Assert, any reason why we removed it ?
Assert(bufHdr->flags & BM_VALID);
It was missed. In the attached patch I've put it back.
2. And also if we don't need assert then can't we directly clear BM_VALID flag from state variable (if not locked) like pin/unpin buffer does, without taking buffer header lock?
In this version of patch it could be also done as loop of CAS operation. But I'm not intended to it so because it would significantly complicate code. It's not yes clear that traffic in this place is high enough to make such optimizations.
Since v4 patch implements slightly different approach. Could you please test it? We need to check that this approach worth putting more efforts on it. Or through it away otherwise.