lockd's interactions with locks.c

lockd's interactions with locks.c

[Matthew Wilcox]

There's no-one specifically listed as maintaining lockd, so i'm cc'ing
everyone involved with NFS.

It's clearly a good idea to remove the BKL from the file locking code
-- we have user-provokable O(n^2) behaviour in there; let's limit the
damage to this subsystem.  There are other good cleanups I want to do
that influence this too.

Let's look at the current code in nlmsvc_lock().  We do (paraphrased):

        if (!(conflock = posix_test_lock(&file->f_file, &lock->fl))) {
                error = posix_lock_file(&file->f_file, &lock->fl, 0);
		...
	}

	if (posix_locks_deadlock(&lock->fl, conflock))
		return nlm_deadlock;

	nlmsvc_insert_block(block, NLM_NEVER);
	
	if (list_empty(&block->b_call.a_args.lock.fl.fl_block))
		posix_block_lock(conflock, &block->b_call.a_args.lock.fl);

Now, unless we export a lock from locks.c that lockd can grab around
all this, we're pretty much hosed.  I believe that lockd runs with the
BKL at this point, so there's no race currently.  Here's my preferred
alternative (untested, i want to get comments on the idea):

	if (wait)
		lock->fl.fl_flags |= FL_SLEEP;

 again:
	error = posix_lock_file(&file->f_file, &lock->fl, 0);

	if (!error)
		return 0;
	if (!wait && error != -EAGAIN)
		return error;

	grab_lockd_blocking_lock();
	nlmsvc_insert_block(block, NLM_NEVER);
	have_been_woken_up_already = ...;
	release_lockd_blocking_lock();
	if (have_been_woken_up_already)
		goto again;

	return error;

notice we've now got _one_ call into locks.c instead of 4.  comments?

you can see a modified locks.c (which is also known-buggy) which supports
the FL_SLEEP semantics at
http://ftp.linux.org.uk/pub/linux/willy/patches/flock-2.5.22/flock-A.diff

-- 
Revolutions do not require corporate support.

Re: lockd's interactions with locks.c

[Trond Myklebust]

>>>>> " " == Matthew Wilcox <willy@debian.org> writes:

     > There's no-one specifically listed as maintaining lockd, so i'm
     > cc'ing everyone involved with NFS.

<snip>

     > Now, unless we export a lock from locks.c that lockd can grab
     > around all this, we're pretty much hosed.  I believe that lockd
     > runs with the BKL at this point, so there's no race currently.
     > Here's my preferred alternative (untested, i want to get
     > comments on the idea):

I'm not sure I understand how are you are planning on protecting
against races with the blocking code? For instance

     lockd:                                 Another process:

                                            posix_lock_file();
   posix_lock_file();

                                           releases file lock;

  grab_lockd_blocking_lock();
  nlmsvc_insert_block();
  have_been_woken_up_already = ...;
  release_lockd_blocking_lock();


Is this a situation where the mysterious 'have_been_woken_up_already'
kicks in in order to tell lockd not to block after all? If so, how do
you see that part being implemented?

Cheers,
  Trond

Re: lockd's interactions with locks.c

[Matthew Wilcox]

[nfs-devel trimmed from recipient list as it appears to be an unmaintained
email alias]

On Fri, Aug 02, 2002 at 05:56:49PM +0200, Trond Myklebust wrote:
> I'm not sure I understand how are you are planning on protecting
> against races with the blocking code? For instance
> 
>      lockd:                                 Another process:
> 
>                                             posix_lock_file();
>    posix_lock_file();
> 
>                                            releases file lock;
> 
>   grab_lockd_blocking_lock();
>   nlmsvc_insert_block();
>   have_been_woken_up_already = ...;
>   release_lockd_blocking_lock();
> 
> 
> Is this a situation where the mysterious 'have_been_woken_up_already'
> kicks in in order to tell lockd not to block after all? If so, how do
> you see that part being implemented?

Yes, that's exactly where it kicks in.  I did some hacking on the plane
yesterday and now I think I understand how it should work.  Also, I now
understand that the model I had in mind for posix_lock_file() cannot
work for lockd without a major overhaul (and I don't particularly want
to start hacking on lockd).  So consider the flock-A patch I pointed at
as nothing more than a proof-of-concept.  Anyway... here's what I have:

u32 nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
			struct nlm_lock *lock, int wait,
			struct nlm_cookie *cookie)
{
	struct file_lock	*conflock;
	struct nlm_block	*block;
	int			error;

	dprintk("lockd: nlmsvc_lock(%s/%ld, ty=%d, pi=%d, %Ld-%Ld, bl=%d)\n",
				file->f_file.f_dentry->d_inode->i_sb->s_id,
				file->f_file.f_dentry->d_inode->i_ino,
				lock->fl.fl_type, lock->fl.fl_pid,
				(long long)lock->fl.fl_start,
				(long long)lock->fl.fl_end,
				wait);

	/* Lock file against concurrent access */
	down(&file->f_sema);

	/* Get existing block (in case client is busy-waiting) */
	block = nlmsvc_lookup_block(file, lock, 0);

	lock->fl.fl_flags |= FL_LOCKD;
	if (wait)
		lock->fl.fl_flags |= FL_SLEEP;

	error = posix_lock_file(&file->f_file, &lock->fl);
	if (!error)
		goto out;

	if (!wait || (error != -EAGAIN))
		goto out;

	/* If we don't have a block, create and initialize it. */
	if (!block) {
		dprintk("lockd: blocking on this lock (allocating).\n");
		block = nlmsvc_create_block(rqstp, file, lock, cookie);
		error = -ENOLCK
		if (!block)
			goto out;
	}

	/* Append to list of blocked */
	nlmsvc_insert_block(block, NLM_NEVER);

	/* A wakeup may have come in between returning from posix_lock_file
	 * and nlmsvc_insert_block.  If it has, we have to move the block
	 * to the head of the list and kick lockd to retry the lock.  If a
	 * wakeup comes in between insert_block and the test, we wake up
	 * the daemon twice.  No big deal.
	 */
	if (!block->b_call.a_args.lock.fl.fl_next) {
		nlmsvc_insert_block(block, 0);
		svc_wake_up(block->b_daemon);
	}

	up(&file->f_sema);
	return nlm_lck_blocked;

 out:
	if (block)
		nlmsvc_delete_block(block, 0);
 unlock:
	up(&file->f_sema);
	switch (error) {
	case 0:
		return nlm_granted;
	case -EDEADLK:
		return nlm_deadlock;
	case -EAGAIN:
		return nlm_lck_denied;
	case -ENOLCK:
		return nlm_lck_denied_nolocks;
	}
}

Note I haven't even tried compiling this yet.

-- 
Revolutions do not require corporate support.