RE: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure

[Dexuan Cui]

> From: Bart Van Assche <bvanassche@xxxxxxx>
> Sent: Thursday, April 23, 2020 4:25 PM
> On 2020-04-23 11:29, Dexuan Cui wrote:
> > So it looks the below patch also works for me:
> >
> > --- a/kernel/power/hibernate.c
> > +++ b/kernel/power/hibernate.c
> > @@ -898,6 +898,11 @@ static int software_resume(void)
> >         error = freeze_processes();
> >         if (error)
> >                 goto Close_Finish;
> > +
> > +       error = freeze_kernel_threads();
> > +       if (error)
> > +               goto Close_Finish;
> > +
> >         error = load_image_and_restore();
> >         thaw_processes();
> >   Finish:
> >
> > Just to be sure, I'll do more tests, but I believe the panic can be fixed
> > by this according to my tests I have done so far.
> 
> If a freeze_kernel_threads() call is added in software_resume(), should
> a thaw_kernel_threads() call be added too?

Good catch! 

Note: thaw_processes() thaws every frozen process, including both user
space processes and kernel processes.

In software_resume():
1. If freeze_kernel_threads() fails, I should add a "thaw_processes(); "
before "goto Close_Finish; " so that all the user space processes can 
be thawed.

2. If freeze_kernel_threads() succeeds, but load_image_and_restore()
Fails, there is already a thaw_processes().

3. If load_image_and_restore() succeeds, it won't return, and the
execution will return from the 'old' kernel's hibernate() -> 
hibernation_snapshot() -> create_image() -> swsusp_arch_suspend(),
and later hibernate() -> thaw_processes() will thaw every frozen
process of the 'old' kernel.
 
> Anyway, please Cc me if a patch for software_resume() is submitted.

Sure. Will do.

> > I'm still not sure what the comment before scsi_device_quiesce() means:
> >  *  ... Since special requests may also be requeued requests,
> >  *      a successful return doesn't guarantee the device will be
> >  *      totally quiescent.
> >
> > I don't know if there can be some other I/O submitted after
> > scsi_device_quiesce() returns in the case of hibernation, and I don't
> > know if aac_suspend() -> scsi_host_block() should be fixed/removed,
> > but as far as the panic is concerned, I'm very glad I have found a better
> > fix with your help.
> 
> The function blk_set_pm_only() increments the q->pm_only counter while
> the blk_clear_pm_only() function decrements the q->pm_only counter.
> If q->pm_only > 0, blk_queue_enter() only succeeds if the flag
> BLK_MQ_REQ_PREEMPT is set in the second argument passed to that
> function. blk_get_request() calls blk_queue_enter(). The result is that
> while q->pm_only > 0 blk_get_request() only submits a request without
> waiting if the BLK_MQ_REQ_PREEMPT flag is set in its second argument.
> scsi_execute() sets the BLK_MQ_REQ_PREEMPT flag. In other words,
> scsi_device_quiesce() blocks requests submitted by filesystems but still
> allows SCSI commands submitted by the SCSI core to be executed.
> "special" refers to requests with the BLK_MQ_REQ_PREEMPT flag set.
> 
> Bart.

Thanks for the detailed clarification! So it sounds like we're safe here,
and I guess the scsi_host_block() in aac_suspend() should be removed
to discourage people from trying to use scsi_host_block() in a .suspend()
callback. :-)

Thanks,
-- Dexuan