Siyao Meng created HDDS-16822:
---------------------------------
Summary: 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
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]
