* Re: locking (or LOCKDEP) problem with mark_buffer_dirty()
2021-01-14 15:25 ` locking (or LOCKDEP) problem with mark_buffer_dirty() Bob Peterson
@ 2021-01-15 22:18 ` Dave Chinner
0 siblings, 0 replies; 2+ messages in thread
From: Dave Chinner @ 2021-01-15 22:18 UTC (permalink / raw)
To: Bob Peterson; +Cc: linux-fsdevel, tj, Andreas Gruenbacher
On Thu, Jan 14, 2021 at 10:25:56AM -0500, Bob Peterson wrote:
> Hi Tejun and linux-fsdevel,
>
> I have a question about function mark_buffer_dirty and LOCKDEP.
>
> Background: Func mark_buffer_dirty() has a calling sequence that looks kind
> of like this (simplified):
>
> mark_buffer_dirty()
> __set_page_dirty()
> account_page_dirtied()
> inode_to_wb() which contains:
> #ifdef CONFIG_LOCKDEP
> WARN_ON_ONCE(debug_locks &&
> (!lockdep_is_held(&inode->i_lock) &&
> !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
> !lockdep_is_held(&inode->i_wb->list_lock)));
> #endif
inode_to_wb() gets called:
- under the xa_lock from page accounting (such as the above path),
- under the inode->i_lock from wbc_attach_and_unlock_inode() and
other places,
- and wb->list_lock is what protects the writeback list that the
inode is on, so it held whenever the inode is added/removed from a
writeback list.
Essentially, one of those locks has to be held to keep inode_to_wb()
stable and valid. The function inode_switch_wbs_work_fn() explains
this in a roundabout way:
.....
* Grabbing old_wb->list_lock, inode->i_lock and the i_pages lock
* gives us exclusion against all wb related operations on @inode
* including IO list manipulations and stat updates.
*/
So, essentially, inode_to_wb() is checking at least one of these
locks is held when it is called.
In the above case, __set_page_dirty() takes the xa_lock before
calling account_page_dirtied(), so the lockdep warning should not
ever fire in this path. It also means that you can't hold the
xa_lock when calling mark_buffer_dirty()....
> ...
> __mark_inode_dirty()
> spin_lock(&inode->i_lock);
> ...
> spin_unlock(&inode->i_lock);
> ...
__mark_inode_dirty() also takes the wb->list_lock (via
locked_inode_to_wb_and_lock_list() because the inode->i_lock is held
so the lockdep check won't fire). Hence you can't hold the
wb->list_lock when calling mark_buffer_dirty() either.
> The LOCKDEP checks were added with Tejun Heo's 2015 patch, aaa2cacf8184e2a92accb8e443b1608d65f9a13f.
>
> Since mark_buffer_dirty()'s call to __mark_inode_dirty() locks the inode->i_lock,
> functions must not call mark_buffer_dirty() with inode->i_lock locked: or deadlock.
You can't hold any of those three locks inode_to_wb() checks when calling
mark_buffer_dirty(), nor should you. And, AFAICT,
mark_buffer_dirty() is doing all the right locking, so maybe there's
something else going on here that isn't actually a bug in
mark_buffer_dirty().
> If they're not doing anything with the xarrays or the i_wb list (i.e. holding the
> other two locks), they get these LOCKDEP warnings.
>
> So either:
> (a) the LOCKDEP warnings are not valid in all cases -or-
> (b) mark_buffer_dirty() should be grabbing inode->i_lock at some point like __mark_inode_dirty() does.
> My question is: which is it, a or b? TIA.
c) something else?
Perhaps you've got some other inode->i_lock locking bug, and this is
the first place that happens to notice it? Or perhap lockdep itself
has been broken again?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 2+ messages in thread