These are small changes to the POSIX locking code. Most of them are
needed for locking support in FUSE, but also make sense independently
as bug fixes and/or cleanups.
Miklos
FUSE file locking needs to pass an inode to posix_lock_file() which is
actually the interface provided by __posix_lock_file().
This patch exports __posix_lock_file() and makes posix_lock_file() an
inline function since it doesn't actually do anything other than get
the inode from the file pointer.
Since the posix_lock_file_conf() export was just recently added, move
the single user of it to the __posix_lock_file() interface, thus
actually decreasing the number of exported functions.
Signed-off-by: Miklos Szeredi <[email protected]>
Index: linux/fs/locks.c
===================================================================
--- linux.orig/fs/locks.c 2006-03-31 18:55:10.000000000 +0200
+++ linux/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
@@ -781,7 +781,18 @@ out:
return error;
}
-static int __posix_lock_file_conf(struct inode *inode, struct file_lock *request, struct file_lock *conflock)
+/**
+ * posix_lock_file - Apply a POSIX-style lock to a file
+ * @filp: The file to apply the lock to
+ * @fl: The lock to be applied
+ * @conflock: Place to return a copy of the conflicting lock, if found.
+ *
+ * Add a POSIX style lock to a file.
+ * We merge adjacent & overlapping locks whenever possible.
+ * POSIX locks are sorted by owner task, then by starting address
+ */
+int __posix_lock_file(struct inode *inode, struct file_lock *request,
+ struct file_lock *conflock)
{
struct file_lock *fl;
struct file_lock *new_fl, *new_fl2;
@@ -964,36 +975,7 @@ static int __posix_lock_file_conf(struct
locks_free_lock(new_fl2);
return error;
}
-
-/**
- * posix_lock_file - Apply a POSIX-style lock to a file
- * @filp: The file to apply the lock to
- * @fl: The lock to be applied
- *
- * Add a POSIX style lock to a file.
- * We merge adjacent & overlapping locks whenever possible.
- * POSIX locks are sorted by owner task, then by starting address
- */
-int posix_lock_file(struct file *filp, struct file_lock *fl)
-{
- return __posix_lock_file_conf(filp->f_dentry->d_inode, fl, NULL);
-}
-EXPORT_SYMBOL(posix_lock_file);
-
-/**
- * posix_lock_file_conf - Apply a POSIX-style lock to a file
- * @filp: The file to apply the lock to
- * @fl: The lock to be applied
- * @conflock: Place to return a copy of the conflicting lock, if found.
- *
- * Except for the conflock parameter, acts just like posix_lock_file.
- */
-int posix_lock_file_conf(struct file *filp, struct file_lock *fl,
- struct file_lock *conflock)
-{
- return __posix_lock_file_conf(filp->f_dentry->d_inode, fl, conflock);
-}
-EXPORT_SYMBOL(posix_lock_file_conf);
+EXPORT_SYMBOL(__posix_lock_file);
/**
* posix_lock_file_wait - Apply a POSIX-style lock to a file
@@ -1081,7 +1063,7 @@ int locks_mandatory_area(int read_write,
fl.fl_end = offset + count - 1;
for (;;) {
- error = __posix_lock_file_conf(inode, &fl, NULL);
+ error = __posix_lock_file(inode, &fl, NULL);
if (error != -EAGAIN)
break;
if (!(fl.fl_flags & FL_SLEEP))
Index: linux/fs/nfsd/nfs4state.c
===================================================================
--- linux.orig/fs/nfsd/nfs4state.c 2006-03-31 18:55:10.000000000 +0200
+++ linux/fs/nfsd/nfs4state.c 2006-03-31 18:55:33.000000000 +0200
@@ -2754,7 +2754,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struc
* locks_copy_lock: */
conflock.fl_ops = NULL;
conflock.fl_lmops = NULL;
- status = posix_lock_file_conf(filp, &file_lock, &conflock);
+ status = __posix_lock_file(filp->f_dentry->d_inode, &file_lock,
+ &conflock);
dprintk("NFSD: nfsd4_lock: posix_lock_file_conf status %d\n",status);
switch (-status) {
case 0: /* success! */
Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h 2006-03-31 18:55:10.000000000 +0200
+++ linux/include/linux/fs.h 2006-03-31 18:55:33.000000000 +0200
@@ -763,8 +763,7 @@ extern void locks_copy_lock(struct file_
extern void locks_remove_posix(struct file *, fl_owner_t);
extern void locks_remove_flock(struct file *);
extern int posix_test_lock(struct file *, struct file_lock *, struct file_lock *);
-extern int posix_lock_file_conf(struct file *, struct file_lock *, struct file_lock *);
-extern int posix_lock_file(struct file *, struct file_lock *);
+extern int __posix_lock_file(struct inode *, struct file_lock *, struct file_lock *);
extern int posix_lock_file_wait(struct file *, struct file_lock *);
extern int posix_unblock_lock(struct file *, struct file_lock *);
extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl);
@@ -775,6 +774,11 @@ extern int lease_modify(struct file_lock
extern int lock_may_read(struct inode *, loff_t start, unsigned long count);
extern int lock_may_write(struct inode *, loff_t start, unsigned long count);
+static inline int posix_lock_file(struct file *filp, struct file_lock *fl)
+{
+ return __posix_lock_file(filp->f_dentry->d_inode, fl, NULL);
+}
+
struct fasync_struct {
int magic;
int fa_fd;
posix_lock_file() was too cautious, failing operations on OOM, even if
they didn't actually require an allocation.
This has the disadvantage, that a failing unlock on process exit could
lead to a memory leak. There are two possibilites for this:
- filesystem implements .lock() and calls back to posix_lock_file().
On cleanup of files_struct locks_remove_posix() is called which should
remove all locks belonging to files_struct. However if filesystem
calls posix_lock_file() which fails, then those locks will never be
freed.
- if a file is closed while a lock is blocked, then after acquiring
fcntl_setlk() will undo the lock. But this unlock itself might fail
on OOM, again possibly leaking the lock.
The solution is to move the checking of the allocations until after it
is sure that they will be needed. This will solve the above problem
since unlock will always succeed unless it splits an existing region.
Signed-off-by: Miklos Szeredi <[email protected]>
Index: linux/fs/locks.c
===================================================================
--- linux.orig/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
+++ linux/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
@@ -835,14 +835,7 @@ int __posix_lock_file(struct inode *inod
if (request->fl_flags & FL_ACCESS)
goto out;
- error = -ENOLCK; /* "no luck" */
- if (!(new_fl && new_fl2))
- goto out;
-
/*
- * We've allocated the new locks in advance, so there are no
- * errors possible (and no blocking operations) from here on.
- *
* Find the first old lock with the same owner as the new lock.
*/
@@ -939,6 +932,18 @@ int __posix_lock_file(struct inode *inod
before = &fl->fl_next;
}
+ /*
+ * The above code only modifies existing locks in case of
+ * merging or replacing. If new lock(s) need to be inserted
+ * all modifications are done bellow this, so it's safe yet to
+ * bail out.
+ */
+ error = -ENOLCK; /* "no luck" */
+ if (!added && request->fl_type != F_UNLCK && !new_fl)
+ goto out;
+ if (right && left == right && !new_fl2)
+ goto out;
+
error = 0;
if (!added) {
if (request->fl_type == F_UNLCK)
posix_lock_file() always allocates new locks in advance, even if it's
easy to determine that no allocations will be needed.
Optimize these cases:
- FL_ACCESS flag is set
- Unlocking the whole range
Signed-off-by: Miklos Szeredi <[email protected]>
Index: linux/fs/locks.c
===================================================================
--- linux.orig/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
+++ linux/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
@@ -795,7 +795,8 @@ int __posix_lock_file(struct inode *inod
struct file_lock *conflock)
{
struct file_lock *fl;
- struct file_lock *new_fl, *new_fl2;
+ struct file_lock *new_fl = NULL;
+ struct file_lock *new_fl2 = NULL;
struct file_lock *left = NULL;
struct file_lock *right = NULL;
struct file_lock **before;
@@ -804,9 +805,15 @@ int __posix_lock_file(struct inode *inod
/*
* We may need two file_lock structures for this operation,
* so we get them in advance to avoid races.
+ *
+ * In some cases we can be sure, that no new locks will be needed
*/
- new_fl = locks_alloc_lock();
- new_fl2 = locks_alloc_lock();
+ if (!(request->fl_flags & FL_ACCESS) &&
+ (request->fl_type != F_UNLCK ||
+ request->fl_start != 0 || request->fl_end != OFFSET_MAX)) {
+ new_fl = locks_alloc_lock();
+ new_fl2 = locks_alloc_lock();
+ }
lock_kernel();
if (request->fl_type != F_UNLCK) {
locks_remove_posix() can use posix_lock_file() instead of doing the
lock removal by hand. posix_lock_file() now does exacly the same.
The comment about pids no longer applies, posix_lock_file() takes only
the owner into account.
Signed-off-by: Miklos Szeredi <[email protected]>
Index: linux/fs/locks.c
===================================================================
--- linux.orig/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
+++ linux/fs/locks.c 2006-03-31 18:55:34.000000000 +0200
@@ -1866,15 +1866,15 @@ out:
*/
void locks_remove_posix(struct file *filp, fl_owner_t owner)
{
- struct file_lock lock, **before;
+ struct file_lock lock;
+ struct inode *inode = filp->f_dentry->d_inode;
/*
* If there are no locks held on this file, we don't need to call
* posix_lock_file(). Another process could be setting a lock on this
* file at the same time, but we wouldn't remove that lock anyway.
*/
- before = &filp->f_dentry->d_inode->i_flock;
- if (*before == NULL)
+ if (!inode->i_flock)
return;
lock.fl_type = F_UNLCK;
@@ -1887,25 +1887,11 @@ void locks_remove_posix(struct file *fil
lock.fl_ops = NULL;
lock.fl_lmops = NULL;
- if (filp->f_op && filp->f_op->lock != NULL) {
+ if (filp->f_op && filp->f_op->lock != NULL)
filp->f_op->lock(filp, F_SETLK, &lock);
- goto out;
- }
+ else
+ __posix_lock_file(inode, &lock, NULL);
- /* Can't use posix_lock_file here; we need to remove it no matter
- * which pid we have.
- */
- lock_kernel();
- while (*before != NULL) {
- struct file_lock *fl = *before;
- if (IS_POSIX(fl) && posix_same_owner(fl, &lock)) {
- locks_delete_lock(before);
- continue;
- }
- before = &fl->fl_next;
- }
- unlock_kernel();
-out:
if (lock.fl_ops && lock.fl_ops->fl_release_private)
lock.fl_ops->fl_release_private(&lock);
}
On Fri, 2006-03-31 at 19:30 +0200, Miklos Szeredi wrote:
> posix_lock_file() was too cautious, failing operations on OOM, even if
> they didn't actually require an allocation.
>
> This has the disadvantage, that a failing unlock on process exit could
> lead to a memory leak. There are two possibilites for this:
>
> - filesystem implements .lock() and calls back to posix_lock_file().
> On cleanup of files_struct locks_remove_posix() is called which should
> remove all locks belonging to files_struct. However if filesystem
> calls posix_lock_file() which fails, then those locks will never be
> freed.
>
> - if a file is closed while a lock is blocked, then after acquiring
> fcntl_setlk() will undo the lock. But this unlock itself might fail
> on OOM, again possibly leaking the lock.
>
> The solution is to move the checking of the allocations until after it
> is sure that they will be needed. This will solve the above problem
> since unlock will always succeed unless it splits an existing region.
>
> Signed-off-by: Miklos Szeredi <[email protected]>
>
> Index: linux/fs/locks.c
> ===================================================================
> --- linux.orig/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
> +++ linux/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
> @@ -835,14 +835,7 @@ int __posix_lock_file(struct inode *inod
> if (request->fl_flags & FL_ACCESS)
> goto out;
>
> - error = -ENOLCK; /* "no luck" */
> - if (!(new_fl && new_fl2))
> - goto out;
> -
> /*
> - * We've allocated the new locks in advance, so there are no
> - * errors possible (and no blocking operations) from here on.
> - *
> * Find the first old lock with the same owner as the new lock.
> */
>
> @@ -939,6 +932,18 @@ int __posix_lock_file(struct inode *inod
> before = &fl->fl_next;
> }
>
> + /*
> + * The above code only modifies existing locks in case of
> + * merging or replacing. If new lock(s) need to be inserted
> + * all modifications are done bellow this, so it's safe yet to
> + * bail out.
> + */
> + error = -ENOLCK; /* "no luck" */
> + if (!added && request->fl_type != F_UNLCK && !new_fl)
> + goto out;
> + if (right && left == right && !new_fl2)
> + goto out;
> +
> error = 0;
> if (!added) {
> if (request->fl_type == F_UNLCK)
NACK.
This changes the behaviour of F_UNLCK. Currently, if the allocation
fails, the inode locking state remains unchanged. With your change, an
unlock request may end up unlocking part of the inode, but not the rest.
Furthermore, AFAICS you are now able to Oops if (!added && !new_fl).
Cheers,
Trond
On Fri, 2006-03-31 at 13:27 -0500, Trond Myklebust wrote:
> On Fri, 2006-03-31 at 19:30 +0200, Miklos Szeredi wrote:
> > posix_lock_file() was too cautious, failing operations on OOM, even if
> > they didn't actually require an allocation.
> >
> > This has the disadvantage, that a failing unlock on process exit could
> > lead to a memory leak. There are two possibilites for this:
> >
> > - filesystem implements .lock() and calls back to posix_lock_file().
> > On cleanup of files_struct locks_remove_posix() is called which should
> > remove all locks belonging to files_struct. However if filesystem
> > calls posix_lock_file() which fails, then those locks will never be
> > freed.
> >
> > - if a file is closed while a lock is blocked, then after acquiring
> > fcntl_setlk() will undo the lock. But this unlock itself might fail
> > on OOM, again possibly leaking the lock.
> >
> > The solution is to move the checking of the allocations until after it
> > is sure that they will be needed. This will solve the above problem
> > since unlock will always succeed unless it splits an existing region.
> >
> > Signed-off-by: Miklos Szeredi <[email protected]>
> >
> > Index: linux/fs/locks.c
> > ===================================================================
> > --- linux.orig/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
> > +++ linux/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
> > @@ -835,14 +835,7 @@ int __posix_lock_file(struct inode *inod
> > if (request->fl_flags & FL_ACCESS)
> > goto out;
> >
> > - error = -ENOLCK; /* "no luck" */
> > - if (!(new_fl && new_fl2))
> > - goto out;
> > -
> > /*
> > - * We've allocated the new locks in advance, so there are no
> > - * errors possible (and no blocking operations) from here on.
> > - *
> > * Find the first old lock with the same owner as the new lock.
> > */
> >
> > @@ -939,6 +932,18 @@ int __posix_lock_file(struct inode *inod
> > before = &fl->fl_next;
> > }
> >
> > + /*
> > + * The above code only modifies existing locks in case of
> > + * merging or replacing. If new lock(s) need to be inserted
> > + * all modifications are done bellow this, so it's safe yet to
> > + * bail out.
> > + */
> > + error = -ENOLCK; /* "no luck" */
> > + if (!added && request->fl_type != F_UNLCK && !new_fl)
> > + goto out;
> > + if (right && left == right && !new_fl2)
> > + goto out;
> > +
> > error = 0;
> > if (!added) {
> > if (request->fl_type == F_UNLCK)
>
> NACK.
>
> This changes the behaviour of F_UNLCK. Currently, if the allocation
> fails, the inode locking state remains unchanged. With your change, an
> unlock request may end up unlocking part of the inode, but not the rest.
>
> Furthermore, AFAICS you are now able to Oops if (!added && !new_fl).
Sorry: the Oops turned out to be a mirage.
However you are also changing the behaviour of F_SETLK for the case
where the user is trying to up/downgrade a set of existing READ/WRITE
locks. Again you may end up with a situation where some of the existing
locks get up/downgraded, and yet the lock request fails.
Cheers,
Trond
> NACK.
>
> This changes the behaviour of F_UNLCK. Currently, if the allocation
> fails, the inode locking state remains unchanged. With your change, an
> unlock request may end up unlocking part of the inode, but not the rest.
No, look more closer. There are two cases:
- some locks are partially or completely removed
- the unlock splits an existing lock in two.
In the first case no new locks are needed. In the second, no locks
are modified prior to the check.
Miklos
> However you are also changing the behaviour of F_SETLK for the case
> where the user is trying to up/downgrade a set of existing READ/WRITE
> locks. Again you may end up with a situation where some of the existing
> locks get up/downgraded, and yet the lock request fails.
Can you please point out the exact case when this happens?
I've carefully reviewd the code, and found none.
Thanks,
Miklos
On Fri, 2006-03-31 at 21:25 +0200, Miklos Szeredi wrote:
> > NACK.
> >
> > This changes the behaviour of F_UNLCK. Currently, if the allocation
> > fails, the inode locking state remains unchanged. With your change, an
> > unlock request may end up unlocking part of the inode, but not the rest.
>
> No, look more closer. There are two cases:
>
> - some locks are partially or completely removed
>
> - the unlock splits an existing lock in two.
>
> In the first case no new locks are needed. In the second, no locks
> are modified prior to the check.
Consider something like
fcntl(SETLK, 0, 100)
fcntl(SETLK, 0, 100)
fcntl(SETLK, 0, 100)
> > In the first case no new locks are needed. In the second, no locks
> > are modified prior to the check.
>
> Consider something like
>
> fcntl(SETLK, 0, 100)
> fcntl(SETLK, 0, 100)
> fcntl(SETLK, 0, 100)
Huh? What is the type of lock in each case.
But anyway your example is no good. If the new lock completely covers
the previous one, then the old lock will simply be adjusted and no new
lock is inserted.
Miklos
VFS,locks: locks: don't unnecessarily fail posix lock operations
---
fs/locks.c | 27 ++++++++++++++++++++-------
1 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 6ba3756..973c1d9 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -839,10 +839,6 @@ static int __posix_lock_file_conf(struct
if (request->fl_flags & FL_ACCESS)
goto out;
- error = -ENOLCK; /* "no luck" */
- if (!(new_fl && new_fl2))
- goto out;
-
/*
* We've allocated the new locks in advance, so there are no
* errors possible (and no blocking operations) from here on.
@@ -943,9 +939,25 @@ static int __posix_lock_file_conf(struct
before = &fl->fl_next;
}
- error = 0;
- if (!added) {
- if (request->fl_type == F_UNLCK)
+ if (request->fl_type == F_UNLCK) {
+ if (!added)
+ goto out;
+ if (!new_fl2 && right && left == right) {
+ new_fl2 = new_fl;
+ error = -ENOLCK;
+ if (!new_fl2)
+ goto out;
+ new_fl = NULL;
+ }
+ } else if (!added) {
+ error = -ENOLCK;
+ if (!new_fl) {
+ new_fl = new_fl2;
+ if (!new_fl)
+ goto out;
+ new_fl2 = NULL;
+ }
+ if (!new_fl2 && right && left == right)
goto out;
locks_copy_lock(new_fl, request);
locks_insert_lock(before, new_fl);
@@ -968,6 +980,7 @@ static int __posix_lock_file_conf(struct
left->fl_end = request->fl_start - 1;
locks_wake_up_blocks(left);
}
+ error = 0;
out:
unlock_kernel();
/*