From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-f193.google.com ([209.85.214.193]:40869 "EHLO mail-pl1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726815AbeJ0CtW (ORCPT ); Fri, 26 Oct 2018 22:49:22 -0400 Received: by mail-pl1-f193.google.com with SMTP id b9-v6so861948pls.7 for ; Fri, 26 Oct 2018 11:11:21 -0700 (PDT) Message-ID: <1540577478.66186.108.camel@acm.org> Subject: Re: [PATCH RFC] kernel/locking, fs/direct-io: Introduce and use down_write_nolockdep() From: Bart Van Assche To: Matthew Wilcox Cc: Alexander Viro , linux-fsdevel@vger.kernel.org, Johannes Berg , Peter Zijlstra , Ingo Molnar , Theodore Ts'o Date: Fri, 26 Oct 2018 11:11:18 -0700 In-Reply-To: <20181026174352.GT25444@bombadil.infradead.org> References: <20181026164905.214474-1-bvanassche@acm.org> <20181026174352.GT25444@bombadil.infradead.org> Content-Type: text/plain; charset="UTF-7" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri, 2018-10-26 at 10:43 -0700, Matthew Wilcox wrote: +AD4 On Fri, Oct 26, 2018 at 09:49:05AM -0700, Bart Van Assche wrote: +AD4 +AD4 +-+-+- b/include/linux/rwsem.h +AD4 +AD4 +AEAAQA -41,6 +-41,10 +AEAAQA struct rw+AF8-semaphore +AHs +AD4 +AD4 +ACM-endif +AD4 +AD4 +ACM-ifdef CONFIG+AF8-DEBUG+AF8-LOCK+AF8-ALLOC +AD4 +AD4 struct lockdep+AF8-map dep+AF8-map+ADs +AD4 +AD4 +- /+ACo +AD4 +AD4 +- +ACo Number of up+AF8-write() calls that must skip rwsem+AF8-release(). +AD4 +AD4 +- +ACo-/ +AD4 +AD4 +- unsigned nolockdep+ADs +AD4 +AD4 This reads a bit weird. By definition, only one writer is allowed +AD4 at a time. And you can't call up+AF8-write() before down+AF8-write(). +AD4 So functionally, this is a bool, and the comment should at least +AD4 ackowledge that, even if it remains implemented as an unsigned int. +AD4 +AD4 I'd suggest the implementation uses '+AD0 1' and '+AD0 0' rather than +-+- and --. Hi Matthew, That sounds like a good idea to me. +AD4 +AD4 diff --git a/mm/rmap.c b/mm/rmap.c +AD4 +AD4 index 1e79fac3186b..2a953d3b7431 100644 +AD4 +AD4 --- a/mm/rmap.c +AD4 +AD4 +-+-+- b/mm/rmap.c +AD4 +AD4 +AEAAQA -81,6 +-81,7 +AEAAQA static inline struct anon+AF8-vma +ACo-anon+AF8-vma+AF8-alloc(void) +AD4 +AD4 +AD4 +AD4 anon+AF8-vma +AD0 kmem+AF8-cache+AF8-alloc(anon+AF8-vma+AF8-cachep, GFP+AF8-KERNEL)+ADs +AD4 +AD4 if (anon+AF8-vma) +AHs +AD4 +AD4 +- init+AF8-rwsem(+ACY-anon+AF8-vma-+AD4-rwsem)+ADs +AD4 +AD4 atomic+AF8-set(+ACY-anon+AF8-vma-+AD4-refcount, 1)+ADs +AD4 +AD4 anon+AF8-vma-+AD4-degree +AD0 1+ADs /+ACo Reference for first vma +ACo-/ +AD4 +AD4 anon+AF8-vma-+AD4-parent +AD0 anon+AF8-vma+ADs +AD4 +AD4 Why is this needed? The anon+AF8-vma+AF8-ctor() already calls init+AF8-rwsem(). +AD4 +AD4 (I suspect this is one of those ctors that isn't actually useful and +AD4 should be inlined into anon+AF8-vma+AF8-alloc()) Without that call I noticed that the +ACI-nolockdep+ACI variable was sometimes set when down+AF8-write() got called. Does that mean that it can happen that an anon+AF8-vma structure is freed without releasing anon+AF8-vma-+AD4-rwsem? Thanks, Bart.