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);
> >
> > (and initialize wb to NULL to make sure it does not contain stale value).
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?
If run with the logic in the comments, wb will only be initialized
when was_dirty != 0,
suppose was_dirty is 0, wb will not be initialized, and continue
running, will hit
if (!was_dirty) {
dirty_list = &wb->b_dirty_time;
}
will hit NULL pointer.
Is there something I have missed?
>
> > @@ -2409,7 +2414,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> > * list, based upon its state.
> > */
> > if (inode->i_state & I_SYNC_QUEUED)
> > - goto out_unlock_inode;
> > + goto out_unlock;
> >
> > /*
> > * Only add valid (hashed) inodes to the superblock's
> > @@ -2417,22 +2422,19 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> > */
> > if (!S_ISBLK(inode->i_mode)) {
> > if (inode_unhashed(inode))
> > - goto out_unlock_inode;
> > + goto out_unlock;
> > }
> > if (inode->i_state & I_FREEING)
> > - goto out_unlock_inode;
> > + goto out_unlock;
> >
> > /*
> > * If the inode was already on b_dirty/b_io/b_more_io, don't
> > * reposition it (that would break b_dirty time-ordering).
> > */
> > if (!was_dirty) {
> > - struct bdi_writeback *wb;
> > struct list_head *dirty_list;
> > bool wakeup_bdi = false;
> >
> > - wb = locked_inode_to_wb_and_lock_list(inode);
> > -
> > inode->dirtied_when = jiffies;
> > if (dirtytime)
> > inode->dirtied_time_when = jiffies;
> > @@ -2446,6 +2448,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> > dirty_list);
> >
> > spin_unlock(&wb->list_lock);
> > + spin_unlock(&inode->i_lock);
> > trace_writeback_dirty_inode_enqueue(inode);
> >
> > /*
> > @@ -2460,6 +2463,8 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> > return;
> > }
> > }
>
> > > +out_unlock:
> > > + spin_unlock(&wb->list_lock);
Ouch, this is so obvious now that you mention it. Really stupid
mistake on my side. It surprised me that local compile do not have warnings..
>
> wb->list lock will not be locked in some cases here. So you have to be more
> careful about when you need to unlock it. Probably something like:
>
> if (wb)
> spin_unlock(&wb->list_lock);
>
> and you can put this at the end inside the block "if ((inode->i_state &
> flags) != flags)".
>
>
> > Also I'd note it is good to test your changes (it would likely uncover the
> > locking problem). For these filesystem related things xfstests are useful:
> >
> > https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/
Thanks a lot! I'll test this patch for ext4 fs using this tool.
>
> Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
