[PATCH 0/2] locks: allow mandatory locking to work with file-private locks

[PATCH 0/2] locks: allow mandatory locking to work with file-private locks

[Jeff Layton]

This patchset fixes the problems that Trond pointed out last week,
namely that you can end up deadlocking yourself if you set a
file-private lock on a file and then do some I/O on the same.

With this set, mandatory locking should work more or less as you'd
expect with file-private locks. If you set a lock on an open file
and then do some I/O on it, it won't block. If you try to lock and
do I/O on different open files, then the I/O may end up blocked.

Note that this approach is just as racy as the existing mandatory
lock implementation, but I don't think it makes anything worse there.

Jeff Layton (2):
  locks: fix locks_mandatory_locked to respect file-private locks
  locks: make locks_mandatory_area check for file-private locks

 fs/locks.c         | 22 +++++++++++++++++-----
 fs/namei.c         |  2 +-
 include/linux/fs.h | 20 ++++++++++----------
 mm/mmap.c          |  2 +-
 mm/nommu.c         |  2 +-
 5 files changed, 30 insertions(+), 18 deletions(-)

-- 
1.8.5.3

[PATCH 1/2] locks: fix locks_mandatory_locked to respect file-private locks

[Jeff Layton]

As Trond pointed out, you can currently deadlock yourself by setting a
file-private lock on a file that requires mandatory locking and then
trying to do I/O on it.

Avoid this problem by plumbing some knowledge of file-private locks into
the mandatory locking code. In order to do this, we must pass down
information about the struct file that's being used to
locks_verify_locked.

Reported-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/locks.c         |  7 ++++---
 fs/namei.c         |  2 +-
 include/linux/fs.h | 20 ++++++++++----------
 mm/mmap.c          |  2 +-
 mm/nommu.c         |  2 +-
 5 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 6fdf26a79cc8..b2b3e97b64d4 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1155,13 +1155,14 @@ EXPORT_SYMBOL(posix_lock_file_wait);
 
 /**
  * locks_mandatory_locked - Check for an active lock
- * @inode: the file to check
+ * @file: the file to check
  *
  * Searches the inode's list of locks to find any POSIX locks which conflict.
  * This function is called from locks_verify_locked() only.
  */
-int locks_mandatory_locked(struct inode *inode)
+int locks_mandatory_locked(struct file *file)
 {
+	struct inode *inode = file_inode(file);
 	fl_owner_t owner = current->files;
 	struct file_lock *fl;
 
@@ -1172,7 +1173,7 @@ int locks_mandatory_locked(struct inode *inode)
 	for (fl = inode->i_flock; fl != NULL; fl = fl->fl_next) {
 		if (!IS_POSIX(fl))
 			continue;
-		if (fl->fl_owner != owner)
+		if (fl->fl_owner != owner && fl->fl_owner != (fl_owner_t)file)
 			break;
 	}
 	spin_unlock(&inode->i_lock);
diff --git a/fs/namei.c b/fs/namei.c
index d580df2e6804..dc51bac037c9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2542,7 +2542,7 @@ static int handle_truncate(struct file *filp)
 	/*
 	 * Refuse to truncate files with mandatory locks held on them.
 	 */
-	error = locks_verify_locked(inode);
+	error = locks_verify_locked(filp);
 	if (!error)
 		error = security_path_truncate(path);
 	if (!error) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ae91dce8a547..e39d83db72c0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1912,6 +1912,11 @@ extern int current_umask(void);
 extern void ihold(struct inode * inode);
 extern void iput(struct inode *);
 
+static inline struct inode *file_inode(struct file *f)
+{
+	return f->f_inode;
+}
+
 /* /sys/fs */
 extern struct kobject *fs_kobj;
 
@@ -1921,7 +1926,7 @@ extern struct kobject *fs_kobj;
 #define FLOCK_VERIFY_WRITE 2
 
 #ifdef CONFIG_FILE_LOCKING
-extern int locks_mandatory_locked(struct inode *);
+extern int locks_mandatory_locked(struct file *);
 extern int locks_mandatory_area(int, struct inode *, struct file *, loff_t, size_t);
 
 /*
@@ -1944,10 +1949,10 @@ static inline int mandatory_lock(struct inode *ino)
 	return IS_MANDLOCK(ino) && __mandatory_lock(ino);
 }
 
-static inline int locks_verify_locked(struct inode *inode)
+static inline int locks_verify_locked(struct file *file)
 {
-	if (mandatory_lock(inode))
-		return locks_mandatory_locked(inode);
+	if (mandatory_lock(file_inode(file)))
+		return locks_mandatory_locked(file);
 	return 0;
 }
 
@@ -2008,7 +2013,7 @@ static inline int break_deleg_wait(struct inode **delegated_inode)
 }
 
 #else /* !CONFIG_FILE_LOCKING */
-static inline int locks_mandatory_locked(struct inode *inode)
+static inline int locks_mandatory_locked(struct file *file)
 {
 	return 0;
 }
@@ -2297,11 +2302,6 @@ static inline bool execute_ok(struct inode *inode)
 	return (inode->i_mode & S_IXUGO) || S_ISDIR(inode->i_mode);
 }
 
-static inline struct inode *file_inode(struct file *f)
-{
-	return f->f_inode;
-}
-
 static inline void file_start_write(struct file *file)
 {
 	if (!S_ISREG(file_inode(file)->i_mode))
diff --git a/mm/mmap.c b/mm/mmap.c
index 20ff0c33274c..5932ce961218 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1299,7 +1299,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 			/*
 			 * Make sure there are no mandatory locks on the file.
 			 */
-			if (locks_verify_locked(inode))
+			if (locks_verify_locked(file))
 				return -EAGAIN;
 
 			vm_flags |= VM_SHARED | VM_MAYSHARE;
diff --git a/mm/nommu.c b/mm/nommu.c
index 8740213b1647..a554e5a451cd 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -995,7 +995,7 @@ static int validate_mmap_request(struct file *file,
 			    (file->f_mode & FMODE_WRITE))
 				return -EACCES;
 
-			if (locks_verify_locked(file_inode(file)))
+			if (locks_verify_locked(file))
 				return -EAGAIN;
 
 			if (!(capabilities & BDI_CAP_MAP_DIRECT))
-- 
1.8.5.3

[PATCH 2/2] locks: make locks_mandatory_area check for file-private locks

[Jeff Layton]

Allow locks_mandatory_area() to handle file-private locks correctly.
If there is a file-private lock set on an open file and we're doing I/O
via the same, then that should not cause anything to block.

Handle this by first doing a non-blocking FL_ACCESS check for a
file-private lock, and then fall back to checking for a classic POSIX
lock (and possibly blocking).

Note that this approach is subject to the same races that have always
plagued mandatory locking on Linux.

Reported-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/locks.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index b2b3e97b64d4..ad37b3eb185d 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1199,19 +1199,30 @@ int locks_mandatory_area(int read_write, struct inode *inode,
 {
 	struct file_lock fl;
 	int error;
+	bool sleep = false;
 
 	locks_init_lock(&fl);
-	fl.fl_owner = current->files;
 	fl.fl_pid = current->tgid;
 	fl.fl_file = filp;
 	fl.fl_flags = FL_POSIX | FL_ACCESS;
 	if (filp && !(filp->f_flags & O_NONBLOCK))
-		fl.fl_flags |= FL_SLEEP;
+		sleep = true;
 	fl.fl_type = (read_write == FLOCK_VERIFY_WRITE) ? F_WRLCK : F_RDLCK;
 	fl.fl_start = offset;
 	fl.fl_end = offset + count - 1;
 
 	for (;;) {
+		if (filp) {
+			fl.fl_owner = (fl_owner_t)filp;
+			fl.fl_flags &= ~FL_SLEEP;
+			error = __posix_lock_file(inode, &fl, NULL);
+			if (!error)
+				break;
+		}
+
+		if (sleep)
+			fl.fl_flags |= FL_SLEEP;
+		fl.fl_owner = current->files;
 		error = __posix_lock_file(inode, &fl, NULL);
 		if (error != FILE_LOCK_DEFERRED)
 			break;
-- 
1.8.5.3

Re: [PATCH 2/2] locks: make locks_mandatory_area check for file-private locks

[Andy Lutomirski]

On Mon, Mar 10, 2014 at 6:36 AM, Jeff Layton <jlayton@redhat.com> wrote:
> Allow locks_mandatory_area() to handle file-private locks correctly.
> If there is a file-private lock set on an open file and we're doing I/O
> via the same, then that should not cause anything to block.
>
> Handle this by first doing a non-blocking FL_ACCESS check for a
> file-private lock, and then fall back to checking for a classic POSIX
> lock (and possibly blocking).

This is ugly, but it doesn't seem much worse than the existing code. :)

--Andy

Re: [PATCH 2/2] locks: make locks_mandatory_area check for file-private locks

[Jeffrey Layton]

On Mon, 10 Mar 2014 10:07:10 -0700
Andy Lutomirski <luto@amacapital.net> wrote:

> On Mon, Mar 10, 2014 at 6:36 AM, Jeff Layton <jlayton@redhat.com>
> wrote:
> > Allow locks_mandatory_area() to handle file-private locks correctly.
> > If there is a file-private lock set on an open file and we're doing
> > I/O via the same, then that should not cause anything to block.
> >
> > Handle this by first doing a non-blocking FL_ACCESS check for a
> > file-private lock, and then fall back to checking for a classic
> > POSIX lock (and possibly blocking).
> 
> This is ugly, but it doesn't seem much worse than the existing
> code. :)
> 
> --Andy


My sentiments exactly. The alternative would to add some mechanism to
check for locks that are owned by more than one owner without dropping
the i_lock in between. That's doable but it'd be pretty ugly too, and
you'd still have the existing races to contend with.

FWIW, now that I've looked at this code it seems possible to make
mandatory locking more or less race-free by having reads and writes
both implicitly set a special sort of lock on the file:

- a read lock (for both the read and write cases)

- one that wouldn't conflict with existing locks held by same process
  or with file-private locks set on the same fd

- but...the same lock could not be merged with either of the above (and
  would therefore need a different sort of ownership model).

Have rw_verify_area set such a blocking lock on the file and then
remove it once the read or write was done. It doesn't sound too hard
to do, but performance would likely suck.

That said, I don't feel terribly motivated to fix mandatory locking
aside from doing my best to avoid the problem that Trond pointed out.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/2] locks: make locks_mandatory_area check for file-private locks

[Andy Lutomirski]

On Mon, Mar 10, 2014 at 10:31 AM, Jeffrey Layton <jlayton@redhat.com> wrote:
> On Mon, 10 Mar 2014 10:07:10 -0700
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Mon, Mar 10, 2014 at 6:36 AM, Jeff Layton <jlayton@redhat.com>
>> wrote:
>> > Allow locks_mandatory_area() to handle file-private locks correctly.
>> > If there is a file-private lock set on an open file and we're doing
>> > I/O via the same, then that should not cause anything to block.
>> >
>> > Handle this by first doing a non-blocking FL_ACCESS check for a
>> > file-private lock, and then fall back to checking for a classic
>> > POSIX lock (and possibly blocking).
>>
>> This is ugly, but it doesn't seem much worse than the existing
>> code. :)
>>
>> --Andy
>
>
> My sentiments exactly. The alternative would to add some mechanism to
> check for locks that are owned by more than one owner without dropping
> the i_lock in between. That's doable but it'd be pretty ugly too, and
> you'd still have the existing races to contend with.
>
> FWIW, now that I've looked at this code it seems possible to make
> mandatory locking more or less race-free by having reads and writes
> both implicitly set a special sort of lock on the file:
>
> - a read lock (for both the read and write cases)
>
> - one that wouldn't conflict with existing locks held by same process
>   or with file-private locks set on the same fd
>
> - but...the same lock could not be merged with either of the above (and
>   would therefore need a different sort of ownership model).
>
> Have rw_verify_area set such a blocking lock on the file and then
> remove it once the read or write was done. It doesn't sound too hard
> to do, but performance would likely suck.
>
> That said, I don't feel terribly motivated to fix mandatory locking
> aside from doing my best to avoid the problem that Trond pointed out.

I think the current patch is fine.  It'll hurt performance for -omand
users a little, but this is the least of their problems.  Let someone
who actually needs the feature fix it :)

--Andy

Re: [PATCH 2/2] locks: make locks_mandatory_area check for file-private locks

[Jeffrey Layton]

On Mon, 10 Mar 2014 10:37:00 -0700
Andy Lutomirski <luto@amacapital.net> wrote:

> On Mon, Mar 10, 2014 at 10:31 AM, Jeffrey Layton <jlayton@redhat.com>
> wrote:
> > On Mon, 10 Mar 2014 10:07:10 -0700
> > Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >> On Mon, Mar 10, 2014 at 6:36 AM, Jeff Layton <jlayton@redhat.com>
> >> wrote:
> >> > Allow locks_mandatory_area() to handle file-private locks
> >> > correctly. If there is a file-private lock set on an open file
> >> > and we're doing I/O via the same, then that should not cause
> >> > anything to block.
> >> >
> >> > Handle this by first doing a non-blocking FL_ACCESS check for a
> >> > file-private lock, and then fall back to checking for a classic
> >> > POSIX lock (and possibly blocking).
> >>
> >> This is ugly, but it doesn't seem much worse than the existing
> >> code. :)
> >>
> >> --Andy
> >
> >
> > My sentiments exactly. The alternative would to add some mechanism
> > to check for locks that are owned by more than one owner without
> > dropping the i_lock in between. That's doable but it'd be pretty
> > ugly too, and you'd still have the existing races to contend with.
> >
> > FWIW, now that I've looked at this code it seems possible to make
> > mandatory locking more or less race-free by having reads and writes
> > both implicitly set a special sort of lock on the file:
> >
> > - a read lock (for both the read and write cases)
> >
> > - one that wouldn't conflict with existing locks held by same
> > process or with file-private locks set on the same fd
> >
> > - but...the same lock could not be merged with either of the above
> > (and would therefore need a different sort of ownership model).
> >
> > Have rw_verify_area set such a blocking lock on the file and then
> > remove it once the read or write was done. It doesn't sound too hard
> > to do, but performance would likely suck.
> >
> > That said, I don't feel terribly motivated to fix mandatory locking
> > aside from doing my best to avoid the problem that Trond pointed
> > out.
> 
> I think the current patch is fine.  It'll hurt performance for -omand
> users a little, but this is the least of their problems.  Let someone
> who actually needs the feature fix it :)
> 
> --Andy

In the interest of full disclosure:

There is potentially a new race here but it'd be tricky to hit it.

It's possible that you could have one thread doing a read or write,
check the inode for file-private locks and then not find any. Then,
just before doing the blocking check for classic locks, another thread
sets a fp lock on the same open file. At that point, you could
potentially deadlock if the removal of that lock ended up blocked on
the thread finishing that read or write.

That does require an especially pathological program *and* mandatory
locking. Hmm...maybe I'll do a v2 of this patch that adds a comment to
outline that race for posterity however...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/2] locks: fix locks_mandatory_locked to respect file-private locks

[J. Bruce Fields]

On Mon, Mar 10, 2014 at 09:36:46AM -0400, Jeff Layton wrote:
> As Trond pointed out, you can currently deadlock yourself by setting a
> file-private lock on a file that requires mandatory locking and then
> trying to do I/O on it.
> 
> Avoid this problem by plumbing some knowledge of file-private locks into
> the mandatory locking code. In order to do this, we must pass down
> information about the struct file that's being used to
> locks_verify_locked.
> 
> Reported-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/locks.c         |  7 ++++---
>  fs/namei.c         |  2 +-
>  include/linux/fs.h | 20 ++++++++++----------
>  mm/mmap.c          |  2 +-
>  mm/nommu.c         |  2 +-
>  5 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 6fdf26a79cc8..b2b3e97b64d4 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1155,13 +1155,14 @@ EXPORT_SYMBOL(posix_lock_file_wait);
>  
>  /**
>   * locks_mandatory_locked - Check for an active lock
> - * @inode: the file to check
> + * @file: the file to check
>   *
>   * Searches the inode's list of locks to find any POSIX locks which conflict.
>   * This function is called from locks_verify_locked() only.
>   */
> -int locks_mandatory_locked(struct inode *inode)
> +int locks_mandatory_locked(struct file *file)
>  {
> +	struct inode *inode = file_inode(file);
>  	fl_owner_t owner = current->files;
>  	struct file_lock *fl;
>  
> @@ -1172,7 +1173,7 @@ int locks_mandatory_locked(struct inode *inode)
>  	for (fl = inode->i_flock; fl != NULL; fl = fl->fl_next) {
>  		if (!IS_POSIX(fl))
>  			continue;
> -		if (fl->fl_owner != owner)
> +		if (fl->fl_owner != owner && fl->fl_owner != (fl_owner_t)file)

I had to think about that logic for a moment.

If this is our traditional lock, then fl_owner will point to our file
table.  And if it's our file-private lock, it will point to our file.
If it points to neither, it's a potentially conflicting lock.

NFS locks will also always be treated as conflicting (since fl_owner
points to some completely different kind of object in that case).

OK.

Acked-by: J. Bruce Fields <bfields@redhat.com>

--b.

>  			break;
>  	}
>  	spin_unlock(&inode->i_lock);
> diff --git a/fs/namei.c b/fs/namei.c
> index d580df2e6804..dc51bac037c9 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2542,7 +2542,7 @@ static int handle_truncate(struct file *filp)
>  	/*
>  	 * Refuse to truncate files with mandatory locks held on them.
>  	 */
> -	error = locks_verify_locked(inode);
> +	error = locks_verify_locked(filp);
>  	if (!error)
>  		error = security_path_truncate(path);
>  	if (!error) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ae91dce8a547..e39d83db72c0 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1912,6 +1912,11 @@ extern int current_umask(void);
>  extern void ihold(struct inode * inode);
>  extern void iput(struct inode *);
>  
> +static inline struct inode *file_inode(struct file *f)
> +{
> +	return f->f_inode;
> +}
> +
>  /* /sys/fs */
>  extern struct kobject *fs_kobj;
>  
> @@ -1921,7 +1926,7 @@ extern struct kobject *fs_kobj;
>  #define FLOCK_VERIFY_WRITE 2
>  
>  #ifdef CONFIG_FILE_LOCKING
> -extern int locks_mandatory_locked(struct inode *);
> +extern int locks_mandatory_locked(struct file *);
>  extern int locks_mandatory_area(int, struct inode *, struct file *, loff_t, size_t);
>  
>  /*
> @@ -1944,10 +1949,10 @@ static inline int mandatory_lock(struct inode *ino)
>  	return IS_MANDLOCK(ino) && __mandatory_lock(ino);
>  }
>  
> -static inline int locks_verify_locked(struct inode *inode)
> +static inline int locks_verify_locked(struct file *file)
>  {
> -	if (mandatory_lock(inode))
> -		return locks_mandatory_locked(inode);
> +	if (mandatory_lock(file_inode(file)))
> +		return locks_mandatory_locked(file);
>  	return 0;
>  }
>  
> @@ -2008,7 +2013,7 @@ static inline int break_deleg_wait(struct inode **delegated_inode)
>  }
>  
>  #else /* !CONFIG_FILE_LOCKING */
> -static inline int locks_mandatory_locked(struct inode *inode)
> +static inline int locks_mandatory_locked(struct file *file)
>  {
>  	return 0;
>  }
> @@ -2297,11 +2302,6 @@ static inline bool execute_ok(struct inode *inode)
>  	return (inode->i_mode & S_IXUGO) || S_ISDIR(inode->i_mode);
>  }
>  
> -static inline struct inode *file_inode(struct file *f)
> -{
> -	return f->f_inode;
> -}
> -
>  static inline void file_start_write(struct file *file)
>  {
>  	if (!S_ISREG(file_inode(file)->i_mode))
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 20ff0c33274c..5932ce961218 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1299,7 +1299,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
>  			/*
>  			 * Make sure there are no mandatory locks on the file.
>  			 */
> -			if (locks_verify_locked(inode))
> +			if (locks_verify_locked(file))
>  				return -EAGAIN;
>  
>  			vm_flags |= VM_SHARED | VM_MAYSHARE;
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 8740213b1647..a554e5a451cd 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -995,7 +995,7 @@ static int validate_mmap_request(struct file *file,
>  			    (file->f_mode & FMODE_WRITE))
>  				return -EACCES;
>  
> -			if (locks_verify_locked(file_inode(file)))
> +			if (locks_verify_locked(file))
>  				return -EAGAIN;
>  
>  			if (!(capabilities & BDI_CAP_MAP_DIRECT))
> -- 
> 1.8.5.3
> 

Re: [PATCH 0/2] locks: allow mandatory locking to work with file-private locks

[J. Bruce Fields]

On Mon, Mar 10, 2014 at 09:36:45AM -0400, Jeff Layton wrote:
> This patchset fixes the problems that Trond pointed out last week,
> namely that you can end up deadlocking yourself if you set a
> file-private lock on a file and then do some I/O on the same.
> 
> With this set, mandatory locking should work more or less as you'd
> expect with file-private locks. If you set a lock on an open file
> and then do some I/O on it, it won't block. If you try to lock and
> do I/O on different open files, then the I/O may end up blocked.
> 
> Note that this approach is just as racy as the existing mandatory
> lock implementation, but I don't think it makes anything worse there.

As another alternative, could we declare file-private locks to never be
mandatory?

The mandatory bit has only ever applied to traditional posix locks, so I
don't think there's necessarily a presumption they'd apply to this new
lock type as well.

That doesn't necessarily simplify the locks_mandatory_area case as it
then needs __posix_lock_file to be able to ignore traditional posix
locks.

--b.

Re: [PATCH 0/2] locks: allow mandatory locking to work with file-private locks

[Jeffrey Layton]

On Mon, 10 Mar 2014 15:21:46 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Mar 10, 2014 at 09:36:45AM -0400, Jeff Layton wrote:
> > This patchset fixes the problems that Trond pointed out last week,
> > namely that you can end up deadlocking yourself if you set a
> > file-private lock on a file and then do some I/O on the same.
> > 
> > With this set, mandatory locking should work more or less as you'd
> > expect with file-private locks. If you set a lock on an open file
> > and then do some I/O on it, it won't block. If you try to lock and
> > do I/O on different open files, then the I/O may end up blocked.
> > 
> > Note that this approach is just as racy as the existing mandatory
> > lock implementation, but I don't think it makes anything worse
> > there.
> 
> As another alternative, could we declare file-private locks to never
> be mandatory?
> 
> The mandatory bit has only ever applied to traditional posix locks,
> so I don't think there's necessarily a presumption they'd apply to
> this new lock type as well.
> 
> That doesn't necessarily simplify the locks_mandatory_area case as it
> then needs __posix_lock_file to be able to ignore traditional posix
> locks.
> 

Erm...I think you mean "ignore file-private locks"...

We certainly could do that, but I'm not sure I really like it any
better and it'd be harder to code that up. What would be the benefit of
doing that instead?

I'm not a real fan of mandatory locking but my aim all along has been
to allow fp locks to work as much like classic locks as possible. Is
there a compelling reason to make them different here?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/2] locks: allow mandatory locking to work with file-private locks

[J. Bruce Fields]

On Mon, Mar 10, 2014 at 03:31:29PM -0400, Jeffrey Layton wrote:
> On Mon, 10 Mar 2014 15:21:46 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Mon, Mar 10, 2014 at 09:36:45AM -0400, Jeff Layton wrote:
> > > This patchset fixes the problems that Trond pointed out last week,
> > > namely that you can end up deadlocking yourself if you set a
> > > file-private lock on a file and then do some I/O on the same.
> > > 
> > > With this set, mandatory locking should work more or less as you'd
> > > expect with file-private locks. If you set a lock on an open file
> > > and then do some I/O on it, it won't block. If you try to lock and
> > > do I/O on different open files, then the I/O may end up blocked.
> > > 
> > > Note that this approach is just as racy as the existing mandatory
> > > lock implementation, but I don't think it makes anything worse
> > > there.
> > 
> > As another alternative, could we declare file-private locks to never
> > be mandatory?
> > 
> > The mandatory bit has only ever applied to traditional posix locks,
> > so I don't think there's necessarily a presumption they'd apply to
> > this new lock type as well.
> > 
> > That doesn't necessarily simplify the locks_mandatory_area case as it
> > then needs __posix_lock_file to be able to ignore traditional posix
> > locks.
> > 
> 
> Erm...I think you mean "ignore file-private locks"...

Yep, sorry.  (well, "optionally ignore file-private locks", with the
option taken in locks_mandatory_area).

> We certainly could do that, but I'm not sure I really like it any
> better and it'd be harder to code that up. What would be the benefit of
> doing that instead?

I'd just rather limit the scope of mandatory locking where possible.

It's not a big deal I guess.

--b.

> I'm not a real fan of mandatory locking but my aim all along has been
> to allow fp locks to work as much like classic locks as possible. Is
> there a compelling reason to make them different here?
> 
> -- 
> Jeff Layton <jlayton@redhat.com>