[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH 00/15] job: replace AioContext lock with job_mutex

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [RFC PATCH 00/15] job: replace AioContext lock with job_mutex
Date: Tue, 2 Nov 2021 17:58:07 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0

02.11.2021 17:13, Emanuele Giuseppe Esposito wrote:

On 02/11/2021 14:08, Vladimir Sementsov-Ogievskiy wrote:
29.10.2021 19:38, Emanuele Giuseppe Esposito wrote:
In this series, we want to remove the AioContext lock and instead
use the already existent job_mutex to protect the job structures
and list. This is part of the work to get rid of AioContext lock
usage in favour of smaller granularity locks.

In patches 1-3-5-6-7, we split the job API in two headers:
job-driver.h and job-monitor.h.
As explained in job.c, job-monitor are the functions mainly used
by the monitor, and require consistency between the search of
a specific job (job_get) and the actual operation/action on it
(e.g. job_user_cancel). Therefore job-monitor API assume that
the job mutex lock is always held by the caller.

job-driver, on the other side, is the collection of functions
that are used by the job drivers or core block layer. These
functions are not aware of the job mutex, and delegate the
locking to the callee instead.

We also have job-common.h contains the job struct definition
and common functions that are not part of monitor or driver APIs.
job.h is left for legacy and to avoid changing all files that
include it.

Honestly, I don't really like the idea of splitting:

1. It's not a functional split: for some functions we have a locked version in 
one header and unlocked in the other. But actually they are the same function. 
I'd prefer such wrappers to live together. All the declarations in the headers 
are about one thing: Job.

This is something I thought made sense, but I understand that it can be 
confusing. We can also have both versions in the same API. In the end, remember 
that we are talking about only 2 functions: job_is_ready_locked and 

I think, splitting make sense when we really split things, split objects into 
some separate entities. But here you just use different header to group 
functions by some criteria not related to their action. I don't like it.

I think, it's enough to specify in a comment above the function, does it need locking or 
not ("foo_locked" naming helps too), and different headers doesn't help to 
understand code but make it more difficult.

I think that having a single comment above a group of functions does not help, 
because one might forget about it (or the function is far below the comment) 
and insert a new function in the wrong category. Adding the same comment to 
each function makes it redundant IMO. And btw each of job-monitor functions has 
the following (redundant) comment:

Called between job_lock and job_unlock.

Splitting an API in two files might force people to notice that there is a 
physical separation and reason between the two APIs, other than the fact that 
it will be easier for the reviewer to notice if a function is added to the 
wrong API.

But how this splitting help someone, I don't see?

With splitting:
- reviewer should check how locking works with new function, does it need 
- check the function comment
- check that function is in correct file

Without splitting:
- reviewer should check how locking works with new function, does it need 
- check the function comment

And where is benefit?

When I want to understand, does some function needs locking or not, I first see 
at the comment above function definition or declaration. If it has comment 
about locking - great. If not, I have to look at function body.

You say comment is redundant. I don't think so. It's the most helpful thing. 
And such comment in Qemu is a common thing. Splitting locked/unlocked functions 
into different headers - I didn't see such practices.

So if we chose this way - we'll split all other headers related to objects with 
mutex to locked/unlocked API header. I don't think we should do it.

2. I don't like file names:

"job-driver" for me sounds like something about JobDriver struct.

Well it is actually related to the JobDriver struct. It is used by the 
files/function that implement a JobDriver, like mirror, commit, stream ...

"job-monitor" - unclear. You define job-monitor as functions mainly used by the 
monitor. But actually they are used by other code paths as well.. Also, jobs don't 
directly relate to monitor, they are abstract, so no reason to establish such a relation 
in file names.

I think you got the reasoning behind those but just in case:

- job-driver.h : used by the "drivers", ie those who implement 
JobDriver/BlockJobDriver callbacks. Drivers have no knowledge of the job lock, so all 
functions acquire and release the lock internally.

Yes, in *two* cases I kind of broke this rule when I implemented custom 
job-driver functions like job_enter_not_paused and job_not_paused_nor_cancelled 
to avoid TOC/TOU, but I am not even sure whether they are necessary or not.

- job-monitor.h : used by the monitor API, but not only there. The main idea of 
this category is that the functions assume that the lock is held. Therefore 
they can used together in the same critical section and avoid TOC/TOU 

Maybe job-driver and job-unlocked?
Feel free to suggest new names :)

I prefer the name to represent the essence, i.e. "what it is" instead of "who is intended to 
use it". As "what it is" is more stable and independent of context.

job-unlocked - is unclear. Does it mean functions which DON"T NEED locking or which 
DON"T DO locking? But yes, we usually call functions that NEED locking foo_locked()..

So, if split, I'd call them job-locked.h (and only functions with _locked() 
suffix here to make it clear, so, we'll have to rename a lot of functions), and 
job-self-locking.h (and here functions that take job mutex internally).

But again, I don't see a benefit of splitting into several headers and only see 
that it would be harder to maintain.

Also, I don't think that you need this splitting for your series.

You can simply do "job: replace AioContext lock with job_mutex" without 
splitting - it will make the series two times shorter, moving the focus to the main thing 
you want to do.  Splitting may simply be done in separate.

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]