Re: [PATCH 06/11] bdi: add a new writeback list for sync

[Dave Chinner <david@fromorbit.com>]

On Wed, Jul 31, 2013 at 05:11:14PM +0200, Jan Kara wrote:
> On Wed 31-07-13 14:15:45, Dave Chinner wrote:
> >  /*
> > + * mark an inode as under writeback on the given bdi
> > + */
> > +void bdi_mark_inode_writeback(struct backing_dev_info *bdi, struct inode *inode)
> > +{
> > +	WARN_ON_ONCE(bdi != inode_to_bdi(inode));
> > +	if (list_empty(&inode->i_wb_list)) {
> > +		spin_lock(&bdi->wb.list_lock);
> > +		if (list_empty(&inode->i_wb_list))
> > +			list_add_tail(&inode->i_wb_list, &bdi->wb.b_writeback);
> > +		spin_unlock(&bdi->wb.list_lock);
> > +	}
> > +}
> > +
> > +/*
> > + * clear an inode as under writeback on the given bdi
> > + */
> > +static void bdi_clear_inode_writeback(struct backing_dev_info *bdi,
> > +				      struct inode *inode)
> > +{
> > +	WARN_ON_ONCE(bdi != inode_to_bdi(inode));
> > +	if (!list_empty(&inode->i_wb_list)) {
> > +		spin_lock(&bdi->wb.list_lock);
> > +		list_del_init(&inode->i_wb_list);
> > +		spin_unlock(&bdi->wb.list_lock);
> > +	}
> > +}
>   Umm, are these list_empty() checks without lock really safe?

I think they are.....

>   Looking into
> the code in more detail, it seems that mapping->tree_lock saves us from
> races between insert & removal but it definitely deserves a comment (or maybe
> even an assertion) that the function requires it.
> bdi_clear_inode_writeback() is safe only because it is called only when the
> inode is practically dead. Again, I think it deserves a comment...

Ok.

> > @@ -1264,9 +1330,21 @@ static void wait_sb_inodes(struct super_block *sb)
> >  
> >  		cond_resched();
> >  
> > -		spin_lock(&sb->s_inode_list_lock);
> > +                /*
> > +                 * the inode has been written back now, so check whether we
> > +                 * still have pages under IO and move it back to the primary
> > +                 * list if necessary
> > +                 */
> > +                spin_lock_irq(&mapping->tree_lock);
> > +                spin_lock(&bdi->wb.list_lock);
> > +                if (mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK)) {
> > +                        WARN_ON(list_empty(&inode->i_wb_list));
> > +                        list_move(&inode->i_wb_list, &bdi->wb.b_writeback);
> > +                } else
> > +			list_del_init(&inode->i_wb_list);
> > +                spin_unlock_irq(&mapping->tree_lock);
>   Whitespace is damaged in the above hunk...

Bizarre. I'll fix it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com