[
https://issues.apache.org/jira/browse/HDDS-17370?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Andy Huang reassigned HDDS-17370:
---------------------------------
Assignee: Andy Huang
> 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
> Assignee: Andy Huang
> 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: [email protected]
For additional commands, e-mail: [email protected]
