blk/aio: fix long batch (64+K entries) submission. by ifed01 · Pull Request #56352 · ceph/ceph

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

  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.

github-actions · 2024-07-02T20:01:36Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

ifed01 · 2024-07-11T10:39:58Z

unstale

kamoltat · 2024-07-18T17:42:34Z

RADOS APPROVED https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues65796

…read blk/aio: fix long batch (64+K entries) submission. Reviewed-by: Adam Kupczyk <akupczyk@redhat.com>

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

github-actions bot added the stale label Jul 2, 2024

aclamk removed the stale label Jul 9, 2024

yuriw merged commit 28955bd into ceph:main Jul 18, 2024

ifed01 deleted the wip-ifed-many-many-extents-read branch July 18, 2024 21:17

This was referenced Jul 19, 2024

quincy: blk/aio: fix long batch (64+K entries) submission #58674

Merged

reef: blk/aio: fix long batch (64+K entries) submission #58675

Merged

squid: blk/aio: fix long batch (64+K entries) submission #58676

Merged

cyx1231st mentioned this pull request Jul 23, 2024

blk/aio: fix compile issue when HAVE_LIBURING isn't defined #58745

Merged

14 tasks

NitzanMordhai pushed a commit to NitzanMordhai/ceph that referenced this pull request Aug 1, 2024

Merge pull request ceph#56352 from ifed01/wip-ifed-many-many-extents-…

e3561fb

…read blk/aio: fix long batch (64+K entries) submission. Reviewed-by: Adam Kupczyk <akupczyk@redhat.com>

Conversation

ifed01 commented Mar 20, 2024

Contribution Guidelines

Checklist

Uh oh!

ifed01 commented Mar 21, 2024

Uh oh!

ifed01 commented Mar 21, 2024

Uh oh!

Uh oh!

aclamk Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

ifed01 Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ifed01 commented Apr 16, 2024

Uh oh!

aclamk Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

ifed01 Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

aclamk Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

ifed01 Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

ifed01 Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aclamk commented Apr 30, 2024

Uh oh!

ifed01 commented May 1, 2024

Uh oh!

yuriw commented May 3, 2024

Uh oh!

github-actions bot commented Jul 2, 2024

Uh oh!

ifed01 commented Jul 11, 2024

Uh oh!

kamoltat commented Jul 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ifed01 Apr 15, 2024 •

edited

Loading