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-21-2009, 03:34 PM
Jeff Layton
 
Default dlm: initialize file_lock struct in GETLK before copying conflicting lock

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>
---
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
 
Old 01-21-2009, 10:42 PM
"J. Bruce Fields"
 
Default dlm: initialize file_lock struct in GETLK before copying conflicting lock

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.

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.

--b.

>
> 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
 
Old 01-22-2009, 05:32 PM
"J. Bruce Fields"
 
Default dlm: initialize file_lock struct in GETLK before copying conflicting lock

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.

> > 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.
 
Old 01-22-2009, 06:16 PM
Jeff Layton
 
Default dlm: initialize file_lock struct in GETLK before copying conflicting lock

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>
---
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
 
Old 01-27-2009, 09:34 PM
"J. Bruce Fields"
 
Default dlm: initialize file_lock struct in GETLK before copying conflicting lock

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.

> ---
> 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
>
 

Thread Tools




All times are GMT. The time now is 01:23 PM.

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