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@xxxxxxxxxx>
> Cc: Dennis Zhou <dennis@xxxxxxxxxx>
> Cc: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
Thanks,
Dennis
