[
https://issues.apache.org/jira/browse/HDDS-17001?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17001:
------------------------------
Attachment: TestBugMC3StaleReconstructionBuffers.java
> EC reconstruction read returns unfilled stripe buffers when the stream is
> read again after a failed read or seek
> ----------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17001
> URL: https://issues.apache.org/jira/browse/HDDS-17001
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Minor
> Attachments:
> MC-3-invalidate-ec-reconstruction-buffers-on-failed-stripe-read.patch,
> TestBugMC3StaleReconstructionBuffers.java
>
>
> h3. Mechanism
> {{ECBlockReconstructedInputStream.readStripe}} calls {{clearBuffers()}},
> which sets every stripe buffer to position 0 and limit of one cell, and then
> {{stripeReader.readStripe(bufs)}}. If the stripe read throws (for example
> {{InsufficientLocationsException}} once too many replicas have failed), the
> buffers stay readable. The next {{read()}} on the same stream finds bytes
> remaining in {{selectNextBuffer}}, copies the buffers to the caller without
> any I/O and advances the position, without an error.
> {{seek()}} has the same problem: it moves the stripe reader to the target
> stripe and loads that stripe before it updates {{position}}. If loading
> fails, the buffers hold part of the target stripe while {{position}} is still
> the old one, and the next {{read()}} returns that data as if it were at the
> old position.
> h3. Trigger
> # A block is read by the reconstruction reader, and a stripe read or a
> {{seek()}} fails because not enough replicas can be read.
> # The caller catches the {{IOException}} and reads the same stream again.
> No in-tree caller does step 2: {{KeyInputStream}}, {{OzoneFSInputStream}},
> the S3 gateway GET and {{ozone sh key get}} pass the exception up, and
> positioned reads use a fresh reader per call. An application that retries a
> read on the same {{FSDataInputStream}} or {{OzoneInputStream}} after an
> exception is exposed.
> h3. Impact
> The repeated read returns up to one stripe from the buffers, in which the
> bytes that were not loaded before the failure (about one cell in the
> reproduction) are not the block data, and the position moves past them. After
> a failed {{seek()}} the returned data belongs to another stripe. A stream
> whose replicas are exhausted should throw again instead.
> h3. Reproduction
> PASS on unmodified source, deterministic.
> [^TestBugMC3StaleReconstructionBuffers.java] ({{hadoop-hdds/client}}) uses
> rs-3-2 with 1 MiB cells, the stock {{ECBlockInputStreamFactoryImpl}} and the
> client test replica streams for datanode read errors. Three cases: failover
> from the plain reader at position 0 followed by exhausted replicas, a failed
> {{seek()}} into stripe 2 after stripe 0 was read, and a failed read through
> {{MultipartInputStream}} (the base class of {{KeyInputStream}}). In each the
> caller catches {{InsufficientLocationsException}} and reads again, gets 3 MiB
> without an exception, and about 1 MiB (3 MiB after the failed seek) differs
> from the block data. A passing test means the defect is present.
> h3. Patch
> [^MC-3-invalidate-ec-reconstruction-buffers-on-failed-stripe-read.patch],
> against 1fb64c8e499161d1452ae6948ffd461fc9d2c302. It also applies to master
> at 8664dc4fc3c.
> When the stripe read fails, with an {{IOException}} or a
> {{RuntimeException}}, {{readStripe}} sets the limit of every stripe buffer to
> 0 and sets the existing {{unBuffered}} flag before rethrowing, so nothing
> from the failed stripe is returned and the next {{read()}} first seeks the
> stripe reader back to the current position, as it does after {{unbuffer()}}.
> With exhausted replicas the next read throws again. After a failed {{seek()}}
> the stream stays at its old position.
> Covered by two tests in the existing {{TestECBlockReconstructedInputStream}}:
> {{testReadFailsAgainAfterFailedStripeRead}} (exhausted replicas; read, read,
> seek and read all throw, position stays 0) and
> {{testFailedSeekLeavesStreamAtOldPosition}} (the stripe read of a {{seek()}}
> fails once with an injected exception after the stripe reader has moved to
> the target stripe; the next read must return the data at the old position).
> Both fail without the change. The second also fails if only the limits are
> reset without the re-seek, and if only {{IOException}} is handled. With the
> change the EC client stream suites (75 tests) and {{TestOzoneECClient}} plus
> {{TestKeyInputStreamEC}} (36 tests) pass and checkstyle is clean. With the
> patch all three reproduction cases fail because the second read throws
> {{InsufficientLocationsException}}.
> Found by TLA+ model checking and code review of the Ozone client read retry
> and failover paths 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]
