[
https://issues.apache.org/jira/browse/HDDS-17371?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17371:
------------------------------
Attachment: CR-2-key-mapped-buffer-cache-by-file-identity.patch
> 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
> Affects Versions: 2.0.0, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1
> Reporter: Siyao Meng
> Priority: Major
> 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]
