From patchwork Wed Feb 22 10:24:25 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Kara X-Patchwork-Id: 9586545 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 426386051E for ; Wed, 22 Feb 2017 10:24:46 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 47111284D3 for ; Wed, 22 Feb 2017 10:24:46 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 39D552869D; Wed, 22 Feb 2017 10:24:46 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 39DBE284D3 for ; Wed, 22 Feb 2017 10:24:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754683AbdBVKYg (ORCPT ); Wed, 22 Feb 2017 05:24:36 -0500 Received: from mx2.suse.de ([195.135.220.15]:49135 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754655AbdBVKYb (ORCPT ); Wed, 22 Feb 2017 05:24:31 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 30222AAB4; Wed, 22 Feb 2017 10:24:27 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id 3E9341E10CA; Wed, 22 Feb 2017 11:24:25 +0100 (CET) Date: Wed, 22 Feb 2017 11:24:25 +0100 From: Jan Kara To: Jens Axboe Cc: Jan Kara , linux-block@vger.kernel.org, Christoph Hellwig , Dan Williams , Thiago Jung Bauermann , Lekshmi Pillai , Tejun Heo , NeilBrown , Omar Sandoval Subject: Re: [PATCH 0/13 v2] block: Fix block device shutdown related races Message-ID: <20170222102425.GB23312@quack2.suse.cz> References: <20170221170958.21845-1-jack@suse.cz> <7734fd93-e4fd-65a7-bf32-08012de83c1e@kernel.dk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <7734fd93-e4fd-65a7-bf32-08012de83c1e@kernel.dk> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue 21-02-17 10:19:28, Jens Axboe wrote: > On 02/21/2017 10:09 AM, Jan Kara wrote: > > Hello, > > > > this is a second revision of the patch set to fix several different races and > > issues I've found when testing device shutdown and reuse. The first three > > patches are fixes to problems in my previous series fixing BDI lifetime issues. > > Patch 4 fixes issues with reuse of BDI name with scsi devices. With it I cannot > > reproduce the BDI name reuse issues using Omar's stress test using scsi_debug > > so it can be used as a replacement of Dan's patches. Patches 5-11 fix oops that > > is triggered by __blkdev_put() calling inode_detach_wb() too early (the problem > > reported by Thiago). Patches 12 and 13 fix oops due to a bug in gendisk code > > where get_gendisk() can return already freed gendisk structure (again triggered > > by Omar's stress test). > > > > People, please have a look at patches. They are mostly simple however the > > interactions are rather complex so I may have missed something. Also I'm > > happy for any additional testing these patches can get - I've stressed them > > with Omar's script, tested memcg writeback, tested static (not udev managed) > > device inodes. > > > > Jens, I think at least patches 1-3 should go in together with my fixes you > > already have in your tree (or shortly after them). It is up to you whether > > you decide to delay my first fixes or pick these up quickly. Patch 4 is > > (IMHO a cleaner) replacement of Dan's patches so consider whether you want > > to use it instead of those patches. > > I have applied 1-3 to my for-linus branch, which will go in after > the initial pull request has been pulled by Linus. Consider fixing up > #4 so it applies, I like it. OK, attached is patch 4 rebased on top of Linus' tree from today which already has linux-block changes pulled in. I've left put_disk_devt() in blk_cleanup_queue() to maintain the logic in the original patch (now commit 0dba1314d4f8) that request_queue and gendisk each hold one devt reference. The bdi_unregister() call that is moved to del_gendisk() by this patch is now protected by the gendisk reference instead of the request_queue one so it still maintains the property that devt reference protects bdi registration-unregistration lifetime (as much as that is not needed anymore after this patch). I have also updated the comment in the code and the changelog - they were somewhat stale after changes to the whole series Tejun suggested. Honza Tested-by: Omar Sandoval From 9abe9565c83af6b653b159a7bf5b895aff638c65 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Wed, 8 Feb 2017 08:05:56 +0100 Subject: [PATCH] block: Move bdi_unregister() to del_gendisk() Commit 6cd18e711dd8 "block: destroy bdi before blockdev is unregistered." moved bdi unregistration (at that time through bdi_destroy()) from blk_release_queue() to blk_cleanup_queue() because it needs to happen before blk_unregister_region() call in del_gendisk() for MD. SCSI though will free up the device number from sd_remove() called through a maze of callbacks from device_del() in __scsi_remove_device() before blk_cleanup_queue() and thus similar races as described in 6cd18e711dd8 can happen for SCSI as well as reported by Omar [1]. Moving bdi_unregister() to del_gendisk() works for MD and fixes the problem for SCSI since del_gendisk() gets called from sd_remove() before freeing the device number. This also makes device_add_disk() (calling bdi_register_owner()) more symmetric with del_gendisk(). [1] http://marc.info/?l=linux-block&m=148554717109098&w=2 Tested-by: Lekshmi Pillai Acked-by: Tejun Heo Signed-off-by: Jan Kara --- block/blk-core.c | 1 - block/genhd.c | 5 +++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/block/blk-core.c b/block/blk-core.c index b9e857f4afe8..1086dac8724c 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -578,7 +578,6 @@ void blk_cleanup_queue(struct request_queue *q) q->queue_lock = &q->__queue_lock; spin_unlock_irq(lock); - bdi_unregister(q->backing_dev_info); put_disk_devt(q->disk_devt); /* @q is and will stay empty, shutdown and put */ diff --git a/block/genhd.c b/block/genhd.c index 2f444b87a5f2..b26a5ea115d0 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -681,6 +681,11 @@ void del_gendisk(struct gendisk *disk) disk->flags &= ~GENHD_FL_UP; sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi"); + /* + * Unregister bdi before releasing device numbers (as they can get + * reused and we'd get clashes in sysfs). + */ + bdi_unregister(disk->queue->backing_dev_info); blk_unregister_queue(disk); blk_unregister_region(disk_devt(disk), disk->minors); -- 2.10.2