From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6C773CA9EA0 for ; Tue, 22 Oct 2019 08:25:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4E0E321920 for ; Tue, 22 Oct 2019 08:25:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388170AbfJVIZZ (ORCPT ); Tue, 22 Oct 2019 04:25:25 -0400 Received: from mx2.suse.de ([195.135.220.15]:38430 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S2388061AbfJVIZZ (ORCPT ); Tue, 22 Oct 2019 04:25:25 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 6F466B314; Tue, 22 Oct 2019 08:25:23 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id 9B09F1E4812; Tue, 22 Oct 2019 10:15:51 +0200 (CEST) Date: Tue, 22 Oct 2019 10:15:51 +0200 From: Jan Kara To: Roman Gushchin Cc: Jan Kara , "linux-mm@kvack.org" , "linux-fsdevel@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Kernel Team , "tj@kernel.org" , Dennis Zhou Subject: Re: [PATCH v2] cgroup, blkcg: prevent dirty inodes to pin dying memory cgroups Message-ID: <20191022081551.GD2436@quack2.suse.cz> References: <20191010234036.2860655-1-guro@fb.com> <20191015090933.GA21104@quack2.suse.cz> <20191015214041.GA24736@tower.DHCP.thefacebook.com> <20191016091840.GC30337@quack2.suse.cz> <20191021234858.GA16251@castle> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191021234858.GA16251@castle> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Mon 21-10-19 23:49:04, Roman Gushchin wrote: > On Wed, Oct 16, 2019 at 11:18:40AM +0200, Jan Kara wrote: > > On Tue 15-10-19 21:40:45, Roman Gushchin wrote: > > > On Tue, Oct 15, 2019 at 11:09:33AM +0200, Jan Kara wrote: > > > > On Thu 10-10-19 16:40:36, Roman Gushchin wrote: > > > > > > > > > @@ -426,7 +431,7 @@ static void inode_switch_wbs_work_fn(struct work_struct *work) > > > > > if (!list_empty(&inode->i_io_list)) { > > > > > struct inode *pos; > > > > > > > > > > - inode_io_list_del_locked(inode, old_wb); > > > > > + inode_io_list_del_locked(inode, old_wb, false); > > > > > inode->i_wb = new_wb; > > > > > list_for_each_entry(pos, &new_wb->b_dirty, i_io_list) > > > > > if (time_after_eq(inode->dirtied_when, > > > > > > > > This bit looks wrong. Not the change you made as such but the fact that you > > > > can now move inode from b_attached list of old wb to the dirty list of new > > > > wb. > > > > > > Hm, can you, please, elaborate a bit more why it's wrong? > > > The reference to the old_wb will be dropped by the switching code. > > > > My point is that the code in full looks like: > > > > if (!list_empty(&inode->i_io_list)) { > > struct inode *pos; > > > > inode_io_list_del_locked(inode, old_wb); > > inode->i_wb = new_wb; > > list_for_each_entry(pos, &new_wb->b_dirty, i_io_list) > > if (time_after_eq(inode->dirtied_when, > > pos->dirtied_when)) > > break; > > inode_io_list_move_locked(inode, new_wb, pos->i_io_list.prev); > > } else { > > > > So inode is always moved from some io list in old_wb to b_dirty list of > > new_wb. This is fine when it could be only on b_dirty, b_io, b_more_io lists > > of old_wb. But once you add b_attached list to the game, it is not correct > > anymore. You should not add clean inode to b_dirty list of new_wb. > > I see... > > Hm, will checking of i_state for not containing I_DIRTY_ALL bits be enough here? > Alternatively, I can introduce a new bit which will explicitly point at the > inode being on the b_attached list, but I'd prefer not to do it. Yeah, keying of i_state should work. And while we are at it, we could also correctly handle I_DIRTY_TIME case and move inode only to b_dirty_time list. That seems to be (mostly harmless) preexisting issue. Honza -- Jan Kara SUSE Labs, CR