From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miklos Szeredi Subject: Re: [PATCH 3/4] locks: Set FL_CLOSE when removing flock locks on close() Date: Wed, 22 Feb 2017 14:25:21 +0100 Message-ID: References: <1487765584.2886.8.camel@redhat.com> <1487766356.2886.11.camel@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Benjamin Coddington , Trond Myklebust , Anna Schumaker , Linux NFS list , "linux-fsdevel-MogPR669STc76Z2rM5mHXA@public.gmane.org" To: Jeff Layton Return-path: In-Reply-To: <1487766356.2886.11.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Sender: linux-nfs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-fsdevel.vger.kernel.org On Wed, Feb 22, 2017 at 1:25 PM, Jeff Layton 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 >> > --- >> > 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 >> > > (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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <1487766356.2886.11.camel@redhat.com> References: <1487765584.2886.8.camel@redhat.com> <1487766356.2886.11.camel@redhat.com> From: Miklos Szeredi Date: Wed, 22 Feb 2017 14:25:21 +0100 Message-ID: Subject: Re: [PATCH 3/4] locks: Set FL_CLOSE when removing flock locks on close() To: Jeff Layton Cc: Benjamin Coddington , Trond Myklebust , Anna Schumaker , Linux NFS list , "linux-fsdevel@vget.kernel.org" Content-Type: text/plain; charset=UTF-8 List-ID: On Wed, Feb 22, 2017 at 1:25 PM, Jeff Layton 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 >> > --- >> > 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 >> > > (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