On Tue, Jan 20, 2009 at 06:05:48PM -0500, bfields wrote:
> 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?
Whoops, sorry, I stopped reading too early:
> > 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.
Yes, OK, makes sense. But the former case (removing flc entirely) might
be simpler:
In the current code, the locks_copy_lock() results in ->fl_copy_lock()
calls without corresponding ->fl_release_private() calls on the copy
that's created; not a problem for the current code, but also not the way
the lock api should work.
--b.
> >
> > 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
