Move process_blocking_lock_queue out of timeout_processing?
Jeremy Allison
jra at samba.org
Sun Mar 18 20:14:42 GMT 2007
On Sun, Mar 18, 2007 at 12:48:03PM +0100, Volker Lendecke wrote:
> Hi, Jeremy!
>
> In the move to simplify our central processing loop code
> attached find a change that I would like to have your
> approval on: It moves process_blocking_lock_queue out of the
> way into a timed event.
>
> The idea is that we have blocking.c:brl_timeout as a timed
> event that is present whenever we do have a blocking lock
> pending. It fires brl_timeout_fn() which calls
> process_blocking_lock_queue(). Please note the
> change_to_root_user() in brl_timeout_fn().
Are the timeouts not normally run as root ? I thought
they were.
> Whenever we make changes to blocking_lock_queue, we trigger
> a recalc_brl_timeout() which sets a new brl_timout event if
> necessary. This makes the call to
> blocking_locks_timeout_ms() in setup_select_timeout()
> unnecessary, this is implicitly done in
> event_add_to_select_args() from the timed events.
>
> Check in? It survives make test :-)
On quick scan it looks ok. I certainly think it's the
right way to move forward (event based code).
> BTW, while looking at the brl code it might become possible
> to add a scheme polling for a os-level fcntl lock on behalf
> of CIFS clients in case that is taken by a Unix process.
> Just add a timed event that looks every second. Ugly as
> hell, but as long as don't get async fcntl locks there's
> nothing we can do but use threads. Not sure which pill is
> more bitter. Polling or pthreads? :-)
Hmmm - the original code would set a select timeout
of 10 seconds for a blocking lock that had infinite
timeout - that was the "poll" mechanism for locks
held by non-smbd processes.
Looking in the code it seems this might have been
lost in some old, old, old rewrite :-).
I think this code in smbd/blocking.c :
614 if (timeval_is_zero(&blr->expire_time)) {
615 continue; /* Never timeout. */
616 }
617
618 tv_dif_us = usec_time_diff(&blr->expire_time, &tv_curr);
Might have needed to be :
...
struct timeval tv_ex_time;
....
tmp_ex_time.tv_sec = blr->expire_time;
if (timeval_is_zero(&tmp_ex_time)) {
/* Set a tmp variable expire time to now + 10 secs */
tmp_exp_time.tv_sec = tv_curr.tv_sec + 30;
tmp_ext_time.tv_usec = tv_curr.tv_usec;
}
tv_dif_us = usec_time_diff(&tmp_ex_time, &tv_curr);
Any chance of adding that into the patch ?
Jeremy.
More information about the samba-technical
mailing list