bobhan1 commented on code in PR #69071: URL: https://github.com/apache/doris/pull/69071#discussion_r4535823710
########## be/src/io/cache/cached_remote_file_reader.cpp: ########## Review Comment: **[P1] Use copy_to() when reading peer-backed inflight buffers** The affected consumer is [`_try_read_from_inflight_buffers()`, line 1254](https://github.com/apache/doris/blob/2490e6548ddfb411e6b239ee199b7f17e30391e9/be/src/io/cache/cached_remote_file_reader.cpp#L1254-L1255): ```cpp std::memcpy(result.data + copy_left - offset, entry->buffer->data() + buffer_offset, copy_size); ``` This PR allows peer IOBufs to enter the shared inflight index, but `_data` remains `nullptr` for those buffers. With async cache writes, peer reads, and the inflight index enabled, query A can enqueue a peer-backed block for persistence. Before that task finishes, query B with `file_cache_query_limit_bytes=0` can read a range fully covered by the same entry. Its remote-only-on-miss path reaches this copy, triggering the DCHECK in `AsyncCacheWriteBuffer::data()` in debug builds or passing a null source pointer to `memcpy` in release builds and crashing the BE. An explicit `NO_WRITE` read can reach the same path. Please replace this copy with `entry->buffer->copy_to(buffer_offset, copy_size, result.data + copy_left - offset)` and add a test that reads a pending peer IOBuf through the NO_WRITE/remote-only-on-miss path. ########## be/src/io/cache/async_cache_write_manager.h: ########## @@ -59,22 +64,42 @@ struct CacheAdmissionContext { CacheContext to_cache_context(ReadStatistics* stats) const; }; -/// Reference-counted payload whose allocation is charged to the async-write memory tracker. +/// Reference-counted payload of one cache block. A storage read owns an allocation charged to the +/// async-write tracker; a peer read keeps the received IOBuf blocks, which bypass Doris Allocator, +/// so their length is charged to the same tracker explicitly. class AsyncCacheWriteBuffer { public: ~AsyncCacheWriteBuffer(); - char* data() { return _data; } - const char* data() const { return _data; } + /// Writable bytes of an allocated buffer; not valid for an IOBuf payload. + char* data() { + DCHECK(_iobuf == nullptr); Review Comment: **[P1] Update the NO_WRITE inflight reader for IOBuf-backed buffers** `_try_read_from_inflight_buffers()` in `cached_remote_file_reader.cpp:1254` still calls `entry->buffer->data()` and passes the result to `memcpy`. This PR allows peer IOBufs to enter the shared inflight index, but `_data` remains `nullptr` for those buffers. For example, with async cache writes, peer reads, and the inflight index enabled, query A can enqueue a peer-backed block for persistence. Before that task finishes, query B with `file_cache_query_limit_bytes=0` can read a range fully covered by the same inflight entry. Its remote-only-on-miss path reaches the remaining `data()` call, triggering this DCHECK in debug builds or passing a null source pointer to `memcpy` in release builds and crashing the BE. An explicit `NO_WRITE` read can reach the same path. Please switch that consumer to `copy_to()` as well and add a test that reads a pending peer IOBuf through the NO_WRITE/remote-only-on-miss path. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
