Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753300AbdHWDep (ORCPT ); Tue, 22 Aug 2017 23:34:45 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:38766 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753057AbdHWDel (ORCPT ); Tue, 22 Aug 2017 23:34:41 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170807193818.GA22737@rage> From: Ming Lei Date: Wed, 23 Aug 2017 11:34:40 +0800 Message-ID: Subject: Re: [PATCH] block: Fix warning when I/O elevator is changed as request_queue is being removed To: David Jeffery Cc: linux-block , Linux Kernel Mailing List , Jens Axboe Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2024 Lines: 57 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 Thanks, Ming Lei