Skip to content

squid: client: cephfs user-space client fixes#64090

Merged
joscollin merged 11 commits intoceph:squidfrom
vshankar:wip-cephfs-client-fixes-squid
Sep 23, 2025
Merged

squid: client: cephfs user-space client fixes#64090
joscollin merged 11 commits intoceph:squidfrom
vshankar:wip-cephfs-client-fixes-squid

Conversation

@vshankar
Copy link
Contributor

@vshankar vshankar commented Jun 23, 2025 •
edited by joscollin
Loading

commit 6dc7756
https://tracker.ceph.com/issues/71510
https://tracker.ceph.com/issues/68146
https://tracker.ceph.com/issues/68552
https://tracker.ceph.com/issues/70726

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands

synarete and others added 11 commits June 23, 2025 09:06
@synarete @vshankar
client: fix memory leak in Client::CRF_iofinish::complete
bd98ba4 
Commit 1210ddf ("Client: Add non-blocking helper classes") introduced
Client::C_Read_Finisher Context object for async READ operations, but
it has a read-after-free bug which may cause memory leak when calling
libcephf's non-blocking ceph_ll_nonblocking_readv_writev API with async
READ:

ceph_ll_nonblocking_readv_writev (READ)
  Client::ll_preadv_pwritev
  ...
    Client::_read_async
      Context::complete
        Client::CRF_iofinish::complete
          Client::CRF_iofinish::finish
          CRF->finish_io()
            Client::C_Read_Finisher::finish_io
            ...
            delete this; // frees CRF_iofinish->CRF
          if (CRF->iofinished) // use-after-free of CRF
            delete this; // may not get here

A possible memory leak depends on timing and race with other thread
allocation which alters the memory address of CRF->iofinished to
false, thus skipping the last delete operation.

The check of `if (CRF->iofinished)` is unnecessary: it is always set to
true upon calling CRF->finish_io(). Thus, there is no need to have the
override function Client::CRF_iofinish::complete() as it now has the
same logic as Context::complete(). Removed.

Signed-off-by: Shachar Sharon <ssharon@redhat.com>
(cherry picked from commit 6dc7756)
@vshankar
client: asynchronous fsync can decrement request ref twice
238d1f9 
After the asynchronous execution context is woken up when waiting
for Fb caps reference to be released causing the clien to crash
as per:

```
0x00007f3115b2452c in __pthread_kill_implementation () from /lib64/libc.so.6
0x00007f3115ad7686 in raise () from /lib64/libc.so.6
0x00007f3115ac1833 in abort () from /lib64/libc.so.6
0x00007f3113375d0a in ceph::__ceph_assert_fail (assertion=<optimized out>, file=<optimized out>, line=<optimized out>, func=<optimized out>) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/common/assert.cc:74
0x00007f3113375e6f in ceph::__ceph_assert_fail (ctx=...) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/common/assert.cc:79
0x00007f311237db1d in xlist<MetaRequest*>::item::~item (this=<optimized out>, this=<optimized out>) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/include/xlist.h:31
MetaRequest::~MetaRequest (this=<optimized out>, this=<optimized out>) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/client/MetaRequest.cc:65
Client::put_request (this=0x564b491726c0, request=0x7f301c0165c0) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/client/Client.cc:2140
0x00007f31123c88ad in Client::C_nonblocking_fsync_state::advance (this=0x7f307002e9f0) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/client/Client.cc:11905
0x00007f3112331ccd in Context::complete (this=0x7f3070009250, r=<optimized out>) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/include/Context.h:99
0x00007f311246a964 in Client::signal_context_list(std::__cxx11::list<Context*, std::allocator<Context*> >&) [clone .constprop.0] (ls=std::__cxx11::list = {...}, this=<optimized out>)
    at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/client/Client.cc:4257
0x00007f3112395f45 in Client::put_cap_ref (this=0x564b491726c0, in=0x7f306807be90, cap=<optimized out>) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/client/Client.cc:3611
0x00007f31123331f3 in Client::C_Write_Finisher::finish_io (r=0, this=0x7f30240442d0) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/client/Client.cc:11381
Client::CWF_iofinish::finish (this=<optimized out>, r=0) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/client/Client.h:1481
0x00007f3112331ccd in Context::complete (this=0x7f302401afd0, r=<optimized out>) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/include/Context.h:99
0x00007f31123c5242 in Client::C_Lock_Client_Finisher::finish (this=0x7f302403c9d0, r=0) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/client/Client.cc:11372
0x00007f3112331ccd in Context::complete (this=0x7f302403c9d0, r=<optimized out>) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/include/Context.h:99
0x00007f31134374ad in Finisher::finisher_thread_entry (this=0x564b491730b0) at /usr/src/debug/ceph-19.2.0-124.el9cp.x86_64/src/common/Finisher.cc:72
0x00007f3115b227e2 in start_thread () from /lib64/libc.so.6
0x00007f3115ba7800 in clone3 () from /lib64/libc.so.6
0x0000000000000000 in ?? ()
```

Fixes: http://tracker.ceph.com/issues/71510
Signed-off-by: Venky Shankar <vshankar@redhat.com>
(cherry picked from commit ad5a42c)
@kotreshhr @vshankar
qa: Add data read/write test for nfs-ganesha
5d7bb27 
Fixes: https://tracker.ceph.com/issues/68146
Signed-off-by: Kotresh HR <khiremat@redhat.com>
(cherry picked from commit 6d8f610)
@kotreshhr @vshankar
qa: Add libcephfs client test with objectcacher disabled
05b1ea5 
Fixes: https://tracker.ceph.com/issues/68146
Signed-off-by: Kotresh HR <khiremat@redhat.com>
(cherry picked from commit 59b996f)
@kotreshhr @vshankar
test/client: Fix aio nonblocking test
9ca11c6 
The same bufferlist is used without cleaning
for multiple calls. The test 'LlreadvLlwritev'
used to fail because of it. Fixed the same.

Fixes: https://tracker.ceph.com/issues/68146
Signed-off-by: Kotresh HR <khiremat@redhat.com>
(cherry picked from commit b5af1c1)
@kotreshhr @vshankar
client: Fix libcephfs aio metadata corruption.
ce59fc0 
Problem:
With cephfs nfs-ganesha, there were following
asserts hit while doing write on a file.

1. FAILED ceph_assert((bool)_front == (bool)_size)
2. FAILED ceph_assert(cap_refs[c] > 0)

Cause:
In aio path, the client_lock was not being held
in the internal callback after the io is done where
it's expected to be taken leading to corruption.

Fix:
Take client_lock in the callback

Fixes: https://tracker.ceph.com/issues/68146
Signed-off-by: Kotresh HR <khiremat@redhat.com>
(cherry picked from commit 3ebe974)
@kotreshhr @vshankar
client: Fix caps_ref[c]<0 assert
c09c4e1 
When libcephfs aio tests (src/test/client) are run
with objectcacher disabled (ceph_test_client --client_oc=false),
the TestClient.LlreadvLlwritev fails and core dumps. The client
hits the assert 'caps_ref[c]<0'.

This patch fixes the same. There is no need to give out cap_ref
and take it again between multiple read because of short reads.
In some cases, the get_caps used to fail in C_Read_Sync_NonBlocking::finish
causing cap_ref to go negative when put_cap_ref is done at last in
C_Read_Finish::finish_io

Fixes: https://tracker.ceph.com/issues/68308
Signed-off-by: Kotresh HR <khiremat@redhat.com>
(cherry picked from commit 10c8330)
@kotreshhr @vshankar
client: Fix aio zerobyte file read
d04f782 
The following test fails when run with objectcacher
disabled.

TestClient.LlreadvLlwritevZeroBytes Failure - nonblocking.cc

ceph/src/osdc/Striper.cc: 186: FAILED ceph_assert(len > 0)

Traceback:
 ceph version Development (no_version) squid (dev)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x125) [0x7fc0a340aafe]
 2: (ceph::register_assert_context(ceph::common::CephContext*)+0) [0x7fc0a340ad20]
 3: (Striper::file_to_extents(ceph::common::CephContext*, file_layout_t const*, ...)+0x184) [0x562727e13ab4]
 4: (Striper::file_to_extents(ceph::common::CephContext*, char const*, ...)+0x97) [0x562727e145d1]
 5: (Striper::file_to_extents(ceph::common::CephContext*, inodeno_t, ...)+0x75) [0x562727d29520]
 6: (Filer::read_trunc(inodeno_t, file_layout_t const*, snapid_t, ...)+0x61) [0x562727d66ea5]
 7: (Client::C_Read_Sync_NonBlocking::retry()+0x10c) [0x562727cd8a8e]
 8: (Client::_read(Fh*, long, unsigned long, ceph::buffer::v15_2_0::list*, Context*)+0x578) [0x562727d10cb6]
 9: (Client::_preadv_pwritev_locked(Fh*, iovec const*, int, long, bool, ...)+0x3a7) [0x562727d18159]
 10: (Client::ll_preadv_pwritev(Fh*, iovec const*, int, long, bool, ...)+0x179) [0x562727d18b99]
 11: (TestClient_LlreadvLlwritevZeroBytes_Test::TestBody()+0x592) [0x562727ca5352]
 12: (void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, ...)+0x1b) [0x562727d9dea3]
 13: (void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, ...)+0x80) [0x562727da2b26]
 14: (testing::Test::Run()+0xb4) [0x562727d927ae]
 15: (testing::TestInfo::Run()+0x104) [0x562727d92988]
 16: (testing::TestSuite::Run()+0xb2) [0x562727d92b34]
 17: (testing::internal::UnitTestImpl::RunAllTests()+0x36b) [0x562727d95303]
 18: (bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, ...)(), char const*)+0x1b) [0x562727d9e15f]
 19: (bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, ...)+0x80) [0x562727da3083]
 20: (testing::UnitTest::Run()+0x63) [0x562727d92813]
 21: (RUN_ALL_TESTS()+0x11) [0x562727c828d9]
 22: main()

The patch fixes the same.

Fixes: https://tracker.ceph.com/issues/68309
Signed-off-by: Kotresh HR <khiremat@redhat.com>
(cherry picked from commit 942474c)
@adamemerson @vshankar
client: Fix signed comparison warning
b141469 
https://tracker.ceph.com/issues/68552

Signed-off-by: Adam Emerson <aemerson@redhat.com>
(cherry picked from commit e81fbe4)
@vshankar
client: do not check file size when inode does not have Fc caps
3c52305 
Since the client is holding Fr caps, the read request can be
directly sent to the OSD. The offset/in->size comparison check
is causing the read request to return with no data since in->size
isn't yet updated when another client does an extending write.

Introduced-by: 942474c
Fixes: http://tracker.ceph.com/issues/70726
Signed-off-by: Venky Shankar <vshankar@redhat.com>
(cherry picked from commit 2b74598)
@vshankar
test: multi client file read/write test for extending writes
be1fc54 
Credit to @anoopcs9 for the reproducer.

Signed-off-by: Venky Shankar <vshankar@redhat.com>
(cherry picked from commit 624cb4c)

 Conflicts:
	src/test/libcephfs/test.cc

Test InodeGetPut is not in squid branch.
@vshankar vshankar requested review from a team and kotreshhr June 23, 2025 04:13
@vshankar vshankar added the cephfs Ceph File System label Jun 23, 2025
@github-actions github-actions bot added this to the squid milestone Jun 23, 2025
@vshankar vshankar mentioned this pull request Jun 23, 2025
Closed
@joscollin
Copy link
Member

joscollin commented Aug 21, 2025

jenkins retest this please

@vshankar
Copy link
Contributor Author

vshankar commented Sep 1, 2025

jenkins test make check

@joscollin joscollin self-assigned this Sep 1, 2025
@joscollin
Copy link
Member

joscollin commented Sep 1, 2025

jenkins test make check

@joscollin joscollin removed their assignment Sep 1, 2025
@joscollin
Copy link
Member

joscollin commented Sep 5, 2025

This PR is under test in https://tracker.ceph.com/issues/72883.

joscollin
joscollin approved these changes Sep 23, 2025
View reviewed changes
Copy link
Member

@joscollin joscollin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests passed:

@joscollin joscollin merged commit bf97696 into ceph:squid Sep 23, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joscollin joscollin joscollin approved these changes

@kotreshhr kotreshhr Awaiting requested review from kotreshhr

Assignees

No one assigned

Labels

cephfs Ceph File System nfs tests

Projects

None yet

Milestone

squid

Development

Successfully merging this pull request may close these issues.

5 participants

@vshankar @joscollin @synarete @kotreshhr @adamemerson