[PATCH linux-cr] cr: fs/inode.c: make sure ckpt_obj_register() actually runs

[PATCH linux-cr] cr: fs/inode.c: make sure ckpt_obj_register() actually runs

[Serge E. Hallyn]

[ 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/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/inode.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 1fcaf64..5121790 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -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)
-- 
1.7.0.4

Re: [PATCH linux-cr] cr: fs/inode.c: make sure ckpt_obj_register() actually runs

[Matt Helsley]

On Tue, Apr 13, 2010 at 03:35:35PM -0500, Serge E. Hallyn wrote:
> [ 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.

> 
> 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/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/inode.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 1fcaf64..5121790 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -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);

The latest set does:

s/ckpt_obj_register/register_checkpoint_obj/

Cheers,
	-Matt Helsley

Re: [PATCH linux-cr] cr: fs/inode.c: make sure ckpt_obj_register() actually runs

[Serge E. Hallyn]

Quoting Matt Helsley (matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> On Tue, Apr 13, 2010 at 03:35:35PM -0500, Serge E. Hallyn wrote:
> > [ 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.

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

Re: [PATCH linux-cr] cr: fs/inode.c: make sure ckpt_obj_register() actually runs

[Matt Helsley]

On Tue, Apr 13, 2010 at 05:09:31PM -0500, Serge E. Hallyn wrote:
> Quoting Matt Helsley (matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> > On Tue, Apr 13, 2010 at 03:35:35PM -0500, Serge E. Hallyn wrote:
> > > [ 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.
> 
> 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
...

> 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