(impala) 02/02: IMPALA-15258: Fix race in buffer pool release

[st...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

asf-gitbox-commits pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 179ba072334ef336d26c08f74a3d69e95760b8db
Author: Michael Smith <mi...@cloudera.com>
AuthorDate: Tue Aug 11 09:13:42 2026 -0700

    IMPALA-15258: Fix race in buffer pool release
    
    target_dirty_bytes in buffer-pool can become negative, which causes the
    while loop at the end of CleanPages to never terminate. This appears to
    be a race condition in DestroyPageInternal, where we remove the page
    from pinned_pages_ while holding lock_, then release the lock and later
    update buffers_allocated_bytes_.
    
        I20260812 22:48:09.199885 141206 buffer-pool.cc:679]
        254fc219ed384ebf:227b64f700000010] target_dirty_bytes=-1048576
        reservation=17825792 buffers_allocated_bytes_=1048576
        pinned_pages_.bytes()=0 len=17825792
    
    Change-Id: Ia247792cbcca24ad2caa82f9b8edc43ddeeaa4f3
    Reviewed-on: http://gerrit.cloudera.org:8080/24689
    Reviewed-by: Michael Smith <mi...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/runtime/bufferpool/buffer-pool.cc | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/be/src/runtime/bufferpool/buffer-pool.cc b/be/src/runtime/bufferpool/buffer-pool.cc
index a3b3c02a0..8b3dd69a0 100644
--- a/be/src/runtime/bufferpool/buffer-pool.cc
+++ b/be/src/runtime/bufferpool/buffer-pool.cc
@@ -481,6 +481,10 @@ void BufferPool::Client::DestroyPageInternal(
     }
     DCHECK(!page->in_queue());
     --num_pages_;
+    // The buffer is being extracted from the page, so it will be accounted for in
+    // buffers_allocated_bytes_ instead of pinned_pages_.bytes(). Count is updated early
+    // because it needs to be updated while holding lock_
+    if (out_buffer != nullptr) buffers_allocated_bytes_ += page->buffer.len();
   }
 
   if (page->write_handle != NULL) {
@@ -490,7 +494,7 @@ void BufferPool::Client::DestroyPageInternal(
   if (out_buffer != NULL) {
     DCHECK(page->buffer.is_open());
     *out_buffer = std::move(page->buffer);
-    buffers_allocated_bytes_ += out_buffer->len();
+    // buffers_allocated_bytes_ was updated above within the lock_.
   } else if (page->buffer.is_open()) {
     pool_->allocator_->Free(move(page->buffer));
   }
@@ -683,6 +687,10 @@ Status BufferPool::Client::CleanPages(
               << " len=" << len << "\n"
               << DebugStringLocked();
   }
+  DCHECK_GE(target_dirty_bytes, 0) << "target_dirty_bytes=" << target_dirty_bytes
+      << " reservation=" << reservation_.GetReservation()
+      << " buffers_allocated_bytes_=" << buffers_allocated_bytes_
+      << " pinned_pages_.bytes()=" << pinned_pages_.bytes() << " len=" << len;
   // Start enough writes to ensure that the loop condition below will eventually become
   // false (or a write error will be encountered).
   int64_t min_bytes_to_write =