Siyao Meng created HDDS-17003:
---------------------------------
Summary: Interrupted EC reconstruction read returns while its cell
reads still write into stripe buffers that close() gives back to the buffer pool
Key: HDDS-17003
URL: https://issues.apache.org/jira/browse/HDDS-17003
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: CR-3-wait-for-ec-cell-reads-on-interrupt.patch
h3. Mechanism
{{ECBlockReconstructedStripeInputStream.loadDataBuffersFromStream}} submits one
cell read per selected index to the EC reconstruct executor and then waits for
the futures in turn. When a wait is interrupted it catches the
{{InterruptedException}}, sets the interrupt flag again and moves to the next
future. With the flag set, {{Future.get()}} on every read that has not finished
throws at once, so the loop ends and the method throws while those reads are
still running. The comment at that catch says the intent is to handle all
futures before throwing; setting the flag inside the loop defeats it. The code
dates from HDDS-5952 and is the same in every release from 1.3.0 to 2.2.1.
The reads that are left running keep writing into the buffers they were given:
the caller's stripe buffers ({{ECBlockReconstructedInputStream}} takes them
from the client's {{ByteBufferPool}}) and the parity buffers the stripe reader
takes from the same pool. {{ECBlockReconstructedInputStream.close()}} and
{{unbuffer()}} put the stripe buffers back into the pool, and the stripe
reader's {{close()}} and {{unbuffer()}} put back its parity buffers, without
waiting for those reads. A read that has not yet opened its replica stream
opens it through {{ECBlockInputStream.getOrOpenStream}} after the close, and
that stream is not closed again (argued from code, not run).
{{BoundedElasticByteBufferPool.getBuffer}} hands out the same {{ByteBuffer}}
object. The client has one pool, used by every EC reconstruction stream and by
{{ECKeyOutputStream}}. A late cell read therefore writes into a buffer that is
already in use by another stream of the same client, and it also moves that
buffer's position.
h3. Trigger
# A key in an EC bucket is read through the reconstruction reader (a data
replica is missing or has failed).
# The reading thread is interrupted while it waits for the cell reads of a
stripe.
# The stream is closed or unbuffered before those reads have finished.
{{ECBlockInputStreamProxy.readPositioned}} closes its per call reader in a try
with resources block, so an interrupted positioned read takes this path by
itself.
h3. Impact
* The late cell read overwrites a buffer that the pool has handed to another
stream. If that stream is a reconstruction read that has just cleared the
buffer, its own read of that cell finds no space left and the stripe is
returned with the wrong cell, and a decoded cell computed from it is wrong too,
without an error.
* Not run: the same applies when the buffer is taken by {{ECKeyOutputStream}},
which would then encode and write a cell with wrong content. The datanode
offline reconstruction ({{ECReconstructionCoordinator}}) uses the same method
with its own pool; there an interrupt comes only from a shutdown.
* Argued, not run: a read started after the interrupt on the same stream
submits new cell reads for the same indexes and buffers while the old ones may
still run.
h3. Reproduction
No standalone reproduction is attached. The regression tests
{{testInterruptedReadWaitsForCellReadRunningOnPoolThread}} and
{{testInterruptedReadRunsCellReadsNotStartedByExecutor}} in the attached patch
fail on unmodified source (see Patch).
h3. Patch
[^CR-3-wait-for-ec-cell-reads-on-interrupt.patch], against
1fb64c8e499161d1452ae6948ffd461fc9d2c302. It also applies to master at
fd2f6092c6d.
{{loadDataBuffersFromStream}} now creates a {{FutureTask}} per cell read and
hands it to the executor with {{execute}}. It then calls {{run()}} on each task
in the calling thread and waits for it with
{{Uninterruptibles.getUninterruptibly}} (Guava, already a dependency of the
module) instead of {{Future.get()}}, and the catch that set the flag again is
removed. {{FutureTask.run}} does nothing once a pool thread has started the
task, so the calling thread only runs a read the executor has not started: one
a pool thread has been handed but has not started yet (the calling thread then
does that read itself and the pool thread skips it, so the reads of a stripe
still run in parallel), one that {{CallerRunsPolicy}} dropped because the
executor was shut down, or one the executor rejected. The calling thread runs
such a read with its interrupt status cleared and restored afterwards, as a
pool thread would, so that the datanode read does not fail because of the
interrupt and mark a healthy replica as failed. Every read that is waited for
therefore runs or has run, and the wait always ends. {{getUninterruptibly}}
restores the interrupt status when it returns, so the existing check after the
loop still throws {{InterruptedException}} and {{read}} still throws an
{{IOException}} with the flag set, but only after every read of the stripe has
completed or failed. No read writes into the stripe or parity buffers after the
method returns, so {{close()}} and {{unbuffer()}} put back only idle buffers.
Read failures seen while waiting are recorded as before. The same approach is
used for the healthy reader in the open pull request for HDDS-16867.
Behaviour change: an interrupted reconstruction read returns only after the
cell reads of the stripe that are in flight have completed or failed, which
takes as long as an uninterrupted read of that stripe would (each datanode call
has the {{ozone.client.read.timeout}} deadline, 30s by default, and a cell read
can make several calls, including read retries and a switch to a spare
replica), instead of at once. A read whose executor has been shut down no
longer waits for a cell read that will never run: on unmodified source it waits
until the thread is interrupted, with the patch the calling thread runs that
cell read. Cancelling the futures was not used: {{cancel(true)}} would make the
datanode reads fail and count as failed locations, and a cancelled future no
longer tells when its read has stopped writing.
Covered by three tests in the existing {{TestECBlockReconstructedInputStream}}.
In the first two, index 3 has no location, so a stripe is read by two data
reads and one parity read, and {{read}} is called with the interrupt flag set.
* {{testInterruptedReadWaitsForCellReadRunningOnPoolThread}} lets the executor
start the first cell read on a pool thread and holds that read until the
reading thread waits for it. It asserts an {{IOException}} caused by
{{InterruptedException}}, the flag still set, and all three cell reads,
including the held one, done when {{read}} throws. On unmodified source it
fails because {{read}} returns while the held read is not done.
* {{testInterruptedReadRunsCellReadsNotStartedByExecutor}} uses an executor
that keeps the cell reads without running them, and replica streams whose read
fails when its thread is interrupted, as a datanode read does. It asserts the
same, plus that no replica was marked as failed, then takes the buffers back
from the pool after {{close()}}, clears them, runs any read left over and
asserts the buffers are untouched. On unmodified source it fails because
{{read}} returns with the three reads not done; run after {{close()}}, those
reads would write into three of the four buffers taken back from the pool,
which the rest of the test checks.
* {{testInterruptedReadReturnsWhenExecutorIsShutDown}} reads with the interrupt
flag set through a {{SynchronousQueue}} executor with {{CallerRunsPolicy}} that
has been shut down, and asserts that the read throws within 10 seconds. It
passes on unmodified source, where the interrupt ends the wait, and fails if
the wait cannot end.
With the change the EC client stream suites (78 tests), {{TestOzoneECClient}}
and {{TestKeyInputStreamEC}} (36 tests) pass, and checkstyle and PMD are clean.
The patch and the patch for HDDS-17001 both add static imports at the top of
{{TestECBlockReconstructedInputStream}}, so whichever lands second needs a
rebase of that import block; the main code changes do not overlap.
Found by code review of the Ozone client read retry and failover paths, as part
of the TLA+ verification effort under HDDS-15926, on commit
1fb64c8e499161d1452ae6948ffd461fc9d2c302. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. The attached patch is
a proposal for review. Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
