[PATCH] hugetlbfs: simplify destroying inode

[PATCH] hugetlbfs: simplify destroying inode

[Hillf Danton]

Just before reclaimed by the hugetlbfs inode cache, little difference
could be made by initializing the list head of the dentry member of
vfs inode, thus the operation of initialization could be removed to
simplify the destroy of hugetlbfs inode.

Signed-off-by: Hillf Danton <gmail.com>
---

--- a/fs/hugetlbfs/inode.c	2011-03-30 03:09:48.000000000 +0800
+++ b/fs/hugetlbfs/inode.c	2011-04-04 11:27:30.000000000 +0800
@@ -665,7 +665,6 @@ static struct inode *hugetlbfs_alloc_ino
 static void hugetlbfs_i_callback(struct rcu_head *head)
 {
 	struct inode *inode = container_of(head, struct inode, i_rcu);
-	INIT_LIST_HEAD(&inode->i_dentry);
 	kmem_cache_free(hugetlbfs_inode_cachep, HUGETLBFS_I(inode));
 }

Re: [PATCH] hugetlbfs: simplify destroying inode

[Andrew Morton]

On Mon, 4 Apr 2011 11:36:56 +0800
Hillf Danton <dhillf@gmail.com> wrote:

> Just before reclaimed by the hugetlbfs inode cache, little difference
> could be made by initializing the list head of the dentry member of
> vfs inode, thus the operation of initialization could be removed to
> simplify the destroy of hugetlbfs inode.
> 
> Signed-off-by: Hillf Danton <gmail.com>
> ---
> 
> --- a/fs/hugetlbfs/inode.c	2011-03-30 03:09:48.000000000 +0800
> +++ b/fs/hugetlbfs/inode.c	2011-04-04 11:27:30.000000000 +0800
> @@ -665,7 +665,6 @@ static struct inode *hugetlbfs_alloc_ino
>  static void hugetlbfs_i_callback(struct rcu_head *head)
>  {
>  	struct inode *inode = container_of(head, struct inode, i_rcu);
> -	INIT_LIST_HEAD(&inode->i_dentry);
>  	kmem_cache_free(hugetlbfs_inode_cachep, HUGETLBFS_I(inode));
>  }

How well was this tested?

hugetlbfs_inode_cachep has a slab constructor,
init_once()->inode_init_once().  The slab protocol requires that
objects be returned to the cache in the same state as that which the
constructor creates.  (It's a bit weird and it would probably be faster
and certainly saner if the ctor were run by kmem_cache_alloc()).

inode_init_once() does INIT_LIST_HEAD(&inode->i_dentry), so objects
should be returned to the cache in that state.  If no other code was
already reliably reinitialising ->i_dentry, the patch will add a bug.

Re: [PATCH] hugetlbfs: simplify destroying inode

[Hillf Danton]

On Thu, Apr 21, 2011 at 12:38 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 4 Apr 2011 11:36:56 +0800
> Hillf Danton <dhillf@gmail.com> wrote:
>
>> Just before reclaimed by the hugetlbfs inode cache, little difference
>> could be made by initializing the list head of the dentry member of
>> vfs inode, thus the operation of initialization could be removed to
>> simplify the destroy of hugetlbfs inode.
>>
>> Signed-off-by: Hillf Danton <gmail.com>
>> ---
>>
>> --- a/fs/hugetlbfs/inode.c    2011-03-30 03:09:48.000000000 +0800
>> +++ b/fs/hugetlbfs/inode.c    2011-04-04 11:27:30.000000000 +0800
>> @@ -665,7 +665,6 @@ static struct inode *hugetlbfs_alloc_ino
>>  static void hugetlbfs_i_callback(struct rcu_head *head)
>>  {
>>       struct inode *inode = container_of(head, struct inode, i_rcu);
>> -     INIT_LIST_HEAD(&inode->i_dentry);
>>       kmem_cache_free(hugetlbfs_inode_cachep, HUGETLBFS_I(inode));
>>  }
>
> How well was this tested?
>
> hugetlbfs_inode_cachep has a slab constructor,
> init_once()->inode_init_once().  The slab protocol requires that
> objects be returned to the cache in the same state as that which the
> constructor creates.  (It's a bit weird and it would probably be faster
> and certainly saner if the ctor were run by kmem_cache_alloc()).
>
> inode_init_once() does INIT_LIST_HEAD(&inode->i_dentry), so objects

Hi Andrew,

Thank you for good explanation about the ctor of inode, first.

Just because the ctor is responsible for initializing newly requested inode,
the same work could be skipped when the inode is then reclaimed.

The overwork could also be observed in other cases of file system,
then I have to check with more care it is really necessary.

thanks

          Hillf

> should be returned to the cache in that state.  If no other code was
> already reliably reinitialising ->i_dentry, the patch will add a bug.