On Thu 05-05-22 12:45:56, Jchao sun wrote:
> On Thu, May 5, 2022 at 3:38 AM Jan Kara <jack@xxxxxxx> wrote:
> >
> > On Wed 04-05-22 11:25:14, Jchao Sun wrote:
> > > Commit b35250c0816c ("writeback: Protect inode->i_io_list with
> > > inode->i_lock") made inode->i_io_list not only protected by
> > > wb->list_lock but also inode->i_lock, but inode_io_list_move_locked()
> > > was missed. Add lock there and also update comment describing things
> > > protected by inode->i_lock.
> > >
> > > Fixes: b35250c0816c ("writeback: Protect inode->i_io_list with
> inode->i_lock")
> > > Signed-off-by: Jchao Sun <sunjunchao2870@xxxxxxxxx>
> >
> > Almost there :). A few comments below:
> >
> > > @@ -2402,6 +2404,9 @@ void __mark_inode_dirty(struct inode *inode, int
> flags)
> > > inode->i_state &= ~I_DIRTY_TIME;
> > > inode->i_state |= flags;
> > >
> > > + wb = locked_inode_to_wb_and_lock_list(inode);
> > > + spin_lock(&inode->i_lock);
> > > +
> >
>
> > > We don't want to lock wb->list_lock if the inode was already dirty (which
> > > is a common path). So you want something like:
> > >
> > > if (was_dirty)
> > > wb = locked_inode_to_wb_and_lock_list(inode);
>
> I'm a little confused about here. The logic of the current source tree is
> like this:
> if (!was_dirty) {
> struct bdi_writeback *wb;
> wb =
> locked_inode_to_wb_and_lock_list(inode);
> ...
> dirty_list = &wb-> b_dirty_time;
> assert_spin_locked(&wb->list_lock);
> }
> The logic is the opposite of the logic in the comments, and it seems like
> that wb will
> absolutely not be NULL.
> Why is this? What is the difference between them?
Sorry, that was a typo in my suggestion. It should have been
if (!was_dirty)
wb = locked_inode_to_wb_and_lock_list(inode);
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
