Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 14 Mar 2018 11:46:36 -0700
From:   "tj@kernel.org" <tj@kernel.org>
To:     Bart Van Assche <Bart.VanAssche@wdc.com>
Cc:     "torvalds@linux-foundation.org" <torvalds@linux-foundation.org>,
        "viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
        "jannh@google.com" <jannh@google.com>,
        "paulmck@linux.vnet.ibm.com" <paulmck@linux.vnet.ibm.com>,
        "bcrl@kvack.org" <bcrl@kvack.org>,
        "kent.overstreet@gmail.com" <kent.overstreet@gmail.com>,
        "security@kernel.org" <security@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kernel-team@fb.com" <kernel-team@fb.com>,
        "axboe@kernel.dk" <axboe@kernel.dk>
Subject: Re: [PATCH 5/7] block: Remove superflous rcu_read_[un]lock_sched()
 in blk_queue_enter()
Message-ID: <20180314184636.GC2943022@devbig577.frc2.facebook.com>
References: <20180306172657.3060270-1-tj@kernel.org>
 <20180306173316.3088458-1-tj@kernel.org>
 <20180306173316.3088458-5-tj@kernel.org>
 <1520358769.3578.21.camel@wdc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1520358769.3578.21.camel@wdc.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello, Bart.

On Tue, Mar 06, 2018 at 05:52:50PM +0000, Bart Van Assche wrote:
> I think the rcu_read_lock() and rcu_read_unlock() are really necessary in this
> code. In the LWN article "The RCU-barrier menagerie" it is explained that RCU
> can be used to enforce write ordering globally if the code that reads the writes

Yeah but, for it to be meaningful, just like barriers, it has to be
paired with something on the other side.

> that are ordered with an RCU read lock (https://lwn.net/Articles/573497/). See
> also the following comment in scsi_device_quiesce():
> 
> 	/*
> 	 * Ensure that the effect of blk_set_preempt_only() will be visible
> 	 * for percpu_ref_tryget() callers that occur after the queue
> 	 * unfreeze even if the queue was already frozen before this function
> 	 * was called. See also https://lwn.net/Articles/573497/.
> 	 */
> 
> Since this patch introduces a subtle and hard to debug race condition, please
> drop this patch.

Hah, the pairing is between scsi_device_quiesce() and
blk_queue_enter()?  But that doesn't make sense either because
scsi_device_quiesce() is doing regular synchronize_rcu() and
blk_queue_entre() is doing rcu_read_lock_sched().  They don't
interlock with each other in any way.

So, the right thing to do here would be somehow moving the RCU
synchronization into blk_set_preempt_only() and switching to regular
RCU protection in blk_queue_enter().  The code as-is isn't really
doing anything.

I'll drop this patch from the series for now.  Let's revisit it later.

Thanks.

-- 
tejun