Skip to content

squid: blk/kernel: bring "bdev_async_discard" config parameter back.#62254

Merged
ifed01 merged 5 commits intoceph:squidfrom
ifed01:wip-ifed-bool-async-discard-back-squi
Apr 29, 2025
Merged

squid: blk/kernel: bring "bdev_async_discard" config parameter back.#62254
ifed01 merged 5 commits intoceph:squidfrom
ifed01:wip-ifed-bool-async-discard-back-squi

Conversation

@ifed01
Copy link
Contributor

@ifed01 ifed01 commented Mar 12, 2025 •
edited
Loading

To ensure backword compatibility for clusters with this parameter previously set to true.

Partially fixes: https://tracker.ceph.com/issues/70327

Additionally brings perf counters to bdev. This includes a new one to track discard threads.

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

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

@ifed01 ifed01 requested a review from a team as a code owner March 12, 2025 16:53
@github-actions github-actions bot added this to the squid milestone Mar 12, 2025
@ifed01 ifed01 requested review from YiteGu and aclamk March 12, 2025 16:56
@YiteGu
Copy link
Member

YiteGu commented Mar 13, 2025

BTW, add this fix(#62150) here.

@ifed01
Copy link
Contributor Author

ifed01 commented Mar 13, 2025

BTW, add this fix(#62150) here.

Done

YiteGu
YiteGu reviewed Mar 13, 2025
View reviewed changes
@ifed01 ifed01 force-pushed the wip-ifed-bool-async-discard-back-squi branch from b8b795f to af0770f Compare March 13, 2025 10:36
@YiteGu
Copy link
Member

YiteGu commented Mar 18, 2025

jenkins test api

@NitzanMordhai
Copy link
Contributor

NitzanMordhai commented Mar 24, 2025

@ifed01 can you please check:
/a/skanta-2025-03-16_07:52:50-rados-wip-bharath11-testing-2025-03-15-1310-squid-distro-default-smithi/8193407/remote/smithi067/log/valgrind/osd.5.log.gz

it looks like
https://github.com/ceph/ceph/pull/59065/files#diff-1b8b5867aeaaa9c7a63ec9488d924befac51d3cbd8ca31806e97ab20a33c8cd7 added some loops that made p invalid

we will ask to remove it from https://tracker.ceph.com/issues/70493 and will be later added to other tests

@YiteGu YiteGu mentioned this pull request Mar 25, 2025
Merged
14 tasks
@ifed01
Copy link
Contributor Author

ifed01 commented Mar 25, 2025

@ifed01 can you please check: /a/skanta-2025-03-16_07:52:50-rados-wip-bharath11-testing-2025-03-15-1310-squid-distro-default-smithi/8193407/remote/smithi067/log/valgrind/osd.5.log.gz

it looks like https://github.com/ceph/ceph/pull/59065/files#diff-1b8b5867aeaaa9c7a63ec9488d924befac51d3cbd8ca31806e97ab20a33c8cd7 added some loops that made p invalid

we will ask to remove it from https://tracker.ceph.com/issues/70493 and will be later added to other tests

@NitzanMordhai - #62471 addresses the issue. Will backport it to sqiud once merged in main

@github-actions
Copy link

github-actions bot commented Apr 9, 2025

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

ifed01 and others added 5 commits April 15, 2025 11:51
@ifed01
blk/kernel: bring "bdev_async_discard" config parameter back.
7b914cb 
To ensure backword compatibility for clusters with this parameter
previously set to true.

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
@YiteGu @ifed01
os/bluestore: passing device type name parameter to kernel device
54af1f6 
Signed-off-by: Yite Gu <yitegu0@gmail.com>
(cherry picked from commit 90835d6)
@YiteGu @ifed01
blk/kerneldevice: add perfcounter for block async discard
d0f30e1 
Adding perfcounter helps to understand the status of async discard.

Signed-off-by: Yite Gu <yitegu0@gmail.com>
(cherry picked from commit f446f4c)
@ifed01
blk/kernel: New perf counter to view amount of discard threads.
d16f268 
Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
(cherry picked from commit 5aba1c9)
@ifed01
blk/kerneldevice: count ops not bytes in discard_op perf counter
967d0d0 
Fixes: https://tracker.ceph.com/issues/70335

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
(cherry picked from commit d7467b0)
@ifed01 ifed01 force-pushed the wip-ifed-bool-async-discard-back-squi branch from af0770f to 967d0d0 Compare April 15, 2025 09:04
@ljflores
Copy link
Member

ljflores commented Apr 28, 2025

Rados approved: https://tracker.ceph.com/projects/rados/wiki/SQUID#httpstrackercephcomissues70983

@ifed01 ifed01 merged commit 814051a into ceph:squid Apr 29, 2025
9 checks passed
@ifed01 ifed01 deleted the wip-ifed-bool-async-discard-back-squi branch April 29, 2025 09:14
@rzarzynski
Copy link
Contributor

rzarzynski commented Sep 17, 2025

@ifed01: is there a PR for main?
I'm grepping in the tentacle branch and... oops:

$ grep -r bdev_async_discard src/
src/blk/kernel/KernelDevice.cc:  uint64_t newcount = cct->_conf.get_val<uint64_t>("bdev_async_discard_threads");
src/blk/kernel/KernelDevice.cc:  auto max_pending = cct->_conf->bdev_async_discard_max_pending;
src/blk/kernel/KernelDevice.cc:    "bdev_async_discard_threads"s,
src/blk/kernel/KernelDevice.cc:  if (changed.count("bdev_async_discard_threads") || changed.count("bdev_enable_discard")) {
src/common/options/global.yaml.in:  - bdev_async_discard_threads
src/common/options/global.yaml.in:- name: bdev_async_discard_threads
src/common/options/global.yaml.in:  - bdev_async_discard_max_pending
src/common/options/global.yaml.in:- name: bdev_async_discard_max_pending
src/common/options/global.yaml.in:  - bdev_async_discard_threads

mkogan1 pushed a commit to mkogan1/ceph that referenced this pull request Sep 18, 2025
@ifed01
Merge pull request ceph#62254 from ifed01/wip-ifed-bool-async-discard…
54365d8 
…-back-squi

squid: blk/kernel: bring "bdev_async_discard" config parameter back.

Resolves rhbz#2394396

Reviewed-by: Yite Gu <guyite@bytedance.com>
(cherry picked from commit 814051a)
@ifed01
Copy link
Contributor Author

ifed01 commented Sep 18, 2025

@ifed01: is there a PR for main? I'm grepping in the tentacle branch and... oops:

$ grep -r bdev_async_discard src/
src/blk/kernel/KernelDevice.cc:  uint64_t newcount = cct->_conf.get_val<uint64_t>("bdev_async_discard_threads");
src/blk/kernel/KernelDevice.cc:  auto max_pending = cct->_conf->bdev_async_discard_max_pending;
src/blk/kernel/KernelDevice.cc:    "bdev_async_discard_threads"s,
src/blk/kernel/KernelDevice.cc:  if (changed.count("bdev_async_discard_threads") || changed.count("bdev_enable_discard")) {
src/common/options/global.yaml.in:  - bdev_async_discard_threads
src/common/options/global.yaml.in:- name: bdev_async_discard_threads
src/common/options/global.yaml.in:  - bdev_async_discard_max_pending
src/common/options/global.yaml.in:- name: bdev_async_discard_max_pending
src/common/options/global.yaml.in:  - bdev_async_discard_threads

yeah, don't recall why I was thinking we don't need this in post-Squid releases... Likely we need to keep that backward compatibility forever... Feel free to make a ticket.

@ifed01
Copy link
Contributor Author

ifed01 commented Sep 19, 2025

@ifed01: is there a PR for main? I'm grepping in the tentacle branch and... oops:

Just submitted:
#65608
#65609
Please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@YiteGu YiteGu YiteGu approved these changes

@aclamk aclamk Awaiting requested review from aclamk

Assignees

No one assigned

Labels

bluestore common core needs-qa wip-yuri-testing

Projects

None yet

Milestone

squid

Development

Successfully merging this pull request may close these issues.

7 participants

@ifed01 @YiteGu @NitzanMordhai @ljflores @rzarzynski @yuriw @SrinivasaBharath