Message-ID: <20140109194930.1692fbbe@tlielax.poochiereds.net>
Date:	Thu, 9 Jan 2014 19:49:30 -0500
From:	Jeff Layton <jlayton@...hat.com>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	linux-fsdevel@...r.kernel.org,
	nfs-ganesha-devel@...ts.sourceforge.net,
	samba-technical@...ts.samba.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 13/14] locks: skip deadlock detection on FL_FILE_PVT
 locks

On Thu, 09 Jan 2014 12:25:25 -0800
Andy Lutomirski <luto@...capital.net> wrote:

> On 01/09/2014 06:19 AM, 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.
> 
> I just looked at the existing deadlock detector, and... eww.
> 

Actually, I find it to be pretty clever, but it's only useful in some
very limited cases. Also, the performance penalty it imposes on a
lock-heavy workload is pretty significant (I had some numbers when I
did the overhaul of the spinlocking around that code, but don't have
them handy).

> When I think of deadlocks caused by r/w locks (which these are), I think
> of two kinds.  First is what the current code tries to detect: two
> processes that are each waiting for each other.  I don't know whether
> POSIX enshrines the idea of detecting that, but I wouldn't be surprised,
> considering how awful the old POSIX locks are.
> 

It can walk down a chain of dependencies (which is the cool part), so
the two processes don't even need to be working on the same inode at
all in order for it to detect a deadlock. The catch is that there is no
real way to deal with stuff like threads with this mechanism.

In any case, the spec is here:

    http://pubs.opengroup.org/onlinepubs/009696899/functions/fcntl.html

...and it just says: "A potential for deadlock occurs if a process
controlling a locked region is put to sleep by attempting to lock
another process' locked region. If the system detects that sleeping
until a locked region is unlocked would cause a deadlock, fcntl() shall
fail with an [EDEADLK] error."

Since it just says "if the system detects", I take it to mean that all
of this deadlock detection stuff is entirely optional.

> The sensible kind of detectable deadlock involves just one lock, and it
> happens when two processes both hold read locks and try to upgrade to
> write locks.  This should be efficiently detectable and makes upgrading
> locks safe(r).
> 
> I think I'd be happier if it's at least documented that the new fcntl
> calls might (some day) detect that kind of deadlock.
> 
> 

Sure, I can toss a comment in to that effect.

Personally, I'm rather keen to avoid dealing with deadlock detection
here since it's a rather large pain to deal with. Deadlock detection is
the only reason we have a global spinlock in this code anymore. I
seriously considered ripping it all out when I was overhauling this a
few months ago.

I was able to get it to perform more reasonably by turning the global
list into a hash table, but I'd have preferred to remove it altogether.

> All that being said, this patch series is awesome.  I've lost count of
> the number of explosions I've seen to due POSIX lock crap.  Thanks!
> 

Thanks for having a look!

-- 
Jeff Layton <jlayton@...hat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists