Re: [PATCH v7 10/18] jobs: rename static functions called with job_mutex held

[Vladimir Sementsov-Ogievskiy]

On 6/16/22 16:18, Emanuele Giuseppe Esposito wrote:
> With the*nop*  job_lock/unlock placed, rename the static
> functions that are always under job_mutex, adding "_locked" suffix.
>
> List of functions that get this suffix:
> job_txn_ref                job_txn_del_job
> job_txn_apply              job_state_transition
> job_should_pause           job_event_cancelled
> job_event_completed        job_event_pending
> job_event_ready            job_event_idle
> job_do_yield               job_timer_not_pending
> job_do_dismiss             job_conclude
> job_update_rc              job_commit
> job_abort                  job_clean
> job_finalize_single        job_cancel_async
> job_completed_txn_abort    job_prepare
> job_needs_finalize         job_do_finalize
> job_transition_to_pending  job_completed_txn_success
> job_completed              job_cancel_err
> job_force_cancel_err
>
> Note that "locked" refers to the*nop*  job_lock/unlock, and not
> real_job_lock/unlock.
>
> No functional change intended.
>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>


Hmm. Maybe it was already discussed.. But for me it seems, that it would be 
simpler to review previous patches, that fix job_ API users to use locking 
properly, if this renaming go earlier.

Anyway, in this series, we can't update everything at once. So patch to patch, 
we make the code more and more correct. (yes I remember that lock() is a noop, 
but I should review thinking that it real, otherwise, how to review?)

So, I'm saying about formal correctness of using lock() unlock() function in 
connection with introduced _locked prifixes and in connection with how it 
should finally work.

You do:

05. introduce some _locked functions, that just duplicates, and 
job_pause_point_locked() is formally inconsistent, as I said.

06. Update a lot of places, to give them their final form (but not final, as 
some functions will be renamed to _locked, some not, hard to imagine)

07,08,09. Update some more, and even more places. very hard to track formal 
correctness of using locks

10-...: rename APIs.


What do you think about the following:

1. Introduce noop lock, and some internal _locked() versions, and keep formal 
consistency inside job.c, considering all public interfaces as unlocked:

 at this point:
  - everything correct inside job.c
  - no public interfaces with _locked prefix
  - all public interfaces take mutex internally
  - no external user take mutex by hand

We can rename all internal static functions at this step too.

2. Introduce some public _locked APIs, that we'll use in next patches

3. Now start fixing external users in several patches:
  
  - protect by mutex direct use of job fields
  - make wider locks and move to _locked APIs inside them where needed


In this scenario, every updated unit becomes formally correct after update, and 
after all steps everything is formally correct, and we can move to turning-on 
the mutex.

--
Best regards,
Vladimir