[
https://issues.apache.org/jira/browse/HDDS-16822?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16822:
------------------------------
Attachment: CR-8-write-buffered-stream-data-before-raft-putblock.patch
> Data stream hsync with PutBlock through Raft commits a block length whose
> last 1MB exists only in datanode memory
> -----------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-16822
> URL: https://issues.apache.org/jira/browse/HDDS-16822
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> CR-8-write-buffered-stream-data-before-raft-putblock.patch,
> TestBugCR8DatastreamHsyncTailNotInBlockFile.java
>
>
> h3. Mechanism
> On the data stream write path the datanode's {{KeyValueStreamDataChannel}}
> keeps at least the last
> {{BlockDataStreamOutput.PUT_BLOCK_REQUEST_LENGTH_MAX}} (1MB) of the stream in
> memory ({{Buffers}}), because the end of the stream may be the PutBlock
> request that the client appends at close (HDDS-6500). Only the data before
> that is written to the block file.
> {{BlockDataStreamOutput.hsync}} waits until all datanodes acknowledged the
> stream writes ({{waitFuturesComplete}}) and then, with
> {{ozone.client.datastream.putblock.without.raft.enabled=false}} (the
> default), sends a PutBlock through Raft ({{putBlockAsync}}).
> {{KeyValueHandler.handlePutBlock}} commits the block length. Nothing writes
> the buffered bytes, and the acknowledgement of a stream write only says that
> the datanode received it.
> HDDS-16746 fixed this for a PutBlock sent as a stream command:
> {{LocalStream.onCommand}} calls {{KeyValueStreamDataChannel.drainBuffers}}
> first. A PutBlock through Raft does not pass through there. So after hsync
> returns, every replica has committed a length whose last 1MB (the whole block
> if it is smaller) is not in its block file. The bytes are written when the
> stream is closed.
> h3. Trigger
> # Datanodes with {{hdds.container.ratis.datastream.enabled=true}}, and hsync
> enabled ({{ozone.fs.hsync.enabled}} with {{ozone.hbase.enhancements.allowed}}
> on OM and {{ozone.client.hbase.enhancements.allowed}} on the client). All of
> these default to false.
> # A client writes through a data stream: {{OzoneBucket.createStreamKey}}, or
> a file system client with {{ozone.fs.datastream.enabled=true}} that writes
> more than {{ozone.fs.datastream.auto.threshold}} (4MB by default) before its
> first flush, hflush or hsync.
> # The client calls hsync and it returns.
> That is enough for the unreadable state. No second client, fault or timing is
> involved. For the loss the writer additionally dies, or loses its
> connections, before it closes the stream.
> h3. Impact
> * While the writer is open, a reader fails in the last 1MB of the hsynced
> length with {{StorageContainerException: Unexpected read size. expected: N,
> actual: -1}}, although OM and all datanodes report that length.
> * If the writer never closes the stream, those bytes are never written to the
> block file. In the run below the file was then closed at the hsynced length
> and stayed unreadable beyond the block file length, also after all datanodes
> were restarted. Hsync had acknowledged the bytes and no replica has them.
> * By code reading, not run on a release: {{CapableOzoneFSDataStreamOutput}}
> advertises {{HSYNC}} and {{HFLUSH}} for a data stream since HDDS-11816
> (2.0.0), and the buffering on the datanode and the PutBlock through Raft on
> hsync are the same in ozone-2.0.0 and ozone-2.2.1.
> h3. Reproduction
> PASS on unmodified source, nothing injected.
> [^TestBugCR8DatastreamHsyncTailNotInBlockFile.java]
> ({{hadoop-ozone/integration-test}}) starts a mini cluster with three
> datanodes, hsync and the file system data stream enabled, and the default
> data stream sizes. It writes 7MB (the block size of the mini cluster is 4MB)
> and calls hsync. The datanodes are only read, for the committed length and
> the block file length of each replica of the last block. A passing test means
> the defect is present.
> First test, writer still open: file length 7340032, last block 3145728,
> committed block length per replica [3145728, 3145728, 3145728], block file
> length per replica [2097152, 2097152, 2097152], and reading the file fails
> with {{Unexpected read size. expected: 262144, actual: -1}}. After the writer
> closes, the block files are 3145728 bytes and the content reads back.
> Second test: after hsync the test closes the writer's datanode clients
> instead of the stream, then closes the file with {{recoverLease}} (the test
> sets {{ozone.om.lease.soft.limit}} to 0s; it returns true, file length
> 7340032). Block files are still 2097152 bytes and the read fails. After
> restarting all three datanodes the committed length is 3145728, the block
> files are 2097152 bytes and the read still fails.
> h3. Suggested fix
> The attached patch writes the buffered bytes before a PutBlock is committed,
> for every PutBlock: {{KeyValueHandler.handlePutBlock}} asks the chunk manager
> to drain the open stream of the block up to the length that the PutBlock
> commits. Points for review:
> * The drain is bounded by the block length of the PutBlock. The existing
> {{drainBuffers()}} writes everything, which is not safe here: at close the
> client sends the PutBlock at the end of the stream and also one through Raft,
> and a full drain during the second could write the first into the block file.
> * {{FilePerBlockStrategy}} now keeps the open stream channel of each block in
> a map, removed on close and on cleanup.
> * The Raft PutBlock is applied on another thread than the stream writes, so
> the methods of {{KeyValueStreamDataChannel}} that use the buffers are
> synchronized. The lock is not held while the PutBlock at the end of the
> stream is dispatched.
> * The PutBlock that a data stream writer sends at each flush size boundary
> without hsync now also writes the buffered tail at that point. The same bytes
> are written, earlier.
> * The drain is a buffered write, without a force to disk. That is what an
> ordinary hsync gets with the default
> {{hdds.container.chunk.write.sync=false}}. The stream channel does not look
> at that setting, before or after the patch: it always opens the block file
> for buffered writes and forces it only when the client asks for it
> ({{ozone.client.datastream.sync.size}}, 0 by default, which is never). By
> code reading.
> * When hsync returns, the Raft leader has applied the PutBlock and so has
> written the bytes. A follower writes them when it applies the entry. In the
> run with the patch all three block files had the committed length right after
> hsync. A replica whose stream is gone when it applies the entry (it restarted
> before applying, or the stream was cleaned up after an error) has nothing to
> write and still commits the length, as before the patch. By code reading, not
> run.
> * A stricter variant would be to reject a PutBlock whose length is beyond the
> block file. The patch does not do that.
> h3. Patch
> [^CR-8-write-buffered-stream-data-before-raft-putblock.patch], against
> ea69b4a7d9abd040e5259dbcbb7c6ce52eb5d199. It also applies to master at
> 84f5594a6a2 (not built there). It touches lines that the open pull request
> for HDDS-16789 changes in {{KeyValueStreamDataChannel}} and its test, so
> whichever is merged second needs a rebase. That pull request removes the
> empty check from {{drainBuffers()}}. The one in {{drainBuffers(long)}} has to
> stay in a rebase, because it keeps the file position from being read after
> the file is closed.
> Covered by {{TestContainerStateMachineStream.testReadAfterHsync}}, which is
> the existing {{testReadAfterHsyncWithPutBlockWithoutRaft}} run for both
> PutBlock modes. Without the change the Raft case fails with {{Unexpected read
> size. expected: 1000, actual: -1}}.
> {{TestKeyValueStreamDataChannel.testDrainBuffersUpToLength}} covers the
> bounded drain, and that the channel is released on close and on cleanup. With
> the change the unit tests of the datanode's key value container, dispatcher
> and container state machine (449 tests), {{TestBlockDataStreamOutput}},
> {{TestHSync}} and {{TestOzoneFileSystemWithStreaming}} pass and checkstyle is
> clean. {{TestLeaseRecovery}} has one failing test,
> {{testGetCommittedBlockLengthWithException}}, with and without the change; it
> writes no data stream. With the patch both tests of the reproduction fail,
> because the block files have the committed length and the content reads back,
> with the writer open and after the writer was killed. The datanode restart
> was not run with the patch.
> Found by code review of the lease recovery paths for hsynced files, as part
> of the TLA+ verification effort under HDDS-15926, on commit
> ea69b4a7d9abd040e5259dbcbb7c6ce52eb5d199. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. The nearest is
> HDDS-16746, which fixed the same gap for a PutBlock sent as a stream command
> only. 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]
