On Thu, Mar 20, 2025 at 01:05:05PM -0400, Andres Freund wrote: > On 2025-03-19 18:17:37 -0400, Andres Freund wrote: > > On 2025-03-19 14:25:30 -0700, Noah Misch wrote: > > > > + * marked as failed. In case of a partial read, some > > > > buffers may be > > > > + * ok. > > > > + */ > > > > + failed = > > > > + prior_result.status == ARS_ERROR > > > > + || prior_result.result <= buf_off; > > > > > > I didn't run an experiment to check the following, but I think this > > > should be > > > s/<=/</. Suppose we requested two blocks and read some amount of bytes > > > [1*BLCKSZ, 2*BLSCKSZ - 1]. md_readv_complete will store result=1. > > > buf_off==0 > > > should compute failed=false here, but buf_off==1 should compute > > > failed=true. > > > > Huh, you might be right. I thought I wrote a test for this, I wonder why it > > didn't catch the problem... > > It was correct as-is. With result=1 you get precisely the result you describe > as the desired outcome, no? > prior_result.result <= buf_off > -> > 1 <= 0 -> failed = 0 > 1 <= 1 -> failed = 1 > > but if it were < as you suggest: > > prior_result.result < buf_off > -> > 1 < 0 -> failed = 0 > 1 < 1 -> failed = 0 > > I.e. we would assume that the second buffer also completed.
That's right. I see it now. My mistake. > What does concern me is that the existing tests do *not* catch the problem if > I turn "<=" into "<". The second buffer in this case wrongly gets marked as > valid. We do retry the read (because bufmgr.c thinks only one block was read), > but find the buffer to already be valid. > > The reason the test doesn't fail, is that the way I set up the "short read" > tests. The injection point runs after the IO completed and just modifies the > result. However, the actual buffer contents still got modified. > > > The easiest way around that seems to be to have the injection point actually > zero out the remaining memory. Sounds reasonable and sufficient. FYI, I've resumed the comprehensive review. That's still ongoing.
