27 Mar, 2015
1 commit
-
locks_delete_lock_ctx() is called inside the loop, so we
should use list_for_each_entry_safe.Fixes: 8634b51f6ca2 (locks: convert lease handling to file_lock_context)
Signed-off-by: "Yan, Zheng"
Signed-off-by: Jeff Layton
14 Mar, 2015
1 commit
-
It's possible that "fl" won't point at a valid lock at this point, so
use "victim" instead which is either a valid lock or NULL.Signed-off-by: Jeff Layton
05 Mar, 2015
1 commit
-
Commit 8634b51f6ca2 (locks: convert lease handling to file_lock_context)
introduced a regression in the handling of lease upgrade/downgrades.In the event that we already have a lease on a file and are going to
either upgrade or downgrade it, we skip doing any list insertion or
deletion and simply re-call lm_setup on the existing lease.As of commit 8634b51f6ca2 however, we end up calling lm_setup on the
lease that was passed in, instead of on the existing lease. This causes
us to leak the fasync_struct that was allocated in the event that there
was not already an existing one (as it always appeared that there
wasn't one).Fixes: 8634b51f6ca2 (locks: convert lease handling to file_lock_context)
Reported-and-Tested-by: Daniel Wagner
Signed-off-by: Jeff Layton
18 Feb, 2015
3 commits
-
In the case where we're splitting a lock in two, the current code
the new "left" lock in the incorrect spot. It's inserted just
before "right" when it should instead be inserted just before the
new lock.When we add a new lock, set "fl" to that value so that we can
add "left" before it.Reported-by: Al Viro
Signed-off-by: Jeff Layton -
As Linus pointed out:
Say we have an existing flock, and now do a new one that conflicts. I
see what looks like three separate bugs.- We go through the first loop, find a lock of another type, and
delete it in preparation for replacing it- we *drop* the lock context spinlock.
- BUG #1? So now there is no lock at all, and somebody can come in
and see that unlocked state. Is that really valid?- another thread comes in while the first thread dropped the lock
context lock, and wants to add its own lock. It doesn't see the
deleted or pending locks, so it just adds it- the first thread gets the context spinlock again, and adds the lock
that replaced the original- BUG #2? So now there are *two* locks on the thing, and the next
time you do an unlock (or when you close the file), it will only
remove/replace the first one....remove the "drop the spinlock" code in the middle of this function as
it has always been suspicious. This should eliminate the potential race
that can leave two locks for the same struct file on the list.He also pointed out another thing as a bug -- namely that you
flock_lock_file removes the lock from the list unconditionally when
doing a lock upgrade, without knowing whether it'll be able to set the
new lock. Bruce pointed out that this is expected behavior and may help
prevent certain deadlock situations.We may want to revisit that at some point, but it's probably best that
we do so in the context of a different patchset.Reported-by: Linus Torvalds
Signed-off-by: Jeff Layton -
We don't want to remove all leases just because one filp was closed.
Signed-off-by: Jeff Layton
17 Feb, 2015
1 commit
-
This reverts commit 9bd0f45b7037fcfa8b575c7e27d0431d6e6dc3bb.
Linus rightly pointed out that I failed to initialize the counters
when adding them, so they don't work as expected. Just revert this
patch for now.Reported-by: Linus Torvalds
Signed-off-by: Jeff Layton
03 Feb, 2015
2 commits
-
This (ab-)uses the file locking code to allow filesystems to recall
outstanding pNFS layouts on a file. This new lease type is similar but
not quite the same as FL_DELEG. A FL_LAYOUT lease can always be granted,
an a per-filesystem lock (XFS iolock for the initial implementation)
ensures not FL_LAYOUT leases granted when we would need to recall them.Also included are changes that allow multiple outstanding read
leases of different types on the same file as long as they have a
differnt owner. This wasn't a problem until now as nfsd never set
FL_LEASE leases, and no one else used FL_DELEG leases, but given that
nfsd will also issues FL_LAYOUT leases we will have to handle it now.Signed-off-by: Christoph Hellwig
-
Just like for other lock types we should allow different owners to have
a read lease on a file. Currently this can't happen, but with the addition
of pNFS layout leases we'll need this feature.Signed-off-by: Christoph Hellwig
22 Jan, 2015
1 commit
-
Signed-off-by: Jeff Layton
17 Jan, 2015
10 commits
-
We have each of the locks_remove_* variants doing this individually.
Have the caller do it instead, and have locks_remove_flock and
locks_remove_lease just assume that it's a valid pointer.Signed-off-by: Jeff Layton
-
This makes things a bit more efficient in the cifs and ceph lock
pushing code.Signed-off-by: Jeff Layton
Acked-by: Christoph Hellwig -
Now that we use standard list_heads for tracking leases, we can have
lm_change take a pointer to the lease to be modified instead of a
double pointer.Signed-off-by: Jeff Layton
Acked-by: Christoph Hellwig -
We can now add a dedicated spinlock without expanding struct inode.
Change to using that to protect the various i_flctx lists.Signed-off-by: Jeff Layton
Acked-by: Christoph Hellwig -
Signed-off-by: Jeff Layton
Acked-by: Christoph Hellwig -
Signed-off-by: Jeff Layton
Acked-by: Christoph Hellwig -
Signed-off-by: Jeff Layton
Acked-by: Christoph Hellwig -
The current scheme of using the i_flock list is really difficult to
manage. There is also a legitimate desire for a per-inode spinlock to
manage these lists that isn't the i_lock.Start conversion to a new scheme to eventually replace the old i_flock
list with a new "file_lock_context" object.We start by adding a new i_flctx to struct inode. For now, it lives in
parallel with i_flock list, but will eventually replace it. The idea is
to allocate a structure to sit in that pointer and act as a locus for
all things file locking.We allocate a file_lock_context for an inode when the first lock is
added to it, and it's only freed when the inode is freed. We use the
i_lock to protect the assignment, but afterward it should mostly be
accessed locklessly.Signed-off-by: Jeff Layton
Acked-by: Christoph Hellwig -
...instead of open-coding it and removing flock locks directly. This
helps consolidate the flock lock removal logic into a single spot.Signed-off-by: Jeff Layton
-
...that we can use to queue file_locks to per-ctx list_heads. Go ahead
and convert locks_delete_lock and locks_dispose_list to use it instead
of the fl_block list.Signed-off-by: Jeff Layton
Acked-by: Christoph Hellwig
13 Jan, 2015
1 commit
-
commit 0efaa7e82f02fe69c05ad28e905f31fc86e6f08e
locks: generic_delete_lease doesn't need a file_lock at allmoves the call to fl->fl_lmops->lm_change() to a place in the
code where fl might be a non-lease lock.
When that happens, fl_lmops is NULL and an Oops ensures.So add an extra test to restore correct functioning.
Reported-by: Linda Walsh
Link: https://bugzilla.suse.com/show_bug.cgi?id=912569
Cc: stable@vger.kernel.org (v3.18)
Fixes: 0efaa7e82f02fe69c05ad28e905f31fc86e6f08e
Signed-off-by: NeilBrown
Signed-off-by: Jeff Layton
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
1 commit
-
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