On Fri, 28 Mar 2014 17:43:22 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> On Wed, Mar 19, 2014 at 04:45:57PM -0400, Jeff Layton wrote:
> > 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.
>
> Yay!
>
> > 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 <luto@xxxxxxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/locks.c | 19 +++++++++++++++++--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/locks.c b/fs/locks.c
> > index f8cd6d7de161..8c5bc07c360f 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -564,7 +564,7 @@ static void __locks_insert_block(struct file_lock *blocker,
> > BUG_ON(!list_empty(&waiter->fl_block));
> > waiter->fl_next = blocker;
> > list_add_tail(&waiter->fl_block, &blocker->fl_block);
> > - if (IS_POSIX(blocker))
> > + if (IS_POSIX(blocker) && !IS_FILE_PVT(blocker))
> > locks_insert_global_blocked(waiter);
> > }
> >
> > @@ -757,8 +757,16 @@ EXPORT_SYMBOL(posix_test_lock);
> > * Note: the above assumption may not be true when handling lock
> > * requests from a broken NFS client. It may also fail in the presence
> > * of tasks (such as posix threads) sharing the same open file table.
> > - *
> > * To handle those cases, we just bail out after a few iterations.
> > + *
> > + * For FL_FILE_PVT locks, the owner is the filp, not the files_struct.
> > + * Because the owner is not even nominally tied to a thread of
> > + * execution, the deadlock detection below can't reasonably work well. Just
> > + * skip it for those.
> > + *
> > + * In principle, we could do a more limited deadlock detection on FL_FILE_PVT
> > + * locks that just checks for the case where two tasks are attempting to
> > + * upgrade from read to write locks on the same inode.
> > */
> >
> > #define MAX_DEADLK_ITERATIONS 10
> > @@ -781,6 +789,13 @@ static int posix_locks_deadlock(struct file_lock *caller_fl,
> > {
> > int i = 0;
> >
> > + /*
> > + * This deadlock detector can't reasonably detect deadlocks with
> > + * FL_FILE_PVT locks, since they aren't owned by a process, per-se.
> > + */
> > + if (IS_FILE_PVT(caller_fl))
> > + return 0;
> > +
>
> This takes care of deadlock detection at the time that you apply a
> file_private lock. What happens when you're doing deadlock detection
> before applying a traditional posix lock and happen to run across a
> file_private lock?
>
> Hm, I guess the posix_same_owner() always fails in that case?
>
> OK, ACK.
>
> --b.
>
Note that we don't ever insert FP locks into the hashtable, so they're
aren't taken into account at all for deadlock detection.
FWIW, as a later cleanup, I'd also like to ensure that we never take the
global spinlock for fplocks as well, but I think that can wait until
after this set has been merged.
> > while ((block_fl = what_owner_is_waiting_for(block_fl))) {
> > if (i++ > MAX_DEADLK_ITERATIONS)
> > return 0;
> > --
> > 1.8.5.3
> >
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
