On Mon, Jan 14, 2008 at 09:29:39PM -0700, Matthew Wilcox wrote:
quoted text >
> Reduce the spaghetti-like nature of flock_lock_file by making the chunk
> of code labelled find_conflict into its own function. Also allocate
> memory before taking the kernel lock in preparation for switching to a
> normal spinlock.
If we did those two steps separately, would the result be two simpler
patches?
quoted text >
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
>
> diff --git a/fs/locks.c b/fs/locks.c
> index b681459..bc691e5 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -699,6 +699,33 @@ next_task:
> return 0;
> }
>
> +/*
> + * A helper function for flock_lock_file(). It searches the list of locks
> + * for @inode looking for one which would conflict with @request.
> + * If it does not find one, it returns the address where the requested lock
> + * should be inserted. If it does find a conflicting lock, it returns NULL.
> + */
The return value is the reverse of what I'd naively expect--I don't
expect something named flock_find_conflict to return something exactly
when conflict is *not* found, but I don't see a better way to do it.
Perhaps there's a better name?
--b.
quoted text > +static struct file_lock **
> +flock_find_conflict(struct inode *inode, struct file_lock *request)
> +{
> + struct file_lock **before;
> +
> + for_each_lock(inode, before) {
> + struct file_lock *fl = *before;
> + if (IS_POSIX(fl))
> + break;
> + if (IS_LEASE(fl))
> + continue;
> + if (!flock_locks_conflict(request, fl))
> + continue;
> +
> + if (request->fl_flags & FL_SLEEP)
> + locks_insert_block(fl, request);
> + return NULL;
> + }
> + return before;
> +}
> +
> /* Try to create a FLOCK lock on filp. We always insert new FLOCK locks
> * after any leases, but before any posix locks.
> *
> @@ -714,18 +741,21 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
> int error = 0;
> int found = 0;
>
> - lock_kernel();
> - if (request->fl_flags & FL_ACCESS)
> - goto find_conflict;
> + if (request->fl_flags & FL_ACCESS) {
> + lock_kernel();
> + before = flock_find_conflict(inode, request);
> + unlock_kernel();
> + return before ? 0 : -EAGAIN;
> + }
>
> if (request->fl_type != F_UNLCK) {
> - error = -ENOMEM;
> new_fl = locks_alloc_lock();
> - if (new_fl == NULL)
> - goto out;
> - error = 0;
> + if (!new_fl)
> + return -ENOMEM;
> }
>
> + lock_kernel();
> +
> for_each_lock(inode, before) {
> struct file_lock *fl = *before;
> if (IS_POSIX(fl))
> @@ -754,22 +784,12 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
> if (found)
> cond_resched();
>
> -find_conflict:
> - for_each_lock(inode, before) {
> - struct file_lock *fl = *before;
> - if (IS_POSIX(fl))
> - break;
> - if (IS_LEASE(fl))
> - continue;
> - if (!flock_locks_conflict(request, fl))
> - continue;
> + before = flock_find_conflict(inode, request);
> + if (!before) {
> error = -EAGAIN;
> - if (request->fl_flags & FL_SLEEP)
> - locks_insert_block(fl, request);
> goto out;
> }
> - if (request->fl_flags & FL_ACCESS)
> - goto out;
> +
> locks_copy_lock(new_fl, request);
> locks_insert_lock(before, new_fl);
> new_fl = NULL;
> --
> Intel are signing my paycheques ... these opinions are still mine
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours. We can't possibly take such
> a retrograde step."
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to
majordomo@vger.kernel.org
More majordomo info at
http://vger.kernel.org/majordomo-info.html
