[v3,2/9] nfs: check for POSIX lock capability on server even for flock locks
diff mbox

Message ID 1474057631-31209-3-git-send-email-jlayton@redhat.com
State New
Headers show

Commit Message

Jeff Layton Sept. 16, 2016, 8:27 p.m. UTC
We may end up in here with a FL_FLOCK lock request. We translate those
to whole-file NFSv4 locks and send them on to the server, so we need to
verify that the server supports them no matter what sort of lock request
this is.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/nfs4proc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Trond Myklebust Sept. 16, 2016, 9:14 p.m. UTC | #1
> On Sep 16, 2016, at 16:27, Jeff Layton <jlayton@redhat.com> wrote:
> 
> We may end up in here with a FL_FLOCK lock request. We translate those
> to whole-file NFSv4 locks and send them on to the server, so we need to
> verify that the server supports them no matter what sort of lock request
> this is.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> fs/nfs/nfs4proc.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 9d38366666f4..a0f25185c78c 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6135,8 +6135,7 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
> 	unsigned char fl_flags = request->fl_flags;
> 	int status = -ENOLCK;
> 
> -	if ((fl_flags & FL_POSIX) &&
> -			!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
> +	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
> 		goto out;
> 	/* Is this a delegated open? */
> 	status = nfs4_set_lock_state(state, request);
> -- 
> 2.7.4

The ability to support FL_FLOCK locks does not depend on the server’s support for POSIX locking semantics. FL_FLOCK can also use stacked lock semantics, precisely because they always cover the whole file.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Sept. 16, 2016, 9:46 p.m. UTC | #2
On Fri, 2016-09-16 at 21:14 +0000, Trond Myklebust wrote:
> > 
> > > > On Sep 16, 2016, at 16:27, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > We may end up in here with a FL_FLOCK lock request. We translate those
> > to whole-file NFSv4 locks and send them on to the server, so we need to
> > verify that the server supports them no matter what sort of lock request
> > this is.
> > 
> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > fs/nfs/nfs4proc.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 9d38366666f4..a0f25185c78c 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -6135,8 +6135,7 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
> > 	unsigned char fl_flags = request->fl_flags;
> > 	int status = -ENOLCK;
> > 
> > > > -	if ((fl_flags & FL_POSIX) &&
> > > > -			!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
> > > > +	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
> > > > 		goto out;
> > 	/* Is this a delegated open? */
> > 	status = nfs4_set_lock_state(state, request);
> > -- 
> > 2.7.4
> 
> The ability to support FL_FLOCK locks does not depend on the server’s
> support for POSIX locking semantics. FL_FLOCK can also use stacked
> lock semantics, precisely because they always cover the whole file.

Oh! I had always thought this flags absence basically meant "I don't
support file locking at all, so don't bother sending any LOCK
requests". Now that I look though, all RFC5661 says is:


   o  OPEN4_RESULT_LOCKTYPE_POSIX indicates that the server's byte-range
      locking behavior supports the complete set of POSIX locking
      techniques [24].  From this, the client can choose to manage byte-
      range locking state in a way to handle a mismatch of byte-range
      locking management.

If this flag isn't there, I guess we can't infer anything about how the
server's locks are implemented. That's just super.

So, ok. If you think this logic is more correct as-is, then I'm fine
with dropping this patch. This check gets moved in a later patch
though, so I'll need to fix that up as well.

-- 
Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust Sept. 16, 2016, 9:59 p.m. UTC | #3
> On Sep 16, 2016, at 17:46, Jeff Layton <jlayton@redhat.com> wrote:

> 

> On Fri, 2016-09-16 at 21:14 +0000, Trond Myklebust wrote:

>>> 

>>>>> On Sep 16, 2016, at 16:27, Jeff Layton <jlayton@redhat.com> wrote:

>>> 

>>> We may end up in here with a FL_FLOCK lock request. We translate those

>>> to whole-file NFSv4 locks and send them on to the server, so we need to

>>> verify that the server supports them no matter what sort of lock request

>>> this is.

>>> 

>>>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>

>>> ---

>>> fs/nfs/nfs4proc.c | 3 +--

>>> 1 file changed, 1 insertion(+), 2 deletions(-)

>>> 

>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c

>>> index 9d38366666f4..a0f25185c78c 100644

>>> --- a/fs/nfs/nfs4proc.c

>>> +++ b/fs/nfs/nfs4proc.c

>>> @@ -6135,8 +6135,7 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock

>>> 	unsigned char fl_flags = request->fl_flags;

>>> 	int status = -ENOLCK;

>>> 

>>>>> -	if ((fl_flags & FL_POSIX) &&

>>>>> -			!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))

>>>>> +	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))

>>>>> 		goto out;

>>> 	/* Is this a delegated open? */

>>> 	status = nfs4_set_lock_state(state, request);

>>> -- 

>>> 2.7.4

>> 

>> The ability to support FL_FLOCK locks does not depend on the server’s

>> support for POSIX locking semantics. FL_FLOCK can also use stacked

>> lock semantics, precisely because they always cover the whole file.

> 

> Oh! I had always thought this flags absence basically meant "I don't

> support file locking at all, so don't bother sending any LOCK

> requests". Now that I look though, all RFC5661 says is:

> 

> 

>    o  OPEN4_RESULT_LOCKTYPE_POSIX indicates that the server's byte-range

>       locking behavior supports the complete set of POSIX locking

>       techniques [24].  From this, the client can choose to manage byte-

>       range locking state in a way to handle a mismatch of byte-range

>       locking management.


Right. You also have:

15.1.8.7.  NFS4ERR_LOCK_RANGE (Error Code 10028)

   A LOCK operation is operating on a range that overlaps in part a
   currently held byte-range lock for the current lock-owner and does
   not precisely match a single such byte-range lock where the server
   does not support this type of request, and thus does not implement
   POSIX locking semantics [24].  See Sections 18.10.4, 18.11.4, and
   18.12.4 for a discussion of how this applies to LOCK, LOCKT, and
   LOCKU respectively.


> 

> If this flag isn't there, I guess we can't infer anything about how the

> server's locks are implemented. That's just super.

> 

> So, ok. If you think this logic is more correct as-is, then I'm fine

> with dropping this patch. This check gets moved in a later patch

> though, so I'll need to fix that up as well.


The current code was designed, as I said, to allow flock() locks at least to work with a server that only supports Windows stacked locks and/or native BSD locks.

Patch
diff mbox

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9d38366666f4..a0f25185c78c 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6135,8 +6135,7 @@  static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
 	unsigned char fl_flags = request->fl_flags;
 	int status = -ENOLCK;
 
-	if ((fl_flags & FL_POSIX) &&
-			!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
+	if (!test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
 		goto out;
 	/* Is this a delegated open? */
 	status = nfs4_set_lock_state(state, request);