From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933240AbcFOOzc (ORCPT ); Wed, 15 Jun 2016 10:55:32 -0400 Received: from mail-qk0-f194.google.com ([209.85.220.194]:35269 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932370AbcFOOz1 (ORCPT ); Wed, 15 Jun 2016 10:55:27 -0400 Date: Wed, 15 Jun 2016 10:55:13 -0400 From: Tejun Heo To: Jan Kara Cc: Tahsin Erdogan , Jens Axboe , Alexander Viro , Jan Kara , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] writeback: inode cgroup wb switch should skip inode with zero i_count Message-ID: <20160615145513.GA24102@mtj.duckdns.org> References: <1465857429-8405-1-git-send-email-tahsin@google.com> <20160615122640.GC1607@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160615122640.GC1607@quack2.suse.cz> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, Jan. On Wed, Jun 15, 2016 at 02:26:40PM +0200, Jan Kara wrote: > Ugh, this looks ugly. Inode with i_count == 0 without I_FREEING set is > sitting in inode LRU list. It may get reused at which point it would be > actually good if it switched WB to the good one, no? Yes, that'd be better but the switching is heuristics driven best effort thing anyway, so occasionally failing to switch isn't critical. > Since we actually hold i_lock and have checked the inode is not being > freed, we can just use __iget() to grab the inode reference. That avoids > the warning and fixes the race as well. Something like: > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 989a2ce..b44ede0 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -478,14 +478,15 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) > goto out_free; > } > inode->i_state |= I_WB_SWITCH; > + __iget(inode); > spin_unlock(&inode->i_lock); > > - ihold(inode); > isw->inode = inode; > > atomic_inc(&isw_nr_in_flight); That said, the above looks better. Thanks! -- tejun