Re: [PATCH] lockd: handle fl_grant callbacks with coalesced locks (RFC)

[David Teigland <teigland@redhat.com>]

On Wed, Dec 17, 2008 at 03:01:56PM -0500, J. Bruce Fields wrote:
> Yep, that looks much better.  Though actually I suspect what was really
> intended was to use "flc" for the notifies, and "fl" for the
> posix_lock_file().
> 
> Also, since flc is never actually handed to the posix lock system, I
> think it should be a "shallow" lock copy--so it should be created with
> __locks_copy_lock().  Something like the below?

With this I'm back to seeing the same problem, but with the mismatch in
the reverse direction.

It seems fl points to lockd's file_lock, and that lockd expects notify()
will be called with a pointer to a file_lock that matches one of its own.
Based on that I think we'd always pass fl to notify().

The question then is whether lockd's file_lock should be coalesced or not.
If so, we'd pass fl to posix_lock_file().  If not, we'd pass flc to
posix_lock_file().  In both cases, fl would be passed to notify() and
would match.  In the former case, I don't see much purpose for flc to even
exist.  The patch I sent was the later case.

In the original code, we coalesce flc which then fails to match the
original (fl) in lockd.  In your patch, we coalesce fl which then fails to
match the copy of the original (flc).

Dave

> 
> diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> index eba87ff..e8d9086 100644
> --- a/fs/dlm/plock.c
> +++ b/fs/dlm/plock.c
> @@ -103,7 +103,7 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
>  		op->info.owner	= (__u64) fl->fl_pid;
>  		xop->callback	= fl->fl_lmops->fl_grant;
>  		locks_init_lock(&xop->flc);
> -		locks_copy_lock(&xop->flc, fl);
> +		__locks_copy_lock(&xop->flc, fl);
>  		xop->fl		= fl;
>  		xop->file	= file;
>  	} else {
> @@ -173,8 +173,8 @@ static int dlm_plock_callback(struct plock_op *op)
>  	}
>  
>  	/* got fs lock; bookkeep locally as well: */
> -	flc->fl_flags &= ~FL_SLEEP;
> -	if (posix_lock_file(file, flc, NULL)) {
> +	fl->fl_flags &= ~FL_SLEEP;
> +	if (posix_lock_file(file, fl, NULL)) {
>  		/*
>  		 * This can only happen in the case of kmalloc() failure.
>  		 * The filesystem's own lock is the authoritative lock,