Skip to contentSkip to conversation

cr: fs/inode.c: make sure ckpt_obj_register() actually runs

PATCH In review Serge E. Hallyn 2 participants 4 messages Apr 13, 2010 3 messages not in the archive VFS

lore
Share & tools
lore

Serge E. Hallyn

author
#0 lore raw

PATCH 1/1 cr: fs/inode.c: make sure ckpt_obj_register() actually runs

[ This is against Matt's code-shuffled patchset ]

So put it in its own, explicitly-called init function.

Without this, on my s390x sles11 system CKPT_OBJ_INODE does not, in fact, get registered. With, it does, and cr_tests/bashckpt passes.

Signed-off-by Serge E. Hallyn <serue-r/[email protected]>
---
 fs/inode.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)
fs/inode.c +9 -4
index 1fcaf64..5121790 100644
@@ -1582,6 +1582,15 @@ static const struct ckpt_obj_ops ckpt_obj_inode_ops = {
 	.ref_drop = obj_inode_drop,
 	.ref_grab = obj_inode_grab,
 };
+
+static int inode_ckpt_init(void)
+{
+	int ret = ckpt_obj_register(&ckpt_obj_inode_ops);
+	printk(KERN_NOTICE "%s: ckpt_obj_register for inode returned %d\n",
+		__func__, ret);
+	return ret;
+}
+__initcall(inode_ckpt_init);
 #endif
 
 void __init inode_init(void)

@@ -1613,10 +1622,6 @@ void __init inode_init(void)
 
 	for (loop = 0; loop < (1 << i_hash_shift); loop++)
 		INIT_HLIST_HEAD(&inode_hashtable[loop]);
-
-#ifdef CONFIG_CHECKPOINT
-	ckpt_obj_register(&ckpt_obj_inode_ops);
-#endif
 }
 
 void init_special_inode(struct inode *inode, umode_t mode, dev_t rdev)
Signature · 1 line
1.7.0.4

A message that is not in the archive (sent off-list or not yet received): 20100413203535.GA9187-r/[email protected]

#2 lore raw
Quoted 3 lines · Serge E. Hallyn

[ This is against Matt's code-shuffled patchset ]

So put it in its own, explicitly-called init function.

I did not spot the early return from inode_init(). So an alternate fix is to move the #ifdef CONFIG_CHECKPOINT block up before this early return:

        /* Hash may have been set up in inode_init_early */
        if (!hashdist)
                return;

hashdist is set to HASHDIST_DEFAULT which is probably different on s390 from x86-32/64. At least that would explain why this wasn't spotted in my earlier testing.

Quoted 31 lines

Without this, on my s390x sles11 system CKPT_OBJ_INODE does not, in fact, get registered. With, it does, and cr_tests/bashckpt passes.

Signed-off-by Serge E. Hallyn <serue-r/[email protected]>
---
 fs/inode.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)
fs/inode.c +9 -0
index 1fcaf64..5121790 100644
@@ -1582,6 +1582,15 @@ static const struct ckpt_obj_ops ckpt_obj_inode_ops = {
 	.ref_drop = obj_inode_drop,
 	.ref_grab = obj_inode_grab,
 };
+
+static int inode_ckpt_init(void)
+{
+	int ret = ckpt_obj_register(&ckpt_obj_inode_ops);
+	printk(KERN_NOTICE "%s: ckpt_obj_register for inode returned %d\n",
+		__func__, ret);
+	return ret;
+}
+__initcall(inode_ckpt_init);
 #endif
 
 void __init inode_init(void)

fs/inode.c void __init inode_init(void) · line 1618

Quoted 6 lines
fs/inode.c +0 -3
@@ -1613,10 +1622,6 @@ void __init inode_init(void)
 
 	for (loop = 0; loop < (1 << i_hash_shift); loop++)
 		INIT_HLIST_HEAD(&inode_hashtable[loop]);
-
-#ifdef CONFIG_CHECKPOINT
-	ckpt_obj_register(&ckpt_obj_inode_ops);

The latest set does:

s/ckpt_obj_register/register_checkpoint_obj/

Cheers,
-Matt Helsley

A message that is not in the archive (sent off-list or not yet received): 20100413213217.GF25363-52DBMbEzqgQ/wnmkkaCWp/[email protected]

author
#4 lore raw
Quoted 9 lines · Matt Helsley
Quoted 3 lines · Serge E. Hallyn

[ This is against Matt's code-shuffled patchset ]

So put it in its own, explicitly-called init function.

I did not spot the early return from inode_init(). So an alternate fix is to move the #ifdef CONFIG_CHECKPOINT block up before this early return:

Quoted 7 lines
        /* Hash may have been set up in inode_init_early */
        if (!hashdist)
                return;

hashdist is set to HASHDIST_DEFAULT which is probably different on s390 from x86-32/64. At least that would explain why this wasn't spotted in my earlier testing.

Right, my x86-64 tests all passed last night too...

Anyway I still prefer it be in its own init function, but whatever works. You have it moved up now in your set? When oren posts a new tree I'll be sure to test that.

thanks,
-serge

A message that is not in the archive (sent off-list or not yet received): 20100413220931.GB13309-r/[email protected]

#6 lore raw
Quoted 14 lines · Serge E. Hallyn
Quoted 13 lines · Matt Helsley
Quoted 3 lines · Serge E. Hallyn

[ This is against Matt's code-shuffled patchset ]

So put it in its own, explicitly-called init function.

I did not spot the early return from inode_init(). So an alternate fix is to move the #ifdef CONFIG_CHECKPOINT block up before this early return:

        /* Hash may have been set up in inode_init_early */
        if (!hashdist)
                return;
Quoted 7 lines
Quoted 3 lines

hashdist is set to HASHDIST_DEFAULT which is probably different on s390 from x86-32/64. At least that would explain why this wasn't spotted in my earlier testing.

Right, my x86-64 tests all passed last night too...

Anyway I still prefer it be in its own init function, but whatever

Yeah, I think there are a few advantages your approach:

1) Stays inside one #ifdef CONFIG_CHECKPOINT nicely
2) Happens at device init callback time -- we don't need to plug into early boot
3) Is obviously not dependent on the inode cache initialization code this way
...

Quoted 2 lines

works. You have it moved up now in your set? When oren posts a new tree I'll be sure to test that.

I'll fold your patch into my set and Oren can let me know what he wants to do.

Cheers,
-Matt Helsley