| Message ID | 1279820660.2104.27.camel@achroite.uk.solarflarecom.com |
|---|---|
| State | New, archived |
| Headers | show |
This is done on purpose. Otherwise, as soon as someone registers mtd translation layer with partitions, the add_gendisk will scan partitions and thus call blktrans_open() and mtd_table_mutex is already held. Here I can unload both mtd and blktrans driver after mtd driver removes mtd device (that happens when I remove the xD card) While card is inserted its indeed not possible to remove nether mtd nor translation layer driver.
On Sat, 2010-07-24 at 19:07 +0300, Maxim Levitsky wrote: > This is done on purpose. > > Otherwise, as soon as someone registers mtd translation layer with partitions, > the add_gendisk will scan partitions and thus call blktrans_open() > and mtd_table_mutex is already held. OK, I get it. Maybe that should be deferred to a work item. > Here I can unload both mtd and blktrans driver after mtd driver > removes mtd device (that happens when I remove the xD card) > While card is inserted its indeed not possible to remove nether mtd > nor translation layer driver. This only works if the MTD itself is hotplugged, and not if the MTD's parent is hotplugged. In fixing one case you have broken the other. Ben.
On Sat, Jul 24, 2010 at 8:03 PM, Ben Hutchings <bhutchings@solarflare.com> wrote: > OK, I get it. Maybe that should be deferred to a work item. > I thought about that too. It doesn't solve the races, because now the deferred work races with device removal, just like the function that adds new mtd device. Unless it takes mtd_table_mutex.... > This only works if the MTD itself is hotplugged, and not if the MTD's > parent is hotplugged. In fixing one case you have broken the other. Why? In my case the mtd device does have a parent, a pci device. If pci device is removed, I just remove the mtd device from the pci driver. (although this codepath it untested)
On Sun, 2010-07-25 at 10:41 +0300, Maxim Levitsky wrote: > On Sat, Jul 24, 2010 at 8:03 PM, Ben Hutchings > <bhutchings@solarflare.com> wrote: > > OK, I get it. Maybe that should be deferred to a work item. > > > > I thought about that too. > It doesn't solve the races, because now the deferred work races with > device removal, just like the function that adds new mtd device. > Unless it takes mtd_table_mutex.... You can cancel it in device removal. > > This only works if the MTD itself is hotplugged, and not if the MTD's > > parent is hotplugged. In fixing one case you have broken the other. > > Why? > > In my case the mtd device does have a parent, a pci device. > If pci device is removed, I just remove the mtd device from the pci driver. > (although this codepath it untested) OK, I have tested this and you are right. But it is a pain to have to unbind all devices before removing the module. Ben.
On Thu, Jul 22, 2010 at 10:44 AM, Ben Hutchings <bhutchings@solarflare.com> wrote: > commit 048d87199566663e4edc4880df3703c04bcf41d9 "mtd: blktrans: Hotplug > fixes" causes the MTD and the MTD driver module's ref-counts to be > bumped for each block device that is created, whether or not it's > actually in use. > > This means that before removing an MTD or driver module one must first > remove all the associated block devices. But the block devices will > be removed only when the MTDs themselves are removed, which is no > longer possible. Just updated to 2.6.36-rc3 and noticed a related issue. Configuration is: CONFIG_MTD_BLOCK=y CONFIG_MTD_BLKDEVS=y CONFIG_MTD_UBI_GLUEBI=y When I "ubiattach" a physical MTD device, a gluebi MTD device is automatically created for each UBI volume. The notifiers cause add_mtd_blktrans_dev() to get called for each new gluebi device. But add_mtd_blktrans_dev() now calls __get_mtd_device(), locking up the gluebi device and preventing it from being used: # ubiattach -m3 UBI: attaching mtd3 to ubi0 UBI: physical eraseblock size: 131072 bytes (128 KiB) UBI: logical eraseblock size: 130944 bytes UBI: smallest flash I/O unit: 1 UBI: VID header offset: 64 (aligned 64) UBI: data offset: 128 UBI: max. sequence number: 14 UBI: attached mtd3 to ubi0 UBI: MTD device name: "root" UBI: MTD device size: 16 MiB UBI: number of good PEBs: 128 UBI: number of bad PEBs: 0 UBI: max. allowed volumes: 128 UBI: wear-leveling threshold: 4096 UBI: number of internal volumes: 1 UBI: number of user volumes: 1 UBI: available PEBs: 0 UBI: total number of reserved PEBs: 128 UBI: number of PEBs reserved for bad PEB handling: 0 UBI: max/mean erase counter: 1/0 UBI: image sequence number: 0 UBI: background thread "ubi_bgt0d" started, PID 448 UBI device number 0, total 128 LEBs (16760832 bytes, 16.0 MiB), available 0 LEBs (0 bytes), LEB size 130944 bytes (127.9 KiB) # mount -t ubifs ubi0:rootfs /mnt UBI error: ubi_open_volume: cannot open device 0, volume 0, error -16 mount: mounting ubi0:rootfs on /mnt failed: Device or resource busy # ubidetach -m3 ubidetach: error!: cannot detach mtd3 error 16 (Device or resource busy) My short-term workaround is to disable CONFIG_MTD_BLOCK and CONFIG_MTD_BLKDEVS. It would be nice to fix the code for 2.6.36 though.
On Sun, 2010-09-05 at 16:34 -0700, Kevin Cernekee wrote: > On Thu, Jul 22, 2010 at 10:44 AM, Ben Hutchings > <bhutchings@solarflare.com> wrote: > > commit 048d87199566663e4edc4880df3703c04bcf41d9 "mtd: blktrans: Hotplug > > fixes" causes the MTD and the MTD driver module's ref-counts to be > > bumped for each block device that is created, whether or not it's > > actually in use. > > > > This means that before removing an MTD or driver module one must first > > remove all the associated block devices. But the block devices will > > be removed only when the MTDs themselves are removed, which is no > > longer possible. > > Just updated to 2.6.36-rc3 and noticed a related issue. Configuration is: Sounds like a regression which should be either fixed or the patch which causes this should be reverted. Maxim?
On Tue, 2010-09-07 at 13:27 +0300, Artem Bityutskiy wrote: > On Sun, 2010-09-05 at 16:34 -0700, Kevin Cernekee wrote: > > On Thu, Jul 22, 2010 at 10:44 AM, Ben Hutchings > > <bhutchings@solarflare.com> wrote: > > > commit 048d87199566663e4edc4880df3703c04bcf41d9 "mtd: blktrans: Hotplug > > > fixes" causes the MTD and the MTD driver module's ref-counts to be > > > bumped for each block device that is created, whether or not it's > > > actually in use. > > > > > > This means that before removing an MTD or driver module one must first > > > remove all the associated block devices. But the block devices will > > > be removed only when the MTDs themselves are removed, which is no > > > longer possible. > > > > Just updated to 2.6.36-rc3 and noticed a related issue. Configuration is: > > Sounds like a regression which should be either fixed or the patch which > causes this should be reverted. Maxim? We have a tough issue here. The way mtd core is built, it assumes that one can always remove an mtd device, therefore all users must let it go as soon as asked first time. Its possible to fix that of course, but I'll need to touch a lot of code I can't test. Really mtd core wasn't build with hotplug in the mind. I'll think of something (and you all are welcome to help). Best regards, Maxim Levitsky
On Sun, 2010-09-05 at 16:34 -0700, Kevin Cernekee wrote: > On Thu, Jul 22, 2010 at 10:44 AM, Ben Hutchings > <bhutchings@solarflare.com> wrote: > > commit 048d87199566663e4edc4880df3703c04bcf41d9 "mtd: blktrans: Hotplug > > fixes" causes the MTD and the MTD driver module's ref-counts to be > > bumped for each block device that is created, whether or not it's > > actually in use. > > > > This means that before removing an MTD or driver module one must first > > remove all the associated block devices. But the block devices will > > be removed only when the MTDs themselves are removed, which is no > > longer possible. > > Just updated to 2.6.36-rc3 and noticed a related issue. Configuration is: > > CONFIG_MTD_BLOCK=y > CONFIG_MTD_BLKDEVS=y > CONFIG_MTD_UBI_GLUEBI=y Would you please check if Maxim's patch fixes this: [PATCH V2] MTD: blktrans: Final version of hotplug support
On Mon, Sep 20, 2010 at 1:30 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Sun, 2010-09-05 at 16:34 -0700, Kevin Cernekee wrote: >> Just updated to 2.6.36-rc3 and noticed a related issue. Configuration is: >> >> CONFIG_MTD_BLOCK=y >> CONFIG_MTD_BLKDEVS=y >> CONFIG_MTD_UBI_GLUEBI=y > > Would you please check if Maxim's patch fixes this: > [PATCH V2] MTD: blktrans: Final version of hotplug support I applied the following patch to my 2.6.36-rc4 tree: Subject: [PATCH 1/3] MTD: blktrans: Final version of hotplug support Date: Sun, 19 Sep 2010 01:12:36 +0200 Message-Id: <1284851558-1154-2-git-send-email-maximlevitsky@gmail.com> This fixed the MTD_BLOCK + MTD_UBI_GLUEBI regression that I reported earlier. Thanks.
On Mon, 2010-09-20 at 14:47 -0700, Kevin Cernekee wrote: > On Mon, Sep 20, 2010 at 1:30 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > On Sun, 2010-09-05 at 16:34 -0700, Kevin Cernekee wrote: > >> Just updated to 2.6.36-rc3 and noticed a related issue. Configuration is: > >> > >> CONFIG_MTD_BLOCK=y > >> CONFIG_MTD_BLKDEVS=y > >> CONFIG_MTD_UBI_GLUEBI=y > > > > Would you please check if Maxim's patch fixes this: > > [PATCH V2] MTD: blktrans: Final version of hotplug support > > I applied the following patch to my 2.6.36-rc4 tree: > > Subject: [PATCH 1/3] MTD: blktrans: Final version of hotplug support > Date: Sun, 19 Sep 2010 01:12:36 +0200 > Message-Id: <1284851558-1154-2-git-send-email-maximlevitsky@gmail.com> > > This fixed the MTD_BLOCK + MTD_UBI_GLUEBI regression that I reported earlier. Sorry for not sending updates, I did split this patch up, and will send it soon. Was finding workarounds, filling bugs for Ubuntu 10.10 brain damage. (Had to install it somewhen). Look one cosmetic, but annoying regression turned out to be a explicit feature, and this 'bug' fixes it... https://bugs.launchpad.net/ubuntu/+source/gnome-power-manager/+bug/619816 Sorry for this ooftopic rant, but it really got my nerves. Best regards, Maxim Levitsky
diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c index 03e19c1..686240b 100644 --- a/drivers/mtd/mtd_blkdevs.c +++ b/drivers/mtd/mtd_blkdevs.c @@ -166,6 +166,7 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode) if (!dev) return -ERESTARTSYS; + mutex_lock(&mtd_table_mutex); mutex_lock(&dev->lock); if (!dev->mtd) { @@ -173,14 +174,22 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode) goto unlock; } + __get_mtd_device(dev->mtd); + __module_get(dev->tr->owner); ret = !dev->open++ && dev->tr->open ? dev->tr->open(dev) : 0; - /* Take another reference on the device so it won't go away till - last release */ - if (!ret) + if (!ret) { + /* Take another reference on the device so it won't go + * away till last release */ kref_get(&dev->ref); + } else { + module_put(dev->tr->owner); + __put_mtd_device(dev->mtd); + } + unlock: mutex_unlock(&dev->lock); + mutex_unlock(&mtd_table_mutex); blktrans_dev_put(dev); return ret; } @@ -193,6 +202,7 @@ static int blktrans_release(struct gendisk *disk, fmode_t mode) if (!dev) return ret; + mutex_lock(&mtd_table_mutex); mutex_lock(&dev->lock); /* Release one reference, we sure its not the last one here*/ @@ -202,8 +212,12 @@ static int blktrans_release(struct gendisk *disk, fmode_t mode) goto unlock; ret = !--dev->open && dev->tr->release ? dev->tr->release(dev) : 0; + module_put(dev->tr->owner); + __put_mtd_device(dev->mtd); + unlock: mutex_unlock(&dev->lock); + mutex_unlock(&mtd_table_mutex); blktrans_dev_put(dev); return ret; } @@ -363,9 +377,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) gd->queue = new->rq; - __get_mtd_device(new->mtd); - __module_get(tr->owner); - /* Create processing thread */ /* TODO: workqueue ? */ new->thread = kthread_run(mtd_blktrans_thread, new, @@ -388,8 +399,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) } return 0; error4: - module_put(tr->owner); - __put_mtd_device(new->mtd); blk_cleanup_queue(new->rq); error3: put_disk(new->disk); @@ -432,9 +441,6 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old) old->open = 0; } - __put_mtd_device(old->mtd); - module_put(old->tr->owner); - /* At that point, we don't touch the mtd anymore */ old->mtd = NULL;
commit 048d87199566663e4edc4880df3703c04bcf41d9 "mtd: blktrans: Hotplug fixes" causes the MTD and the MTD driver module's ref-counts to be bumped for each block device that is created, whether or not it's actually in use. This means that before removing an MTD or driver module one must first remove all the associated block devices. But the block devices will be removed only when the MTDs themselves are removed, which is no longer possible. Change blktrans_open() and blktrans_release() to grab the mtd_table_mutex as well as the mtd_blktrans_dev lock, and move the calls to __mtd_{get,put}_device() and {__get,put}_module() back into these functions. Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> --- Currently the sfc module and presumably any other MTD driver module cannot be unloaded if mtd_blkdevs is loaded. This patch restores the behaviour of 2.6.34 and ealier releases: it can be unloaded if and only if no related MTD char or block devices are currently open. Please send this to Linus in time for 2.6.35. Ben. drivers/mtd/mtd_blkdevs.c | 28 +++++++++++++++++----------- 1 files changed, 17 insertions(+), 11 deletions(-)