Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751773AbdH1QyI (ORCPT ); Mon, 28 Aug 2017 12:54:08 -0400 Received: from mail-io0-f173.google.com ([209.85.223.173]:32884 "EHLO mail-io0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751727AbdH1QyG (ORCPT ); Mon, 28 Aug 2017 12:54:06 -0400 X-Google-Smtp-Source: ADKCNb7l7sGCRJwPT5WL1YvpvTyx/vFWPBOz6kNk8kxwRruNXOyDauCgT7UDuf1UP75I9+N0M+AVvA== Subject: Re: [PATCH] block: Fix warning when I/O elevator is changed as request_queue is being removed To: Ming Lei , David Jeffery Cc: linux-block , Linux Kernel Mailing List References: <20170807193818.GA22737@rage> From: Jens Axboe Message-ID: <1e189293-8b82-d279-38e0-22735afde00c@kernel.dk> Date: Mon, 28 Aug 2017 10:54:03 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2360 Lines: 65 On 08/27/2017 07:36 PM, Ming Lei wrote: > On Wed, Aug 23, 2017 at 11:34 AM, Ming Lei wrote: >> On Wed, Aug 9, 2017 at 9:44 AM, Ming Lei wrote: >>> Hi David, >>> >>> On Wed, Aug 9, 2017 at 2:13 AM, David Jeffery wrote: >>>> On 08/07/2017 07:53 PM, Ming Lei wrote: >>>>> On Tue, Aug 8, 2017 at 3:38 AM, David Jeffery wrote: >>>> >>>>>> >>>>>> Signed-off-by: David Jeffery >>>>>> --- >>>>>> block/blk-sysfs.c | 2 ++ >>>>>> block/elevator.c | 4 ++++ >>>>>> 2 files changed, 6 insertions(+) >>>>>> >>>>>> >>>>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c >>>>>> index 27aceab..b8362c0 100644 >>>>>> --- a/block/blk-sysfs.c >>>>>> +++ b/block/blk-sysfs.c >>>>>> @@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk) >>>>>> if (WARN_ON(!q)) >>>>>> return; >>>>>> >>>>>> + mutex_lock(&q->sysfs_lock); >>>>>> queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q); >>>>>> + mutex_unlock(&q->sysfs_lock); >>>>> >>>>> Could you share why the lock of 'q->sysfs_lock' is needed here? >>>> >>>> As the elevator change is initiated through a sysfs attr file in the >>>> queue directory, the task doing the elevator change already acquires the >>>> q->sysfs_lock before it can try and change the elevator. Adding the >>> >>> It is e->sysfs_lock which is held in elv_attr_store(), instead of q->sysfs_lock. >> >> Looks I was wrong, and the store is from queue_attr_store() instead of >> elv_attr_store(). >> >> I can reproduce the issue and this patch can fix the issue in my test >> on scsi_debug, >> so: >> >> Tested-by: Ming Lei >> >> And it is a typical race between removing queue kobj and adding children of >> this queue kobj, we can't acquire q->sysfs_lock in blk_unregister_queue() >> because of sysfs's limit(may cause deadlock), so one state has to be used >> for this purpose, just like what register/unregister hctx kobjs does, >> and it should >> be fine to use QUEUE_FLAG_REGISTERED here. More changes are required if >> we use e->registered, so this patch looks fine: >> >> Reviewed-by: Ming Lei > > > Hi Jens, > > Could you consider this patch for v4.13 or v4.14? Yep, added for 4.14, thanks. -- Jens Axboe