From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933126AbdCKWwl (ORCPT ); Sat, 11 Mar 2017 17:52:41 -0500 Received: from mail-pg0-f44.google.com ([74.125.83.44]:33915 "EHLO mail-pg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753882AbdCKWwg (ORCPT ); Sat, 11 Mar 2017 17:52:36 -0500 Subject: Re: [PATCH v5] blkcg: allocate struct blkcg_gq outside request queue spinlock To: Tahsin Erdogan , Tejun Heo References: <20170306200319.GF19696@htj.duckdns.org> <20170309080531.9048-1-tahsin@google.com> Cc: linux-block@vger.kernel.org, David Rientjes , linux-kernel@vger.kernel.org From: Jens Axboe Message-ID: Date: Sat, 11 Mar 2017 15:52:33 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: 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 On 03/11/2017 03:42 PM, Jens Axboe wrote: >> @@ -185,31 +187,53 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg, >> goto err_free_blkg; >> } >> >> + if (drop_locks) { >> + spin_unlock_irq(q->queue_lock); >> + rcu_read_unlock(); >> + } > > I have a general dislike for code like that, where you conditionally > drop locks. And this one seems even worse, since the knowledge of > whether the locks should/could be dropped or not is embedded in the gfp > flags. Talked to Tejun about this as well, and we both agree that the splitting this into separate init/alloc paths would be much cleaner. I can't apply the current patch, sorry, it's just too ugly to live. >> +/** >> + * blkg_lookup_create - lookup blkg, try to create one if not there >> + * >> + * Performs an initial queue bypass check and then passes control to >> + * __blkg_lookup_create(). >> + */ >> +struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg, >> + struct request_queue *q, gfp_t gfp, >> + const struct blkcg_policy *pol) >> +{ >> + WARN_ON_ONCE(!rcu_read_lock_held()); >> + lockdep_assert_held(q->queue_lock); > > This seems problematic, as blkcg_bio_issue_check() calls with the rcu > read lock held. Brain fart, that part is fine. -- Jens Axboe