Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751769AbdH1BgU (ORCPT ); Sun, 27 Aug 2017 21:36:20 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:37242 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751493AbdH1BgS (ORCPT ); Sun, 27 Aug 2017 21:36:18 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170807193818.GA22737@rage> From: Ming Lei Date: Mon, 28 Aug 2017 09:36:16 +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: 2228 Lines: 63 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? Thanks, Ming Lei