From: Dhairya Parmar <dparmar@redhat.com>
Date: Thu, 11 Jan 2024 10:51:33 +0000 (+0530)
Subject: client: fix file cache cap leak which can stall async read call
X-Git-Tag: testing/wip-vshankar-testing-20240806.164130-debug^2
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=9a7645e9d6f90789aa7726a506c211cf66ed3d4c;p=ceph-ci.git

client: fix file cache cap leak which can stall async read call

We need the `Fc` cap ref when ObjectCacher::file_read returns zero bytes; then
wait for the I/O to finish; then do readahead but this cap ref needs to be put
down as well at the right place. The problem with the current implementation is
that it is difficult to decide whether to revoke the `Fc` cap ref in
Client::C_Read_Async_Finisher::finish or not since the destructor cannot
differenciate between the cases as it is fed with the bytes that are read after
waiting for the I/O to be finished.

Therefore provide `Fc` cap ref to the inode before making a call to
ObjectCacher::file_read in Client::_read_async and handle revoking it:
- if async read then in Client::C_Read_Async_Finisher::finish
- else if sync read then within Client::_read_async right before calling
  Client::do_readahead.

Fixes: https://tracker.ceph.com/issues/63896
Signed-off-by: Dhairya Parmar <dparmar@redhat.com>
---

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 1e672635031..9c7cdcffd85 100644
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -11058,6 +11058,9 @@ int Client::_read_async(Fh *f, uint64_t off, uint64_t len, bufferlist *bl,
 
   ldout(cct, 10) << __func__ << " " << *in << " " << off << "~" << len << dendl;
 
+  // get Fc cap ref before commencing read
+  get_cap_ref(in, CEPH_CAP_FILE_CACHE);
+
   if (onfinish != nullptr) {
     io_finish.reset(new C_Read_Async_Finisher(this, onfinish, f, in,
                                               f->pos, off, len));
@@ -11065,9 +11068,14 @@ int Client::_read_async(Fh *f, uint64_t off, uint64_t len, bufferlist *bl,
 
   // trim read based on file size?
   if ((off >= in->size) || (len == 0)) {
+    // read is requested at the EOF or the read len is zero, therefore release
+    // Fc cap first before proceeding further
+    put_cap_ref(in, CEPH_CAP_FILE_CACHE);
+
     // If not async, immediate return of 0 bytes
-    if (onfinish == nullptr) 
+    if (onfinish == nullptr) {
       return 0;
+    }
 
     // Release C_Read_Async_Finisher from managed pointer, we need to complete
     // immediately. The C_Read_Async_Finisher is safely handled and won't be
@@ -11100,6 +11108,8 @@ int Client::_read_async(Fh *f, uint64_t off, uint64_t len, bufferlist *bl,
 			      off, len, bl, 0, io_finish.get());
 
   if (onfinish != nullptr) {
+    // put the cap ref since we're releasing C_Read_Async_Finisher
+    put_cap_ref(in, CEPH_CAP_FILE_CACHE);
     // Release C_Read_Async_Finisher from managed pointer, either
     // file_read will result in non-blocking complete, or we need to complete
     // immediately. In either case, the C_Read_Async_Finisher is safely
@@ -11108,19 +11118,19 @@ int Client::_read_async(Fh *f, uint64_t off, uint64_t len, bufferlist *bl,
     if (r != 0) {
       // need to do readahead, so complete the crf
       crf->complete(r);
-    } else {
-      get_cap_ref(in, CEPH_CAP_FILE_CACHE);
     }
     return 0;
   }
 
+  // Wait for the blocking read to complete and then do readahead
   if (r == 0) {
-    get_cap_ref(in, CEPH_CAP_FILE_CACHE);
     client_lock.unlock();
     r = io_finish_cond->wait();
     client_lock.lock();
     put_cap_ref(in, CEPH_CAP_FILE_CACHE);
     update_read_io_size(bl->length());
+  } else {
+    put_cap_ref(in, CEPH_CAP_FILE_CACHE);
   }
 
   do_readahead(f, in, off, len);