bcache: explicitly destroy mutex while exiting

PATCH In review Liang Chen 4 participants 7 messages Oct 10, 2017 – Oct 30, 2017 bcache (block layer cache)

Share & tools

Liang Chen

author

2017-10-10 09:00 UTC #0 lore raw

PATCH v2 1/1 bcache: explicitly destroy mutex while exiting

mutex_destroy does nothing most of time, but it's better to call it to make the code future proof and it also has some meaning for like mutex debug.

As Coly pointed out in a previous review, bcache_exit() may not be able to handle all the references properly if userspace registers cache and backing devices right before bch_debug_init runs and bch_debug_init failes later. So not exposing userspace interface until everything is ready to avoid that issue.

Signed-off-by Liang Chen <[email protected]>

---
 drivers/md/bcache/super.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/md/bcache/super.c +4 -2

index fc0a31b..25bf003 100644
@@ -2085,6 +2085,7 @@ static void bcache_exit(void)
 	if (bcache_major)
 		unregister_blkdev(bcache_major, "bcache");
 	unregister_reboot_notifier(&reboot);
+	mutex_destroy(&bch_register_lock);
 }
 
 static int __init bcache_init(void)

@@ -2103,14 +2104,15 @@ static int __init bcache_init(void)
 	bcache_major = register_blkdev(0, "bcache");
 	if (bcache_major < 0) {
 		unregister_reboot_notifier(&reboot);
+		mutex_destroy(&bch_register_lock);
 		return bcache_major;
 	}
 
 	if (!(bcache_wq = alloc_workqueue("bcache", WQ_MEM_RECLAIM, 0)) ||
 	    !(bcache_kobj = kobject_create_and_add("bcache", fs_kobj)) ||
-	    sysfs_create_files(bcache_kobj, files) ||
 	    bch_request_init() ||
-	    bch_debug_init(bcache_kobj))
+	    bch_debug_init(bcache_kobj) ||
+	    sysfs_create_files(bcache_kobj, files))
 		goto err;
 
 	return 0;

Signature · 1 line

1.8.3.1

Coly Li

Reviewed-by on v2 1/1

2017-10-10 12:25 UTC #1 lore raw

Quoted 11 lines · Liang Chen

mutex_destroy does nothing most of time, but it's better to call it to make the code future proof and it also has some meaning for like mutex debug.
As Coly pointed out in a previous review, bcache_exit() may not be able to handle all the references properly if userspace registers cache and backing devices right before bch_debug_init runs and bch_debug_init failes later. So not exposing userspace interface until everything is ready to avoid that issue.
Signed-off-by Liang Chen <[email protected]>

Hi Liang,

No more comment from me, it looks good. Thanks.

Reviewed-by Coly Li <[email protected]>

Quoted 35 lines

---
 drivers/md/bcache/super.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/md/bcache/super.c +4 -2

index fc0a31b..25bf003 100644
@@ -2085,6 +2085,7 @@ static void bcache_exit(void)
 	if (bcache_major)
 		unregister_blkdev(bcache_major, "bcache");
 	unregister_reboot_notifier(&reboot);
+	mutex_destroy(&bch_register_lock);
 }
 
 static int __init bcache_init(void)

@@ -2103,14 +2104,15 @@ static int __init bcache_init(void)
 	bcache_major = register_blkdev(0, "bcache");
 	if (bcache_major < 0) {
 		unregister_reboot_notifier(&reboot);
+		mutex_destroy(&bch_register_lock);
 		return bcache_major;
 	}
 
 	if (!(bcache_wq = alloc_workqueue("bcache", WQ_MEM_RECLAIM, 0)) ||
 	    !(bcache_kobj = kobject_create_and_add("bcache", fs_kobj)) ||
-	    sysfs_create_files(bcache_kobj, files) ||
 	    bch_request_init() ||
-	    bch_debug_init(bcache_kobj))
+	    bch_debug_init(bcache_kobj) ||
+	    sysfs_create_files(bcache_kobj, files))
 		goto err;
 
 	return 0;

Signature · 1 line

Coly Li

Michael Lyle

Reviewed-by

2017-10-10 15:44 UTC #2 lore raw

Quoted 11 lines · Coly Li

Quoted 10 lines · Liang Chen
mutex_destroy does nothing most of time, but it's better to call it to make the code future proof and it also has some meaning for like mutex debug.
As Coly pointed out in a previous review, bcache_exit() may not be able to handle all the references properly if userspace registers cache and backing devices right before bch_debug_init runs and bch_debug_init failes later. So not exposing userspace interface until everything is ready to avoid that issue.

Quoted 7 lines

Quoted 1 line
Signed-off-by Liang Chen <[email protected]>
Hi Liang,
No more comment from me, it looks good. Thanks.
Reviewed-by Coly Li <[email protected]>

Looks good to me too.

Reviewed-by Michael Lyle <[email protected]>

Eric Wheeler

Reviewed-by

2017-10-27 19:10 UTC #3 lore raw

Quoted 16 lines · Michael Lyle

Quoted 15 lines · Coly Li
Quoted 11 lines · Liang Chen
mutex_destroy does nothing most of time, but it's better to call it to make the code future proof and it also has some meaning for like mutex debug.
As Coly pointed out in a previous review, bcache_exit() may not be able to handle all the references properly if userspace registers cache and backing devices right before bch_debug_init runs and bch_debug_init failes later. So not exposing userspace interface until everything is ready to avoid that issue.
Signed-off-by Liang Chen <[email protected]>
Hi Liang,

Quoted 7 lines

Quoted 3 lines
No more comment from me, it looks good. Thanks.
Reviewed-by Coly Li <[email protected]>
Looks good to me too.
Reviewed-by Michael Lyle <[email protected]>

Should this Cc: stable to avoid the register race (possible crash?) described by Liang in other stable kernels?

Reviewed-by Eric Wheeler <[email protected]>

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michael Lyle

2017-10-27 19:15 UTC #4 lore raw

Quoted 4 lines · Eric Wheeler

Should this Cc: stable to avoid the register race (possible crash?) described by Liang in other stable kernels?
Reviewed-by Eric Wheeler <[email protected]>

This seems like an unlikely failure; basically you must have built bcache for debug (which not many of us are doing on previous stable series), and then be critically out of resources at the time that bcache is registering. Seems like more trouble than it's worth to send it to -stable.

Mike

Liang C

author

2017-10-30 12:33 UTC #5 lore raw

In reply to Michael Lyle

Hi Michael,
Would you please to include this patch in your tree for the next release? It seems passed the review. Thank you.

Thanks,
Liang

Quoted 24 lines · Michael Lyle

Quoted 11 lines · Coly Li
Quoted 10 lines · Liang Chen
mutex_destroy does nothing most of time, but it's better to call it to make the code future proof and it also has some meaning for like mutex debug.
As Coly pointed out in a previous review, bcache_exit() may not be able to handle all the references properly if userspace registers cache and backing devices right before bch_debug_init runs and bch_debug_init failes later. So not exposing userspace interface until everything is ready to avoid that issue.
Quoted 7 lines
Quoted 1 line
Signed-off-by Liang Chen <[email protected]>
Hi Liang,
No more comment from me, it looks good. Thanks.
Reviewed-by Coly Li <[email protected]>
Looks good to me too.
Reviewed-by Michael Lyle <[email protected]>

Michael Lyle

2017-10-30 18:02 UTC #6 lore raw

Hi Liang--

Quoted 6 lines · Liang C

Hi Michael,
Would you please to include this patch in your tree for the next release? It seems passed the review. Thank you.
Thanks,
Liang

Thanks for the reminder.

It's in my bcache-for-next tree at https://github.com/mlyle/linux/ along with a few other small changes. I am testing today and plan on sending them up to Jens soon if I have good luck overall (and after reading overall diff).

Though: it is getting late for non-critical changes in 4.15 so I need to have perfect test sequences with no unexplained anomalies to send these to Jens for 4.15.

Mike

Quoted 31 lines · Michael Lyle

Quoted 16 lines · Coly Li
Quoted 15 lines · Liang Chen
Quoted 11 lines
mutex_destroy does nothing most of time, but it's better to call it to make the code future proof and it also has some meaning for like mutex debug.
As Coly pointed out in a previous review, bcache_exit() may not be able to handle all the references properly if userspace registers cache and backing devices right before bch_debug_init runs and bch_debug_init failes later. So not exposing userspace interface until everything is ready to avoid that issue.
Signed-off-by Liang Chen <[email protected]>
Hi Liang,
Quoted 8 lines
Quoted 3 lines
No more comment from me, it looks good. Thanks.
Reviewed-by Coly Li <[email protected]>
Looks good to me too.
Reviewed-by Michael Lyle <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html