Re: [PATCH 3/7] dlm: recovery

From: Nish Aravamudan
Date: Tue Apr 26 2005 - 11:40:37 EST


On 4/26/05, David Teigland <teigland@xxxxxxxxxx> wrote:
> On Mon, Apr 25, 2005 at 09:42:21AM -0700, Nish Aravamudan wrote:
> > On 4/25/05, David Teigland <teigland@xxxxxxxxxx> wrote:

<snip>

> > Rather than wrap an infinite loop in an infinite loop, since all you
> > really want the second loop for is a periodic gurantee of
> > recover_timer seconds, wouldn't it be easier to have a sof-timer set
> > up to go off in that amount of time, and have it's callback do an
> > unconditional wake-up on the local wait-queue then mod_timer the timer
> > back in? Or might there be other tasks sleeping on the same
> > wait-queue? At that point, since your code does not seem to care about
> > signals (uses interruptible version, but doesn't check for
> > signal_pending() return value from wait_event), you probably could
> > just use the stock version of wait_event(). The conditions would be
> > checked in wait_event() on the desired periodic basis, just as they
> > are now, but maybe slightly more efficiently (and cleaner code, IMO :)
> > ). If you do *need* interruptible, please put in a comment as to why.
>
> Don't need inter, wait_event_timeout should work. Let's see if I followed
> all that; are you suggesting:
>
> static void dlm_wait_timer_fn(unsigned long data)
> {
> struct dlm_ls *ls = (struct dlm_ls *) data;

You can do the mod_timer() here if you make the timer_list global
(dlm_timer or something).

mod_timer(&dlm_timer, jiffies + dlm_config.recover_timer * HZ;

> wake_up(&ls->ls_wait_general);
> }
>
> int dlm_wait_function(struct dlm_ls *ls, int (*testfn) (struct dlm_ls *ls))
> {
> struct timer_list timer;
> int error = 0;
>
> init_timer(&timer);
> timer.function = dlm_wait_timer_fn;
> timer.data = (long) ls;

do an

timer.expires = jiffies + (dlm_config.recover_timer * HZ);
add_timer(&dlm_timer);

before wait_event().

> for (;;) {

Should not be need anymore.

> mod_timer(&timer, jiffies + (dlm_config.recover_timer * HZ));

Neither should this.

> wait_event(ls->ls_wait_general,
> testfn(ls) || dlm_recovery_stopped(ls));

So, now, you get woken up every so often to check the conditions *in*
wait_event() and if either is set you'll return to this context.
Otherwise, you'll go back to sleep in wait_event() -- that's why you
shouldn't need the infinite loop here.

> if (timer_pending(&timer))
> del_timer(&timer);

Good, this makes sure that you won't have the callback happening again
:) Do you need the sync version?

> if (testfn(ls))
> break;

Since you don't modify error in this case, you won't need this anymore

> if (dlm_recovery_stopped(ls)) {
> error = -1;
> break;
> }

Shouldn't need the break anymore, since you there is no loop.

> }
> return error;
> }
>
> [Another thread should usually be calling wake_up().]
>
> This is actually how it was some months ago, but I thought the timers made
> it more complicated given that wait_event_timeout is available. Do you
> really think the timers are nicer if we don't use interruptible?

The reason I think timers are nicer is that you don't need to nest
wait_event*() in a loop (and really, it looks strange when you do).
Complicated or not, I think they are the "right thing" for this case.

> > > +int dlm_wait_status_low(struct dlm_ls *ls, unsigned int wait_status)
> > > +{
> > > + struct dlm_rcom *rc = (struct dlm_rcom *) ls->ls_recover_buf;
> > > + int error = 0, nodeid = ls->ls_low_nodeid;
> > > +
> > > + for (;;) {
> > > + error = dlm_recovery_stopped(ls);
> > > + if (error)
> > > + goto out;
> > > +
> > > + error = dlm_rcom_status(ls, nodeid);
> > > + if (error)
> > > + break;
> > > +
> > > + if (rc->rc_result & wait_status)
> > > + break;
> > > + else {
> > > + set_current_state(TASK_INTERRUPTIBLE);
> > > + schedule_timeout(HZ >> 1);
> >
> > 500 ms is a long time! What's the justification? No comments?
> > Especially considering you will wake up on *every* signal. It's
> > unlikely you'll actually sleep for 500 ms. Also, please use
> > msleep_interruptible(), unless you expect to be woken by wait-queues
> > (I didn't have enough time to trace all the possible code paths :) )
> > -- in which case, make it explicit with comments, please.
>
> This is polling the status of a remote node. All I'm after here is a
> delay between the dlm_rcom_status() calls so that repeated status messages
> aren't flooding the network. We keep sending status messages until the
> remote node reports the status we want to see.
>
> When the remote node has _failed_ we're repeating these status requests
> hopelessly until the membership system detects the node failure and
> recovery is aborted (dlm_recovery_stopped) -- up to several seconds. When
> the remote node is just _slower_ at doing recovery, I'm guessing the
> difference is on average a second.
>
> Given that info, do you have a suggested delay? I've already switched to
> msleep.

Nope, that seems reasonable (500ms).

Thanks,
Nish
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/