SUNRPC: Restore missing synchronization on transport_lock

[dinhngtu]

Recent crashes revealed a pattern of locking issues around xprt_prepare_transmit. LLM identified 242a3e0 "SUNRPC: avoid race between mod_timer() and del_timer_sync()" as a missing backport related to this deadlock.

On top of that, the removal of the transport_lock synchronization around xs_tcp_state_change/TCP_ESTABLISHED in "XSI-2150: Avoid deadlocks on NFS spinlocks" came into question. This synchronization was first removed in b5e9241 "SUNRPC: Remove the bh-safe lock requirement on xprt->transport_lock", but it was never backported in our kernel. Notably, CIP retains the lock around this area.

Example traces:

WARN:  queued_spin_lock_slowpath+0x7/0x17
WARN:  xprt_prepare_transmit+0x36/0x100 [sunrpc]
WARN:  ? call_transmit_status+0x140/0x140 [sunrpc]
WARN:  call_transmit+0x3e/0x290 [sunrpc]
WARN:  ? call_transmit_status+0x140/0x140 [sunrpc]
WARN:  __rpc_execute+0x80/0x3f0 [sunrpc]
WARN:  process_one_work+0x165/0x370
WARN:  worker_thread+0x49/0x3e0
WARN:  kthread+0xf8/0x130
WARN:  ? rescuer_thread+0x310/0x310
WARN:  ? kthread_bind+0x10/0x10
WARN:  ret_from_fork+0x1f/0x40

WARN:  _raw_spin_unlock_bh+0xa/0x20
WARN:  xprt_prepare_transmit+0x9f/0x100 [sunrpc]
WARN:  ? call_transmit_status+0x140/0x140 [sunrpc]
WARN:  call_transmit+0x3e/0x290 [sunrpc]
WARN:  ? call_transmit_status+0x140/0x140 [sunrpc]
WARN:  __rpc_execute+0x80/0x3f0 [sunrpc]
WARN:  process_one_work+0x165/0x370
WARN:  worker_thread+0x49/0x3e0
WARN:  kthread+0xf8/0x130
WARN:  ? rescuer_thread+0x310/0x310
WARN:  ? kthread_bind+0x10/0x10
WARN:  ret_from_fork+0x1f/0x40

[casasnovas]

In vates/kernel/xcpng-4.19.19-8.0.44.1/patched, we definitely had 429ae02 ("SUNRPC: Remove the bh-safe lock requirement on xprt->transport_lock") which removes the spin_lock(&xprt->transport_lock) in xs_tcp_state_change(TCP_ESTABLISHED) , but the issue (that we identified during the review of this
PR) is that they re-added the spin_lock, inadvertently, in 2918cd6 ("SUNRPC: Replace the queue timer with a delayed work function").

This was later fixed in vates/kernel/xcpng-4.19.19-8.0.45.1/patched, where the updated c6b4118 ("SUNRPC: Replace the queue timer with a delayed work function") does not add it back.

You can check how the commit was reworked between 8.0.44 and 8.0.45 with:

  colordiff -y -W200  <(git show -W 7e0a0e38fcfe) <(git show -W 0872df410f5f)

Now, in 8.0.46, they removed the two:

• 8b630bc SUNRPC: Remove the bh-safe lock requirement on xprt->transport_lock
• 5b02ea6 SUNRPC: Remove the bh-safe lock requirement on the rpc_wait_queue->lock

And instead added b8952d0 ("XSI-2150: Avoid deadlocks on NFS spinlocks") which looks like an incomplete version of 8b630bc ("SUNRPC: Remove the bh-safe lock requirement on xprt->transport_lock").

I would tend to think that the best option would be a correct backport of the two above commits (that really remove the need for the bh-safe lock requirements), so that the locking for TCP_ESTABLISHED becomes uneeded as
it should improve performances and would me more complete ?

As it stands, you've only partially reverted 7ea053a ("XSI-2150: Avoid deadlocks on NFS spinlocks") so I'm not quite sure this is the way to go, especially as we had many reports of deadlock with this extra lock in the past (though from my digging it were with the two "SUNRPC: Remove the bh-safe..." commits). Also, you're using the bh-safe version, when prior XS's patch, it was a regular spinlock, making it even more confusing for me.

[dinhngtu]

[...]

Now, in 8.0.46, they removed the two:

• 8b630bc SUNRPC: Remove the bh-safe lock requirement on xprt->transport_lock
• 5b02ea6 SUNRPC: Remove the bh-safe lock requirement on the rpc_wait_queue->lock

Hm yes I was looking at the 8.0.46 history so missed that 8.0.44 tried to carry this patch.

And instead added b8952d0 ("XSI-2150: Avoid deadlocks on NFS spinlocks") which looks like an incomplete version of 8b630bc ("SUNRPC: Remove the bh-safe lock requirement on xprt->transport_lock").

"XSI-2150: Avoid deadlocks on NFS spinlocks" is a very confusing patch as it does 2 separate things:

1. It converts the synchronization of transport_lock in xprt_init_autodisconnect to spin_lock_bh. I don't think this is actually necessary, since xprt_init_autodisconnect is called as a timer function which runs in softirq/bh context, and so a normal spin_lock is sufficient with or without the "SUNRPC: Remove the bh-safe lock requirement on xprt->transport_lock" patch.
2. It removes the synchronization around TCP_ESTABLISHED in xs_tcp_state_change. For me, this is suspect in the context of 8.0.46, because the entire "SUNRPC: Remove the bh-safe lock requirement on xprt->transport_lock" was not backported to 8.0.46. Nor was the subsequent "SUNRPC: Remove the bh-safe lock requirement on the rpc_wait_queue->lock", but I don't know for sure if these 2 patches are meant to go together.

I would tend to think that the best option would be a correct backport of the two above commits (that really remove the need for the bh-safe lock requirements), so that the locking for TCP_ESTABLISHED becomes uneeded as it should improve performances and would me more complete ?

As it stands, you've only partially reverted 7ea053a ("XSI-2150: Avoid deadlocks on NFS spinlocks") so I'm not quite sure this is the way to go, especially as we had many reports of deadlock with this extra lock in the past (though from my digging it were with the two "SUNRPC: Remove the bh-safe..." commits).

Is the deadlock we observed specifically in xs_tcp_state_change/TCP_ESTABLISHED? I thought it was rather because of the botched backport in 8.0.44 in general. But lockdep may help us answer this.

Also, you're using the bh-safe version, when prior XS's patch, it was a regular spinlock, making it even more confusing for me.

I see, xs_tcp_state_change had a read_lock_bh right at the beginning. So this should be a plain spin_lock. I'll rewrite the patch simply revert/drop the XSI-2150 patch.

I'm not confident about backporting "SUNRPC: Remove the bh-safe lock requirement on xprt->transport_lock" and the remaining parts of the series however. Digging into the original posting (https://lore.kernel.org/all/39608ABA-9E3F-443A-9F4C-7B91B885C7DD@oracle.com/T/#u), it looks like the series was meant purely as an optimization and not as bug fixes, CIP doesn't carry it either. Backporting it might very well require bringing in even more patches as hidden dependencies that we haven't identified.

[dinhngtu]

I've redone the PR with the following sequence:

• SUNRPC: avoid race between mod_timer() and del_timer_sync()
  (cherry picked from commit 242a3e0)
• SUNRPC: use _bh spinlocking on ->transport_lock
  (cherry picked from commit bcab4d5)
• Revert "XSI-2150: Avoid deadlocks on NFS spinlocks"

[casasnovas]

Sorry it took a while, just wanted to make sure I understand fully.

The patchset looks good to me, and so is the rationale for reverting the XS patch. Can you share the link to the support ticket with the full kernel logs? From looking at the backtrace you shared I am not convinced that 5837f88 SUNRPC: avoid race between mod_timer() and del_timer_sync() will prevent the same issue (although it should help with the race the patch mentions, so thumbs up from me to take it!).

[stormi]

Is it something we should try to contribute via https://github.com/xenserver/kernel.spec (in parallel, not as a requirement for fixing it in XCP-ng) ?