client: fix async/sync I/O stalling due to buffer list exceeding INT_MAX by dparmar18 · Pull Request #58564 · ceph/ceph

dparmar18 · 2024-07-12T15:30:02Z

In linux, systems calls like write() anyways don't allow writes > 2GiB, the total write size passed to the Client::_write is clamped to INT_MAX but bufferlist is not handled. This PR addresses this issue which stalls async i/o due to:

unknown file: Failure
C++ exception with description "End of buffer [buffer:2]" thrown in the test body.
2024-05-28T16:17:06.854+0530 7f9a5d24c9c0  2 client.4311 unmount
2024-05-28T16:17:06.854+0530 7f9a5d24c9c0  2 client.4311 unmounting

which results in disconnected inode and cap leaks:

2024-05-28T16:17:11.855+0530 7f9a5d24c9c0  1 client.4311 dump_inode: DISCONNECTED inode 0x10000000001 #0x10000000001 ref 3 0x10000000001.head(faked_ino=0 nref=3 ll_ref=0 cap_refs={4=0,1024=1,4096=1,8192=2} open={3=0} mode=100666 size=0/4294967296 nlink=1 btime=2024-05-28T16:17:03.387546+0530 mtime=2024-05-28T16:17:03.387546+0530 ctime=2024-05-28T16:17:03.387546+0530 change_attr=0 caps=pAsxLsXsxFsxcrwb(0=pAsxLsXsxFsxcrwb) objectset[0x10000000001 ts 0/0 objects 1 dirty_or_tx 0] 0x7f9a2c009530)
2024-05-28T16:17:11.855+0530 7f9a5d24c9c0  2 client.4311 cache still has 0+1 items, waiting (for caps to release?)

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

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)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

dparmar18 · 2024-07-12T15:37:23Z

this passes locally:

[----------] 19 tests from TestClient (27089 ms total)

[----------] Global test environment tear-down
[==========] 19 tests from 1 test suite ran. (27089 ms total)
[  PASSED  ] 19 tests.

chrisphoffman · 2024-07-16T19:10:55Z

I've tried compiling this on my local machine am I'm getting:

nonblocking.cc:637:18: error: aggregate ‘statvfs stbuf’ has incomplete type and cannot be defined
637 |   struct statvfs stbuf;

I was able to work around the issue by adding
#include <sys/statvfs.h>

dparmar18 · 2024-07-17T09:21:56Z

I've tried compiling this on my local machine am I'm getting:
nonblocking.cc:637:18: error: aggregate ‘statvfs stbuf’ has incomplete type and cannot be defined
637 |   struct statvfs stbuf;
I was able to work around the issue by adding #include <sys/statvfs.h>

this is added here. I'm expecting that to be merged first but I guess I should add a temp commit adding the dependency

dparmar18 · 2024-07-17T13:04:18Z

Hey @chrisphoffman, added a temp commit. Thanks for taking a look :)

mchangir · 2024-07-30T12:02:04Z

src/client/Client.cc

+        if (total_appended >= static_cast<ssize_t>(size)) {
+          bl.append(nullptr, 0);


shouldn't we just stop appending nullptr buffer pointers and break out of the for loop here ?

makes sense

src/test/client/nonblocking.cc

dparmar18 · 2024-08-21T12:08:40Z

last push enhanced the code to handle cases where if:

current iov is gte INT_MAX
total size appended is gte INT_MAX
both 1 and 2.

github-actions · 2024-08-22T09:47:17Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

vshankar · 2024-08-28T12:52:08Z

RFR @ceph/cephfs

vshankar · 2024-08-28T12:52:19Z

@dparmar18 this required a rebase please.

src/client/Client.cc

dparmar18 · 2025-05-08T07:46:42Z

@vshankar ping

vshankar · 2025-05-13T14:28:23Z

@vshankar ping

Noted. In my review for this week.

dparmar18 · 2025-05-28T11:43:58Z

@vshankar do we have any update on this?

vshankar · 2025-05-28T16:07:02Z

@vshankar do we have any update on this?

I was catching up on backlog. Getting to this soon.

vshankar · 2025-05-28T16:09:02Z

jenkins retest this please

dparmar18 · 2025-05-28T18:34:45Z

jenkins test make check

vshankar · 2025-05-29T12:20:12Z

jenkins test make check

dparmar18 · 2025-06-23T11:48:56Z

@vshankar was this tested?

vshankar · 2025-06-23T12:05:09Z

@vshankar was this tested?

No, not really. But I'll put this with the other client side fixes.

vshankar · 2025-06-25T09:47:56Z

jenkins test make check

vshankar · 2025-06-25T09:58:27Z

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

* refs/pull/58564/head: client: clamp sizes to INT_MAX in sync i/o code paths client: restrict bufferlist to total write size src/test: test sync/async i/o code paths with huge (4GiB) buffers Reviewed-by: Venky Shankar <vshankar@redhat.com> Reviewed-by: Kotresh Hiremath Ravishankar <khiremat@redhat.com> Reviewed-by: Christopher Hoffman <choffman@redhat.com>

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-vshankar-testing-20250625095802-debug

Seems to be that the comments have been addressed.

In linux, systems calls like write() anyways don't allow writes > 2GiB, the total write size passed to the Client::_write is clamped to INT_MAX but bufferlist is not handled. This edge case is patched here. bug that arises due to buffer list beyond INT_MAX stalls async i/o due to: unknown file: Failure C++ exception with description "End of buffer [buffer:2]" thrown in the test body. 2024-05-28T16:17:06.854+0530 7f9a5d24c9c0 2 client.4311 unmount 2024-05-28T16:17:06.854+0530 7f9a5d24c9c0 2 client.4311 unmounting which results in disconnected inode and cap leaks: 2024-05-28T16:17:11.855+0530 7f9a5d24c9c0 1 client.4311 dump_inode: DISCONNECTED inode 0x10000000001 #0x10000000001 ref 3 0x10000000001.head(faked_ino=0 nref=3 ll_ref=0 cap_refs={4=0,1024=1,4096=1,8192=2} open={3=0} mode=100666 size=0/4294967296 nlink=1 btime=2024-05-28T16:17:03.387546+0530 mtime=2024-05-28T16:17:03.387546+0530 ctime=2024-05-28T16:17:03.387546+0530 change_attr=0 caps=pAsxLsXsxFsxcrwb(0=pAsxLsXsxFsxcrwb) objectset[0x10000000001 ts 0/0 objects 1 dirty_or_tx 0] 0x7f9a2c009530) 2024-05-28T16:17:11.855+0530 7f9a5d24c9c0 2 client.4311 cache still has 0+1 items, waiting (for caps to release?) This commit changes the way Client::_write accepts data. So, now instead of accepting ptr, iovec array and iovcnt, the helper now accepts a bufferlist which should be contructed by the caller itself. The reason behind this change is simple - to declutter the API. For more context checkout this conversation ceph/ceph#58564 (comment) Fixes: https://tracker.ceph.com/issues/66245 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

In linux, systems calls like write() anyways don't allow writes > 2GiB, the total write size passed to the Client::_write is clamped to INT_MAX but bufferlist is not handled. This edge case is patched here. bug that arises due to buffer list beyond INT_MAX stalls async i/o due to: unknown file: Failure C++ exception with description "End of buffer [buffer:2]" thrown in the test body. 2024-05-28T16:17:06.854+0530 7f9a5d24c9c0 2 client.4311 unmount 2024-05-28T16:17:06.854+0530 7f9a5d24c9c0 2 client.4311 unmounting which results in disconnected inode and cap leaks: 2024-05-28T16:17:11.855+0530 7f9a5d24c9c0 1 client.4311 dump_inode: DISCONNECTED inode 0x10000000001 #0x10000000001 ref 3 0x10000000001.head(faked_ino=0 nref=3 ll_ref=0 cap_refs={4=0,1024=1,4096=1,8192=2} open={3=0} mode=100666 size=0/4294967296 nlink=1 btime=2024-05-28T16:17:03.387546+0530 mtime=2024-05-28T16:17:03.387546+0530 ctime=2024-05-28T16:17:03.387546+0530 change_attr=0 caps=pAsxLsXsxFsxcrwb(0=pAsxLsXsxFsxcrwb) objectset[0x10000000001 ts 0/0 objects 1 dirty_or_tx 0] 0x7f9a2c009530) 2024-05-28T16:17:11.855+0530 7f9a5d24c9c0 2 client.4311 cache still has 0+1 items, waiting (for caps to release?) This commit changes the way Client::_write accepts data. So, now instead of accepting ptr, iovec array and iovcnt, the helper now accepts a bufferlist which should be contructed by the caller itself. The reason behind this change is simple - to declutter the API. For more context checkout this conversation ceph#58564 (comment) Fixes: https://tracker.ceph.com/issues/66245 Signed-off-by: Dhairya Parmar <dparmar@redhat.com> (cherry picked from commit f4f359e)

github-actions bot added cephfs Ceph File System tests labels Jul 12, 2024

dparmar18 force-pushed the wip-66245 branch from e706e36 to a137d6d Compare July 12, 2024 15:33

dparmar18 requested a review from a team July 12, 2024 15:35

dparmar18 force-pushed the wip-66245 branch from a137d6d to 770b60d Compare July 17, 2024 13:03

mchangir reviewed Jul 30, 2024

View reviewed changes

dparmar18 force-pushed the wip-66245 branch from 770b60d to 862f18b Compare July 30, 2024 12:34

chrisphoffman reviewed Jul 30, 2024

View reviewed changes

src/test/client/nonblocking.cc Outdated Show resolved Hide resolved

dparmar18 force-pushed the wip-66245 branch from 862f18b to c823059 Compare August 21, 2024 12:06

dparmar18 force-pushed the wip-66245 branch 2 times, most recently from 57bed9a to c4b591d Compare August 21, 2024 12:34

dparmar18 changed the title ~~Client: Fix async I/O stalling due to buffer list exceeding total write size~~ Client: Fix async/sync I/O stalling due to buffer list exceeding INT_MAX Aug 21, 2024

github-actions bot added the needs-rebase label Aug 22, 2024

dparmar18 force-pushed the wip-66245 branch from c4b591d to 5bdfa53 Compare August 28, 2024 12:42

dparmar18 requested review from a team, chrisphoffman and mchangir August 28, 2024 12:43

vshankar reviewed Aug 29, 2024

View reviewed changes

src/client/Client.cc Outdated Show resolved Hide resolved

dparmar18 force-pushed the wip-66245 branch from 5bdfa53 to 0216885 Compare September 24, 2024 13:49

github-actions bot removed the needs-rebase label Sep 24, 2024

dparmar18 force-pushed the wip-66245 branch 2 times, most recently from dd26301 to 4beb883 Compare September 24, 2024 13:53

dparmar18 requested a review from batrick May 8, 2025 07:46

vshankar added the wip-vshankar-testing label May 29, 2025

vshankar added wip-vshankar-testing1 and removed wip-vshankar-testing labels May 29, 2025

vshankar added wip-vshankar-testing and removed wip-vshankar-testing1 labels Jun 25, 2025

vshankar approved these changes Jul 17, 2025

View reviewed changes

vshankar merged commit fa6e884 into ceph:main Jul 21, 2025
12 checks passed

vshankar removed the wip-vshankar-testing label Jul 21, 2025

joscollin mentioned this pull request Aug 27, 2025

tentacle: client: fix async/sync I/O stalling due to buffer list exceeding INT_MAX #65256

Merged

This was referenced Aug 28, 2025

squid: client: fix async/sync I/O stalling due to buffer list exceeding INT_MAX #65287

Open

tentacle: client: fix async/sync I/O stalling due to buffer list exceeding INT_MAX #65288

Closed

		if (total_appended >= static_cast<ssize_t>(size)) {
		bl.append(nullptr, 0);

Conversation

dparmar18 commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

dparmar18 commented Jul 12, 2024

Uh oh!

chrisphoffman commented Jul 16, 2024

Uh oh!

dparmar18 commented Jul 17, 2024

Uh oh!

dparmar18 commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mchangir Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

dparmar18 Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

dparmar18 Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dparmar18 commented Aug 21, 2024

Uh oh!

github-actions bot commented Aug 22, 2024

Uh oh!

vshankar commented Aug 28, 2024

Uh oh!

vshankar commented Aug 28, 2024

Uh oh!

Uh oh!

dparmar18 commented May 8, 2025

Uh oh!

vshankar commented May 13, 2025

Uh oh!

dparmar18 commented May 28, 2025

Uh oh!

vshankar commented May 28, 2025

Uh oh!

vshankar commented May 28, 2025

Uh oh!

dparmar18 commented May 28, 2025

Uh oh!

vshankar commented May 29, 2025

Uh oh!

dparmar18 commented Jun 23, 2025

Uh oh!

vshankar commented Jun 23, 2025

Uh oh!

vshankar commented Jun 25, 2025

Uh oh!

vshankar commented Jun 25, 2025

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dparmar18 commented Jul 12, 2024 •

edited

Loading

dparmar18 commented Jul 17, 2024 •

edited

Loading