From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751988AbbEHAOq (ORCPT ); Thu, 7 May 2015 20:14:46 -0400 Received: from mail-pa0-f50.google.com ([209.85.220.50]:33411 "EHLO mail-pa0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751197AbbEHAOn (ORCPT ); Thu, 7 May 2015 20:14:43 -0400 Date: Fri, 8 May 2015 09:15:01 +0900 From: Sergey Senozhatsky To: Minchan Kim Cc: Sergey Senozhatsky , Andrew Morton , Nitin Gupta , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCHv4 00/10] add on-demand device creation Message-ID: <20150508001501.GA615@swordfish> References: <20150506052557.GA820@swordfish> <20150506070756.GC820@swordfish> <20150506072656.GA8163@blaptop> <20150506081029.GA8385@swordfish> <20150506082036.GB8163@blaptop> <20150507003322.GA4153@swordfish> <20150507074159.GA3624@blaptop> <20150507120944.GA681@swordfish> <20150507130454.GA1208@swordfish> <20150507151154.GA12217@blaptop> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150507151154.GA12217@blaptop> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On (05/08/15 00:11), Minchan Kim wrote: > It's good timing. When I'm about to dig in, I got your patches > and tested it. > > Passed my test script! > > BTW, patches you sent landed on mainline? > > Anyway, Thanks! > Hello Minchan, thanks a ton! not mainlined yet, to the best of my knowledge. will keep an eye on these patches. I'll rename zram-contol interface and resubmit zram hot add/remove patchset today. thanks. -ss > > From 7056096317825dbe582d005385bc4077ecb62c76 Mon Sep 17 00:00:00 2001 > > From: NeilBrown > > Date: Mon, 27 Apr 2015 04:12:22 +1000 > > Subject: [PATCH 2/2] block: destroy bdi before blockdev is unregistered. > > > > Because of the peculiar way that md devices are created (automatically > > when the device node is opened), a new device can be created and > > registered immediately after the > > blk_unregister_region(disk_devt(disk), disk->minors); > > call in del_gendisk(). > > > > Therefore it is important that all visible artifacts of the previous > > device are removed before this call. In particular, the 'bdi'. > > > > Since: > > commit c4db59d31e39ea067c32163ac961e9c80198fd37 > > Author: Christoph Hellwig > > fs: don't reassign dirty inodes to default_backing_dev_info > > > > moved the > > device_unregister(bdi->dev); > > call from bdi_unregister() to bdi_destroy() it has been quite easy to > > lose a race and have a new (e.g.) "md127" be created after the > > blk_unregister_region() call and before bdi_destroy() is ultimately > > called by the final 'put_disk', which must come after del_gendisk(). > > > > The new device finds that the bdi name is already registered in sysfs > > and complains > > > > > [ 9627.630029] WARNING: CPU: 18 PID: 3330 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x5a/0x70() > > > [ 9627.630032] sysfs: cannot create duplicate filename '/devices/virtual/bdi/9:127' > > > > We can fix this by moving the bdi_destroy() call out of > > blk_release_queue() (which can happen very late when a refcount > > reaches zero) and into blk_cleanup_queue() - which happens exactly when the md > > device driver calls it. > > > > Then it is only necessary for md to call blk_cleanup_queue() before > > del_gendisk(). As loop.c devices are also created on demand by > > opening the device node, we make the same change there. > > > > Fixes: c4db59d31e39ea067c32163ac961e9c80198fd37 > > Reported-by: Azat Khuzhin > > Cc: Christoph Hellwig > > Cc: stable@vger.kernel.org (v4.0) > > Signed-off-by: NeilBrown > > Reviewed-by: Christoph Hellwig > > Signed-off-by: Jens Axboe > > --- > > block/blk-core.c | 2 ++ > > block/blk-sysfs.c | 2 -- > > drivers/block/loop.c | 2 +- > > drivers/md/md.c | 4 ++-- > > 4 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/block/blk-core.c b/block/blk-core.c > > index fd154b9..7871603 100644 > > --- a/block/blk-core.c > > +++ b/block/blk-core.c > > @@ -552,6 +552,8 @@ void blk_cleanup_queue(struct request_queue *q) > > q->queue_lock = &q->__queue_lock; > > spin_unlock_irq(lock); > > > > + bdi_destroy(&q->backing_dev_info); > > + > > /* @q is and will stay empty, shutdown and put */ > > blk_put_queue(q); > > } > > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c > > index faaf36a..2b8fd30 100644 > > --- a/block/blk-sysfs.c > > +++ b/block/blk-sysfs.c > > @@ -522,8 +522,6 @@ static void blk_release_queue(struct kobject *kobj) > > > > blk_trace_shutdown(q); > > > > - bdi_destroy(&q->backing_dev_info); > > - > > ida_simple_remove(&blk_queue_ida, q->id); > > call_rcu(&q->rcu_head, blk_free_queue_rcu); > > } > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > > index ae3fcb4..d7173cb 100644 > > --- a/drivers/block/loop.c > > +++ b/drivers/block/loop.c > > @@ -1620,8 +1620,8 @@ out: > > > > static void loop_remove(struct loop_device *lo) > > { > > - del_gendisk(lo->lo_disk); > > blk_cleanup_queue(lo->lo_queue); > > + del_gendisk(lo->lo_disk); > > blk_mq_free_tag_set(&lo->tag_set); > > put_disk(lo->lo_disk); > > kfree(lo); > > diff --git a/drivers/md/md.c b/drivers/md/md.c > > index d4f31e1..593a024 100644 > > --- a/drivers/md/md.c > > +++ b/drivers/md/md.c > > @@ -4818,12 +4818,12 @@ static void md_free(struct kobject *ko) > > if (mddev->sysfs_state) > > sysfs_put(mddev->sysfs_state); > > > > + if (mddev->queue) > > + blk_cleanup_queue(mddev->queue); > > if (mddev->gendisk) { > > del_gendisk(mddev->gendisk); > > put_disk(mddev->gendisk); > > } > > - if (mddev->queue) > > - blk_cleanup_queue(mddev->queue); > > > > kfree(mddev); > > } > > -- > > 2.4.0 > > > > > From ef8878259ce360ec352b591746761249393cf952 Mon Sep 17 00:00:00 2001 > > From: NeilBrown > > Date: Thu, 7 May 2015 21:49:41 +0900 > > Subject: [PATCH 1/2] block: bdi_unregister() now contains very little > > functionality. > > > > It contains a "WARN_ON" if bdi->dev is NULL. This warning is of no > > real consequence as bdi->dev isn't needed by anything else in the function, > > and it triggers if > > blk_cleanup_queue() -> bdi_destroy() > > is called before bdi_unregister, which a subsequent patch will make happen. > > So this isn't wanted. > > > > It also calls bdi_set_min_ratio(). This needs to be called after > > writes through the bdi have all been flushed, and before the bdi is destroyed. > > Calling it early is better than calling it late as it frees up a global > > resource. > > > > Calling it immediately after bdi_wb_shutdown() in bdi_destroy() > > perfectly fits these requirements. > > > > So bdi_unregister can be discarded with the important content moved to > > bdi_destroy, as can the > > writeback_bdi_unregister > > event which is already not used. > > > > This is tagged for 'stable' as it is a pre-requisite for a subsequent > > patch which moves calls to blk_cleanup_queue() before calls to > > del_gendisk(). The commit identified as 'Fixes' removed a lot of > > other functionality from bdi_unregister(), and made a change which > > necessitated moving the blk_cleanup_queue() calls. > > > > Reported-by: Mike Snitzer > > Cc: Christoph Hellwig > > Cc: Peter Zijlstra > > Cc: stable@vger.kernel.org (v4.0) > > Fixes: c4db59d31e39ea067c32163ac961e9c80198fd37 > > Signed-off-by: NeilBrown > > --- > > block/genhd.c | 1 - > > include/linux/backing-dev.h | 1 - > > include/trace/events/writeback.h | 1 - > > mm/backing-dev.c | 18 +----------------- > > 4 files changed, 1 insertion(+), 20 deletions(-) > > > > diff --git a/block/genhd.c b/block/genhd.c > > index 64600e9..2fba82c 100644 > > --- a/block/genhd.c > > +++ b/block/genhd.c > > @@ -653,7 +653,6 @@ void del_gendisk(struct gendisk *disk) > > disk->flags &= ~GENHD_FL_UP; > > > > sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi"); > > - bdi_unregister(&disk->queue->backing_dev_info); > > blk_unregister_queue(disk); > > blk_unregister_region(disk_devt(disk), disk->minors); > > > > diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h > > index aff923a..d87d8ec 100644 > > --- a/include/linux/backing-dev.h > > +++ b/include/linux/backing-dev.h > > @@ -116,7 +116,6 @@ __printf(3, 4) > > int bdi_register(struct backing_dev_info *bdi, struct device *parent, > > const char *fmt, ...); > > int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev); > > -void bdi_unregister(struct backing_dev_info *bdi); > > int __must_check bdi_setup_and_register(struct backing_dev_info *, char *); > > void bdi_start_writeback(struct backing_dev_info *bdi, long nr_pages, > > enum wb_reason reason); > > diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h > > index 880dd74..c178d13 100644 > > --- a/include/trace/events/writeback.h > > +++ b/include/trace/events/writeback.h > > @@ -250,7 +250,6 @@ DEFINE_EVENT(writeback_class, name, \ > > DEFINE_WRITEBACK_EVENT(writeback_nowork); > > DEFINE_WRITEBACK_EVENT(writeback_wake_background); > > DEFINE_WRITEBACK_EVENT(writeback_bdi_register); > > -DEFINE_WRITEBACK_EVENT(writeback_bdi_unregister); > > > > DECLARE_EVENT_CLASS(wbc_class, > > TP_PROTO(struct writeback_control *wbc, struct backing_dev_info *bdi), > > diff --git a/mm/backing-dev.c b/mm/backing-dev.c > > index 6dc4580..000e7b3 100644 > > --- a/mm/backing-dev.c > > +++ b/mm/backing-dev.c > > @@ -359,23 +359,6 @@ static void bdi_wb_shutdown(struct backing_dev_info *bdi) > > flush_delayed_work(&bdi->wb.dwork); > > } > > > > -/* > > - * Called when the device behind @bdi has been removed or ejected. > > - * > > - * We can't really do much here except for reducing the dirty ratio at > > - * the moment. In the future we should be able to set a flag so that > > - * the filesystem can handle errors at mark_inode_dirty time instead > > - * of only at writeback time. > > - */ > > -void bdi_unregister(struct backing_dev_info *bdi) > > -{ > > - if (WARN_ON_ONCE(!bdi->dev)) > > - return; > > - > > - bdi_set_min_ratio(bdi, 0); > > -} > > -EXPORT_SYMBOL(bdi_unregister); > > - > > static void bdi_wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi) > > { > > memset(wb, 0, sizeof(*wb)); > > @@ -443,6 +426,7 @@ void bdi_destroy(struct backing_dev_info *bdi) > > int i; > > > > bdi_wb_shutdown(bdi); > > + bdi_set_min_ratio(bdi, 0); > > > > WARN_ON(!list_empty(&bdi->work_list)); > > WARN_ON(delayed_work_pending(&bdi->wb.dwork)); > > -- > > 2.4.0 > > > > > -- > Kind regards, > Minchan Kim >