Patchwork [01/17] MTD: create lockless versions of {get, put}_mtd_device This will be used to resolve deadlock in block translation layer.

login
register
mail settings
Submitter Maxim Levitsky
Date Feb. 9, 2010, 4:57 p.m.
Message ID <1265734665-22656-2-git-send-email-maximlevitsky@gmail.com>
Download mbox | patch
Permalink /patch/44928/
State New
Headers show

Comments

Maxim Levitsky - Feb. 9, 2010, 4:57 p.m.
These functions can be used as long as we don't need access to global mtd table, but have
a pointer to the mtd device.

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
---
 drivers/mtd/mtdcore.c   |   60 ++++++++++++++++++++++++++++++----------------
 include/linux/mtd/mtd.h |    3 +-
 2 files changed, 41 insertions(+), 22 deletions(-)
Peter Zijlstra - Feb. 9, 2010, 5:14 p.m.
On Tue, 2010-02-09 at 18:57 +0200, Maxim Levitsky wrote:
> These functions can be used as long as we don't need access to global mtd table, but have
> a pointer to the mtd device.
> 
> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> ---
>  drivers/mtd/mtdcore.c   |   60 ++++++++++++++++++++++++++++++----------------
>  include/linux/mtd/mtd.h |    3 +-
>  2 files changed, 41 insertions(+), 22 deletions(-)

> +int __get_mtd_device(struct mtd_info *mtd)
> +{
> +	int err;
> +
> +	if (!try_module_get(mtd->owner))
> +		return -ENODEV;
> +
> +	if (mtd->get_device) {
> +
> +		err = mtd->get_device(mtd);
> +
> +		if (err) {
> +			module_put(mtd->owner);
> +			return err;
> +		}
> +	}
> +	mtd->usecount++;
> +	return 0;
>  }

> +void __put_mtd_device(struct mtd_info *mtd)
> +{
> +	--mtd->usecount;
> +	BUG_ON(mtd->usecount < 0);
> +
>  	if (mtd->put_device)
>  		mtd->put_device(mtd);
>  
>  	module_put(mtd->owner);
>  }

That's racy, use kref.
Maxim Levitsky - Feb. 9, 2010, 5:23 p.m.
On Tue, 2010-02-09 at 18:14 +0100, Peter Zijlstra wrote: 
> On Tue, 2010-02-09 at 18:57 +0200, Maxim Levitsky wrote:
> > These functions can be used as long as we don't need access to global mtd table, but have
> > a pointer to the mtd device.
> > 
> > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > ---
> >  drivers/mtd/mtdcore.c   |   60 ++++++++++++++++++++++++++++++----------------
> >  include/linux/mtd/mtd.h |    3 +-
> >  2 files changed, 41 insertions(+), 22 deletions(-)
> 
> > +int __get_mtd_device(struct mtd_info *mtd)
> > +{
> > +	int err;
> > +
> > +	if (!try_module_get(mtd->owner))
> > +		return -ENODEV;
> > +
> > +	if (mtd->get_device) {
> > +
> > +		err = mtd->get_device(mtd);
> > +
> > +		if (err) {
> > +			module_put(mtd->owner);
> > +			return err;
> > +		}
> > +	}
> > +	mtd->usecount++;
> > +	return 0;
> >  }
> 
> > +void __put_mtd_device(struct mtd_info *mtd)
> > +{
> > +	--mtd->usecount;
> > +	BUG_ON(mtd->usecount < 0);
> > +
> >  	if (mtd->put_device)
> >  		mtd->put_device(mtd);
> >  
> >  	module_put(mtd->owner);
> >  }
> 
> That's racy, use kref.
> 
Couldn't agree with you more.

However, these functions aren't intended for general use, and probably
will be used by mtd translation layer only. I do have a lock that
protects concurrent use of these functions.

Thus, I better add a comment about this?

Best regards,
Maxim Levitsky
Maxim Levitsky - Feb. 9, 2010, 5:46 p.m.
On Tue, 2010-02-09 at 19:23 +0200, Maxim Levitsky wrote: 
> On Tue, 2010-02-09 at 18:14 +0100, Peter Zijlstra wrote: 
> > On Tue, 2010-02-09 at 18:57 +0200, Maxim Levitsky wrote:
> > > These functions can be used as long as we don't need access to global mtd table, but have
> > > a pointer to the mtd device.
> > > 
> > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > > ---
> > >  drivers/mtd/mtdcore.c   |   60 ++++++++++++++++++++++++++++++----------------
> > >  include/linux/mtd/mtd.h |    3 +-
> > >  2 files changed, 41 insertions(+), 22 deletions(-)
> > 
> > > +int __get_mtd_device(struct mtd_info *mtd)
> > > +{
> > > +	int err;
> > > +
> > > +	if (!try_module_get(mtd->owner))
> > > +		return -ENODEV;
> > > +
> > > +	if (mtd->get_device) {
> > > +
> > > +		err = mtd->get_device(mtd);
> > > +
> > > +		if (err) {
> > > +			module_put(mtd->owner);
> > > +			return err;
> > > +		}
> > > +	}
> > > +	mtd->usecount++;
> > > +	return 0;
> > >  }
> > 
> > > +void __put_mtd_device(struct mtd_info *mtd)
> > > +{
> > > +	--mtd->usecount;
> > > +	BUG_ON(mtd->usecount < 0);
> > > +
> > >  	if (mtd->put_device)
> > >  		mtd->put_device(mtd);
> > >  
> > >  	module_put(mtd->owner);
> > >  }
> > 
> > That's racy, use kref.
> > 
> Couldn't agree with you more.
> 
> However, these functions aren't intended for general use, and probably
> will be used by mtd translation layer only. I do have a lock that
> protects concurrent use of these functions.
> 
> Thus, I better add a comment about this?
However on second thought, there is still a race if two FTLs access same
mtd device.
While this might seem impossible, and I say it is quite dangerous. I can
imagine using both some FTL and mtdblock for testing.

Just using kref (nothing against it) won't help here.
The mtd->get_device/put_device aren't expecting to be called
concurrently ether...

I can add per mtd lock, but it is a bit ugly...

What do you think?

Best regards,
Maxim Levitsky
Maxim Levitsky - Feb. 10, 2010, 9:22 p.m.
On Tue, 2010-02-09 at 19:46 +0200, Maxim Levitsky wrote: 
> On Tue, 2010-02-09 at 19:23 +0200, Maxim Levitsky wrote: 
> > On Tue, 2010-02-09 at 18:14 +0100, Peter Zijlstra wrote: 
> > > On Tue, 2010-02-09 at 18:57 +0200, Maxim Levitsky wrote:
> > > > These functions can be used as long as we don't need access to global mtd table, but have
> > > > a pointer to the mtd device.
> > > > 
> > > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > > > ---

David, I just got a great idea how to resolve the same issue in much
cleaner way.

Why not to drop the mtd table lock before calling the add notifers?
If we assume no add_mtd_device/del_mtd_device running concurrently this
should be ok. After all mtd table lock should protect only the mtd
table.
And the table only purpose it to locate an mtd device by number or name.
This purpose will always be useful, be it a static table or not.

I also remove all that mtd table junk from mtd_blktrans.c

Best regards,
Maxim Levitsky

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index c356c0a..3bdb7e8 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -453,27 +453,38 @@  struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num)
 			ret = NULL;
 	}
 
-	if (!ret)
-		goto out_unlock;
-
-	if (!try_module_get(ret->owner))
-		goto out_unlock;
-
-	if (ret->get_device) {
-		err = ret->get_device(ret);
-		if (err)
-			goto out_put;
+	if (!ret) {
+		ret = ERR_PTR(err);
+		goto out;
 	}
 
-	ret->usecount++;
+	err = __get_mtd_device(ret);
+	if (err)
+		ret = ERR_PTR(err);
+out:
 	mutex_unlock(&mtd_table_mutex);
 	return ret;
+}
 
-out_put:
-	module_put(ret->owner);
-out_unlock:
-	mutex_unlock(&mtd_table_mutex);
-	return ERR_PTR(err);
+
+int __get_mtd_device(struct mtd_info *mtd)
+{
+	int err;
+
+	if (!try_module_get(mtd->owner))
+		return -ENODEV;
+
+	if (mtd->get_device) {
+
+		err = mtd->get_device(mtd);
+
+		if (err) {
+			module_put(mtd->owner);
+			return err;
+		}
+	}
+	mtd->usecount++;
+	return 0;
 }
 
 /**
@@ -524,14 +535,19 @@  out_unlock:
 
 void put_mtd_device(struct mtd_info *mtd)
 {
-	int c;
-
 	mutex_lock(&mtd_table_mutex);
-	c = --mtd->usecount;
+	__put_mtd_device(mtd);
+	mutex_unlock(&mtd_table_mutex);
+
+}
+
+void __put_mtd_device(struct mtd_info *mtd)
+{
+	--mtd->usecount;
+	BUG_ON(mtd->usecount < 0);
+
 	if (mtd->put_device)
 		mtd->put_device(mtd);
-	mutex_unlock(&mtd_table_mutex);
-	BUG_ON(c < 0);
 
 	module_put(mtd->owner);
 }
@@ -569,7 +585,9 @@  EXPORT_SYMBOL_GPL(add_mtd_device);
 EXPORT_SYMBOL_GPL(del_mtd_device);
 EXPORT_SYMBOL_GPL(get_mtd_device);
 EXPORT_SYMBOL_GPL(get_mtd_device_nm);
+EXPORT_SYMBOL_GPL(__get_mtd_device);
 EXPORT_SYMBOL_GPL(put_mtd_device);
+EXPORT_SYMBOL_GPL(__put_mtd_device);
 EXPORT_SYMBOL_GPL(register_mtd_user);
 EXPORT_SYMBOL_GPL(unregister_mtd_user);
 EXPORT_SYMBOL_GPL(default_mtd_writev);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 0f32a9b..662d747 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -290,8 +290,9 @@  extern int add_mtd_device(struct mtd_info *mtd);
 extern int del_mtd_device (struct mtd_info *mtd);
 
 extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
+extern int __get_mtd_device(struct mtd_info *mtd);
+extern void __put_mtd_device(struct mtd_info *mtd);
 extern struct mtd_info *get_mtd_device_nm(const char *name);
-
 extern void put_mtd_device(struct mtd_info *mtd);