| Message ID | CAPcyv4gGLfLbCWmzb5Yt+oVp2pKuGwVv6iY__O_DPmaV__OZDQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/30/2016 11:52 PM, Dan Williams wrote: > On Thu, Jul 28, 2016 at 8:50 AM, Dan Williams <dan.j.williams@intel.com> wrote: >> [ adding linux-block ] >> >> On Wed, Jul 27, 2016 at 8:20 PM, Yi Zhang <yizhan@redhat.com> wrote: >>> Hello everyone >>> >>> Could you help check this issue, thanks. >>> >>> Steps I used: >>> 1. Reserve 4*8G of memory for pmem by add kernel parameter "memmap=8G!4G memmap=8G!12G memmap=8G!20G memmap=8G!28G" >>> 2. Execute below script >>> #!/bin/bash >>> pmem_btt_switch() { >>> sector_size_list="512 520 528 4096 4104 4160 4224" >>> for sector_size in $sector_size_list; do >>> ndctl create-namespace -f -e namespace${1}.0 --mode=sector -l $sector_size >>> ndctl create-namespace -f -e namespace${1}.0 --mode=raw >>> done >>> } >>> >>> for i in 0 1 2 3; do >>> pmem_btt_switch $i & >>> done >> Thanks for the report. This looks like del_gendisk() frees the >> previous usage of the devt before the bdi is unregistered. This >> appears to be a general problem with all block drivers, not just >> libnvdimm, since blk_cleanup_queue() is typically called after >> del_gendisk(). I.e. it will always be the case that the bdi >> registered with the devt allocated at add_disk() will still be alive >> when del_gendisk()->disk_release() frees the previous devt number. >> >> I *think* the path forward is to allow the bdi to hold a reference >> against the blk_alloc_devt() allocation until it is done with it. Any >> other ideas on fixing this object lifetime problem? > Does the attached patch solve this for you? Hi Dan This patch works and the issue cannot be reproduced after several times' test, thanks Another thing is during the bug verifying, I found below error message, could you check whether it is reasonable: [ 150.464620] Dev pmem1: unable to read RDB block 0 [ 150.486897] pmem1: unable to read partition table [ 150.486901] pmem1: partition table beyond EOD, truncated [ 151.133287] Buffer I/O error on dev pmem3, logical block 2, async page read [ 151.164620] Buffer I/O error on dev pmem3, logical block 2, async page read Best Regards Yi Zhang
On Sun, Jul 31, 2016 at 10:19 AM, yizhan <yizhan@redhat.com> wrote: > On 07/30/2016 11:52 PM, Dan Williams wrote: >> >> On Thu, Jul 28, 2016 at 8:50 AM, Dan Williams <dan.j.williams@intel.com> >> wrote: >>> >>> [ adding linux-block ] >>> >>> On Wed, Jul 27, 2016 at 8:20 PM, Yi Zhang <yizhan@redhat.com> wrote: >>>> >>>> Hello everyone >>>> >>>> Could you help check this issue, thanks. >>>> >>>> Steps I used: >>>> 1. Reserve 4*8G of memory for pmem by add kernel parameter "memmap=8G!4G >>>> memmap=8G!12G memmap=8G!20G memmap=8G!28G" >>>> 2. Execute below script >>>> #!/bin/bash >>>> pmem_btt_switch() { >>>> sector_size_list="512 520 528 4096 4104 4160 4224" >>>> for sector_size in $sector_size_list; do >>>> ndctl create-namespace -f -e namespace${1}.0 >>>> --mode=sector -l $sector_size >>>> ndctl create-namespace -f -e namespace${1}.0 --mode=raw >>>> done >>>> } >>>> >>>> for i in 0 1 2 3; do >>>> pmem_btt_switch $i & >>>> done >>> >>> Thanks for the report. This looks like del_gendisk() frees the >>> previous usage of the devt before the bdi is unregistered. This >>> appears to be a general problem with all block drivers, not just >>> libnvdimm, since blk_cleanup_queue() is typically called after >>> del_gendisk(). I.e. it will always be the case that the bdi >>> registered with the devt allocated at add_disk() will still be alive >>> when del_gendisk()->disk_release() frees the previous devt number. >>> >>> I *think* the path forward is to allow the bdi to hold a reference >>> against the blk_alloc_devt() allocation until it is done with it. Any >>> other ideas on fixing this object lifetime problem? >> >> Does the attached patch solve this for you? > > Hi Dan > This patch works and the issue cannot be reproduced after several times' > test, thanks Thank you! > Another thing is during the bug verifying, I found below error message, > could you check whether it is reasonable: > [ 150.464620] Dev pmem1: unable to read RDB block 0 > [ 150.486897] pmem1: unable to read partition table > [ 150.486901] pmem1: partition table beyond EOD, truncated > [ 151.133287] Buffer I/O error on dev pmem3, logical block 2, async page > read > [ 151.164620] Buffer I/O error on dev pmem3, logical block 2, async page > read > This test is racing block device registration versus teardown. These messages are expected and are likely coming from the block queue percpu ref being marked dead while the partition scan runs. When this happens blk_queue_enter() in generic_make_request() returns errors for every new I/O submission while blk_cleanup_queue() runs. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/01/2016 01:54 AM, Dan Williams wrote: > On Sun, Jul 31, 2016 at 10:19 AM, yizhan <yizhan@redhat.com> wrote: >> On 07/30/2016 11:52 PM, Dan Williams wrote: >>> On Thu, Jul 28, 2016 at 8:50 AM, Dan Williams <dan.j.williams@intel.com> >>> wrote: >>>> [ adding linux-block ] >>>> >>>> On Wed, Jul 27, 2016 at 8:20 PM, Yi Zhang <yizhan@redhat.com> wrote: >>>>> Hello everyone >>>>> >>>>> Could you help check this issue, thanks. >>>>> >>>>> Steps I used: >>>>> 1. Reserve 4*8G of memory for pmem by add kernel parameter "memmap=8G!4G >>>>> memmap=8G!12G memmap=8G!20G memmap=8G!28G" >>>>> 2. Execute below script >>>>> #!/bin/bash >>>>> pmem_btt_switch() { >>>>> sector_size_list="512 520 528 4096 4104 4160 4224" >>>>> for sector_size in $sector_size_list; do >>>>> ndctl create-namespace -f -e namespace${1}.0 >>>>> --mode=sector -l $sector_size >>>>> ndctl create-namespace -f -e namespace${1}.0 --mode=raw >>>>> done >>>>> } >>>>> >>>>> for i in 0 1 2 3; do >>>>> pmem_btt_switch $i & >>>>> done >>>> Thanks for the report. This looks like del_gendisk() frees the >>>> previous usage of the devt before the bdi is unregistered. This >>>> appears to be a general problem with all block drivers, not just >>>> libnvdimm, since blk_cleanup_queue() is typically called after >>>> del_gendisk(). I.e. it will always be the case that the bdi >>>> registered with the devt allocated at add_disk() will still be alive >>>> when del_gendisk()->disk_release() frees the previous devt number. >>>> >>>> I *think* the path forward is to allow the bdi to hold a reference >>>> against the blk_alloc_devt() allocation until it is done with it. Any >>>> other ideas on fixing this object lifetime problem? >>> Does the attached patch solve this for you? >> Hi Dan >> This patch works and the issue cannot be reproduced after several times' >> test, thanks > Thank you! > >> Another thing is during the bug verifying, I found below error message, >> could you check whether it is reasonable: >> [ 150.464620] Dev pmem1: unable to read RDB block 0 >> [ 150.486897] pmem1: unable to read partition table >> [ 150.486901] pmem1: partition table beyond EOD, truncated >> [ 151.133287] Buffer I/O error on dev pmem3, logical block 2, async page >> read >> [ 151.164620] Buffer I/O error on dev pmem3, logical block 2, async page >> read >> > This test is racing block device registration versus teardown. These > messages are expected and are likely coming from the block queue > percpu ref being marked dead while the partition scan runs. When this > happens blk_queue_enter() in generic_make_request() returns errors for > every new I/O submission while blk_cleanup_queue() runs. OK, thanks for your explanation. Best Regards Yi Zhang -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From 44bcbf8c531e9249d09e6bf502d3696668f3d22c Mon Sep 17 00:00:00 2001 From: Dan Williams <dan.j.williams@intel.com> Date: Sat, 30 Jul 2016 08:23:06 -0700 Subject: [PATCH] block: fix bdi vs gendisk lifetime mismatch The bdi for gendisk is named after the gendisk. However, since the gendisk is destroyed before the bdi it leaves a window where a new gendisk could dynamically reuse the same devt while a bdi while a bdi with the same name is still live. Arrange for the bdi to hold a reference against its "owner" disk device while it is registered. Otherwise we can hit sysfs duplicate name collisions like the following: WARNING: CPU: 10 PID: 2078 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x64/0x80 sysfs: cannot create duplicate filename '/devices/virtual/bdi/259:1' Hardware name: HP ProLiant DL580 Gen8, BIOS P79 05/06/2015 0000000000000286 0000000002c04ad5 ffff88006f24f970 ffffffff8134caec ffff88006f24f9c0 0000000000000000 ffff88006f24f9b0 ffffffff8108c351 0000001f0000000c ffff88105d236000 ffff88105d1031e0 ffff8800357427f8 Call Trace: [<ffffffff8134caec>] dump_stack+0x63/0x87 [<ffffffff8108c351>] __warn+0xd1/0xf0 [<ffffffff8108c3cf>] warn_slowpath_fmt+0x5f/0x80 [<ffffffff812a0d34>] sysfs_warn_dup+0x64/0x80 [<ffffffff812a0e1e>] sysfs_create_dir_ns+0x7e/0x90 [<ffffffff8134faaa>] kobject_add_internal+0xaa/0x320 [<ffffffff81358d4e>] ? vsnprintf+0x34e/0x4d0 [<ffffffff8134ff55>] kobject_add+0x75/0xd0 [<ffffffff816e66b2>] ? mutex_lock+0x12/0x2f [<ffffffff8148b0a5>] device_add+0x125/0x610 [<ffffffff8148b788>] device_create_groups_vargs+0xd8/0x100 [<ffffffff8148b7cc>] device_create_vargs+0x1c/0x20 [<ffffffff811b775c>] bdi_register+0x8c/0x180 [<ffffffff811b7877>] bdi_register_dev+0x27/0x30 [<ffffffff813317f5>] add_disk+0x175/0x4a0 Reported-by: Yi Zhang <yizhan@redhat.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- block/genhd.c | 2 +- include/linux/backing-dev-defs.h | 1 + include/linux/backing-dev.h | 1 + mm/backing-dev.c | 18 ++++++++++++++++++ 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/block/genhd.c b/block/genhd.c index 3c9dede4e04f..f6f7ffcd4eab 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -614,7 +614,7 @@ void device_add_disk(struct device *parent, struct gendisk *disk) /* Register BDI before referencing it from bdev */ bdi = &disk->queue->backing_dev_info; - bdi_register_dev(bdi, disk_devt(disk)); + bdi_register_owner(bdi, disk_to_dev(disk)); blk_register_region(disk_devt(disk), disk->minors, NULL, exact_match, exact_lock, disk); diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h index 3f103076d0bf..c357f27d5483 100644 --- a/include/linux/backing-dev-defs.h +++ b/include/linux/backing-dev-defs.h @@ -163,6 +163,7 @@ struct backing_dev_info { wait_queue_head_t wb_waitq; struct device *dev; + struct device *owner; struct timer_list laptop_mode_wb_timer; diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h index 491a91717788..43b93a947e61 100644 --- a/include/linux/backing-dev.h +++ b/include/linux/backing-dev.h @@ -24,6 +24,7 @@ __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); +int bdi_register_owner(struct backing_dev_info *bdi, struct device *owner); void bdi_unregister(struct backing_dev_info *bdi); int __must_check bdi_setup_and_register(struct backing_dev_info *, char *); diff --git a/mm/backing-dev.c b/mm/backing-dev.c index efe237742074..7b51cb7905be 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -825,6 +825,19 @@ int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev) } EXPORT_SYMBOL(bdi_register_dev); +int bdi_register_owner(struct backing_dev_info *bdi, struct device *owner) +{ + int rc; + + rc = bdi_register(bdi, NULL, "%u:%u", MAJOR(owner->devt), + MINOR(owner->devt)); + if (rc) + return rc; + bdi->owner = owner; + get_device(owner); +} +EXPORT_SYMBOL(bdi_register_owner); + /* * Remove bdi from bdi_list, and ensure that it is no longer visible */ @@ -849,6 +862,11 @@ void bdi_unregister(struct backing_dev_info *bdi) device_unregister(bdi->dev); bdi->dev = NULL; } + + if (bdi->owner) { + put_device(bdi->owner); + bdi->owner = NULL; + } } void bdi_exit(struct backing_dev_info *bdi) -- 2.5.5