[
https://issues.apache.org/jira/browse/HDDS-17367?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17367:
------------------------------
Affects Version/s: 2.2.1
2.0.0
2.1.2
2.1.0
2.1.1
2.2.0
> 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: [email protected]
For additional commands, e-mail: [email protected]
