locking (or LOCKDEP) problem with mark_buffer_dirty()

locking (or LOCKDEP) problem with mark_buffer_dirty()

[Bob Peterson]

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
   ...
   __mark_inode_dirty()
      spin_lock(&inode->i_lock);
      ...
      spin_unlock(&inode->i_lock);
   ...      

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.

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.

(My situation is that the gfs2 file system gets these LOCKDEP warnings
when it calls mark_buffer_dirty() [obviously only if LOCKDEP is set], and it's
not appropriate to lock xa_lock or i_wb->list_lock, and we cannot lock i_lock
for the reasons stated above).

Regards,

Bob Peterson
GFS2 File System

Re: locking (or LOCKDEP) problem with mark_buffer_dirty()

[Dave Chinner]

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