Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754748AbbBTOjX (ORCPT ); Fri, 20 Feb 2015 09:39:23 -0500 Received: from mail.bmw-carit.de ([62.245.222.98]:52653 "EHLO mail.bmw-carit.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754297AbbBTOjE (ORCPT ); Fri, 20 Feb 2015 09:39:04 -0500 Message-ID: <54E745B5.4020605@bmw-carit.de> Date: Fri, 20 Feb 2015 15:33:25 +0100 From: Daniel Wagner User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Jeff Layton CC: , Alexander Viro , "J. Bruce Fields" , John Kacur , , Subject: Re: [RFC v0 1/1] fs/locks: Use plain percpu spinlocks instead of lglock to protect file_lock References: <1424355974-30952-1-git-send-email-daniel.wagner@bmw-carit.de> <1424355974-30952-2-git-send-email-daniel.wagner@bmw-carit.de> <20150219154943.47dc1e4f@tlielax.poochiereds.net> In-Reply-To: <20150219154943.47dc1e4f@tlielax.poochiereds.net> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3985 Lines: 97 Hi Jeff, On 02/19/2015 09:49 PM, Jeff Layton wrote: > On Thu, 19 Feb 2015 15:26:14 +0100 > Daniel Wagner wrote: >> Whenever we insert a new lock we are going to grab besides the i_lock >> also the corresponding percpu file_lock_lock. The global >> blocked_lock_lock is only used when blocked_hash is involved. >> > > Ok, good. I like that part -- I hate the blocked_lock_lock, but freezing > the file locking state is the only way I've found to ensure the > correctness of deadlock detection. Bummer. Okay, I'll look into that. >> file_lock_list exists to be being able to produce the content of >> /proc/locks. For listing the all locks it seems a bit excessive to >> grab all locks at once. We should be okay just grabbing the >> corresponding lock when iterating over the percpu file_lock_list. >> > > True, but that's not a terribly common event, which is why I figured > the lg_lock was an acceptable tradeoff there. That said, if you can get > rid of it in favor of something more efficient then that sounds good to > me. If it helps the -rt kernels, then so much the better... Great! I was hoping to hear that :) >> file_lock_lock protects now file_lock_list and fl_link, fl_block and >> fl_next allone. That means we need to define which file_lock_lock is >> used for all waiters. Luckely, fl_link_cpu can be reused for fl_block >> and fl_next. >> > > Ok, so when a lock is blocked, we'll insert the waiter onto the > fl_block list for the blocker, and use the blocker's per-cpu spinlock > to protect that list. Good idea. Let's hope it doesn't explode. So far I am still confident it works. >> Signed-off-by: Daniel Wagner >> Cc: Alexander Viro >> Cc: Jeff Layton >> Cc: "J. Bruce Fields" >> Cc: John Kacur >> Cc: linux-fsdevel@vger.kernel.org >> Cc: linux-rt-users@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> > > Thanks for the patch. Some general comments first: > > - fs/locks.c has seen a fair bit of overhaul during the current merge > window, and this patch won't apply directly. I'd suggest cleaning it up > after -rc1 is cut. I just rebased it and splited it a bit up. Couldn't wait... > - the basic idea seems sound, but this is very "fiddly" code. It would > be nice to see if you can break this up into multiple patches. For > instance, maybe convert the lglock to the percpu spinlocks first in a > separate patch, and just keep it protecting the file_lock_list. Once > that's done, start changing other pieces to be protected by the percpu > locks. Small, incremental changes like that are much easier to deal > with if something breaks, since we can at least run a bisect to narrow > down the offending change. They're also easier to review. I complete agree. Sorry to send such a bad initial version. I should have known it better. > - the open-coded seqfile stuff is pretty nasty. When I did the original > change to use lglocks, I ended up adding seq_hlist_*_percpu macros to > support it. Maybe consider doing something like that here too, though > this is a bit more complex obviously since you want to be able to just > hold one spinlock at a time. Ok. > - it would also be good to start thinking about adding lockdep > assertions to this code. I simply didn't realize how wonderful they > were when I did the global spinlock to i_lock conversion a year or two > ago. That can really help catch bugs, and as the (spin)locking gets > more complex in this code, that'd be good to help ensure correctness. I'll give it a try. Many thanks for the quick review. Really appreciated! cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/