Re: inconsistent lock state in the new fserror code

[Dave Chinner <dgc@kernel.org>]

On Fri, Feb 13, 2026 at 11:07:57AM -0800, Darrick J. Wong wrote:
> On Fri, Feb 13, 2026 at 08:00:41AM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 12, 2026 at 10:15:57PM -0800, Christoph Hellwig wrote:
> > > [  149.498163] other info that might help us debug this:
> > > [  149.498163]  Possible unsafe locking scenario:
> > > [  149.498163] 
> > > [  149.498163]        CPU0
> > > [  149.498163]        ----
> > > [  149.498163]   lock(&sb->s_type->i_lock_key#33);
> > > [  149.498163]   <Interrupt>
> > > [  149.498163]     lock(&sb->s_type->i_lock_key#33);
> > 
> > Er... is lockdep telling us here that we could take i_lock in
> > unlock_new_inode, get interrupted, and then take another i_lock?

Yes.

> > > [  149.498163] 
> > > [  149.498163]  *** DEADLOCK ***
> > > [  149.498163] 
> > > [  149.498163] 1 lock held by swapper/1/0:
> > > [  149.498163]  #0: ffff8881052c81a0 (&vblk->vqs[i].lock){-.-.}-{3:3}, at: virtblk_done+0x4b/0x110
> > > [  149.498163] 
> > > [  149.498163] stack backtrace:
> > > [  149.498163] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Tainted: G                 N  6.19.0+ #4827 PREEMPT(full) 
> > > [  149.498163] Tainted: [N]=TEST
> > > [  149.498163] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014
> > > [  149.498163] Call Trace:
> > > [  149.498163]  <IRQ>
> > > [  149.498163]  dump_stack_lvl+0x5b/0x80
> > > [  149.498163]  print_usage_bug.part.0+0x22c/0x2c0
> > > [  149.498163]  mark_lock+0xa6f/0xe90
> > > [  149.498163]  __lock_acquire+0x10b6/0x25e0
> > > [  149.498163]  lock_acquire+0xca/0x2c0
> > > [  149.498163]  _raw_spin_lock+0x2e/0x40
> > > [  149.498163]  igrab+0x1a/0xb0
> > > [  149.498163]  fserror_report+0x135/0x260
> > > [  149.498163]  iomap_finish_ioend_buffered+0x170/0x210
> > > [  149.498163]  clone_endio+0x8f/0x1c0
> > > [  149.498163]  blk_update_request+0x1e4/0x4d0
> > > [  149.498163]  blk_mq_end_request+0x1b/0x100
> > > [  149.498163]  virtblk_done+0x6f/0x110
> > > [  149.498163]  vring_interrupt+0x59/0x80

Ok, so why are we calling iomap_finish_ioend_buffered() from IRQ
context? That looks like a bug because the only IO completion call
chain that can get into iomap_finish_ioend_buffered() is supposedly:

iomap_finish_ioends
  iomap_finish_ioend
    iomap_finish_ioend_buffered

And the comment above iomap_finish_ioends() says:

/*
 * Ioend completion routine for merged bios. This can only be called from task
 * contexts as merged ioends can be of unbound length. Hence we have to break up
 * the writeback completions into manageable chunks to avoid long scheduler
 * holdoffs. We aim to keep scheduler holdoffs down below 10ms so that we get
 * good batch processing throughput without creating adverse scheduler latency
 * conditions.
 */

Ah, there's the problem - pure buffered overwrites from XFS use
ioend_writeback_end_bio(), not xfs_end_bio(). Hence the buffered
write completion is not punted to a workqueue, and it calls
iomap_finish_ioend_buffered() direct from the bio completion
context.

Yeah, that seems like a bug that needs fixing in the
ioend_writeback_end_bio() function - if there's an IO error, it
needs to punt the processing of the ioend to a workqueue...

> > > [  149.498163]  __handle_irq_event_percpu+0x8a/0x2e0
> > > [  149.498163]  handle_irq_event+0x33/0x70
> > > [  149.498163]  handle_edge_irq+0xdd/0x1e0
> > > [  149.498163]  __common_interrupt+0x6f/0x180
> > > [  149.498163]  common_interrupt+0xb7/0xe0
> > 
> > Hrmm, so we're calling fserror_report/igrab from an interrupt handler.
> > The bio endio function is for writeback ioend completion.

Yup, this is one of the reasons writeback doesn't hold an inode
reference over IO - we can't call iput() from an interrupt context.

> > igrab takes i_lock to check if the inode is in FREEING or WILL_FREE
> > state.  However, the fact that it's in writeback presumably means that
> > the vfs still holds an i_count on this inode,

Writeback holds an inode reference over submission only.

> > so the inode cannot be
> > freed until iomap_finish_ioend_buffered completes.

iput()->iput_final()->evict will block in inode_wait_for_writeback()
waiting for outstanding writeback to complete before it starts
tearing down the inode. This isn't controlled by reference counts.

> /me hands himself another cup of coffee, changes that to:
> 
> 	/*
> 	 * Can't iput from non-sleeping context, so grabbing another
> 	 * reference to the inode must be the last thing before
> 	 * submitting the event.  Open-code the igrab here to avoid
> 	 * taking i_lock in interrupt context.
> 	 */
> 	if (inode) {
> 		WARN_ON_ONCE(inode_unhashed(inode));
> 		WARN_ON_ONCE(inode_state_read_once(inode) &
> 					(I_NEW | I_FREEING | I_WILL_FREE));

It is valid for the inode have a zero reference count and have either
I_FREEING or I_WILL_FREE set here if another task has dropped the
final inode reference while writeback IO is still in flight.

> 		if (!atomic_inc_not_zero(&inode->i_count))
> 			goto lost_event;

Overall, I'm not sure using atomic_inc_not_zero() is safe here. It
may be, but I don't think this is how the problem should be solved.
Punt ioend w/ IO errors to a work queue, and then nothing needs to
change w.r.t. the fserror handling of the inodes. i.e. it will be
save to use inode->i_lock and hence igrab()...

Cheers,

Dave.
-- 
Dave Chinner
dgc@kernel.org