Re: [PATCH 6/8] bdi: add a new writeback list for sync

[Jan Kara <jack@suse.cz>]

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 <jack@suse.cz>
SUSE Labs, CR