Re: [PATCH/RFC] SCHED: allow wait_on_bit functions to support a timeout.

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3638 bytes --]

On Tue, 29 Apr 2014 12:32:17 +0200 Peter Zijlstra <peterz@infradead.org>
wrote:

> On Tue, Apr 29, 2014 at 07:44:06PM +1000, NeilBrown wrote:
> > 
> > 
> > It is currently not possible for various wait_on_bit functions to
> > implement a timeout.
> > While the "action" function that is called to do the waiting could
> > certainly use schedule_timeout(), there is no way to carry forward the
> > remaining timeout after a false wake-up.
> > As false-wakeups a clearly possible at least due to possible
> > hash collisions in bit_waitqueue(), this is a real problem.
> > 
> > The 'action' function is currently passed a pointer to the word
> > containing the bit being waited on.  Of the 27 currently defined
> > action functions, zero of them use this pointer.
> > So changing it to something else will be a little noisy but will have
> > no immediate effect.
> > 
> > This patch changes the 'action' function to take a pointer to the
> > "struct wait_bit_key", which contains a pointer to the word
> > containing the bit so nothing is really lost.
> > 
> > It also adds a 'private' field to "struct wait_bit_key", which is
> > initialized to zero.
> > 
> > An action function can now implement a timeout with something like
> > 
> > static int timed_out_waiter(struct wait_bit_key *key)
> > {
> > 	unsigned long waited;
> > 	if (key->private == 0) {
> > 		key->private = jiffies;
> > 		if (key->private == 0)
> > 			key->private -= 1;
> > 	}
> > 	waited = jiffies - key->private;
> > 	if (waited > 10 * HZ)
> > 		return -EAGAIN;
> > 	schedule_timeout(waited - 10 * HZ);
> > 	return 0;
> > }
> > 
> > If any other need for context in a waiter were found it would be easy
> > to use ->private for some other purpose, or even extend "struct
> > wait_bit_key".
> > 
> > My particular need is to support timeouts in nfs_release_page() to
> > avoid deadlocks with loopback mounted NFS.
> 
> So I'm sure I'm not getting it; but why is all the wait_bit crap so
> entirely different from the normal wait stuff?
> 
> Surely something like:
> 
> 	wait_event_timeout(&wq, test_bit(bit, word), timeout);
> 
> Is pretty much the same, no? The only thing that's different is the wake
> function, but surely we can thread that into there somehow without all
> this silly repetition.
> 
> Furthermore, I count about 23 action functions, of which there appear to
> be only like 4 actual variants. Surely such repetition sucks arse and
> should be avoided?
> 

Sure, we could replace the interface with something that matches a more
common pattern.
The wait_queue is chosen based on a hash of the bit and the word, and we
sometimes want "test_and_set_bit", and sometimes "test_bit" but we could
probably come up with reasonable definitions for

   wait_bit{,_lock}{,_interruptible,_killable}{,_io}{,_freezable}{,_timeout}(
           bit, word [, timeout]);

there are 48 functions there.  We don't need all of them of course.

My particular use case (as currently designed) wouldn't actually be met by
these.  In the 'action' function I current check to see if the connection
that the NFS client has is to the local machine or a remote machine and
adjust the timeout accordingly (and this state can change while waiting).

So I guess we add a "_cmd" set of interfaces too.

I'm not sure it's worth the effort - can we just stick with my idea?
Maybe defined and export action wrappers for

 io_schedule  schedule schedule(interruptible), 

that covers everything except a couple of NFS/RPC things which which should
probably stay local to NFS/RPC.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]