[jira] [Updated] (HDDS-17370) MappedBufferManager hands out the cached mapped buffer itself, so a consumer that changes its position and limit corrupts the cached entry

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

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

Siyao Meng updated HDDS-17370:
------------------------------
    Affects Version/s: 2.2.1
                       2.0.0
                       2.1.2
                       2.1.0
                       2.1.1
                       2.2.0

> MappedBufferManager hands out the cached mapped buffer itself, so a consumer that changes its position and limit corrupts the cached entry
> ------------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: HDDS-17370
>                 URL: https://issues.apache.org/jira/browse/HDDS-17370
>             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-1-duplicate-cached-mapped-buffer.patch
>
>
> h3. Mechanism
> {{MappedBufferManager.computeIfAbsent}} returns the cached {{ByteBuffer}} object itself, both when it finds the range in its cache and when it has just mapped and cached it. {{ChunkUtils.readData}} wraps that object into the {{ChunkBuffer}} it returns, so every read of the same cached range holds the same position and limit as the cache entry.
> {{ChunkBufferToByteString.toByteString}} and {{toByteStringList}} document that the conversion function must preserve the position and limit of the input buffer. {{ByteStringConversion.safeWrap}}, used when {{ozone.UnsafeByteOperations.enabled=false}}, preserves them only when nothing else uses the buffer: it copies with {{ByteString.copyFrom}}, which moves the position to the limit, and then calls {{flip()}}. Because the buffer is the cache entry, each conversion moves the entry's position and limit. When two conversions of the same cached range interleave so that the second copies after the first copied but before the first flipped, the second copies 0 bytes, the first flip restores the full range and the second flip sets the limit to 0. The cache entry is left at limit 0.
> {{ChunkUtils.readData}} then gets a buffer with 0 remaining for that range. Its mapped read loop counts the read length by {{mapped.remaining()}}, so it makes no progress, and each pass through the cache calls {{releaseQuota(1)}}.
> h3. Trigger
> All of these on one datanode:
> * Mapped reads are on ({{ozone.chunk.read.mapped.buffer.max.count}} > 0, default 0).
> * {{ozone.UnsafeByteOperations.enabled=false}} (default true).
> * A read is served from the mapped path: it is longer than {{ozone.chunk.read.mapped.buffer.threshold}} (32KB) and its buffer capacity is above that threshold, and for gRPC reads {{ozone.chunk.read.netty.ChunkedNioFile}} is false (the default).
> * Two conversions of the same cached range overlap in time.
> No fault is needed.
> h3. Impact
> * Reads of that range return an empty payload or one that does not match the stored data, or fail with a runtime exception, for example {{IllegalStateException}} from {{ChunkBufferToByteString.applyAndAssertFunction}} ({{Unexpected limit: expected = 65536 but computed = 0}}), {{BufferUnderflowException}} or {{IndexOutOfBoundsException}}.
> * Later reads of that range on the mapped path do not complete.
> * The permit count of {{MappedBufferManager}} grows past the configured {{ozone.chunk.read.mapped.buffer.max.count}} on each pass of such a read, so the limit on mapped buffers no longer holds.
> * This lasts until the cached entry is replaced or the datanode restarts, and the extra permits stay until the datanode restarts (argued, not run).
> h3. Reproduction
> No standalone reproduction is attached. The regression test in the patch covers it: {{TestMappedBufferManager.testComputeIfAbsentReturnsIndependentPositionAndLimit}} ({{hadoop-hdds/container-service}}) moves the position of the buffer returned for a new mapping and checks that the next cache hit still covers the full range, then sets the limit of that hit to 0 and checks that a third call still gets position 0 and the full limit, and that the cached buffer itself is unchanged. Its supplier fails the test if the cache is not reused. On unmodified source it fails with {{expected: 1024 but was: 0}} at the first check. It also fails if only one of the two paths is changed.
> h3. Patch
> [^CR-1-duplicate-cached-mapped-buffer.patch], against 5fed99dab3c3204cd7bf1516074969a38d5eb00d. It also applies to master at 76b5c6947a2.
> {{MappedBufferManager.computeIfAbsent}} returns {{duplicate()}} of the buffer on both paths, so each caller has its own position and limit and the cached buffer itself never leaves the manager. This covers every consumer of the returned buffers, not only {{safeWrap}}: any code that reads a returned buffer moves only its own copy. Content, read only mode, byte order and class ({{MappedByteBuffer}}) are unchanged. GC behaviour is unchanged as well: a duplicate of a mapped buffer stores the original in its attachment ({{DirectByteBuffer.att}} in the JDK), so the cache entry stays reachable while any duplicate is in use and can be cleared once all are dropped, as with the object returned today. The cost is one buffer object per mapped piece per read. A progress check in the {{readData}} loop would be a reasonable separate hardening; with this change a returned buffer always starts at position 0 with the mapped size as its limit, so the loop always advances.
> With the patch {{TestMappedBufferManager}}, {{TestChunkUtils}}, {{TestFilePerBlockStrategy}}, {{TestFilePerChunkStrategy}}, {{TestKeyValueHandler}}, {{TestKeyValueHandlerWithUnhealthyContainer}}, {{TestChunkManagerDummyImpl}}, {{TestBlockManagerImpl}} and {{TestKeyValueContainerCheck}} pass (283 tests, 20 skipped in {{TestBlockManagerImpl}}), and checkstyle on {{hadoop-hdds/container-service}} is clean.
> Affected releases: 2.0.0 to 2.2.1, since HDDS-10488 introduced the cache.
> One coordination note: the patches proposed for two related {{MappedBufferManager}} issues filed with this one (HDDS-17368, the permit of a range that is mapped again after GC cleared its entry, and HDDS-17367, the mapping map shared by all managers) also change {{TestMappedBufferManager}}. This test change conflicts with both in the static imports and with the first of them at the end of the class. The product change applies on top of either. Whichever lands later needs a trivial rebase.
> 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 PRs for duplicates before filing. Related but not duplicates: HDDS-10488 (introduced the cache), HDDS-15357 (WeakReference races in the same method). 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