[PATCH 0/2] nfsd/dlm: fix knfsd panic when NFSv4 client does GETLK call

[PATCH 0/2] nfsd/dlm: fix knfsd panic when NFSv4 client does GETLK call

[Jeff Layton]

This patchset fixes a regression due to this patch:

----------------[snip]----------------
commit 55ef1274dddd4de387c54d110e354ffbb6cdc706
Author: J. Bruce Fields <bfields@citi.umich.edu>
Date:   Sat Dec 20 11:58:38 2008 -0800

    nfsd: Ensure nfsv4 calls the underlying filesystem on LOCKT
----------------[snip]----------------

To reproduce, set up a nfs server that is serving out a GFS2 filesystem.
Set a lock locally on a file on the GFS2 export on the server. From a
NFSv4 client, do a GETLK against the same file. The server will oops due
to a NULL pointer dereference. The fl_lmops will be set, but the
fl_owner will be a NULL pointer. The knfsd code does not account for
this possibility. It assumes that when fl_lmops is set this way that
the fl_owner will point to a valid nfs4_stateowner struct.

In actuality, Bruce's patch is correct, but it exposes a bug in DLM's
GETLK codepath. The first patch in this set fixes that.

The second patch fixes knfsd to be a little more careful about the
file_lock struct it builds to pass to vfs_test_lock.

Either patch should prevent the panic, though I think applying both
patches is the best approach to fixing this.

Jeff Layton (2):
  dlm: initialize file_lock struct in GETLK before copying conflicting
    lock
  nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is
    found

 fs/dlm/plock.c      |    2 ++
 fs/nfsd/nfs4state.c |    1 -
 2 files changed, 2 insertions(+), 1 deletions(-)

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

[Jeff Layton]

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

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

[J. Bruce Fields]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff Layton]

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>

[PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[Jeff Layton]

nfsd4_lockt does a search for a lockstateowner when building the lock
struct to test. If one is found, it'll set fl_owner to it. Regardless of
whether that happens, it'll also set fl_lmops. Given that this lock is
basically a "lightweight" lock that's just used for checking conflicts,
setting fl_lmops is probably not appropriate for it.

This behavior exposed a bug in DLM's GETLK implementation where it
wasn't clearing out the fields in the file_lock before filling in
conflicting lock info. While we were able to fix this in DLM, it
still seems pointless and dangerous to set the fl_lmops this way
when we may have a NULL lockstateowner.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4state.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 88db7d3..b6f60f4 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2871,7 +2871,6 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
 	file_lock.fl_pid = current->tgid;
 	file_lock.fl_flags = FL_POSIX;
-	file_lock.fl_lmops = &nfsd_posix_mng_ops;
 
 	file_lock.fl_start = lockt->lt_offset;
 	file_lock.fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
-- 
1.5.5.6

Re: [PATCH 2/2] nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is found

[J. Bruce Fields]

On Thu, Jan 22, 2009 at 02:16:04PM -0500, Jeff Layton wrote:
> nfsd4_lockt does a search for a lockstateowner when building the lock
> struct to test. If one is found, it'll set fl_owner to it. Regardless of
> whether that happens, it'll also set fl_lmops. Given that this lock is
> basically a "lightweight" lock that's just used for checking conflicts,
> setting fl_lmops is probably not appropriate for it.
> 
> This behavior exposed a bug in DLM's GETLK implementation where it
> wasn't clearing out the fields in the file_lock before filling in
> conflicting lock info. While we were able to fix this in DLM, it
> still seems pointless and dangerous to set the fl_lmops this way
> when we may have a NULL lockstateowner.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Thanks, applied.--b.

> ---
>  fs/nfsd/nfs4state.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 88db7d3..b6f60f4 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2871,7 +2871,6 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		file_lock.fl_owner = (fl_owner_t)lockt->lt_stateowner;
>  	file_lock.fl_pid = current->tgid;
>  	file_lock.fl_flags = FL_POSIX;
> -	file_lock.fl_lmops = &nfsd_posix_mng_ops;
>  
>  	file_lock.fl_start = lockt->lt_offset;
>  	file_lock.fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
> -- 
> 1.5.5.6
> 

Re: [PATCH 0/2] nfsd/dlm: fix knfsd panic when NFSv4 client does GETLK call

[J. Bruce Fields]

On Thu, Jan 22, 2009 at 02:16:02PM -0500, Jeff Layton wrote:
> This patchset fixes a regression due to this patch:

These look reasonable, thanks.

My testing infrastructure and stuff is still down, so I may be a little
slow getting these upstream.... Bug me again next week if I haven't done
anything and you think of it.

--b.

> 
> ----------------[snip]----------------
> commit 55ef1274dddd4de387c54d110e354ffbb6cdc706
> Author: J. Bruce Fields <bfields@citi.umich.edu>
> Date:   Sat Dec 20 11:58:38 2008 -0800
> 
>     nfsd: Ensure nfsv4 calls the underlying filesystem on LOCKT
> ----------------[snip]----------------
> 
> To reproduce, set up a nfs server that is serving out a GFS2 filesystem.
> Set a lock locally on a file on the GFS2 export on the server. From a
> NFSv4 client, do a GETLK against the same file. The server will oops due
> to a NULL pointer dereference. The fl_lmops will be set, but the
> fl_owner will be a NULL pointer. The knfsd code does not account for
> this possibility. It assumes that when fl_lmops is set this way that
> the fl_owner will point to a valid nfs4_stateowner struct.
> 
> In actuality, Bruce's patch is correct, but it exposes a bug in DLM's
> GETLK codepath. The first patch in this set fixes that.
> 
> The second patch fixes knfsd to be a little more careful about the
> file_lock struct it builds to pass to vfs_test_lock.
> 
> Either patch should prevent the panic, though I think applying both
> patches is the best approach to fixing this.
> 
> Jeff Layton (2):
>   dlm: initialize file_lock struct in GETLK before copying conflicting
>     lock
>   nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is
>     found
> 
>  fs/dlm/plock.c      |    2 ++
>  fs/nfsd/nfs4state.c |    1 -
>  2 files changed, 2 insertions(+), 1 deletions(-)
> 

[PATCH 0/2] nfsd/dlm: fix knfsd panic when NFSv4 client does GETLK call on GFS2 (regression)

[Jeff Layton]

This patchset fixes a regression due to this patch:

----------------[snip]----------------
commit 55ef1274dddd4de387c54d110e354ffbb6cdc706
Author: J. Bruce Fields <bfields@citi.umich.edu>
Date:   Sat Dec 20 11:58:38 2008 -0800

    nfsd: Ensure nfsv4 calls the underlying filesystem on LOCKT
----------------[snip]----------------

To reproduce, set up a nfs server that is serving out a GFS2 filesystem.
Set a lock locally on a file on the GFS2 export on the server. From a
NFSv4 client, do a GETLK against the same file. The server will oops due
to a NULL pointer dereference. The fl_lmops will be set, but the
fl_owner will be a NULL pointer. The knfsd code does not account for
this possibility. It assumes that when fl_lmops is set this way that
the fl_owner will point to a valid nfs4_stateowner struct.

In actuality, Bruce's patch is correct, but it exposes a bug in DLM's
GETLK codepath. The first patch in this set fixes that.

The second patch fixes knfsd to be a little more careful about the
file_lock struct it builds to pass to vfs_test_lock.

Either patch should prevent the panic, though I think applying both
patches is the best approach to fixing this.

Jeff Layton (2):
  dlm: initialize file_lock struct in GETLK before copying conflicting
    lock
  nfsd: only set file_lock.fl_lmops in nfsd4_lockt if a stateowner is
    found

 fs/dlm/plock.c      |    2 ++
 fs/nfsd/nfs4state.c |    6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

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

[Jeff Layton]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[J. Bruce Fields]

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

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

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

[J. Bruce Fields]

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.

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

[Jeff Layton]

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>