FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Cluster Development

 
 
LinkBack Thread Tools
 
Old 01-22-2009, 01:26 AM
Jeff Layton
 
Default dlm: initialize file_lock struct in GETLK before copying conflicting lock

On Wed, 21 Jan 2009 18:42:39 -0500
"J. Bruce Fields" <bfields@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@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@linux-nfs.org
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4


--
Jeff Layton <jlayton@redhat.com>
 
Old 01-22-2009, 05:37 PM
Jeff Layton
 
Default dlm: initialize file_lock struct in GETLK before copying conflicting lock

On Thu, 22 Jan 2009 13:32:41 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Jan 21, 2009 at 09:26:08PM -0500, Jeff Layton wrote:
> > On Wed, 21 Jan 2009 18:42:39 -0500
> > "J. Bruce Fields" <bfields@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.
>
> Right, so in your second patch, I think the fl_lmops assignment in
> nfsd4_lockt should also be removed.
>

Ok, that works too. I'll respin and repost once we come to some
concensus on the first patch.

> > > 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.
>
> Whoops, sorry, OK.
>
> > 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.
>
> Yes, this patch seems fine. I'm less sure of the second.
>
> --b.

Thanks,
--
Jeff Layton <jlayton@redhat.com>
 
Old 01-27-2009, 10:30 PM
Jeff Layton
 
Default dlm: initialize file_lock struct in GETLK before copying conflicting lock

On Tue, 27 Jan 2009 17:34:01 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Jan 22, 2009 at 02:16:03PM -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.
> >
> > Fix this by making DLM reinitialize the file_lock before copying the
> > fields from the conflicting lock.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
>
> I'll leave this one to gfs2 people to apply unless I'm told otherwise.
>
> --b.
>

That should be fine. Dave T. has taken this into his tree and is
pushing it to Linus.

Thanks,
--
Jeff Layton <jlayton@redhat.com>
 

Thread Tools




All times are GMT. The time now is 03:48 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org