From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 7/7] writeback: Avoid iput() from flusher thread Date: Mon, 30 Apr 2012 23:58:55 +0200 Message-ID: <20120430215855.GD10105@quack.suse.cz> References: <1332284191-21076-1-git-send-email-jack@suse.cz> <1332284191-21076-8-git-send-email-jack@suse.cz> <20120430153027.GA24324@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kara , Wu Fengguang , linux-fsdevel@vger.kernel.org, Al Viro To: Christoph Hellwig Return-path: Received: from cantor2.suse.de ([195.135.220.15]:36074 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756454Ab2D3V66 (ORCPT ); Mon, 30 Apr 2012 17:58:58 -0400 Content-Disposition: inline In-Reply-To: <20120430153027.GA24324@infradead.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon 30-04-12 11:30:27, Christoph Hellwig wrote: > > - * Wait for writeback on an inode to complete. > > + * Wait for writeback on an inode to complete. Called with i_lock held. > > + * Caller must make sure inode cannot go away when we drop i_lock. > > */ > > -static void inode_wait_for_writeback(struct inode *inode) > > +void inode_wait_for_writeback(struct inode *inode) > > { > > DEFINE_WAIT_BIT(wq, &inode->i_state, __I_SYNC); > > wait_queue_head_t *wqh; > > @@ -340,6 +341,26 @@ static void inode_wait_for_writeback(struct inode *inode) > > } > > } > > This needs way better documentation, preferably using kernel doc. OK, will improve. > I also suspect we should rename inode_wait_for_writeback to > __inode_wait_for_writeback and make inode_wait_for_writeback the public > wrapper that takes i_lock. Good idea. Will do. > > diff --git a/fs/inode.c b/fs/inode.c > > index d3ebdbe..3869714 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -510,7 +510,6 @@ void end_writeback(struct inode *inode) > > BUG_ON(!list_empty(&inode->i_data.private_list)); > > BUG_ON(!(inode->i_state & I_FREEING)); > > BUG_ON(inode->i_state & I_CLEAR); > > - inode_sync_wait(inode); > > /* don't need i_lock here, no concurrent mods to i_state */ > > inode->i_state = I_FREEING | I_CLEAR; > > } > > end_writeback is really misnamed now and needs a better name. Yes. I think old good clear_inode() may be fine? The only thing we really *do* in that function is setting of I_CLEAR. I guess I'll do the rename in one big jumbo patch. Would that be OK? I don't want to spam with 50 one-line patches... Honza -- Jan Kara SUSE Labs, CR