[Cluster-devel] Re: [PATCH 1/2] dlm: initialize file_lock struct in GETLK before copying conflicting lock

[Jeff Layton]

On Wed, 21 Jan 2009 18:42:39 -0500
"J. Bruce Fields" <bfields at fieldses.org> wrote:

> On Wed, Jan 21, 2009 at 11:34:50AM -0500, Jeff Layton wrote:
> > dlm_posix_get fills out the relevant fields in the file_lock before
> > returning when there is a lock conflict, but doesn't clean out any of
> > the other fields in the file_lock.
> > 
> > When nfsd does a NFSv4 lockt call, it sets the fl_lmops to
> > nfsd_posix_mng_ops before calling the lower fs. When the lock comes back
> > after testing a lock on GFS2, it still has that field set. This confuses
> > nfsd into thinking that the file_lock is a nfsd4 lock.
> 
> I think of the lock system as supporting two types of objects, both
> stored in "struct lock"'s:
> 
> 	- Heavyweight locks: these have callbacks set and the filesystem
> 	  or lock manager could in theory have some private data
> 	  associated with them, so it's important that the appropriate
> 	  callbacks be called when they're released or copied.  These
> 	  are what are actually passed to posix_lock_file() and kept on
> 	  the inode lock lists.
> 	- Lightweight locks: just start, end, pid, flags, and type, with
> 	  everything zeroed out and/or ignored.
> 
> I don't see any reason why the lock passed into dlm_posix_get() needs to
> be a heavyweight lock.  In any case, if it were, then dlm_posix_get()
> would need to release the passed-in-lock before initializing the new one
> that it's returning.
> 


>From what I can tell, dlm_posix_lock is always passed a "lightweight"
lock.

> The returned lock should probably also be a lightweight lock that's a
> copy of whatever conflicting lock was found; otherwise we need to
> require the caller to for example release the thing correctly.
> 
> That's unfortunate for nfsv4 since that doesn't allow returning the
> lockowner information to the client.  But it's not terribly important
> to get that right.
> 
> Since gfs2 doesn't report the conflicting lock, I guess we just punt and
> return a copy of the passed-in lock, OK.
> 

I'm not sure I follow you here...

GFS2/DLM does report the conflicting lock. It's just that when there is
one, it's only overwriting some of the fields in the lock. The idea
with this patch is to basically try and make dlm_posix_get() fill out
the same fields as __locks_copy_lock() and make sure the rest are
initialized.


> > 
> > Fix this by making DLM reinitialize the file_lock before copying the
> > fields from the conflicting lock.
> > 
> > Signed-off-by: Jeff Layton <jlayton at redhat.com>
> > ---
> >  fs/dlm/plock.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> > index eba87ff..ca46f11 100644
> > --- a/fs/dlm/plock.c
> > +++ b/fs/dlm/plock.c
> > @@ -304,7 +304,9 @@ int dlm_posix_get(dlm_lockspace_t *lockspace, u64 number, struct file *file,
> >  	if (rv == -ENOENT)
> >  		rv = 0;
> >  	else if (rv > 0) {
> > +		locks_init_lock(fl);
> >  		fl->fl_type = (op->info.ex) ? F_WRLCK : F_RDLCK;
> > +		fl->fl_flags = FL_POSIX;
> >  		fl->fl_pid = op->info.pid;
> >  		fl->fl_start = op->info.start;
> >  		fl->fl_end = op->info.end;
> > -- 
> > 1.5.5.6
> > 
> > _______________________________________________
> > NFSv4 mailing list
> > NFSv4 at linux-nfs.org
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4


-- 
Jeff Layton <jlayton at redhat.com>