[PATCH v3 0/4] Skipped unlocks

[PATCH v3 0/4] Skipped unlocks

[Benjamin Coddington]

Well over a year ago I made other attempts to fix the problem of NFS failing
to send an unlock when signalled.  Those attempts were terrible.

Here's another, smaller version that keeps two simple fixes and takes the
approach of skipping the wait if FL_CLOSE is set as was suggested by Trond.

I think this is probably a little late for 4.11.  Comments and review are
welcomed.

since v1:
	- add Christoph's reviewed-by on 1/4 and 2/4 and fixup switch
	  indentation on 2/4
since v2:
	- don't sleep in rpciod to wait for I/O completion, just send the unlock
	  immediately for both v3 and v4.

Benjamin Coddington (4):
  NFS4: remove a redundant lock range check
  NFS: Move the flock open mode check into nfs_flock()
  locks: Set FL_CLOSE when removing flock locks on close()
  NFS: Always send an unlock for FL_CLOSE

 fs/locks.c        |  2 +-
 fs/nfs/file.c     | 28 +++++++++++++++++++++-------
 fs/nfs/nfs4proc.c | 17 -----------------
 3 files changed, 22 insertions(+), 25 deletions(-)

-- 
2.9.3

[PATCH 1/4] NFS4: remove a redundant lock range check

[Benjamin Coddington]

flock64_to_posix_lock() is already doing this check

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
Reviewed-by: Jeff Layton <jeff.layton@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/nfs4proc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 0a0eaecf9676..9388899e4050 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6570,9 +6570,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 	ctx = nfs_file_open_context(filp);
 	state = ctx->state;
 
-	if (request->fl_start < 0 || request->fl_end < 0)
-		return -EINVAL;
-
 	if (IS_GETLK(cmd)) {
 		if (state != NULL)
 			return nfs4_proc_getlk(state, F_GETLK, request);
-- 
2.9.3

[PATCH 2/4] NFS: Move the flock open mode check into nfs_flock()

[Benjamin Coddington]

We only need to check lock exclusive/shared types against open mode when
flock() is used on NFS, so move it into the flock-specific path instead of
checking it for all locks.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/file.c     | 18 ++++++++++++++++--
 fs/nfs/nfs4proc.c | 14 --------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 26dbe8b0c10d..a490f45df4db 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -820,9 +820,23 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
 	if (NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FLOCK)
 		is_local = 1;
 
-	/* We're simulating flock() locks using posix locks on the server */
-	if (fl->fl_type == F_UNLCK)
+	/*
+	 * VFS doesn't require the open mode to match a flock() lock's type.
+	 * NFS, however, may simulate flock() locking with posix locking which
+	 * requires the open mode to match the lock type.
+	 */
+	switch (fl->fl_type) {
+	case F_UNLCK:
 		return do_unlk(filp, cmd, fl, is_local);
+	case F_RDLCK:
+		if (!(filp->f_mode & FMODE_READ))
+			return -EBADF;
+		break;
+	case F_WRLCK:
+		if (!(filp->f_mode & FMODE_WRITE))
+			return -EBADF;
+	}
+
 	return do_setlk(filp, cmd, fl, is_local);
 }
 EXPORT_SYMBOL_GPL(nfs_flock);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9388899e4050..91f88bfbbe79 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6592,20 +6592,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 	    !test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
 		return -ENOLCK;
 
-	/*
-	 * Don't rely on the VFS having checked the file open mode,
-	 * since it won't do this for flock() locks.
-	 */
-	switch (request->fl_type) {
-	case F_RDLCK:
-		if (!(filp->f_mode & FMODE_READ))
-			return -EBADF;
-		break;
-	case F_WRLCK:
-		if (!(filp->f_mode & FMODE_WRITE))
-			return -EBADF;
-	}
-
 	status = nfs4_set_lock_state(state, request);
 	if (status != 0)
 		return status;
-- 
2.9.3

[PATCH 3/4] locks: Set FL_CLOSE when removing flock locks on close()

[Benjamin Coddington]

Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks.
NFS will check for this flag to ensure an unlock is sent in a following
patch.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/locks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/locks.c b/fs/locks.c
index 26811321d39b..af2031a1fcff 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
 		.fl_owner = filp,
 		.fl_pid = current->tgid,
 		.fl_file = filp,
-		.fl_flags = FL_FLOCK,
+		.fl_flags = FL_FLOCK | FL_CLOSE,
 		.fl_type = F_UNLCK,
 		.fl_end = OFFSET_MAX,
 	};
-- 
2.9.3

[PATCH 4/4] NFS: Always send an unlock for FL_CLOSE

[Benjamin Coddington]

NFS attempts to wait for read and write completion before unlocking in
order to ensure that the data returned was protected by the lock.  When
this waiting is interrupted by a signal, the unlock may be skipped, and
messages similar to the following are seen in the kernel ring buffer:

[20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
[20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 fl_pid=20183
[20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 fl_pid=20185

For NFSv3, the missing unlock will cause the server to refuse conflicting
locks indefinitely.  For NFSv4, the leftover lock will be removed by the
server after the lease timeout.

This patch fixes this issue by skipping the wait in order to immediately send
the unlock if the FL_CLOSE flag is set when signaled.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/file.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index a490f45df4db..df695f52bb9d 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 	if (!IS_ERR(l_ctx)) {
 		status = nfs_iocounter_wait(l_ctx);
 		nfs_put_lock_context(l_ctx);
-		if (status < 0)
+		/*  NOTE: special case
+		 * 	If we're signalled while cleaning up locks on process exit, we
+		 * 	still need to complete the unlock.
+		 */
+		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
 			return status;
 	}
 
-	/* NOTE: special case
-	 * 	If we're signalled while cleaning up locks on process exit, we
-	 * 	still need to complete the unlock.
-	 */
 	/*
 	 * Use local locking if mounted with "-onolock" or with appropriate
 	 * "-olocal_lock="
-- 
2.9.3

Re: [PATCH 2/4] NFS: Move the flock open mode check into nfs_flock()

[Jeff Layton]

On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
> We only need to check lock exclusive/shared types against open mode when
> flock() is used on NFS, so move it into the flock-specific path instead of
> checking it for all locks.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/file.c     | 18 ++++++++++++++++--
>  fs/nfs/nfs4proc.c | 14 --------------
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 26dbe8b0c10d..a490f45df4db 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -820,9 +820,23 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
>  	if (NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FLOCK)
>  		is_local = 1;
>  
> -	/* We're simulating flock() locks using posix locks on the server */
> -	if (fl->fl_type == F_UNLCK)
> +	/*
> +	 * VFS doesn't require the open mode to match a flock() lock's type.
> +	 * NFS, however, may simulate flock() locking with posix locking which
> +	 * requires the open mode to match the lock type.
> +	 */
> +	switch (fl->fl_type) {
> +	case F_UNLCK:
>  		return do_unlk(filp, cmd, fl, is_local);
> +	case F_RDLCK:
> +		if (!(filp->f_mode & FMODE_READ))
> +			return -EBADF;
> +		break;
> +	case F_WRLCK:
> +		if (!(filp->f_mode & FMODE_WRITE))
> +			return -EBADF;
> +	}
> +
>  	return do_setlk(filp, cmd, fl, is_local);
>  }
>  EXPORT_SYMBOL_GPL(nfs_flock);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 9388899e4050..91f88bfbbe79 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6592,20 +6592,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
>  	    !test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
>  		return -ENOLCK;
>  
> -	/*
> -	 * Don't rely on the VFS having checked the file open mode,
> -	 * since it won't do this for flock() locks.
> -	 */
> -	switch (request->fl_type) {
> -	case F_RDLCK:
> -		if (!(filp->f_mode & FMODE_READ))
> -			return -EBADF;
> -		break;
> -	case F_WRLCK:
> -		if (!(filp->f_mode & FMODE_WRITE))
> -			return -EBADF;
> -	}
> -
>  	status = nfs4_set_lock_state(state, request);
>  	if (status != 0)
>  		return status;

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

Re: [PATCH 3/4] locks: Set FL_CLOSE when removing flock locks on close()

[Jeff Layton]

On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
> Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks.
> NFS will check for this flag to ensure an unlock is sent in a following
> patch.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/locks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 26811321d39b..af2031a1fcff 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
>  		.fl_owner = filp,
>  		.fl_pid = current->tgid,
>  		.fl_file = filp,
> -		.fl_flags = FL_FLOCK,
> +		.fl_flags = FL_FLOCK | FL_CLOSE,
>  		.fl_type = F_UNLCK,
>  		.fl_end = OFFSET_MAX,
>  	};

Looks good. I'm fine with merging this one via the NFS tree, btw...

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

Re: [PATCH 3/4] locks: Set FL_CLOSE when removing flock locks on close()

[Jeff Layton]

On Wed, 2017-02-22 at 07:13 -0500, Jeff Layton wrote:
> On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
> > Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks.
> > NFS will check for this flag to ensure an unlock is sent in a following
> > patch.
> > 
> > Signed-off-by: Benjamin Coddington <bcodding-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/locks.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 26811321d39b..af2031a1fcff 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
> >  		.fl_owner = filp,
> >  		.fl_pid = current->tgid,
> >  		.fl_file = filp,
> > -		.fl_flags = FL_FLOCK,
> > +		.fl_flags = FL_FLOCK | FL_CLOSE,
> >  		.fl_type = F_UNLCK,
> >  		.fl_end = OFFSET_MAX,
> >  	};
> 
> Looks good. I'm fine with merging this one via the NFS tree, btw...
> 
> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 

(cc'ing linux-fsdevel and Miklos)

Hmm, that said, this potentially may affect other filesystems. If you do
any more postings of this set, please cc linux-fsdevel.

In particular, I'll note that fuse has this:

        /* Unlock on close is handled by the flush method */
        if (fl->fl_flags & FL_CLOSE)
                return 0;

I don't see any lock handling in fuse_flush (though it does pass down a
lockowner). I guess the expectation is that the userland driver should
close down POSIX locks when the flush method is called.

Miklos, does this also apply to flock locks? Would Ben's patch cause any
grief here?

Thanks,
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
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 3/4] locks: Set FL_CLOSE when removing flock locks on close()

[Jeff Layton]

On Wed, 2017-02-22 at 07:13 -0500, Jeff Layton wrote:
> On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
> > Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks.
> > NFS will check for this flag to ensure an unlock is sent in a following
> > patch.
> > 
> > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > ---
> >  fs/locks.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 26811321d39b..af2031a1fcff 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
> >  		.fl_owner = filp,
> >  		.fl_pid = current->tgid,
> >  		.fl_file = filp,
> > -		.fl_flags = FL_FLOCK,
> > +		.fl_flags = FL_FLOCK | FL_CLOSE,
> >  		.fl_type = F_UNLCK,
> >  		.fl_end = OFFSET_MAX,
> >  	};
> 
> Looks good. I'm fine with merging this one via the NFS tree, btw...
> 
> Reviewed-by: Jeff Layton <jlayton@redhat.com>
> 

(cc'ing linux-fsdevel and Miklos)

Hmm, that said, this potentially may affect other filesystems. If you do
any more postings of this set, please cc linux-fsdevel.

In particular, I'll note that fuse has this:

        /* Unlock on close is handled by the flush method */
        if (fl->fl_flags & FL_CLOSE)
                return 0;

I don't see any lock handling in fuse_flush (though it does pass down a
lockowner). I guess the expectation is that the userland driver should
close down POSIX locks when the flush method is called.

Miklos, does this also apply to flock locks? Would Ben's patch cause any
grief here?

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

Re: [PATCH 4/4] NFS: Always send an unlock for FL_CLOSE

[Jeff Layton]

On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
> NFS attempts to wait for read and write completion before unlocking in
> order to ensure that the data returned was protected by the lock.  When
> this waiting is interrupted by a signal, the unlock may be skipped, and
> messages similar to the following are seen in the kernel ring buffer:
> 
> [20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
> [20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 fl_pid=20183
> [20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 fl_pid=20185
> 
> For NFSv3, the missing unlock will cause the server to refuse conflicting
> locks indefinitely.  For NFSv4, the leftover lock will be removed by the
> server after the lease timeout.
> 
> This patch fixes this issue by skipping the wait in order to immediately send
> the unlock if the FL_CLOSE flag is set when signaled.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/file.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index a490f45df4db..df695f52bb9d 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
>  	if (!IS_ERR(l_ctx)) {
>  		status = nfs_iocounter_wait(l_ctx);
>  		nfs_put_lock_context(l_ctx);
> -		if (status < 0)
> +		/*  NOTE: special case
> +		 * 	If we're signalled while cleaning up locks on process exit, we
> +		 * 	still need to complete the unlock.
> +		 */
> +		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
>  			return status;


Hmm, I don't know if this is safe...

Suppose we have a bunch of buffered writeback going on, and we're
sitting here waiting for it so we can do the unlock. The task catches a
signal, and then issues the unlock while writeback is still going on.
Another client then grabs the lock, and starts doing reads and writes
while this one is still writing back.

I think the unlock really needs to wait until writeback is done,
regardless of whether you catch a signal or not.


>  	}
>  
> -	/* NOTE: special case
> -	 * 	If we're signalled while cleaning up locks on process exit, we
> -	 * 	still need to complete the unlock.
> -	 */
>  	/*
>  	 * Use local locking if mounted with "-onolock" or with appropriate
>  	 * "-olocal_lock="

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 3/4] locks: Set FL_CLOSE when removing flock locks on close()

[Miklos Szeredi]

On Wed, Feb 22, 2017 at 1:25 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 2017-02-22 at 07:13 -0500, Jeff Layton wrote:
>> On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
>> > Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks.
>> > NFS will check for this flag to ensure an unlock is sent in a following
>> > patch.
>> >
>> > Signed-off-by: Benjamin Coddington <bcodding-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  fs/locks.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/fs/locks.c b/fs/locks.c
>> > index 26811321d39b..af2031a1fcff 100644
>> > --- a/fs/locks.c
>> > +++ b/fs/locks.c
>> > @@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
>> >             .fl_owner = filp,
>> >             .fl_pid = current->tgid,
>> >             .fl_file = filp,
>> > -           .fl_flags = FL_FLOCK,
>> > +           .fl_flags = FL_FLOCK | FL_CLOSE,
>> >             .fl_type = F_UNLCK,
>> >             .fl_end = OFFSET_MAX,
>> >     };
>>
>> Looks good. I'm fine with merging this one via the NFS tree, btw...
>>
>> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>
>
> (cc'ing linux-fsdevel and Miklos)
>
> Hmm, that said, this potentially may affect other filesystems. If you do
> any more postings of this set, please cc linux-fsdevel.
>
> In particular, I'll note that fuse has this:
>
>         /* Unlock on close is handled by the flush method */
>         if (fl->fl_flags & FL_CLOSE)
>                 return 0;
>
> I don't see any lock handling in fuse_flush (though it does pass down a
> lockowner). I guess the expectation is that the userland driver should
> close down POSIX locks when the flush method is called.

Right.

>
> Miklos, does this also apply to flock locks? Would Ben's patch cause any
> grief here?

Yes, it would make the final close of file not unlock flocks on that open file.

Simple fix is to change that condition to

#define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)

        if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
                return 0;

Thanks,
Miklos
--
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 3/4] locks: Set FL_CLOSE when removing flock locks on close()

[Miklos Szeredi]

On Wed, Feb 22, 2017 at 1:25 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Wed, 2017-02-22 at 07:13 -0500, Jeff Layton wrote:
>> On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
>> > Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks.
>> > NFS will check for this flag to ensure an unlock is sent in a following
>> > patch.
>> >
>> > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> > ---
>> >  fs/locks.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/fs/locks.c b/fs/locks.c
>> > index 26811321d39b..af2031a1fcff 100644
>> > --- a/fs/locks.c
>> > +++ b/fs/locks.c
>> > @@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
>> >             .fl_owner = filp,
>> >             .fl_pid = current->tgid,
>> >             .fl_file = filp,
>> > -           .fl_flags = FL_FLOCK,
>> > +           .fl_flags = FL_FLOCK | FL_CLOSE,
>> >             .fl_type = F_UNLCK,
>> >             .fl_end = OFFSET_MAX,
>> >     };
>>
>> Looks good. I'm fine with merging this one via the NFS tree, btw...
>>
>> Reviewed-by: Jeff Layton <jlayton@redhat.com>
>>
>
> (cc'ing linux-fsdevel and Miklos)
>
> Hmm, that said, this potentially may affect other filesystems. If you do
> any more postings of this set, please cc linux-fsdevel.
>
> In particular, I'll note that fuse has this:
>
>         /* Unlock on close is handled by the flush method */
>         if (fl->fl_flags & FL_CLOSE)
>                 return 0;
>
> I don't see any lock handling in fuse_flush (though it does pass down a
> lockowner). I guess the expectation is that the userland driver should
> close down POSIX locks when the flush method is called.

Right.

>
> Miklos, does this also apply to flock locks? Would Ben's patch cause any
> grief here?

Yes, it would make the final close of file not unlock flocks on that open file.

Simple fix is to change that condition to

#define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)

        if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
                return 0;

Thanks,
Miklos

Re: [PATCH 3/4] locks: Set FL_CLOSE when removing flock locks on close()

[Benjamin Coddington]

On 22 Feb 2017, at 8:25, Miklos Szeredi wrote:

> On Wed, Feb 22, 2017 at 1:25 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 
> wrote:
>> On Wed, 2017-02-22 at 07:13 -0500, Jeff Layton wrote:
>>> On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
>>>> Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing 
>>>> locks.
>>>> NFS will check for this flag to ensure an unlock is sent in a 
>>>> following
>>>> patch.
>>>>
>>>> Signed-off-by: Benjamin Coddington <bcodding-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>  fs/locks.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>> index 26811321d39b..af2031a1fcff 100644
>>>> --- a/fs/locks.c
>>>> +++ b/fs/locks.c
>>>> @@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct 
>>>> file_lock_context *flctx)
>>>>             .fl_owner = filp,
>>>>             .fl_pid = current->tgid,
>>>>             .fl_file = filp,
>>>> -           .fl_flags = FL_FLOCK,
>>>> +           .fl_flags = FL_FLOCK | FL_CLOSE,
>>>>             .fl_type = F_UNLCK,
>>>>             .fl_end = OFFSET_MAX,
>>>>     };
>>>
>>> Looks good. I'm fine with merging this one via the NFS tree, btw...
>>>
>>> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>
>>
>> (cc'ing linux-fsdevel and Miklos)
>>
>> Hmm, that said, this potentially may affect other filesystems. If you 
>> do
>> any more postings of this set, please cc linux-fsdevel.
>>
>> In particular, I'll note that fuse has this:
>>
>>         /* Unlock on close is handled by the flush method */
>>         if (fl->fl_flags & FL_CLOSE)
>>                 return 0;
>>
>> I don't see any lock handling in fuse_flush (though it does pass down 
>> a
>> lockowner). I guess the expectation is that the userland driver 
>> should
>> close down POSIX locks when the flush method is called.
>
> Right.
>
>>
>> Miklos, does this also apply to flock locks? Would Ben's patch cause 
>> any
>> grief here?
>
> Yes, it would make the final close of file not unlock flocks on that 
> open file.
>
> Simple fix is to change that condition to
>
> #define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
>
>         if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
>                 return 0;

OK, I can that in the next version.  Thanks for that catch Jeff.

Ben
--
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 3/4] locks: Set FL_CLOSE when removing flock locks on close()

[Benjamin Coddington]

On 22 Feb 2017, at 8:25, Miklos Szeredi wrote:

> On Wed, Feb 22, 2017 at 1:25 PM, Jeff Layton <jlayton@redhat.com> 
> wrote:
>> On Wed, 2017-02-22 at 07:13 -0500, Jeff Layton wrote:
>>> On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
>>>> Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing 
>>>> locks.
>>>> NFS will check for this flag to ensure an unlock is sent in a 
>>>> following
>>>> patch.
>>>>
>>>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>>>> ---
>>>>  fs/locks.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>> index 26811321d39b..af2031a1fcff 100644
>>>> --- a/fs/locks.c
>>>> +++ b/fs/locks.c
>>>> @@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct 
>>>> file_lock_context *flctx)
>>>>             .fl_owner = filp,
>>>>             .fl_pid = current->tgid,
>>>>             .fl_file = filp,
>>>> -           .fl_flags = FL_FLOCK,
>>>> +           .fl_flags = FL_FLOCK | FL_CLOSE,
>>>>             .fl_type = F_UNLCK,
>>>>             .fl_end = OFFSET_MAX,
>>>>     };
>>>
>>> Looks good. I'm fine with merging this one via the NFS tree, btw...
>>>
>>> Reviewed-by: Jeff Layton <jlayton@redhat.com>
>>>
>>
>> (cc'ing linux-fsdevel and Miklos)
>>
>> Hmm, that said, this potentially may affect other filesystems. If you 
>> do
>> any more postings of this set, please cc linux-fsdevel.
>>
>> In particular, I'll note that fuse has this:
>>
>>         /* Unlock on close is handled by the flush method */
>>         if (fl->fl_flags & FL_CLOSE)
>>                 return 0;
>>
>> I don't see any lock handling in fuse_flush (though it does pass down 
>> a
>> lockowner). I guess the expectation is that the userland driver 
>> should
>> close down POSIX locks when the flush method is called.
>
> Right.
>
>>
>> Miklos, does this also apply to flock locks? Would Ben's patch cause 
>> any
>> grief here?
>
> Yes, it would make the final close of file not unlock flocks on that 
> open file.
>
> Simple fix is to change that condition to
>
> #define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
>
>         if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
>                 return 0;

OK, I can that in the next version.  Thanks for that catch Jeff.

Ben

Re: [PATCH 4/4] NFS: Always send an unlock for FL_CLOSE

[Benjamin Coddington]

On 22 Feb 2017, at 8:20, Jeff Layton wrote:

> On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
>> NFS attempts to wait for read and write completion before unlocking 
>> in
>> order to ensure that the data returned was protected by the lock.  
>> When
>> this waiting is interrupted by a signal, the unlock may be skipped, 
>> and
>> messages similar to the following are seen in the kernel ring buffer:
>>
>> [20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
>> [20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 
>> fl_pid=20183
>> [20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 
>> fl_pid=20185
>>
>> For NFSv3, the missing unlock will cause the server to refuse 
>> conflicting
>> locks indefinitely.  For NFSv4, the leftover lock will be removed by 
>> the
>> server after the lease timeout.
>>
>> This patch fixes this issue by skipping the wait in order to 
>> immediately send
>> the unlock if the FL_CLOSE flag is set when signaled.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>>  fs/nfs/file.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
>> index a490f45df4db..df695f52bb9d 100644
>> --- a/fs/nfs/file.c
>> +++ b/fs/nfs/file.c
>> @@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct 
>> file_lock *fl, int is_local)
>>  	if (!IS_ERR(l_ctx)) {
>>  		status = nfs_iocounter_wait(l_ctx);
>>  		nfs_put_lock_context(l_ctx);
>> -		if (status < 0)
>> +		/*  NOTE: special case
>> +		 * 	If we're signalled while cleaning up locks on process exit, we
>> +		 * 	still need to complete the unlock.
>> +		 */
>> +		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
>>  			return status;
>
>
> Hmm, I don't know if this is safe...
>
> Suppose we have a bunch of buffered writeback going on, and we're
> sitting here waiting for it so we can do the unlock. The task catches 
> a
> signal, and then issues the unlock while writeback is still going on.
> Another client then grabs the lock, and starts doing reads and writes
> while this one is still writing back.
>
> I think the unlock really needs to wait until writeback is done,
> regardless of whether you catch a signal or not.

The only other way to do this is as you've sugggested many times -- by
putting the LOCKU on a rpc_wait_queue.  But that won't work for NFSv3.

For NFSv4, the other thing to do might be to look up the lock state and 
set
NFS_LOCK_LOST, then unlock.  That will cause subsequent writes to not
attempt recovery.

How can this be fixed for NFSv3 without voilating the NLM layers?

Ben

Re: [PATCH 4/4] NFS: Always send an unlock for FL_CLOSE

[Jeff Layton]

On Wed, 2017-02-22 at 09:10 -0500, Benjamin Coddington wrote:
> On 22 Feb 2017, at 8:20, Jeff Layton wrote:
> 
> > On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
> > > NFS attempts to wait for read and write completion before unlocking 
> > > in
> > > order to ensure that the data returned was protected by the lock.  
> > > When
> > > this waiting is interrupted by a signal, the unlock may be skipped, 
> > > and
> > > messages similar to the following are seen in the kernel ring buffer:
> > > 
> > > [20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
> > > [20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 
> > > fl_pid=20183
> > > [20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 
> > > fl_pid=20185
> > > 
> > > For NFSv3, the missing unlock will cause the server to refuse 
> > > conflicting
> > > locks indefinitely.  For NFSv4, the leftover lock will be removed by 
> > > the
> > > server after the lease timeout.
> > > 
> > > This patch fixes this issue by skipping the wait in order to 
> > > immediately send
> > > the unlock if the FL_CLOSE flag is set when signaled.
> > > 
> > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > > ---
> > >  fs/nfs/file.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > > index a490f45df4db..df695f52bb9d 100644
> > > --- a/fs/nfs/file.c
> > > +++ b/fs/nfs/file.c
> > > @@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct 
> > > file_lock *fl, int is_local)
> > >  	if (!IS_ERR(l_ctx)) {
> > >  		status = nfs_iocounter_wait(l_ctx);
> > >  		nfs_put_lock_context(l_ctx);
> > > -		if (status < 0)
> > > +		/*  NOTE: special case
> > > +		 * 	If we're signalled while cleaning up locks on process exit, we
> > > +		 * 	still need to complete the unlock.
> > > +		 */
> > > +		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
> > >  			return status;
> > 
> > 
> > Hmm, I don't know if this is safe...
> > 
> > Suppose we have a bunch of buffered writeback going on, and we're
> > sitting here waiting for it so we can do the unlock. The task catches 
> > a
> > signal, and then issues the unlock while writeback is still going on.
> > Another client then grabs the lock, and starts doing reads and writes
> > while this one is still writing back.
> > 
> > I think the unlock really needs to wait until writeback is done,
> > regardless of whether you catch a signal or not.
> 
> The only other way to do this is as you've sugggested many times -- by
> putting the LOCKU on a rpc_wait_queue.  But that won't work for NFSv3.
> 

IDGI -- why doesn't that (or something similar) not work for v3?

> For NFSv4, the other thing to do might be to look up the lock state and 
> set
> NFS_LOCK_LOST, then unlock.  That will cause subsequent writes to not
> attempt recovery.
> 
No! LOCK_LOST is really for the case where we've lost contact with the
server. We don't want to get kicked into recovery just because of a
SIGKILL.

Ideally, I think what we want is for the effect of every write that
occurred up until the SIGKILL to be visible to other clients after the
SIGKILL. Doing anything else could be problematic for applications.

> How can this be fixed for NFSv3 without voilating the NLM layers?

That, I'm not sure of. Conceptually, I think what we want though is to
wait until writeback is done, and then issue an unlock.

One way might be to embed some new infrastructure in nlm_rqst and
nfs_lock_context that would allow you to idle NLM requests until the
io_count goes to 0.

Maybe have nlmclnt_unlock() put the nlm_rqst on a list in the lock
context if there are still outstanding NFS requests. When the io_count
goes to zero, queue some work that will scrape out the list and kick
off those requests. It'll take some creativity to do that without
turning it into one huge layering violation though.

Alternately, you could idle them in the NFS layer, but that probably
means having to do another allocation, which may be problematic in
situations where you may be dealing with a SIGKILL (e.g. OOM killer).
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 4/4] NFS: Always send an unlock for FL_CLOSE

[Trond Myklebust]

T24gV2VkLCAyMDE3LTAyLTIyIGF0IDEwOjQyIC0wNTAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g
T24gV2VkLCAyMDE3LTAyLTIyIGF0IDA5OjEwIC0wNTAwLCBCZW5qYW1pbiBDb2RkaW5ndG9uIHdy
b3RlOg0KPiA+IE9uIDIyIEZlYiAyMDE3LCBhdCA4OjIwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g
PiANCj4gPiA+IE9uIFR1ZSwgMjAxNy0wMi0yMSBhdCAxMDozOSAtMDUwMCwgQmVuamFtaW4gQ29k
ZGluZ3RvbiB3cm90ZToNCj4gPiA+ID4gTkZTIGF0dGVtcHRzIHRvIHdhaXQgZm9yIHJlYWQgYW5k
IHdyaXRlIGNvbXBsZXRpb24gYmVmb3JlDQo+ID4gPiA+IHVubG9ja2luZ8KgDQo+ID4gPiA+IGlu
DQo+ID4gPiA+IG9yZGVyIHRvIGVuc3VyZSB0aGF0IHRoZSBkYXRhIHJldHVybmVkIHdhcyBwcm90
ZWN0ZWQgYnkgdGhlDQo+ID4gPiA+IGxvY2suwqDCoA0KPiA+ID4gPiBXaGVuDQo+ID4gPiA+IHRo
aXMgd2FpdGluZyBpcyBpbnRlcnJ1cHRlZCBieSBhIHNpZ25hbCwgdGhlIHVubG9jayBtYXkgYmUN
Cj4gPiA+ID4gc2tpcHBlZCzCoA0KPiA+ID4gPiBhbmQNCj4gPiA+ID4gbWVzc2FnZXMgc2ltaWxh
ciB0byB0aGUgZm9sbG93aW5nIGFyZSBzZWVuIGluIHRoZSBrZXJuZWwgcmluZw0KPiA+ID4gPiBi
dWZmZXI6DQo+ID4gPiA+IA0KPiA+ID4gPiBbMjAuMTY3ODc2XSBMZWFrZWQgbG9ja3Mgb24gZGV2
PTB4MDoweDJiIGlubz0weDhkZDRjMzoNCj4gPiA+ID4gWzIwLjE2ODI4Nl0gUE9TSVg6IGZsX293
bmVyPWZmZmY4ODAwNzhiMDY5NDAgZmxfZmxhZ3M9MHgxDQo+ID4gPiA+IGZsX3R5cGU9MHgwwqAN
Cj4gPiA+ID4gZmxfcGlkPTIwMTgzDQo+ID4gPiA+IFsyMC4xNjg3MjddIFBPU0lYOiBmbF9vd25l
cj1mZmZmODgwMDc4YjA2NjgwIGZsX2ZsYWdzPTB4MQ0KPiA+ID4gPiBmbF90eXBlPTB4MMKgDQo+
ID4gPiA+IGZsX3BpZD0yMDE4NQ0KPiA+ID4gPiANCj4gPiA+ID4gRm9yIE5GU3YzLCB0aGUgbWlz
c2luZyB1bmxvY2sgd2lsbCBjYXVzZSB0aGUgc2VydmVyIHRvIHJlZnVzZcKgDQo+ID4gPiA+IGNv
bmZsaWN0aW5nDQo+ID4gPiA+IGxvY2tzIGluZGVmaW5pdGVseS7CoMKgRm9yIE5GU3Y0LCB0aGUg
bGVmdG92ZXIgbG9jayB3aWxsIGJlDQo+ID4gPiA+IHJlbW92ZWQgYnnCoA0KPiA+ID4gPiB0aGUN
Cj4gPiA+ID4gc2VydmVyIGFmdGVyIHRoZSBsZWFzZSB0aW1lb3V0Lg0KPiA+ID4gPiANCj4gPiA+
ID4gVGhpcyBwYXRjaCBmaXhlcyB0aGlzIGlzc3VlIGJ5IHNraXBwaW5nIHRoZSB3YWl0IGluIG9y
ZGVyIHRvwqANCj4gPiA+ID4gaW1tZWRpYXRlbHkgc2VuZA0KPiA+ID4gPiB0aGUgdW5sb2NrIGlm
IHRoZSBGTF9DTE9TRSBmbGFnIGlzIHNldCB3aGVuIHNpZ25hbGVkLg0KPiA+ID4gPiANCj4gPiA+
ID4gU2lnbmVkLW9mZi1ieTogQmVuamFtaW4gQ29kZGluZ3RvbiA8YmNvZGRpbmdAcmVkaGF0LmNv
bT4NCj4gPiA+ID4gLS0tDQo+ID4gPiA+IMKgZnMvbmZzL2ZpbGUuYyB8IDEwICsrKysrLS0tLS0N
Cj4gPiA+ID4gwqAxIGZpbGUgY2hhbmdlZCwgNSBpbnNlcnRpb25zKCspLCA1IGRlbGV0aW9ucygt
KQ0KPiA+ID4gPiANCj4gPiA+ID4gZGlmZiAtLWdpdCBhL2ZzL25mcy9maWxlLmMgYi9mcy9uZnMv
ZmlsZS5jDQo+ID4gPiA+IGluZGV4IGE0OTBmNDVkZjRkYi4uZGY2OTVmNTJiYjlkIDEwMDY0NA0K
PiA+ID4gPiAtLS0gYS9mcy9uZnMvZmlsZS5jDQo+ID4gPiA+ICsrKyBiL2ZzL25mcy9maWxlLmMN
Cj4gPiA+ID4gQEAgLTY5NywxNCArNjk3LDE0IEBAIGRvX3VubGsoc3RydWN0IGZpbGUgKmZpbHAs
IGludCBjbWQsDQo+ID4gPiA+IHN0cnVjdMKgDQo+ID4gPiA+IGZpbGVfbG9jayAqZmwsIGludCBp
c19sb2NhbCkNCj4gPiA+ID4gwqAJaWYgKCFJU19FUlIobF9jdHgpKSB7DQo+ID4gPiA+IMKgCQlz
dGF0dXMgPSBuZnNfaW9jb3VudGVyX3dhaXQobF9jdHgpOw0KPiA+ID4gPiDCoAkJbmZzX3B1dF9s
b2NrX2NvbnRleHQobF9jdHgpOw0KPiA+ID4gPiAtCQlpZiAoc3RhdHVzIDwgMCkNCj4gPiA+ID4g
KwkJLyrCoMKgTk9URTogc3BlY2lhbCBjYXNlDQo+ID4gPiA+ICsJCcKgKsKgCUlmIHdlJ3JlIHNp
Z25hbGxlZCB3aGlsZSBjbGVhbmluZw0KPiA+ID4gPiB1cCBsb2NrcyBvbiBwcm9jZXNzIGV4aXQs
IHdlDQo+ID4gPiA+ICsJCcKgKsKgCXN0aWxsIG5lZWQgdG8gY29tcGxldGUgdGhlIHVubG9jay4N
Cj4gPiA+ID4gKwkJwqAqLw0KPiA+ID4gPiArCQlpZiAoc3RhdHVzIDwgMCAmJiAhKGZsLT5mbF9m
bGFncyAmIEZMX0NMT1NFKSkNCj4gPiA+ID4gwqAJCQlyZXR1cm4gc3RhdHVzOw0KPiA+ID4gDQo+
ID4gPiANCj4gPiA+IEhtbSwgSSBkb24ndCBrbm93IGlmIHRoaXMgaXMgc2FmZS4uLg0KPiA+ID4g
DQo+ID4gPiBTdXBwb3NlIHdlIGhhdmUgYSBidW5jaCBvZiBidWZmZXJlZCB3cml0ZWJhY2sgZ29p
bmcgb24sIGFuZCB3ZSdyZQ0KPiA+ID4gc2l0dGluZyBoZXJlIHdhaXRpbmcgZm9yIGl0IHNvIHdl
IGNhbiBkbyB0aGUgdW5sb2NrLiBUaGUgdGFzaw0KPiA+ID4gY2F0Y2hlc8KgDQo+ID4gPiBhDQo+
ID4gPiBzaWduYWwsIGFuZCB0aGVuIGlzc3VlcyB0aGUgdW5sb2NrIHdoaWxlIHdyaXRlYmFjayBp
cyBzdGlsbCBnb2luZw0KPiA+ID4gb24uDQo+ID4gPiBBbm90aGVyIGNsaWVudCB0aGVuIGdyYWJz
IHRoZSBsb2NrLCBhbmQgc3RhcnRzIGRvaW5nIHJlYWRzIGFuZA0KPiA+ID4gd3JpdGVzDQo+ID4g
PiB3aGlsZSB0aGlzIG9uZSBpcyBzdGlsbCB3cml0aW5nIGJhY2suDQo+ID4gPiANCj4gPiA+IEkg
dGhpbmsgdGhlIHVubG9jayByZWFsbHkgbmVlZHMgdG8gd2FpdCB1bnRpbCB3cml0ZWJhY2sgaXMg
ZG9uZSwNCj4gPiA+IHJlZ2FyZGxlc3Mgb2Ygd2hldGhlciB5b3UgY2F0Y2ggYSBzaWduYWwgb3Ig
bm90Lg0KPiA+IA0KPiA+IFRoZSBvbmx5IG90aGVyIHdheSB0byBkbyB0aGlzIGlzIGFzIHlvdSd2
ZSBzdWdnZ2VzdGVkIG1hbnkgdGltZXMgLS0NCj4gPiBieQ0KPiA+IHB1dHRpbmcgdGhlIExPQ0tV
IG9uIGEgcnBjX3dhaXRfcXVldWUuwqDCoEJ1dCB0aGF0IHdvbid0IHdvcmsgZm9yDQo+ID4gTkZT
djMuDQo+ID4gDQo+IA0KPiBJREdJIC0tIHdoeSBkb2Vzbid0IHRoYXQgKG9yIHNvbWV0aGluZyBz
aW1pbGFyKSBub3Qgd29yayBmb3IgdjM/DQo+IA0KPiA+IEZvciBORlN2NCwgdGhlIG90aGVyIHRo
aW5nIHRvIGRvIG1pZ2h0IGJlIHRvIGxvb2sgdXAgdGhlIGxvY2sgc3RhdGUNCj4gPiBhbmTCoA0K
PiA+IHNldA0KPiA+IE5GU19MT0NLX0xPU1QsIHRoZW4gdW5sb2NrLsKgwqBUaGF0IHdpbGwgY2F1
c2Ugc3Vic2VxdWVudCB3cml0ZXMgdG8NCj4gPiBub3QNCj4gPiBhdHRlbXB0IHJlY292ZXJ5Lg0K
PiA+IA0KPiANCj4gTm8hIExPQ0tfTE9TVCBpcyByZWFsbHkgZm9yIHRoZSBjYXNlIHdoZXJlIHdl
J3ZlIGxvc3QgY29udGFjdCB3aXRoDQo+IHRoZQ0KPiBzZXJ2ZXIuIFdlIGRvbid0IHdhbnQgdG8g
Z2V0IGtpY2tlZCBpbnRvIHJlY292ZXJ5IGp1c3QgYmVjYXVzZSBvZiBhDQo+IFNJR0tJTEwuDQoN
Ckkgc3VnZ2VzdCB3ZSBhZGQgYSBuZXcgbG9jayBzdGF0ZTogTkZTX0xPQ0tfVU5MT0NLRUQgdG8g
YWxsb3cgdGhlDQpjbGllbnQgdG8gcmVjb2duaXNlIHRoYXQgdGhlIGZhaWxlZCBvcGVyYXRpb24g
c2hvdWxkIG5vdCBiZSByZXRyaWVkLg0KDQpJbiBmYWN0LCB3ZSBtaWdodCB3YW50IHRvIGhhdmUg
bmZzNF9zZXRfcndfc3RhdGVpZCgpIHNjYW4gZm9yIHRoYXQsIGFuZA0KcmV0dXJuIGFuIGVycm9y
IHRoYXQgY291bGQgYmUgbWFkZSB0byBmYWlsIHRoZSBvcGVyYXRpb24uDQpGdXJ0aGVybW9yZSwg
bWF5YmUgX19uZnM0X2ZpbmRfbG9ja19zdGF0ZSgpIHNob3VsZCBhbHNvIGlnbm9yZSBsb2NrDQpz
dHJ1Y3R1cmVzIGluIHRoaXMgc3RhdGUgc28gdGhhdCBvbmNlIHRoZSBsb2NrIGlzIGluIHRoaXMg
c3RhdGUsIHdlDQphbHdheXMgaWdub3JlIG5ldyBhdHRlbXB0cy4NClRoaXMgY291bGQgYWxzbyBi
ZSBtYWRlIHRvIHdvcmsgd2l0aCBwTkZTOyBhbHRob3VnaCBmb3IgbW9zdCBsYXlvdXQNCnR5cGVz
LCB0aGUgbG9ja2luZyB3aWxsIG5vdCBiZSBlbmZvcmNlZCBieSB0aGUgc2VydmVyLCB3ZSBjYW4g
YXQgbGVhc3QNCnByZXZlbnQgbmV3IEkvTyByZXF1ZXN0cyBmcm9tIGJlaW5nIGdlbmVyYXRlZCBh
ZnRlciB0aGUNCk5GU19MT0NLX1VOTE9DS0VEIHN0YXRlIGhhcyBiZWVuIGRlY2xhcmVkLg0KDQpZ
b3UgY2FuJ3QgZG8gYW55IG9mIHRoZSBhYm92ZSB3aXRoIE5GU3YzLCBzaW5jZSB0aGUgbG9ja2lu
ZyBpcw0KZGVjb3VwbGVkIGZyb20gdGhlIGFjdHVhbCBJL08uIElzIGFueW9uZSBhY3R1YWxseSB1
c2luZyBOTE0gZm9yIG1pc3Npb24NCmNyaXRpY2FsIGFwcGxpY2F0aW9ucyB0aGF0IG5lZWQgdGhp
cyBraW5kIG9mIHByb3RlY3Rpb24/IChmb2xsb3d1cA0KcXVlc3Rpb246IGlzIHRoZXJlIGEgZ29v
ZCByZWFzb24gd2h5IHRoZXkgYXJlIGRvaW5nIHNvIGluc3RlYWQgb2YgdXNpbmcNCk5GU3Y0PykN
Cg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lciwgUHJp
bWFyeURhdGENCnRyb25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20NCg==

Re: [PATCH 4/4] NFS: Always send an unlock for FL_CLOSE

[Benjamin Coddington]

On 22 Feb 2017, at 11:27, Trond Myklebust wrote:

> On Wed, 2017-02-22 at 10:42 -0500, Jeff Layton wrote:
>> On Wed, 2017-02-22 at 09:10 -0500, Benjamin Coddington wrote:
>>> On 22 Feb 2017, at 8:20, Jeff Layton wrote:
>>>
>>>> On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
>>>>> NFS attempts to wait for read and write completion before
>>>>> unlocking 
>>>>> in
>>>>> order to ensure that the data returned was protected by the
>>>>> lock.  
>>>>> When
>>>>> this waiting is interrupted by a signal, the unlock may be
>>>>> skipped, 
>>>>> and
>>>>> messages similar to the following are seen in the kernel ring
>>>>> buffer:
>>>>>
>>>>> [20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
>>>>> [20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1
>>>>> fl_type=0x0 
>>>>> fl_pid=20183
>>>>> [20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1
>>>>> fl_type=0x0 
>>>>> fl_pid=20185
>>>>>
>>>>> For NFSv3, the missing unlock will cause the server to refuse 
>>>>> conflicting
>>>>> locks indefinitely.  For NFSv4, the leftover lock will be
>>>>> removed by 
>>>>> the
>>>>> server after the lease timeout.
>>>>>
>>>>> This patch fixes this issue by skipping the wait in order to 
>>>>> immediately send
>>>>> the unlock if the FL_CLOSE flag is set when signaled.
>>>>>
>>>>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>>>>> ---
>>>>>  fs/nfs/file.c | 10 +++++-----
>>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
>>>>> index a490f45df4db..df695f52bb9d 100644
>>>>> --- a/fs/nfs/file.c
>>>>> +++ b/fs/nfs/file.c
>>>>> @@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd,
>>>>> struct 
>>>>> file_lock *fl, int is_local)
>>>>>  	if (!IS_ERR(l_ctx)) {
>>>>>  		status = nfs_iocounter_wait(l_ctx);
>>>>>  		nfs_put_lock_context(l_ctx);
>>>>> -		if (status < 0)
>>>>> +		/*  NOTE: special case
>>>>> +		 * 	If we're signalled while cleaning
>>>>> up locks on process exit, we
>>>>> +		 * 	still need to complete the unlock.
>>>>> +		 */
>>>>> +		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
>>>>>  			return status;
>>>>
>>>>
>>>> Hmm, I don't know if this is safe...
>>>>
>>>> Suppose we have a bunch of buffered writeback going on, and we're
>>>> sitting here waiting for it so we can do the unlock. The task catches 
>>>> a signal, and then issues the unlock while writeback is still going on.
>>>> Another client then grabs the lock, and starts doing reads and writes
>>>> while this one is still writing back.
>>>>
>>>> I think the unlock really needs to wait until writeback is done,
>>>> regardless of whether you catch a signal or not.
>>>
>>> The only other way to do this is as you've sugggested many times -- by
>>> putting the LOCKU on a rpc_wait_queue.  But that won't work for NFSv3.
>>>
>>
>> IDGI -- why doesn't that (or something similar) not work for v3?

Because of the layering, as discussed further on.

>>> For NFSv4, the other thing to do might be to look up the lock state and 
>>> set NFS_LOCK_LOST, then unlock.  That will cause subsequent writes to
>>> not attempt recovery.
>>>
>>
>> No! LOCK_LOST is really for the case where we've lost contact with the
>> server. We don't want to get kicked into recovery just because of a
>> SIGKILL.

True, it was added for the case where contact is lost, but it does exactly
what we'd want.

If we're coming through do_unlk() with FL_CLOSE, then we've just done a
vfs_fsync() and I believe that all dirty data are now in requests queued up
and being transmitted.  The signal has had us bail out of the wait for those
pages to writeback, and I don't think new pages can be dirtied under this
lock.  So, an unlock at this point will at worst cause some of that I/O to
fail with BAD_STATEID (or OLD_STATEID?).  What we want to prevent then is
the client retrying those writes under another stateid, which is what
NFS_LOCK_LOST does.

> I suggest we add a new lock state: NFS_LOCK_UNLOCKED to allow the client
> to recognise that the failed operation should not be retried.
>
> In fact, we might want to have nfs4_set_rw_stateid() scan for that, and
> return an error that could be made to fail the operation.  Furthermore,
> maybe __nfs4_find_lock_state() should also ignore lock structures in this
> state so that once the lock is in this state, we always ignore new
> attempts.  This could also be made to work with pNFS; although for most
> layout types, the locking will not be enforced by the server, we can at
> least prevent new I/O requests from being generated after the
> NFS_LOCK_UNLOCKED state has been declared.
>
> You can't do any of the above with NFSv3, since the locking is decoupled
> from the actual I/O. Is anyone actually using NLM for mission critical
> applications that need this kind of protection? (followup question: is
> there a good reason why they are doing so instead of using NFSv4?)

Yes, people are using it.  And I don't think those people care much about
whether or not their I/O completes under the lock after a signal.  They care
that when their job on a cluster gets hung up and their scheduler kills it,
they can't submit the job again anywhere else on the cluster because the
server thinks the job's file is locked.  The only fix is to restart the
server, which is very disruptive.

So, I'm happy to work on a NFS_LOCK_UNLOCKED approach for v4 and I'd also be
happy to just leave v3 to always unlock.

Ben

Re: [PATCH 4/4] NFS: Always send an unlock for FL_CLOSE

[Jeff Layton]

On Wed, 2017-02-22 at 12:39 -0500, Benjamin Coddington wrote:
> 
> On 22 Feb 2017, at 11:27, Trond Myklebust wrote:
> 
> > On Wed, 2017-02-22 at 10:42 -0500, Jeff Layton wrote:
> > > On Wed, 2017-02-22 at 09:10 -0500, Benjamin Coddington wrote:
> > > > On 22 Feb 2017, at 8:20, Jeff Layton wrote:
> > > > 
> > > > > On Tue, 2017-02-21 at 10:39 -0500, Benjamin Coddington wrote:
> > > > > > NFS attempts to wait for read and write completion before
> > > > > > unlocking 
> > > > > > in
> > > > > > order to ensure that the data returned was protected by the
> > > > > > lock.  
> > > > > > When
> > > > > > this waiting is interrupted by a signal, the unlock may be
> > > > > > skipped, 
> > > > > > and
> > > > > > messages similar to the following are seen in the kernel ring
> > > > > > buffer:
> > > > > > 
> > > > > > [20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
> > > > > > [20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1
> > > > > > fl_type=0x0 
> > > > > > fl_pid=20183
> > > > > > [20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1
> > > > > > fl_type=0x0 
> > > > > > fl_pid=20185
> > > > > > 
> > > > > > For NFSv3, the missing unlock will cause the server to refuse 
> > > > > > conflicting
> > > > > > locks indefinitely.  For NFSv4, the leftover lock will be
> > > > > > removed by 
> > > > > > the
> > > > > > server after the lease timeout.
> > > > > > 
> > > > > > This patch fixes this issue by skipping the wait in order to 
> > > > > > immediately send
> > > > > > the unlock if the FL_CLOSE flag is set when signaled.
> > > > > > 
> > > > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > > > > > ---
> > > > > >  fs/nfs/file.c | 10 +++++-----
> > > > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > > > > > index a490f45df4db..df695f52bb9d 100644
> > > > > > --- a/fs/nfs/file.c
> > > > > > +++ b/fs/nfs/file.c
> > > > > > @@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd,
> > > > > > struct 
> > > > > > file_lock *fl, int is_local)
> > > > > >  	if (!IS_ERR(l_ctx)) {
> > > > > >  		status = nfs_iocounter_wait(l_ctx);
> > > > > >  		nfs_put_lock_context(l_ctx);
> > > > > > -		if (status < 0)
> > > > > > +		/*  NOTE: special case
> > > > > > +		 * 	If we're signalled while cleaning
> > > > > > up locks on process exit, we
> > > > > > +		 * 	still need to complete the unlock.
> > > > > > +		 */
> > > > > > +		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
> > > > > >  			return status;
> > > > > 
> > > > > 
> > > > > Hmm, I don't know if this is safe...
> > > > > 
> > > > > Suppose we have a bunch of buffered writeback going on, and we're
> > > > > sitting here waiting for it so we can do the unlock. The task catches 
> > > > > a signal, and then issues the unlock while writeback is still going on.
> > > > > Another client then grabs the lock, and starts doing reads and writes
> > > > > while this one is still writing back.
> > > > > 
> > > > > I think the unlock really needs to wait until writeback is done,
> > > > > regardless of whether you catch a signal or not.
> > > > 
> > > > The only other way to do this is as you've sugggested many times -- by
> > > > putting the LOCKU on a rpc_wait_queue.  But that won't work for NFSv3.
> > > > 
> > > 
> > > IDGI -- why doesn't that (or something similar) not work for v3?
> 
> Because of the layering, as discussed further on.
> 
> > > > For NFSv4, the other thing to do might be to look up the lock state and 
> > > > set NFS_LOCK_LOST, then unlock.  That will cause subsequent writes to
> > > > not attempt recovery.
> > > > 
> > > 
> > > No! LOCK_LOST is really for the case where we've lost contact with the
> > > server. We don't want to get kicked into recovery just because of a
> > > SIGKILL.
> 
> True, it was added for the case where contact is lost, but it does exactly
> what we'd want.
> 
> If we're coming through do_unlk() with FL_CLOSE, then we've just done a
> vfs_fsync() and I believe that all dirty data are now in requests queued up
> and being transmitted.  The signal has had us bail out of the wait for those
> pages to writeback, and I don't think new pages can be dirtied under this
> lock.  So, an unlock at this point will at worst cause some of that I/O to
> fail with BAD_STATEID (or OLD_STATEID?).  What we want to prevent then is
> the client retrying those writes under another stateid, which is what
> NFS_LOCK_LOST does.
> 
> > I suggest we add a new lock state: NFS_LOCK_UNLOCKED to allow the client
> > to recognise that the failed operation should not be retried.
> > 
> > In fact, we might want to have nfs4_set_rw_stateid() scan for that, and
> > return an error that could be made to fail the operation.  Furthermore,
> > maybe __nfs4_find_lock_state() should also ignore lock structures in this
> > state so that once the lock is in this state, we always ignore new
> > attempts.  This could also be made to work with pNFS; although for most
> > layout types, the locking will not be enforced by the server, we can at
> > least prevent new I/O requests from being generated after the
> > NFS_LOCK_UNLOCKED state has been declared.
> > 
> > You can't do any of the above with NFSv3, since the locking is decoupled
> > from the actual I/O. Is anyone actually using NLM for mission critical
> > applications that need this kind of protection? (followup question: is
> > there a good reason why they are doing so instead of using NFSv4?)
> 

Well, we do have a nfs_lock_context for all NFS versions though, and
you can get to that via the nfs_page from down in the bowels of the
pageio code. We could have the flag in there instead.

That has the benefit of moving at least some of the logic into non-
version specific code as well.

> Yes, people are using it.  And I don't think those people care much about
> whether or not their I/O completes under the lock after a signal.  They care
> that when their job on a cluster gets hung up and their scheduler kills it,
> they can't submit the job again anywhere else on the cluster because the
> server thinks the job's file is locked.  The only fix is to restart the
> server, which is very disruptive.
> 
> So, I'm happy to work on a NFS_LOCK_UNLOCKED approach for v4 and I'd also be
> happy to just leave v3 to always unlock.
> 

Well, that's where their immediate pain point is. If they have
applications that are relying on NLM locking to serialize access to the
files, then this could potentially corrupt their file data.

It may be that no one would ever notice, but those sorts of bug reports
can be very difficult to track down. If we can fix this in a way that
works across versions, then that's what I'd suggest.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 4/4] NFS: Always send an unlock for FL_CLOSE

[Benjamin Coddington]

On 22 Feb 2017, at 14:20, Jeff Layton wrote:
> Well, that's where their immediate pain point is. If they have
> applications that are relying on NLM locking to serialize access to 
> the
> files, then this could potentially corrupt their file data.
>
> It may be that no one would ever notice, but those sorts of bug 
> reports
> can be very difficult to track down. If we can fix this in a way that
> works across versions, then that's what I'd suggest.

OK, then it occurs to me that we can do this by passing in a new struct
nlmclnt_operations to nlmclnt_proc().  That can be used to supply a 
function
that should be called in rpc_task_prepare to check the iocounter.  Then 
a
global FL_CLOSE rpc_wait_queue could be used, but I probably wouldn't 
want to
wake it on every iocounter reaching zero - probably only if a lock 
context
has a flag.. or maybe there's another way to tell.

Those operations would also help further abstract NLM from NFS by adding
functions to extract the rpc_cred from the file, and obtain the file 
handle and
owner from the file.  Right now, there's some NFS stuff leaking into the 
NLM
client.

Ben