Re: mtd: blktrans: Hotplug fixes

Re: mtd: blktrans: Hotplug fixes

[Ben Hutchings]

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 normally the block devices
will be removed only when the MTDs themselves are removed.  This mutual
dependency can't even be resolved by poking at 'unbind' files in sysfs,
because block-translation drivers don't appear in the driver model!

Unless I'm missing something, this does not fix hotplug but completely
breaks it.  Please revert it and find a better way to fix locking in the
block-translation layer.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Ben Hutchings]

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(-)

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;
 
-- 
1.6.2.5

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Maxim Levitsky]

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.

Re: [PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Ben Hutchings]

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.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Maxim Levitsky]

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)

Re: [PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Ben Hutchings]

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.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Kevin Cernekee]

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.

Re: [PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Artem Bityutskiy]

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?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Maxim Levitsky]

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

Re: [PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Artem Bityutskiy]

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

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Kevin Cernekee]

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.

Re: [PATCH 2.6.35] mtd: blktrans: Move device and module ref-counting back to open/release

[Maxim Levitsky]

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