08 Oct, 2014
12 commits
-
Eliminate the need for a return pointer.
Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig -
Like flock locks, leases are owned by the file description. Now that the
i_have_this_lease check in __break_lease is gone, we don't actually use
the fl_owner for leases for anything. So, it's now safe to set this more
appropriately to the same value as the fl_file.While we're at it, fix up the comments over the fl_owner_t definition
since they're rather out of date.Signed-off-by: Jeff Layton
-
Christoph suggests:
"Add a return value to lm_break so that the lock manager can tell the
core code "you can delete this lease right now". That gets rid of
the games with the timeout which require all kinds of race avoidance
code in the users."Do that here and have the nfsd lease break routine use it when it detects
that there was a race between setting up the lease and it being broken.Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig -
Eliminate an unneeded "flock" variable. We can use "fl" as a loop cursor
everywhere. Add a any_leases_conflict helper function as well to
consolidate a bit of code.Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig -
I think that the intent of this code was to ensure that a process won't
deadlock if it has one fd open with a lease on it and then breaks that
lease by opening another fd. In that case it'll treat the __break_lease
call as if it were non-blocking.This seems wrong -- the process could (for instance) be multithreaded
and managing different fds via different threads. I also don't see any
mention of this limitation in the (somewhat sketchy) documentation.Remove the check and the non-blocking behavior when i_have_this_lease
is true.Signed-off-by: Jeff Layton
-
There was only one place where we still could free a file_lock while
holding the i_lock -- lease_modify. Add a new list_head argument to the
lm_change operation, pass in a private list when calling it, and fix
those callers to dispose of the list once the lock has been dropped.Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig -
Now that we have a saner internal API for managing leases, we no longer
need to mandate that the inode->i_lock be held over most of the lease
code. Push it down into generic_add_lease and generic_delete_lease.Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig -
...and move the fasync setup into it for fcntl lease calls. At the same
time, change the semantics of how the file_lock double-pointer is
handled. Up until now, on a successful lease return you got a pointer to
the lock on the list. This is bad, since that pointer can no longer be
relied on as valid once the inode->i_lock has been released.Change the code to instead just zero out the pointer if the lease we
passed in ended up being used. Then the callers can just check to see
if it's NULL after the call and free it if it isn't.The priv argument has the same semantics. The lm_setup function can
zero the pointer out to signal to the caller that it should not be
freed after the function returns.Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig -
In later patches, we're going to add a new lock_manager_operation to
finish setting up the lease while still holding the i_lock. To do
this, we'll need to pass a little bit of info in the fcntl setlease
case (primarily an fasync structure). Plumb the extra pointer into
there in advance of that.We declare this pointer as a void ** to make it clear that this is
private info, and that the caller isn't required to set this unless
the lm_setup specifically requires it.Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig -
Some of the latter paragraphs seem ambiguous and just plain wrong.
In particular the break_lease comment makes no sense. We call
break_lease (and break_deleg) from all sorts of vfs-layer functions,
so there is clearly such a method.Also get rid of some of the other comments about what's needed for
a full implementation.Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig -
Ensure that it's OK to pass in a NULL file_lock double pointer on
a F_UNLCK request and convert the vfs_setlease F_UNLCK callers to
do just that.Finally, turn the BUG_ON in generic_setlease into a WARN_ON_ONCE
with an error return. That's a problem we can handle without
crashing the box if it occurs.Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig -
lease_get_mtime is called without the i_lock held, so there's no
guarantee about the stability of the list. Between the time when we
assign "flock" and then dereference it to check whether it's a lease
and for write, the lease could be freed.Ensure that that doesn't occur by taking the i_lock before trying
to check the lease.Cc: J. Bruce Fields
Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig
10 Sep, 2014
6 commits
-
security_file_set_fowner always returns 0, so make it f_setown and
__f_setown void return functions and fix up the error handling in the
callers.Cc: linux-security-module@vger.kernel.org
Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig -
There are no callers of these functions.
Signed-off-by: Jeff Layton
-
Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
fl_lmops field in file_lock for checking nfsd4 lockowner.But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
of conflicting locks) causes the fl_lmops of conflock always be NULL.Also, commit 0996905f93 (lockd: posix_test_lock() should not call
locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.Make sure copy the private information by fl_copy_lock() in struct
file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().Jeff advice, "Set fl_lmops on conflocks, but don't set fl_ops.
fl_ops are superfluous, since they are callbacks into the filesystem.
There should be no need to bother the filesystem at all with info
in a conflock. But, lock _ownership_ matters for conflocks and that's
indicated by the fl_lmops. So you really do want to copy the fl_lmops
for conflocks I think."v5: add missing calling of locks_release_private() in nlmsvc_testlock()
v4: only copy fl_lmops for conflock, don't copy fl_opsSigned-off-by: Kinglong Mee
Signed-off-by: Jeff Layton -
NFSD or other lockmanager may increase the owner's reference,
so adds two new options for copying and releasing owner.v5: change order from 2/6 to 3/6
v4: rename lm_copy_owner/lm_release_owner to lm_get_owner/lm_put_ownerReviewed-by: Jeff Layton
Signed-off-by: Kinglong Mee
Signed-off-by: Jeff Layton -
Jeff advice, " Right now __locks_copy_lock is only used to copy
conflocks. It would be good to rename that to something more
distinct (i.e.locks_copy_conflock), to make it clear that we're
generating a conflock there."v5: change order from 3/6 to 2/6
v4: new patch only renaming function nameSigned-off-by: Kinglong Mee
Signed-off-by: Jeff Layton -
The argument to locks_unlink_lock can't be just any pointer to a
pointer. It must be a pointer to the fl_next field in the previous
lock in the list.Cc: # v3.15+
Signed-off-by: Jeff Layton
Reviewed-by: Christoph Hellwig
14 Aug, 2014
3 commits
-
There's no need to call locks_free_lock here while still holding the
i_lock. Defer that until the lock has been dropped.Acked-by: J. Bruce Fields
Signed-off-by: Jeff Layton -
In commit 72f98e72551fa (locks: turn lock_flocks into a spinlock), we
moved from using the BKL to a global spinlock. With this change, we lost
the ability to block in the fl_release_private operation.This is problematic for NFS (and probably some other filesystems as
well). Add a new list_head argument to locks_delete_lock. If that
argument is non-NULL, then queue any locks that we want to free to the
list instead of freeing them.Then, add a new locks_dispose_list function that will walk such a list
and call locks_free_lock on them after the i_lock has been dropped.Finally, change all of the callers of locks_delete_lock to pass in a
list_head, except for lease_modify. That function can be called long
after the i_lock has been acquired. Deferring the freeing of a lease
after unlocking it in that function is non-trivial until we overhaul
some of the spinlocking in the lease code.Currently though, no filesystem that sets fl_release_private supports
leases, so this is not currently a problem. We'll eventually want to
make the same change in the lease code, but it needs a lot more work
before we can reasonably do so.Acked-by: J. Bruce Fields
Signed-off-by: Jeff Layton -
Currently in the case where a new file lock completely replaces the old
one, we end up overwriting the existing lock with the new info. This
means that we have to call fl_release_private inside i_lock. Change the
code to instead copy the info to new_fl, insert that lock into the
correct spot and then delete the old lock. In a later patch, we'll defer
the freeing of the old lock until after the i_lock has been dropped.Acked-by: J. Bruce Fields
Signed-off-by: Jeff Layton
12 Aug, 2014
2 commits
-
All callers of locks_copy_lock pass in a brand new file_lock struct, so
there's no need to call locks_release_private on it. Replace that with
a warning that fires in the event that we receive a target lock that
doesn't look like it's properly initialized.Acked-by: J. Bruce Fields
Signed-off-by: Jeff Layton -
Now that they are a distinct lease type, show them as such.
Cc: J. Bruce Fields
Signed-off-by: Jeff Layton
14 Jul, 2014
1 commit
-
Signed-off-by: Christoph Hellwig
Signed-off-by: Jeff Layton
11 Jun, 2014
1 commit
-
This fixes a regression due to commit 130d1f956ab3 (locks: ensure that
fl_owner is always initialized properly in flock and lease codepaths). I
had mistakenly thought that the fl_owner wasn't used in the lease code,
but I missed the place in __break_lease that does use it.The i_have_this_lease check in generic_add_lease uses it. While I'm not
sure that check is terribly helpful [1], reset it back to using
current->files in order to ensure that there's no behavior change here.[1]: leases are owned by the file description. It's possible that this
is a threaded program, and the lease breaker and the task that
would handle the signal are different, even if they have the same
file table. So, there is the potential for false positives with
this check.Fixes: 130d1f956ab3 (locks: ensure that fl_owner is always initialized properly in flock and lease codepaths)
Signed-off-by: Jeff Layton
02 Jun, 2014
3 commits
-
v2: add a __break_lease tracepoint for non-blocking case
Recently, I needed these to help track down a softlockup when recalling a
delegation, but they might be helpful in other situations as well.Cc: "J. Bruce Fields"
Signed-off-by: Jeff Layton -
Replace seq_printf where possible
Cc: Jeff Layton
Cc: Alexander Viro
Cc: Andrew Morton
Signed-off-by: Fabian Frederick
Signed-off-by: Jeff Layton -
Currently, the fl_owner isn't set for flock locks. Some filesystems use
byte-range locks to simulate flock locks and there is a common idiom in
those that does:fl->fl_owner = (fl_owner_t)filp;
fl->fl_start = 0;
fl->fl_end = OFFSET_MAX;Since flock locks are generally "owned" by the open file description,
move this into the common flock lock setup code. The fl_start and fl_end
fields are already set appropriately, so remove the unneeded setting of
that in flock ops in those filesystems as well.Finally, the lease code also sets the fl_owner as if they were owned by
the process and not the open file description. This is incorrect as
leases have the same ownership semantics as flock locks. Set them the
same way. The lease code doesn't actually use the fl_owner value for
anything, so this is more for consistency's sake than a bugfix.Reported-by: Trond Myklebust
Signed-off-by: Jeff Layton
Acked-by: Greg Kroah-Hartman (Staging portion)
Acked-by: J. Bruce Fields
09 May, 2014
1 commit
-
v2: replace missing break in switch statement (as pointed out by Dave
Jones)commit bce7560d4946 (locks: consolidate checks for compatible
filp->f_mode values in setlk handlers) introduced a regression in the
F_GETLK handler.flock64_to_posix_lock is a shared codepath between F_GETLK and F_SETLK,
but the f_mode checks should only be applicable to the F_SETLK codepaths
according to POSIX.Instead of just reverting the patch, add a new function to do this
checking and have the F_SETLK handlers call it.Cc: Dave Jones
Reported-and-Tested-by: Reuben Farrelly
Signed-off-by: Jeff Layton
24 Apr, 2014
1 commit
-
File-private locks have been re-christened as "open file description"
locks. Finish the symbol name cleanup in the internal implementation.Signed-off-by: Jeff Layton
22 Apr, 2014
1 commit
-
File-private locks have been merged into Linux for v3.15, and *now*
people are commenting that the name and macro definitions for the new
file-private locks suck....and I can't even disagree. The names and command macros do suck.
We're going to have to live with these for a long time, so it's
important that we be happy with the names before we're stuck with them.
The consensus on the lists so far is that they should be rechristened as
"open file description locks".The name isn't a big deal for the kernel, but the command macros are not
visually distinct enough from the traditional POSIX lock macros. The
glibc and documentation folks are recommending that we change them to
look like F_OFD_{GETLK|SETLK|SETLKW}. That lessens the chance that a
programmer will typo one of the commands wrong, and also makes it easier
to spot this difference when reading code.This patch makes the following changes that I think are necessary before
v3.15 ships:1) rename the command macros to their new names. These end up in the uapi
headers and so are part of the external-facing API. It turns out that
glibc doesn't actually use the fcntl.h uapi header, but it's hard to
be sure that something else won't. Changing it now is safest.2) make the the /proc/locks output display these as type "OFDLCK"
Cc: Michael Kerrisk
Cc: Christoph Hellwig
Cc: Carlos O'Donell
Cc: Stefan Metzmacher
Cc: Andy Lutomirski
Cc: Frank Filz
Cc: Theodore Ts'o
Signed-off-by: Jeff Layton
15 Apr, 2014
1 commit
-
A fl->fl_break_time of 0 has a special meaning to the lease break code
that basically means "never break the lease". knfsd uses this to ensure
that leases don't disappear out from under it.Unfortunately, the code in __break_lease can end up passing this value
to wait_event_interruptible as a timeout, which prevents it from going
to sleep at all. This makes __break_lease to spin in a tight loop and
causes soft lockups.Fix this by ensuring that we pass a minimum value of 1 as a timeout
instead.Cc:
Cc: J. Bruce Fields
Reported-by: Terry Barnaby
Signed-off-by: Jeff Layton
31 Mar, 2014
8 commits
-
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
Signed-off-by: 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
Signed-off-by: Jeff Layton
Acked-by: J. Bruce Fields -
Neil Brown suggested potentially overloading the l_pid value as a "lock
context" field for file-private locks. While I don't think we will
probably want to do that here, it's probably a good idea to ensure that
in the future we could extend this API without breaking existing
callers.Typically the l_pid value is ignored for incoming struct flock
arguments, serving mainly as a place to return the pid of the owner if
there is a conflicting lock. For file-private locks, require that it
currently be set to 0 and return EINVAL if it isn't. If we eventually
want to make a non-zero l_pid mean something, then this will help ensure
that we don't break legacy programs that are using file-private locks.Cc: Neil Brown
Signed-off-by: Jeff Layton -
Due to some unfortunate history, POSIX locks have very strange and
unhelpful semantics. The thing that usually catches people by surprise
is that they are dropped whenever the process closes any file descriptor
associated with the inode.This is extremely problematic for people developing file servers that
need to implement byte-range locks. Developers often need a "lock
management" facility to ensure that file descriptors are not closed
until all of the locks associated with the inode are finished.Additionally, "classic" POSIX locks are owned by the process. Locks
taken between threads within the same process won't conflict with one
another, which renders them useless for synchronization between threads.This patchset adds a new type of lock that attempts to address these
issues. These locks conflict with classic POSIX read/write locks, but
have semantics that are more like BSD locks with respect to inheritance
and behavior on close.This is implemented primarily by changing how fl_owner field is set for
these locks. Instead of having them owned by the files_struct of the
process, they are instead owned by the filp on which they were acquired.
Thus, they are inherited across fork() and are only released when the
last reference to a filp is put.These new semantics prevent them from being merged with classic POSIX
locks, even if they are acquired by the same process. These locks will
also conflict with classic POSIX locks even if they are acquired by
the same process or on the same file descriptor.The new locks are managed using a new set of cmd values to the fcntl()
syscall. The initial implementation of this converts these values to
"classic" cmd values at a fairly high level, and the details are not
exposed to the underlying filesystem. We may eventually want to push
this handing out to the lower filesystem code but for now I don't
see any need for it.Also, note that with this implementation the new cmd values are only
available via fcntl64() on 32-bit arches. There's little need to
add support for legacy apps on a new interface like this.Signed-off-by: Jeff Layton
-
It's not really feasible to do deadlock detection with FL_FILE_PVT
locks since they aren't owned by a single task, per-se. Deadlock
detection also tends to be rather expensive so just skip it for
these sorts of locks.Also, add a FIXME comment about adding more limited deadlock detection
that just applies to ro -> rw upgrades, per Andy's request.Cc: Andy Lutomirski
Signed-off-by: Jeff Layton -
Once we introduce file private locks, we'll need to know what cmd value
was used, as that affects the ownership and whether a conflict would
arise.Signed-off-by: Jeff Layton
-
FL_FILE_PVT locks are no longer tied to a particular pid, and are
instead inheritable by child processes. Report a l_pid of '-1' for
these sorts of locks since the pid is somewhat meaningless for them.This precedent comes from FreeBSD. There, POSIX and flock() locks can
conflict with one another. If fcntl(F_GETLK, ...) returns a lock set
with flock() then the l_pid member cannot be a process ID because the
lock is not held by a process as such.Acked-by: J. Bruce Fields
Signed-off-by: Jeff Layton -
In a later patch, we'll be adding a new type of lock that's owned by
the struct file instead of the files_struct. Those sorts of locks
will be flagged with a new FL_FILE_PVT flag.Report these types of locks as "FLPVT" in /proc/locks to distinguish
them from "classic" POSIX locks.Acked-by: J. Bruce Fields
Signed-off-by: Jeff Layton