Bug 8971 - cleanup_timeout_fn() is called too often, on exiting when an smbd is idle.
Summary: cleanup_timeout_fn() is called too often, on exiting when an smbd is idle.
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-01 21:13 UTC by Jeremy Allison
Modified: 2012-07-05 11:36 UTC (History)
0 users

See Also:


Attachments
git-am fix for 3.6.next, back-ported from master. (4.15 KB, patch)
2012-06-01 21:13 UTC, Jeremy Allison
ira: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2012-06-01 21:13:35 UTC
Created attachment 7619 [details]
git-am fix for 3.6.next, back-ported from master.

We should only initiate the cleanup from processes that were explicitly in the child list. Plus calls to exit_server_cleanly() should be treated as a "clean" shutdown.

Jeremy.
Comment 1 Jeremy Allison 2012-06-04 21:29:26 UTC
Comment on attachment 7619 [details]
git-am fix for 3.6.next, back-ported from master.

Trying to get some coverage.
Comment 2 Volker Lendecke 2012-06-05 06:34:32 UTC
(In reply to comment #1)
> Comment on attachment 7619 [details]
> git-am fix for 3.6.next, back-ported from master.
> 
> Trying to get some coverage.

This is on my list. This is code added by Andrew Tridgell. Please make him comment why he did it this way and not the obvious way. There must be a reason for him to make smbd send out these messages much more often than it seems necessary at first sight. It must be a subtle interaction that we don't know about.
Comment 3 Jeremy Allison 2012-06-05 15:40:28 UTC
tridge is not answering email right now for very good reasons. Just review it, or not, as you wish. Don't push it onto someone else. FYI. This is already in master.
Jeremy.
Comment 4 Jeremy Allison 2012-06-05 15:41:16 UTC
Comment on attachment 7619 [details]
git-am fix for 3.6.next, back-ported from master.

Removing vl as he doesn't wish to review, adding ira instead.
Jeremy.
Comment 5 Ira Cooper 2012-06-05 19:43:37 UTC
Comment on attachment 7619 [details]
git-am fix for 3.6.next, back-ported from master.

From tridge in blocking.c:

114	/*
115	 to account for unclean shutdowns by clients we need a
116	 maximum timeout that we use for checking pending locks. If
117	 we have any pending locks at all, then check if the pending
118	 lock can continue at least every brl:recalctime seconds
119	 (default 5 seconds).
120
121	 This saves us needing to do a message_send_all() in the
122	 SIGCHLD handler in the parent daemon. That
123	 message_send_all() caused O(n^2) work to be done when IP
124	 failovers happened in clustered Samba, which could make the
125	 entire system unusable for many minutes.
126	*/

I reviewed for that locks are correctly put on the queue, and they appear to be.  If anything Jeremy did not go far ENOUGH with these changes, if I am to believe this comment.  It looks like tridge intended to remove the broadcast notification.  At least in the cluster case.  That may be a future target for cleanup.

Thanks,
Comment 6 Jeremy Allison 2012-06-05 20:13:14 UTC
Re-assigning to Karolin for inclusion in 3.6.next.
Jeremy.
Comment 7 Karolin Seeger 2012-06-13 17:41:56 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!