[jira] [Updated] (HDDS-17003) Interrupted EC reconstruction read returns while its cell reads still write into stripe buffers that close() gives back to the buffer pool

["Siyao Meng (Jira)" <ji...@apache.org>]

     [ 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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org