Re: [PATCH 1/3] writeback: Avoid skipping inode writeback

[Jan Kara <jack@suse.cz>]

On Wed 10-06-20 08:02:03, Christoph Hellwig wrote:
> This generall looks ok, but a few nitpicks below:
> 
> > -static void redirty_tail(struct inode *inode, struct bdi_writeback *wb)
> > +static void __redirty_tail(struct inode *inode, struct bdi_writeback *wb)
> 
> I think redirty_tail_locked would be a more decriptive name, and also
> fit other uses in this file (e.g. inode_io_list_move_locked and
> inode_io_list_del_locked).

Fair enough, will do.

> >  {
> > +	assert_spin_locked(&inode->i_lock);
> >  	if (!list_empty(&wb->b_dirty)) {
> 
> Nit: I find an empty line after asserts and before the real code starts
> nice on the eye.

Sure.

> >  			break;
> >  		list_move(&inode->i_io_list, &tmp);
> >  		moved++;
> > +		spin_lock(&inode->i_lock);
> >  		if (flags & EXPIRE_DIRTY_ATIME)
> > -			set_bit(__I_DIRTY_TIME_EXPIRED, &inode->i_state);
> > +			inode->i_state |= I_DIRTY_TIME_EXPIRED;
> > +		inode->i_state |= I_SYNC_QUEUED;
> > +		spin_unlock(&inode->i_lock);
> 
> I wonder if the locking changes should go into a prep patch vs the
> actual logic changes related to I_SYNC_QUEUED?  That would untangle
> the patch quite a bit and make it easier to follow.

OK, will do.
 
> >  #define I_WB_SWITCH		(1 << 13)
> >  #define I_OVL_INUSE		(1 << 14)
> >  #define I_CREATING		(1 << 15)
> > +#define I_SYNC_QUEUED		(1 << 16)
> 
> FYI, this conflicts with the I_DONTCAT addition in mainline now.

Yup, I've already found out when rebasing...

Thanks for review!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR