[Qemu-devel] [PATCH] block: for jobs, do not clear user_paused until after the resume

[Qemu-devel] [PATCH] block: for jobs, do not clear user_paused until after the resume

[Jeff Cody]

The function job_cancel_async() will always cause an assert for blockjob
user resume.  We set job->user_paused to false, and then call
job->driver->user_resume().  In the case of blockjobs, this is the
block_job_user_resume() function.

In that function, we assert that job.user_paused is set to true.
Unfortunately, right before calling this function, it has explicitly
been set to false.

The fix is pretty simple: set job->user_paused to false only after the
job user_resume() function has been called.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 job.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/job.c b/job.c
index fa671b431a..e36ebaafd8 100644
--- a/job.c
+++ b/job.c
@@ -732,10 +732,10 @@ static void job_cancel_async(Job *job, bool force)
 {
     if (job->user_paused) {
         /* Do not call job_enter here, the caller will handle it.  */
-        job->user_paused = false;
         if (job->driver->user_resume) {
             job->driver->user_resume(job);
         }
+        job->user_paused = false;
         assert(job->pause_count > 0);
         job->pause_count--;
     }
-- 
2.17.1

Re: [Qemu-devel] [PATCH] block: for jobs, do not clear user_paused until after the resume

[John Snow]

On 08/15/2018 11:59 AM, Jeff Cody wrote:
> The function job_cancel_async() will always cause an assert for blockjob
> user resume.  We set job->user_paused to false, and then call
> job->driver->user_resume().  In the case of blockjobs, this is the
> block_job_user_resume() function.
> 
> In that function, we assert that job.user_paused is set to true.
> Unfortunately, right before calling this function, it has explicitly
> been set to false.
> 
> The fix is pretty simple: set job->user_paused to false only after the
> job user_resume() function has been called.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  job.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/job.c b/job.c
> index fa671b431a..e36ebaafd8 100644
> --- a/job.c
> +++ b/job.c
> @@ -732,10 +732,10 @@ static void job_cancel_async(Job *job, bool force)
>  {
>      if (job->user_paused) {
>          /* Do not call job_enter here, the caller will handle it.  */
> -        job->user_paused = false;
>          if (job->driver->user_resume) {
>              job->driver->user_resume(job);
>          }
> +        job->user_paused = false;
>          assert(job->pause_count > 0);
>          job->pause_count--;
>      }
> 

Looks right to me; are you going to make good on your threat of a v2
with a unit test?

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH] block: for jobs, do not clear user_paused until after the resume

[Eric Blake]

On 08/15/2018 10:59 AM, Jeff Cody wrote:
> The function job_cancel_async() will always cause an assert for blockjob
> user resume.  We set job->user_paused to false, and then call
> job->driver->user_resume().  In the case of blockjobs, this is the
> block_job_user_resume() function.
> 
> In that function, we assert that job.user_paused is set to true.
> Unfortunately, right before calling this function, it has explicitly
> been set to false.
> 
> The fix is pretty simple: set job->user_paused to false only after the
> job user_resume() function has been called.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   job.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Does this need to CC qemu-stable?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] block: for jobs, do not clear user_paused until after the resume

[Jeff Cody]

On Wed, Aug 15, 2018 at 04:25:16PM -0500, Eric Blake wrote:
> On 08/15/2018 10:59 AM, Jeff Cody wrote:
> >The function job_cancel_async() will always cause an assert for blockjob
> >user resume.  We set job->user_paused to false, and then call
> >job->driver->user_resume().  In the case of blockjobs, this is the
> >block_job_user_resume() function.
> >
> >In that function, we assert that job.user_paused is set to true.
> >Unfortunately, right before calling this function, it has explicitly
> >been set to false.
> >
> >The fix is pretty simple: set job->user_paused to false only after the
> >job user_resume() function has been called.
> >
> >Signed-off-by: Jeff Cody <jcody@redhat.com>
> >---
> >  job.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> 
> Does this need to CC qemu-stable?
> 

Good point, yes.  I'm going to do a v2 with an iotest, and I'll CC
qemu-stable on that one.

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks


-Jeff

Re: [Qemu-devel] [PATCH] block: for jobs, do not clear user_paused until after the resume

[Jeff Cody]

On Wed, Aug 15, 2018 at 05:23:43PM -0400, John Snow wrote:
> 
> 
> On 08/15/2018 11:59 AM, Jeff Cody wrote:
> > The function job_cancel_async() will always cause an assert for blockjob
> > user resume.  We set job->user_paused to false, and then call
> > job->driver->user_resume().  In the case of blockjobs, this is the
> > block_job_user_resume() function.
> > 
> > In that function, we assert that job.user_paused is set to true.
> > Unfortunately, right before calling this function, it has explicitly
> > been set to false.
> > 
> > The fix is pretty simple: set job->user_paused to false only after the
> > job user_resume() function has been called.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  job.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/job.c b/job.c
> > index fa671b431a..e36ebaafd8 100644
> > --- a/job.c
> > +++ b/job.c
> > @@ -732,10 +732,10 @@ static void job_cancel_async(Job *job, bool force)
> >  {
> >      if (job->user_paused) {
> >          /* Do not call job_enter here, the caller will handle it.  */
> > -        job->user_paused = false;
> >          if (job->driver->user_resume) {
> >              job->driver->user_resume(job);
> >          }
> > +        job->user_paused = false;
> >          assert(job->pause_count > 0);
> >          job->pause_count--;
> >      }
> > 
> 
> Looks right to me; are you going to make good on your threat of a v2
> with a unit test?
>

Yep!

> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks

-Jeff