From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 6/8] bdi: add a new writeback list for sync Date: Wed, 17 Jun 2015 12:34:19 +0200 Message-ID: <20150617103419.GE1614@quack.suse.cz> References: <1434051673-13838-1-git-send-email-jbacik@fb.com> <1434051673-13838-7-git-send-email-jbacik@fb.com> <20150615141221.GH4368@quack.suse.cz> <558043E3.9070006@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kara , linux-fsdevel@vger.kernel.org, kernel-team@fb.com, viro@ZenIV.linux.org.uk, hch@infradead.org, david@fromorbit.com, Dave Chinner To: Josef Bacik Return-path: Received: from cantor2.suse.de ([195.135.220.15]:39165 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752695AbbFQKeY (ORCPT ); Wed, 17 Jun 2015 06:34:24 -0400 Content-Disposition: inline In-Reply-To: <558043E3.9070006@fb.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue 16-06-15 08:42:27, Josef Bacik wrote: > >> /* > >>- * Wait for writeback on an inode to complete. Caller must have inode pinned. > >>+ * Wait for writeback on an inode to complete during eviction. Caller must have > >>+ * inode pinned. > >> */ > >> void inode_wait_for_writeback(struct inode *inode) > >> { > >>+ BUG_ON(!(inode->i_state & I_FREEING)); > >>+ > >> spin_lock(&inode->i_lock); > >> __inode_wait_for_writeback(inode); > >> spin_unlock(&inode->i_lock); > >>+ > >>+ /* > >>+ * bd_inode's will have already had the bdev free'd so don't bother > >>+ * doing the bdi_clear_inode_writeback. > >>+ */ > >>+ if (!sb_is_blkdev_sb(inode->i_sb)) > >>+ bdi_clear_inode_writeback(inode_to_bdi(inode), inode); > >> } > > > >Why do we bother with not putting bdev inode back? > > > > My memory is rusty on this, but if the inode is the inode for a bdev > we will have already free'd the bdev at this point and we get a null > pointer deref, so this just skips that bit. Ah, the reason likely is that bdev->bd_disk is NULL (already cleaned up in __blkdev_put()) at this moment and thus bdev_get_queue() called from inode_to_bdi() will oops. Can you please add these details to the comment? It's a bit subtle... Also we shouldn't have any pages in the block device mapping anymore because of the work done in __blkdev_put() (and thus inode shouldn't be in the writeback list) but I'd be calmer if we asserted list_empty(&inode->i_wb_list). Can you please add that? Thanks! Honza -- Jan Kara SUSE Labs, CR