mgr/vol: some improvements for async_job.py by rishabh-d-dave · Pull Request #60002 · ceph/ceph

rishabh-d-dave · 2024-09-26T15:06:16Z

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

The attribute self.stopping of class AsyncJob is an instance of threading.Event that is only used to set and check if it is set. For such a case, threading.Event is an overkill, using a boolean variable instead would suffice perfectly. Signed-off-by: Rishabh Dave <ridave@redhat.com>

Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave · 2024-09-27T04:39:30Z

jenkins test api

rishabh-d-dave · 2024-09-27T04:39:44Z

jenkins test windows

rishabh-d-dave · 2024-09-27T04:39:56Z

jenkins test make check arm64

rishabh-d-dave · 2024-09-27T07:42:19Z

jenkins test api

rishabh-d-dave · 2024-09-27T07:42:28Z

jenkins test windows

rishabh-d-dave · 2024-09-30T14:39:40Z

jenkins test windows

rishabh-d-dave · 2024-09-30T14:40:52Z

jenkins test make check arm64

rishabh-d-dave · 2024-10-01T14:14:32Z

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

batrick

The attribute self.stopping of class AsyncJob is an instance of
threading.Event that is only used to set and check if it is set. For
such a case, threading.Event is an overkill, using a boolean variable
instead would suffice perfectly.

huh? Why is it overkill?

rishabh-d-dave · 2024-10-03T17:46:38Z

The attribute self.stopping of class AsyncJob is an instance of
threading.Event that is only used to set and check if it is set. For
such a case, threading.Event is an overkill, using a boolean variable
instead would suffice perfectly.

huh? Why is it overkill?

Events are used for thread synchronization. And since we aren't doing anything even remotely close there's no reason to use Event. A boolean variable exactly the job.

batrick · 2024-10-03T18:05:02Z

The attribute self.stopping of class AsyncJob is an instance of
threading.Event that is only used to set and check if it is set. For
such a case, threading.Event is an overkill, using a boolean variable
instead would suffice perfectly.

huh? Why is it overkill?

Events are used for thread synchronization. And since we aren't doing anything even remotely close there's no reason to use Event. A boolean variable exactly the job.

Except shutdown is called by another thread, no? So using an Event is necessary to construct a memory barrier.

rishabh-d-dave · 2024-10-04T15:08:21Z

The attribute self.stopping of class AsyncJob is an instance of
threading.Event that is only used to set and check if it is set. For
such a case, threading.Event is an overkill, using a boolean variable
instead would suffice perfectly.

huh? Why is it overkill?

Events are used for thread synchronization. And since we aren't doing anything even remotely close there's no reason to use Event. A boolean variable exactly the job.

Except shutdown is called by another thread, no? So using an Event is necessary to construct a memory barrier.

Right. But we don't use any features that class Event provides (like wait()). We only set and clear it, so using Event instance is pointless and boolean instance would work just fine.

batrick · 2024-10-07T21:10:56Z

The attribute self.stopping of class AsyncJob is an instance of
threading.Event that is only used to set and check if it is set. For
such a case, threading.Event is an overkill, using a boolean variable
instead would suffice perfectly.

huh? Why is it overkill?

Events are used for thread synchronization. And since we aren't doing anything even remotely close there's no reason to use Event. A boolean variable exactly the job.

Except shutdown is called by another thread, no? So using an Event is necessary to construct a memory barrier.

Right. But we don't use any features that class Event provides (like wait()). We only set and clear it, so using Event instance is pointless and boolean instance would work just fine.

Yes, you do.

https://github.com/python/cpython/blob/c5df1cb7bde7e86f046196b0e34a0b90f8fc11de/Lib/threading.py#L620-L629

Simply notifying on a condition variable will create a memory barrier. That is essential so that Event.is_set is correctly true on other threads.

vshankar · 2024-10-16T06:28:09Z

Events are used for thread synchronization. And since we aren't doing anything even remotely close there's no reason to use Event. A boolean variable exactly the job.

I really would not bother micro-optimizing this code.

rishabh-d-dave · 2024-10-18T08:43:02Z

Events are used for thread synchronization. And since we aren't doing anything even remotely close there's no reason to use Event. A boolean variable exactly the job.

I really would not bother micro-optimizing this code.

@vshankar
It's not about micro-optimizing code but about not being confused while reading it. Using event gives an impression that we intend to do some waiting somewhere but it's nowhere to be found in this codebase.

rishabh-d-dave

QA run was successful - https://tracker.ceph.com/projects/cephfs/wiki/Main#2024-Oct-18

vshankar · 2024-10-18T11:49:56Z

@vshankar
It's not about micro-optimizing code but about not being confused while reading it. Using event gives an impression that we intend to do some waiting somewhere but it's nowhere to be found in this codebase.

The point to note here is that event uses its own internal lock. Switching to boolean for this specific case is fine since it's a validation check for the while .. loop and note that calling shutdown() is not taking any locks. As I mention, it does not have a negative effect in this case, but at times it might.

batrick · 2024-10-23T15:07:30Z

src/pybind/mgr/volumes/fs/async_job.py


    def shutdown(self):
-        self.stopping.set()
+        self.stopping = True


This is called by module shutdown (which is not even used anymore) but, despite that, the shutdown method would be called from a separate thread. (I mean, that's the entire point of a stopping flag. Why would you need it if it's only set by the thread itself?)

It's entirely conventional for a stopping flag on a thread to be a threading.Event. I really don't understand the motivation to change this.

It's entirely conventional for a stopping flag on a thread to be a threading.Event.

It is conventional to have lock on a boolean value when multiple threads will attempt to modify the boolean value. But since there's only one thread in this case, a boolean value without a lock is equally fine and therefore usage of threading.Event is unnecessary.

Sorry, no. Anyone coming in the future that wants to stop threads now must play roulette with the cache lines hoping that the shutdown method actually does what you would think in a properly multi-threaded class.

I urge you to consider this to be defensive programming and let the matter rest. There is no real performance gain to squeeze from this.

I urge you to consider this to be defensive programming

Defensive programming from the POV of possible future attempt to extend use of self.stopping is a good point.

There is no real performance gain to squeeze from this.

Performance was never the main concern for this PR.

rishabh-d-dave · 2024-10-24T13:33:07Z

What has come up through discussions is that it's fine to merge this patch since in this particular case it would have no negative effect. But in order to prevent accidental introduction of bugs in future, we would like to take the defensive approach and not merge this patch. Closing this PR.

rishabh-d-dave added 2 commits September 26, 2024 20:33

mgr/vol: add a helpful comment in async_job.py

19d42d5

Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave added cephfs Ceph File System pybind needs-review labels Sep 26, 2024

rishabh-d-dave requested review from a team and batrick September 26, 2024 15:06

rishabh-d-dave added the improvements label Sep 26, 2024

rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Oct 1, 2024

batrick requested changes Oct 3, 2024

View reviewed changes

rishabh-d-dave commented Oct 18, 2024

View reviewed changes

vshankar approved these changes Oct 18, 2024

View reviewed changes

batrick requested changes Oct 23, 2024

View reviewed changes

rishabh-d-dave closed this Oct 24, 2024

rishabh-d-dave deleted the mgr-vol-stopping branch October 24, 2024 13:33

rishabh-d-dave restored the mgr-vol-stopping branch January 27, 2025 13:35

Conversation

rishabh-d-dave commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

rishabh-d-dave commented Sep 27, 2024

Uh oh!

rishabh-d-dave commented Sep 27, 2024

Uh oh!

rishabh-d-dave commented Sep 27, 2024

Uh oh!

rishabh-d-dave commented Sep 27, 2024

Uh oh!

rishabh-d-dave commented Sep 27, 2024

Uh oh!

rishabh-d-dave commented Sep 30, 2024

Uh oh!

rishabh-d-dave commented Sep 30, 2024

Uh oh!

rishabh-d-dave commented Oct 1, 2024

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave commented Oct 3, 2024

Uh oh!

batrick commented Oct 3, 2024

Uh oh!

rishabh-d-dave commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

batrick commented Oct 7, 2024

Uh oh!

vshankar commented Oct 16, 2024

Uh oh!

rishabh-d-dave commented Oct 18, 2024

Uh oh!

rishabh-d-dave left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented Oct 18, 2024

Uh oh!

batrick Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

batrick Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave commented Oct 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rishabh-d-dave commented Sep 26, 2024 •

edited

Loading

rishabh-d-dave commented Oct 4, 2024 •

edited

Loading