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@xxxxxxxx>
SUSE Labs, CR
