Sorry for the delay responding!
On Wed, Dec 17, 2008 at 03:28:27PM -0600, David Teigland wrote:
> 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 lockd grant function that's called is nlmsvc_grant_deferred(), and
it uses the passed-in fl only in nlm_compare_locks().
Perhaps the problem is that the posix_lock_file() modifies the original
file lock which lockd is also holding a pointer to, and thus the
coalescing has also changed the lock that *lockd*'s sees?
--b.
>
> 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,
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
