From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-f194.google.com ([209.85.223.194]:49978 "EHLO mail-io0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755372AbdKJBky (ORCPT ); Thu, 9 Nov 2017 20:40:54 -0500 MIME-Version: 1.0 In-Reply-To: <8760ajf6al.fsf@notabene.neil.brown.name> References: <151019756744.30101.3832608128627682973.stgit@noble> <151019772763.30101.16040338743875884111.stgit@noble> <8760ajf6al.fsf@notabene.neil.brown.name> From: Linus Torvalds Date: Thu, 9 Nov 2017 17:40:52 -0800 Message-ID: Subject: Re: [PATCH 3/3] VFS: close race between getcwd() and d_move() To: NeilBrown Cc: Al Viro , linux-fsdevel , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Nov 9, 2017 at 2:14 PM, NeilBrown wrote: > On Thu, Nov 09 2017, Linus Torvalds wrote: >> >> How nasty would it be to just expand the calls to __d_drop/__d_rehash >> into __d_move itself, and take both has list locks at the same time >> (with the usual ordering and checking if it's the same list, of >> course). > > something like this? Yes. This looks nicer to me. Partly because I hate those "pass flags to functions that modify their behavior" kinds of patches. I'd rather see just straight-line unconditional code with some possible duplication. That said, your conversion isn't the obvious "no semantic changes" one. The BUG_ON() conditions you added are a bit odd. You've taken the BUG_ON() from __d_rehash() that no longe rmakes any sense (because we just explicitly unhashed it), and replaced it with a BUG_ON() that didn't exist before, and is also not the conditionm that __d_drop actually had (or the condition that means that the hash liost might be different - ie the whole IS_ROOT() case). So the patch looks conceptually good., but I worry about the details. They may be right, but that odd IS_ROOT case in __d_drop really worries me. Can we rename one of those disconnected entries? Of course, that then ties into the other thread, where those disconnected entries are a bit too special anyway. I also do wonder if we can avoid all the unhash/rehash games entirely (and avoid the hash list locking) if it turns out that the dentry and target hash lists are the same. I'm not claiming that as an optimization (it's an unusual case), I'm more thinking that it might fall out fairly naturally from the "lock both" case, since that one needs to check for the same list anyway. > Do you like it enough for me to make it into a real patch? Let's see if Al hates this approach, but I definitely prefer it assuming my worries are groundless. > I'd probably move hlist_bl_lock_two() into list_bl.h. Maybe not - see the above issue where maybe the "same hash" might actually merit different codepath. Linus