Re: [PATCH] writeback: Fix wakeup and logging timeouts for !DETECT_HUNG_TASK

[Jan Kara]

On Sat 31-01-26 17:07:24, Huacai Chen wrote:
> Recent changes of fs-writeback cause such warnings if DETECT_HUNG_TASK
> is not enabled:
> 
> INFO: The task sync:1342 has been waiting for writeback completion for more than 1 seconds.
> 
> The reason is sysctl_hung_task_timeout_secs is 0 when DETECT_HUNG_TASK
> is not enabled, then it causes the warning message even if the writeback
> lasts for only one second.
> 
> I believe the wakeup and logging is also useful for !DETECT_HUNG_TASK,
> so I don't want to disable them completely. As DEFAULT_HUNG_TASK_TIMEOUT
> is 120 seconds, so for the !DETECT_HUNG_TASK case let's use 120 seconds
> instead of sysctl_hung_task_timeout_secs.
> 
> Fixes: 1888635532fb ("writeback: Wake up waiting tasks when finishing the writeback of a chunk.")
> Fixes: d6e621590764 ("writeback: Add logging for slow writeback (exceeds sysctl_hung_task_timeout_secs)")
> Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>

Thanks for the patch! I think if !CONFIG_DETECT_HUNG_TASK, we should just
not print the message from wb_wait_for_completion_cb() as well. After all
it's also a type of hung task detection and user explicitely disabled that.
Also there would be no way to tune the timeout so there are high chances
120s will be too much for somebody and too few for somebody else...

> +#ifndef CONFIG_DETECT_HUNG_TASK
> +	unsigned long hung_secs = 120;
> +#else
> +	unsigned long hung_secs = sysctl_hung_task_timeout_secs;
> +#endif
>  
>  	if (work->for_kupdate)
>  		dirtied_before = jiffies -
> @@ -2031,8 +2041,7 @@ static long writeback_sb_inodes(struct super_block *sb,
>  
>  		/* Report progress to inform the hung task detector of the progress. */
>  		if (work->done && work->done->progress_stamp &&
> -		   (jiffies - work->done->progress_stamp) > HZ *
> -		   sysctl_hung_task_timeout_secs / 2)
> +		   (jiffies - work->done->progress_stamp) > HZ * hung_secs / 2)
>  			wake_up_all(work->done->waitq);
>  
>  		wbc_detach_inode(&wbc);

Similarly here I'd just #ifdef out the wakeup when !CONFIG_DETECT_HUNG_TASK.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR