Siyao Meng created HDDS-17371:
---------------------------------
Summary: Datanode mmap read cache is keyed by path only, so a
container replica recreated at the same path can be served the deleted
replica's bytes
Key: HDDS-17371
URL: https://issues.apache.org/jira/browse/HDDS-17371
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: CR-2-key-mapped-buffer-cache-by-file-identity.patch,
TestBugCR2StaleMappingAfterContainerRecreate.java
h3. Mechanism
With memory mapped chunk reads enabled, {{ChunkUtils.readData}} gets each
mapping from {{MappedBufferManager.computeIfAbsent}}, which caches it in one
datanode wide map under the key {{path-offset-size}}. Nothing on delete, import
or overwrite removes an entry; an entry is only skipped once GC has cleared its
weak reference. Until then a read of the same path, offset and length returns
the cached mapping even if the file at that path has been replaced by a new
file (new inode). The cached mapping still shows the old file's bytes, because
a mapped file that has been unlinked keeps its contents. Overwriting the same
file in place is not affected, since the mapping is shared with the file and
sees the new bytes.
h3. Trigger
* {{ozone.chunk.read.mapped.buffer.max.count}} > 0 (default 0, so off by
default) and a read larger than {{ozone.chunk.read.mapped.buffer.threshold}}
(32KB).
* The read buffer capacity must also be above that threshold.
{{ChunkManager.getBufferCapacityForChunkRead}} sets it to the chunk's bytes per
checksum, so with the default {{ozone.client.bytes.per.checksum}} of 16KB the
mapped path is never used. It is used for chunks written with checksum type
NONE (capacity {{ozone.chunk.read.buffer.default.size}}, 1MB), chunks written
with more than 32KB per checksum, and reads from old clients that ask for a
single buffer.
* A replica of container C is read through the mapped path, then deleted
({{KeyValueHandler.deleteInternal}}), and a new replica of C is created on the
same volume, which puts its block files at the same paths. This happens when
the datanode becomes the target of EC reconstruction or replication for a
container it held before. Its block holds different bytes at the same chunk
offset and length, for example another EC replica index, or a healthy copy
replacing a corrupt one. Other routes that put a new file at the same path (a
disk balancer move back to the volume, a FILE_PER_CHUNK chunk rewrite) carry
the same bytes, so they are harmless, and the patch covers them too.
* A ReadChunk of that block on the new replica before GC clears the old
mapping's weak reference. For a buffer that was promoted to the old generation
this can take longer than a young collection interval (argued, not measured).
h3. Impact
ReadChunk returns the deleted replica's bytes with no error on the datanode.
What happens next depends on the checksums:
* Data written with checksum type NONE: nothing downstream can detect the stale
bytes, so they are returned to the application as the block's content, and an
EC reconstruction reading them would encode them into the rebuilt replica.
Argued from the code, not run.
* Data written with more than 32KB per checksum: the client's checksum check
({{ozone.client.verify.checksum}}, on by default) rejects the bytes and the
client fails over to another replica. EC reconstruction forces checksum
verification, and container reconciliation reads with verification on by
default. With verification turned off the stale bytes reach the application.
All of these conditions are needed together: mmap reads enabled, data on the
mapped path (checksum type NONE or more than 32KB per checksum), and an SCM
driven delete and recreate of the same container on the same volume while the
old mapping is still reachable. The stale bytes are always an older copy of the
same block of the same container. Container export and the container scanner
read files directly and are not affected.
h3. Reproduction
[^TestBugCR2StaleMappingAfterContainerRecreate.java]
({{hadoop-hdds/container-service}}) uses one volume and
{{FilePerBlockStrategy}} with mmap reads enabled, and only the calls the
datanode makes: create replica 1 of container 1, WriteChunk 64KB of 'A' with
end of block, ReadChunk, delete it with
{{KeyValueContainerUtil.removeContainer}} and {{KeyValueContainer.delete}},
create replica 2 of the same container at the same path, WriteChunk 'B',
ReadChunk. Its {{ChunkInfo}} carries no checksum data, so the buffer capacity
is the 64KB chunk length and the read takes the mapped path, as a chunk written
with checksum type NONE does; with 16KB CRC32 checksum data the same test reads
'B' on unmodified source, because the mapped path is not taken. The first
read's buffer is kept reachable, which stands for "no GC yet" and makes the
result deterministic. On unmodified source it fails with:
{noformat}
[ReadChunk on the recreated replica must return its own bytes, not the deleted
replica's]
expected: 'B'
but was: 'A'
{noformat}
What was run is this datanode level stale read, on macOS only. Not exercised:
Linux, a mini cluster with real EC reconstruction or replication, and an
application or an EC reconstruction consuming the stale bytes.
h3. Suggested fix
Include the file's identity in the cache key. Invalidating entries on delete,
import and overwrite was considered and not chosen: it needs hooks in several
places, races with a reader that adds the entry back, and misses any other path
that replaces a file.
h3. Patch
[^CR-2-key-mapped-buffer-cache-by-file-identity.patch], against
5fed99dab3c3204cd7bf1516074969a38d5eb00d. It also applies to master at
76b5c6947a2.
The mapped branch of {{ChunkUtils.readData}} now uses the path plus
{{BasicFileAttributes.fileKey()}} (device and inode) as the cache's file key,
so a replaced file misses the cache and an unchanged file still hits. The
identity is read before the file is opened, so a replacement in between caches
the newer file's mapping under the older identity, which a later read can only
hit if that inode number is reused at the same path while the mapping is live.
A failed attribute read is handled like a failed open. The cost is one {{stat}}
per mapped read; that path already opens and closes the file on every read, and
{{FileChannel}} offers no cheaper way to get the identity. If {{fileKey()}} is
null on some file system, the key falls back to today's behaviour.
Covered by {{TestChunkUtils#mappedReadAfterFileReplacedAtSamePath}}: read a
file through the mapped path, replace it at the same path, and expect the new
bytes; then overwrite it in place and expect those bytes too. Without the
change it fails with "read returned bytes of the replaced file". With the
change {{TestChunkUtils}}, {{TestMappedBufferManager}},
{{TestFilePerBlockStrategy}}, {{TestFilePerChunkStrategy}},
{{TestChunkManagerDummyImpl}} and {{TestKeyValueHandler}} pass (91 tests) and
checkstyle is clean. The attached reproduction passes with the change.
Found by code review of the datanode memory mapped chunk read path, as part of
the TLA+ verification effort under HDDS-15926, on commit
5fed99dab3c3204cd7bf1516074969a38d5eb00d. 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
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
