blk/aio: fix long batch (64+K entries) submission. #56352

ifed01 · 2024-03-20T21:06:53Z

This is a revival of #44065
Includes some fixes and UT.

Fixes: https://tracker.ceph.com/issues/64966

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

ifed01 · 2024-03-21T10:40:20Z

jenkins test make check

ifed01 · 2024-03-21T10:40:30Z

jenkins test api

src/test/objectstore/store_test.cc

aclamk · 2024-04-09T07:41:53Z

src/blk/aio/aio.cc

@@ -25,33 +25,43 @@ int aio_queue_t::submit_batch(aio_iter begin, aio_iter end,
  int r;

  aio_iter cur = begin;
-  struct aio_t *piocb[aios_size];


The problem, as I understand it, is very simple: aios_size got an overflow.
I think solution should be to either:

change uint16_t to uint32_t (preferred)

count elements between begin and end; use alloca to allocate piocb.

The only benefit of current solution is that it does minimize stack allocation, but at cost of added complexity. I do not think its worth it.
Note that for every entry in piocb we will be reading at least 4K from the drive. I think we can afford having entire piocb on stack.

@aclamk - you might be missing one more aspect - amount of chunks (and hence amount IOs issued in a single batch) in an object is determined (to some degree) by cluster's users. So it becomes possible for them to use "bad" objects which could DDOS OSDs or do something inappropriate.
And this patch provides a sort of protection against that - at aio level at least..

Given that I'd prefer not to [ab]use stack for this task.

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>

Due to aios_size being a uint16 and the source value for the actual call being an int there was a possible overflow. This was "fixed" with an assert, however that still causes a crash. This commit removes the need for aios_size completely by iterating over the list and submitting it in max_iodepth batches. Fixes: https://tracker.ceph.com/issues/46366 Signed-off-by: Robin Geuze <robin.geuze@nl.team.blue> (cherry picked from commit f87db49)

ifed01 · 2024-04-16T15:20:29Z

jenkins test api

aclamk · 2024-04-18T09:58:40Z

src/blk/aio/aio.cc

+      cur->aio.aiocb.aio_sigevent.sigev_notify = SIGEV_KEVENT;
+      cur->aio.aiocb.aio_sigevent.sigev_notify_kqueue = ctx;
+      cur->aio.aiocb.aio_sigevent.sigev_value.sival_ptr = &(*cur);
+      r = aio_read(&cur->aio.aiocb);


'aio_read'? How on earth POSIXAIO could have ever worked with ceph? I refuse to believe that there were never single aio request to write one chunk.
Proposal:
Lets replace implementation here with #error Implementation removed..

Yeah, that's funny. Replaced with aio_write()

aclamk · 2024-04-18T10:11:37Z

src/blk/aio/aio.cc

+    r = io_submit(ctx, toSubmit, (struct iocb**)(piocb + itemCount0));
+    if (r >= 0 && r < toSubmit) {
+      r = -EAGAIN;
+      itemCount0 += (r > 0 ? r : 0);


Easy task for an optimizer: itemCount0 += 0;, as r==-EAGAIN :)

I believed I've fixed that... Let me check.

missed the push, should be fine now

src/blk/aio/aio.cc

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>

Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>

aclamk · 2024-04-30T13:13:25Z

jenkins test make check

ifed01 · 2024-05-01T18:22:31Z

jenkins test make check

yuriw · 2024-05-03T18:15:59Z

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

ifed01 requested a review from a team as a code owner March 20, 2024 21:06

ifed01 added the bug-fix label Mar 20, 2024

github-actions bot added core tests labels Mar 20, 2024

ifed01 requested review from aclamk and rzarzynski March 20, 2024 21:07

ifed01 force-pushed the wip-ifed-many-many-extents-read branch from c29dd7b to bc78b3b Compare March 20, 2024 21:11

aclamk reviewed Apr 9, 2024

View reviewed changes

src/test/objectstore/store_test.cc Show resolved Hide resolved

aclamk reviewed Apr 9, 2024

View reviewed changes

ifed01 and others added 3 commits April 12, 2024 14:35

test/store_test: add a case for reading an object with 64+K extents.

ccb6614

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>

blk/kernel: always use ceph_assert

62c1ed8

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>

ifed01 force-pushed the wip-ifed-many-many-extents-read branch 2 times, most recently from 444a0ac to 3666d9d Compare April 15, 2024 13:51

aclamk reviewed Apr 18, 2024

View reviewed changes

src/blk/aio/aio.cc Outdated Show resolved Hide resolved

ifed01 force-pushed the wip-ifed-many-many-extents-read branch 2 times, most recently from 41b2fa9 to 4790da3 Compare April 18, 2024 10:58

rzarzynski reviewed Apr 18, 2024

View reviewed changes

src/blk/aio/aio.cc Outdated Show resolved Hide resolved

ifed01 and others added 3 commits April 18, 2024 14:08

blk/aio: fix incomplete patch to get rid off aio_size

8238b60

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>

test/store_test: add a simplified test for readv.

8e32359

Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

bli/aio: replace inappropriate aio_read() with aio_write for POSIXAIO

acf7f15

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>

ifed01 force-pushed the wip-ifed-many-many-extents-read branch from 4790da3 to acf7f15 Compare April 18, 2024 11:08

aclamk approved these changes Apr 30, 2024

View reviewed changes

aclamk added the needs-qa label Apr 30, 2024

ljflores added the wip-yuri2-testing label May 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blk/aio: fix long batch (64+K entries) submission. #56352

blk/aio: fix long batch (64+K entries) submission. #56352

ifed01 commented Mar 20, 2024

ifed01 commented Mar 21, 2024

ifed01 commented Mar 21, 2024

aclamk Apr 9, 2024

ifed01 Apr 15, 2024 •

edited

ifed01 commented Apr 16, 2024

aclamk Apr 18, 2024

ifed01 Apr 18, 2024

aclamk Apr 18, 2024

ifed01 Apr 18, 2024

ifed01 Apr 18, 2024

aclamk commented Apr 30, 2024

ifed01 commented May 1, 2024

yuriw commented May 3, 2024

blk/aio: fix long batch (64+K entries) submission. #56352

Are you sure you want to change the base?

blk/aio: fix long batch (64+K entries) submission. #56352

Conversation

ifed01 commented Mar 20, 2024

Contribution Guidelines

Checklist

ifed01 commented Mar 21, 2024

ifed01 commented Mar 21, 2024

aclamk Apr 9, 2024

Choose a reason for hiding this comment

ifed01 Apr 15, 2024 • edited

Choose a reason for hiding this comment

ifed01 commented Apr 16, 2024

aclamk Apr 18, 2024

Choose a reason for hiding this comment

ifed01 Apr 18, 2024

Choose a reason for hiding this comment

aclamk Apr 18, 2024

Choose a reason for hiding this comment

ifed01 Apr 18, 2024

Choose a reason for hiding this comment

ifed01 Apr 18, 2024

Choose a reason for hiding this comment

aclamk commented Apr 30, 2024

ifed01 commented May 1, 2024

yuriw commented May 3, 2024

ifed01 Apr 15, 2024 •

edited