Re: [PATCH] PM/Freezer: Thaw only kernel threads if freezing of kernel threads fails

[Tejun Heo <tj@kernel.org>]

On Thu, Feb 02, 2012 at 04:01:12AM +0530, Srivatsa S. Bhat wrote:
> If freezing of kernel threads fails, we are expected to automatically thaw
> tasks in the error recovery path. However, at times, we encounter situations
> in which we would like the automatic error recovery path to thaw only the
> kernel threads, because we want to be able to do some more cleanup before
> we thaw userspace. Something like:
> 
> error = freeze_kernel_threads();
> if (error) {
> 	/* Do some cleanup */
> 
> 	/* Only then thaw userspace tasks*/
> 	thaw_processes();
> }
> 
> An example of such a situation is where we freeze/thaw filesystems during
> suspend/hibernation. There, if freezing of kernel threads fails, we would
> like to thaw the frozen filesystems before thawing the userspace tasks.
> 
> So, modify freeze_kernel_threads() to thaw only kernel threads in case of
> freezing failure.
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Acked-by: Tejun Heo <tj@kernel.org>

with nits below,

> ---
> 
>  kernel/power/power.h   |   26 ++++++++++++++++++++++++--
>  kernel/power/process.c |    7 +++++--
>  2 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 0c4defe..cf41c03 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -231,8 +231,30 @@ extern int pm_test_level;
>  #ifdef CONFIG_SUSPEND_FREEZER
>  static inline int suspend_freeze_processes(void)
>  {
> -	int error = freeze_processes();
> -	return error ? : freeze_kernel_threads();
> +	int error;
> +
> +	error = freeze_processes();
> +
> +	/*
> +	 * freeze_processes() automatically thaws
> +	 * every task if freezing fails. So we need
> +	 * not do anything extra upon error.
> +	 */
> +	if (error)
> +		goto Finish;
> +
> +	error = freeze_kernel_threads();
> +
> +	/*
> +	 * freeze_kernel_threads() thaws only kernel
> +	 * threads upon freezing failure. So we have
> +	 * to thaw the userspace tasks ourselves.
> +	 */
> +	if (error)
> +		thaw_processes();
> +
> + Finish:
> +	return error;

Heh, the original ?: code was scary.  That said, IMHO, the above seems
a bit too verbose.  Functions usually shouldn't have side effects on
failure and the above code is usual in that sense and it would be nice
to mention the reformatting (from cryptic to sane) in the commit log.
Also, how many columns are you flowing the text to?  It seems quite
narrower than they usually should be.

Thanks.

-- 
tejun