diff mbox

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

Message ID c43ab767372bd73a60a4e1c97b39bb6fc0722590.1487691345.git.bcodding@redhat.com
State New
Headers show

Commit Message

Benjamin Coddington Feb. 21, 2017, 3:39 p.m. UTC
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(-)

Comments

Jeff Layton Feb. 22, 2017, 12:13 p.m. UTC | #1
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>
--
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 Feb. 22, 2017, 12:25 p.m. UTC | #2
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,
Miklos Szeredi Feb. 22, 2017, 1:25 p.m. UTC | #3
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
--
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
Benjamin Coddington Feb. 22, 2017, 1:27 p.m. UTC | #4
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
--
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
diff mbox

Patch

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,
 	};