[
https://issues.apache.org/jira/browse/HDDS-17003?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17003:
------------------------------
Attachment: CR-3-wait-for-ec-cell-reads-on-interrupt.patch
> 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
> Priority: Major
> 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]
