Re: [PATCH block/for-linus] cgroup,writeback: don't switch wbs immediately on dead wbs if the memcg is dead

[Dennis Zhou <dennis@kernel.org>]

On Fri, Nov 08, 2019 at 12:18:29PM -0800, Tejun Heo wrote:
> cgroup writeback tries to refresh the associated wb immediately if the
> current wb is dead.  This is to avoid keeping issuing IOs on the stale
> wb after memcg - blkcg association has changed (ie. when blkcg got
> disabled / enabled higher up in the hierarchy).
> 
> Unfortunately, the logic gets triggered spuriously on inodes which are
> associated with dead cgroups.  When the logic is triggered on dead
> cgroups, the attempt fails only after doing quite a bit of work
> allocating and initializing a new wb.
> 
> While c3aab9a0bd91 ("mm/filemap.c: don't initiate writeback if mapping
> has no dirty pages") alleviated the issue significantly as it now only
> triggers when the inode has dirty pages.  However, the condition can
> still be triggered before the inode is switched to a different cgroup
> and the logic simply doesn't make sense.
> 
> Skip the immediate switching if the associated memcg is dying.
> 
> This is a simplified version of the following two patches:
> 
>  * https://lore.kernel.org/linux-mm/20190513183053.GA73423@dennisz-mbp/
>  * http://lkml.kernel.org/r/156355839560.2063.5265687291430814589.stgit@buzz
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Dennis Zhou <dennis@kernel.org>
> Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Fixes: e8a7abf5a5bd ("writeback: disassociate inodes from dying bdi_writebacks")
> ---
>  fs/fs-writeback.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 8461a6322039..335607b8c5c0 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -576,10 +576,13 @@ void wbc_attach_and_unlock_inode(struct writeback_control *wbc,
>  	spin_unlock(&inode->i_lock);
>  
>  	/*
> -	 * A dying wb indicates that the memcg-blkcg mapping has changed
> -	 * and a new wb is already serving the memcg.  Switch immediately.
> +	 * A dying wb indicates that either the blkcg associated with the
> +	 * memcg changed or the associated memcg is dying.  In the first
> +	 * case, a replacement wb should already be available and we should
> +	 * refresh the wb immediately.  In the second case, trying to
> +	 * refresh will keep failing.
>  	 */
> -	if (unlikely(wb_dying(wbc->wb)))
> +	if (unlikely(wb_dying(wbc->wb) && !css_is_dying(wbc->wb->memcg_css)))
>  		inode_switch_wbs(inode, wbc->wb_id);
>  }
>  EXPORT_SYMBOL_GPL(wbc_attach_and_unlock_inode);

Acked-by: Dennis Zhou <dennis@kernel.org>

Thanks,
Dennis