[jira] [Updated] (HDDS-17367) MappedBufferManager cleanup returns quota of the other chunk layout's cached mappings because the mapping map is static

["Siyao Meng (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/HDDS-17367?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Siyao Meng updated HDDS-17367:
------------------------------
    Attachment: MC-3-per-instance-mapped-buffer-map.patch

> MappedBufferManager cleanup returns quota of the other chunk layout's cached mappings because the mapping map is static
> -----------------------------------------------------------------------------------------------------------------------
>
>                 Key: HDDS-17367
>                 URL: https://issues.apache.org/jira/browse/HDDS-17367
>             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: Minor
>         Attachments: MC-3-per-instance-mapped-buffer-map.patch, TestBugMC3CleanupCreditsOtherManager.java
>
>
> h3. Mechanism
> {{MappedBufferManager.mappedBuffers}} is a static field, while the quota {{Semaphore}} belongs to each instance. Each datanode builds two instances, one in {{FilePerChunkStrategy}} and one in {{FilePerBlockStrategy}}, each with the full {{ozone.chunk.read.mapped.buffer.max.count}}. The cleanup in {{MappedBufferManager.getQuota}} walks the shared map and returns the permit of every cleared entry to the instance that ran it, including entries the other instance created. The two strategies read different files, so the shared map gives no cache reuse.
> h3. Trigger
> A datanode with mmap reads enabled ({{ozone.chunk.read.mapped.buffer.max.count}} > 0, default 0) that serves memory mapped reads for both FILE_PER_CHUNK and FILE_PER_BLOCK containers. Reads are memory mapped only when the read buffer capacity is above {{ozone.chunk.read.mapped.buffer.threshold}} (32KB), so data written with the default 16KB per checksum is never mapped; chunks written with checksum type NONE or with more than 32KB per checksum, and reads into a single buffer, are. FILE_PER_BLOCK is the default container layout, so this needs a datanode that still holds FILE_PER_CHUNK containers. A mapping of one strategy is cleared by GC, and the other strategy runs out of quota and runs its cleanup.
> h3. Impact
> The strategy that ran the cleanup can hold more live mappings than its limit, while the other can be left with neither quota nor mappings and serves its reads from heap buffers until permits move back. The total over both instances stays within twice {{max.count}}, which the datanode already allows today, so the limit is not exceeded for the datanode as a whole. The effect is on performance only. The static map is also shared by all datanodes in one JVM, for example in mini cluster tests.
> h3. Reproduction
> PASS on unmodified source, which means the defect is present. [^TestBugMC3CleanupCreditsOtherManager.java] ({{hadoop-hdds/container-service}}) builds two managers of capacity 1, one per strategy, and reads through {{ChunkUtils.readData}}: after the chunk manager's mapping is cleared, the block manager's cleanup reclaims its permit, the block manager then holds 2 live mappings with limit 1, and the chunk manager has quota 0 with no live mapping. With the patch the block manager's cleanup reclaims nothing and the test fails ({{expected: 1 but was: 0}}).
> h3. Patch
> [^MC-3-per-instance-mapped-buffer-map.patch], against 5fed99dab3c3204cd7bf1516074969a38d5eb00d. It also applies to master at 76b5c6947a2.
> {{mappedBuffers}} becomes an instance field, so each cleanup reclaims only the entries of its own manager. Datanodes that serve one layout behave as before. One shared manager per datanode would match the description of {{max.count}} ("allowed for a DN") but would halve the effective limit for datanodes that serve both layouts, so it is left as a separate question.
> Covered by {{TestMappedBufferManager#testCleanupReturnsOnlyOwnQuota}}, which fails without the change ({{expected: <1> but was: <2>}}). With it, {{TestMappedBufferManager}}, {{TestChunkUtils}}, {{TestFilePerBlockStrategy}}, {{TestFilePerChunkStrategy}}, {{TestChunkManagerDummyImpl}} and the {{TestKeyValueHandler}} suites pass (159 tests), checkstyle is clean.
> Found by TLA+ model checking and code review of the datanode memory mapped chunk read path 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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org