qemu-devel
[Top][All Lists]
Advanced

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

Re: [PATCH v9 11/21] jobs: group together API calls under the same job l


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v9 11/21] jobs: group together API calls under the same job lock
Date: Wed, 20 Jul 2022 16:10:27 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1

On 7/19/22 15:40, Emanuele Giuseppe Esposito wrote:


Am 11/07/2022 um 15:26 schrieb Vladimir Sementsov-Ogievskiy:
   }
     static bool child_job_drained_poll(BdrvChild *c)
@@ -111,8 +113,10 @@ static bool child_job_drained_poll(BdrvChild *c)
       /* An inactive or completed job doesn't have any pending
requests. Jobs
        * with !job->busy are either already paused or have a pause
point after
        * being reentered, so no job driver code will run before they
pause. */
-    if (!job->busy || job_is_completed(job)) {
-        return false;
+    WITH_JOB_LOCK_GUARD() {
+        if (!job->busy || job_is_completed_locked(job)) {
+            return false;
+        }
       }

This doesn't correspond to commit subject. I'd put such things to
separate commit "correct use of job_mutex in blockjob.c".

         /* Otherwise, assume that it isn't fully stopped yet, but
allow the job to
@@ -127,7 +131,9 @@ static bool child_job_drained_poll(BdrvChild *c)
   static void child_job_drained_end(BdrvChild *c, int
*drained_end_counter)
   {
       BlockJob *job = c->opaque;
-    job_resume(&job->job);
+    WITH_JOB_LOCK_GUARD() {
+        job_resume_locked(&job->job);
+    }
   }

Again, don't see a reason for such change.


[my comments relate to more similar cases in the patch]

I think you misunderstood: I aim to group API calls under the same lock.
One application of such grouping involves loops, but not only them.

I mean that pre-patch job->busy is accessed without any lock at all. So we not 
just group correctly locked calls into one critical section, we also fix direct 
field accesses to be under lock.



Regarding the single-line WITH_JOB_LOCK_GUARD (job_next, job_pause,
job_resume and similar), I guess I will keep the not-locked function.

Emanuele



--
Best regards,
Vladimir



reply via email to

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