Skip to contentSkip to conversation

fix up thinko in jbd2 revoke code

PATCH In review James Bottomley 4 participants 4 messages Mar 31, 2008 – Apr 21, 2008

Subsystems (2)
VFS Journalling layer for block devices (JBD2)
lore
Share & tools
lore

James Bottomley

author
#0 lore raw

PATCH 1/1 fix up thinko in jbd2 revoke code

This commit:

commit e898f3154d78d1520265aade0992981866d827fb

Author: Duane Griffin <[email protected]>
Date:   Tue Mar 25 19:47:39 2008 -0400

jbd2: tidy up revoke cache initialisation and destruction

Introduced a thinko (it doesn't check whether jbd2_revoke_record cache is null, it checks jbd2_revoke_table_cache twice).

Signed-off-by James Bottomley <[email protected]>

---

fs/jbd2/revoke.c +1 -1
index e13584a..4dc5b85 100644
@@ -192,7 +192,7 @@ int __init jbd2_journal_init_revoke_caches(void)
 	jbd2_revoke_table_cache = kmem_cache_create("jbd2_revoke_table",
 					   sizeof(struct jbd2_revoke_table_s),
 					   0, SLAB_TEMPORARY, NULL);
-	if (jbd2_revoke_table_cache && jbd2_revoke_table_cache) {
+	if (jbd2_revoke_record_cache && jbd2_revoke_table_cache) {
 		return 0;
 	} else {
 		jbd2_journal_destroy_revoke_caches();
#1 lore raw
Quoted 6 lines · James Bottomley

This commit:

commit e898f3154d78d1520265aade0992981866d827fb

Author: Duane Griffin <[email protected]>
Date:   Tue Mar 25 19:47:39 2008 -0400
Quoted 7 lines

jbd2: tidy up revoke cache initialisation and destruction

Introduced a thinko (it doesn't check whether jbd2_revoke_record cache is null, it checks jbd2_revoke_table_cache twice).

Signed-off-by James Bottomley <[email protected]>
Signed-off-by Mingming Cao <[email protected]>

Thanks, I picked this up in ext4 patch queue.

Mingming

Quoted 21 lines

---

fs/jbd2/revoke.c +1 -2
index e13584a..4dc5b85 100644
@@ -192,7 +192,7 @@ int __init jbd2_journal_init_revoke_caches(void)
 	jbd2_revoke_table_cache = kmem_cache_create("jbd2_revoke_table",
 					   sizeof(struct jbd2_revoke_table_s),
 					   0, SLAB_TEMPORARY, NULL);
-	if (jbd2_revoke_table_cache && jbd2_revoke_table_cache) {
+	if (jbd2_revoke_record_cache && jbd2_revoke_table_cache) {
 		return 0;
 	} else {
 		jbd2_journal_destroy_revoke_caches();
 
 
--

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

#2 lore raw
Quoted 9 lines · Mingming Cao
Quoted 8 lines · James Bottomley

This commit:

commit e898f3154d78d1520265aade0992981866d827fb

Author: Duane Griffin <[email protected]>
Date:   Tue Mar 25 19:47:39 2008 -0400

jbd2: tidy up revoke cache initialisation and destruction

Quoted 8 lines
Quoted 5 lines

Introduced a thinko (it doesn't check whether jbd2_revoke_record cache is null, it checks jbd2_revoke_table_cache twice).

Signed-off-by James Bottomley <[email protected]>
Signed-off-by Mingming Cao <[email protected]>

Thanks, I picked this up in ext4 patch queue.

anyone care to convert this function to proper goto-based error unwinding?

#3 lore raw
Quoted 13 lines · Christoph Hellwig
Quoted 12 lines · Mingming Cao
Quoted 11 lines · James Bottomley

This commit:

commit e898f3154d78d1520265aade0992981866d827fb

Author: Duane Griffin <[email protected]>
Date:   Tue Mar 25 19:47:39 2008 -0400

jbd2: tidy up revoke cache initialisation and destruction

Introduced a thinko (it doesn't check whether jbd2_revoke_record cache is null, it checks jbd2_revoke_table_cache twice).

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Signed-off-by James Bottomley <[email protected]>
Signed-off-by Mingming Cao <[email protected]>

Thanks, I picked this up in ext4 patch queue.

anyone care to convert this function to proper goto-based error unwinding?

Like below. ?

Mingming,

I folded jbd2-tidy-up-revoe-cache-intialisation-and-destruction.patch jbd2-fix-up-thinko-in-jbd2-revoke-code.patch in the patch queue to this.

-aneesh

jbd2: tidy up revoke cache initialisation and destruction

From: Aneesh Kumar K.V <[email protected]>

Make revocation cache destruction safe to call if initialisation fails partially or entirely. This allows it to be used to cleanup in the case of initialisation failure, simplifying the code slightly.

Signed-off-by Duane Griffin <[email protected]>
Signed-off-by Mingming Cao <[email protected]>
Signed-off-by "Theodore Ts'o" <[email protected]>

---

 fs/jbd2/revoke.c |   36 ++++++++++++++++++++++--------------
 1 files changed, 22 insertions(+), 14 deletions(-)
fs/jbd2/revoke.c +18 -7
index 2e1453a..72b2608 100644
@@ -167,33 +167,41 @@ static struct jbd2_revoke_record_s *find_revoke_record(journal_t *journal,
 	return NULL;
 }
 
+void jbd2_journal_destroy_revoke_caches(void)
+{
+	if (jbd2_revoke_record_cache) {
+		kmem_cache_destroy(jbd2_revoke_record_cache);
+		jbd2_revoke_record_cache = NULL;
+	}
+	if (jbd2_revoke_table_cache) {
+		kmem_cache_destroy(jbd2_revoke_table_cache);
+		jbd2_revoke_table_cache = NULL;
+	}
+}
+
 int __init jbd2_journal_init_revoke_caches(void)
 {
+	J_ASSERT(!jbd2_revoke_record_cache);
+	J_ASSERT(!jbd2_revoke_table_cache);
+
 	jbd2_revoke_record_cache = kmem_cache_create("jbd2_revoke_record",
 					   sizeof(struct jbd2_revoke_record_s),
 					   0,
 					   SLAB_HWCACHE_ALIGN|SLAB_TEMPORARY,
 					   NULL);
 	if (!jbd2_revoke_record_cache)
-		return -ENOMEM;
+		goto record_cache_failure;
 
 	jbd2_revoke_table_cache = kmem_cache_create("jbd2_revoke_table",
 					   sizeof(struct jbd2_revoke_table_s),
 					   0, SLAB_TEMPORARY, NULL);
-	if (!jbd2_revoke_table_cache) {
-		kmem_cache_destroy(jbd2_revoke_record_cache);
-		jbd2_revoke_record_cache = NULL;
-		return -ENOMEM;
-	}
+	if (!jbd2_revoke_table_cache)
+		goto table_cache_failure;
 	return 0;
-}