Discussion:
[PATCH] Patch for bug 13416: cleanupd sends MSG_SMB_UNLOCK twice to interested peers
Ralph Böhme via samba-technical
2018-05-02 10:05:30 UTC
Permalink
Hi!

Stumpled across this one while looking at the MSG_SMB_UNLOCK broadcast storm
issue.

MSG_SMB_UNLOCK should be send to smbd that are waiting on blocked
byte-range-locks when a lock holder died.

In smbd_cleanupd_unlock() we do this twice: once via a broadcast and
then again via brl_revalidate() to processes that are actually recorded
in brlock.tdb.

As brl_revalidate() should already take care of signaling anyone who
would be interested in the message, there's no need to broadcast.

Attached patch removes the broadcast.

Please review carefully and push if ok. I'm still suspecting that I'm missing a
subtlety around MSG_SMB_UNLOCK...

Thanks!
-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
Jeremy Allison via samba-technical
2018-05-03 22:06:55 UTC
Permalink
Post by Ralph Böhme via samba-technical
Hi!
Stumpled across this one while looking at the MSG_SMB_UNLOCK broadcast storm
issue.
MSG_SMB_UNLOCK should be send to smbd that are waiting on blocked
byte-range-locks when a lock holder died.
In smbd_cleanupd_unlock() we do this twice: once via a broadcast and
then again via brl_revalidate() to processes that are actually recorded
in brlock.tdb.
As brl_revalidate() should already take care of signaling anyone who
would be interested in the message, there's no need to broadcast.
Attached patch removes the broadcast.
Please review carefully and push if ok. I'm still suspecting that I'm missing a
subtlety around MSG_SMB_UNLOCK...
No I don't think so (missing the subtlety that is). brl_revalidate() sending
MSG_SMB_UNLOCK is OK as the receivers of MSG_SMB_UNLOCK are designed to be
idempotent, remember this can happen multiple times if smbd's fail so
it's OK to be triggered many times in a row (except for the broadcast
storm aspect of it, which you fixed in the previous patch :-).

RB+ and pushed !

Thanks,

Jeremy.

Loading...