Re: [PATCH 22/24] NFS: add a separate delegation return list

[Christoph Hellwig <hch@lst.de>]

On Sun, Jan 25, 2026 at 05:30:07AM -0800, Chris Mason wrote:
> > +	spin_lock(&server->delegations_lock);
> > +	list_for_each_entry_rcu(d, &server->delegations_return, entry) {
> > +		if (test_bit(NFS_DELEGATION_RETURN_DELAYED, &d->flags))
> > +			clear_bit(NFS_DELEGATION_RETURN_DELAYED, &d->flags);
> >  		ret = true;
> >  	}
> > -out:
> > +
> >  	return ret;
> >  }
> 
> Is there a missing spin_unlock() here? The spin_lock() is taken but
> the function returns without releasing it. This appears to cause a
> deadlock when delegations_lock is next acquired.

Yes.  I'm kinda surprised the sparse lock context tracking didn't
catch it, but maybe it gives up after multiple 'bad constant expression'
from the module parameters.

> > +	if (test_bit(NFS_DELEGATION_RETURN_DELAYED, &delegation->flags) ||
> > +	    test_bit(NFS_DELEGATION_REVOKED, &delegation->flags) ||
> > +	    test_and_set_bit(NFS_DELEGATION_RETURNING, &delegation->flags)) {
> > +		spin_unlock(&delegation->lock);
> > +		goto out_put_inode;
> > +	}
> 
> When a delegation has RETURN_DELAYED set, the code removes it from
> delegations_return (via list_del_init above) and then skips to
> out_put_inode without re-adding it to the list. Since
> nfs_server_clear_delayed_delegations() only iterates delegations_return,
> will this delegation ever have its RETURN_DELAYED flag cleared so it
> can be processed again?
> 
> The scenario is: nfs_end_delegation_return() returns -EAGAIN, which
> sets RETURN_DELAYED via nfs_abort_delegation_return(), then
> nfs_mark_return_delegation() re-adds the delegation to
> delegations_return. On the next iteration of the loop, this check
> triggers and the delegation is dropped from the list without being
> re-queued.

This looks like a mess before and after.  I think the best
idea is a separate list for the delayed returns, but I really
need to find a way to actually generate the delayed returns first.
I'll look into that, thanks!