[PATCH] vfs: remove the unnecessrary code of fs/inode.c

[PATCH] vfs: remove the unnecessrary code of fs/inode.c

[Dong Fang]

These functions, such as find_inode_fast() and find_inode(), iget_lock() and
iget5_lock(), insert_inode_locked() and insert_inode_locked4(), almost have
the same code.

Signed-off-by: Dong Fang <yp.fangdong@gmail.com>
---
 fs/inode.c |  134 ++++++++++++------------------------------------------------
 1 files changed, 26 insertions(+), 108 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 00d5fc3..847eee9 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -790,6 +790,22 @@ void prune_icache_sb(struct super_block *sb, int nr_to_scan)
 }
 
 static void __wait_on_freeing_inode(struct inode *inode);
+
+
+static int test_ino(struct inode *inode, void *data)
+{
+	unsigned long ino = *(unsigned long *) data;
+	return inode->i_ino == ino;
+}
+
+static int set_ino(struct inode *inode, void *data)
+{
+	inode->i_ino = *(unsigned long *) data;
+	return 0;
+}
+
+
+
 /*
  * Called with the inode lock held.
  */
@@ -829,28 +845,7 @@ repeat:
 static struct inode *find_inode_fast(struct super_block *sb,
 				struct hlist_head *head, unsigned long ino)
 {
-	struct inode *inode = NULL;
-
-repeat:
-	hlist_for_each_entry(inode, head, i_hash) {
-		spin_lock(&inode->i_lock);
-		if (inode->i_ino != ino) {
-			spin_unlock(&inode->i_lock);
-			continue;
-		}
-		if (inode->i_sb != sb) {
-			spin_unlock(&inode->i_lock);
-			continue;
-		}
-		if (inode->i_state & (I_FREEING|I_WILL_FREE)) {
-			__wait_on_freeing_inode(inode);
-			goto repeat;
-		}
-		__iget(inode);
-		spin_unlock(&inode->i_lock);
-		return inode;
-	}
-	return NULL;
+	return find_inode(sb, head, test_ino, (void *)&ino);
 }
 
 /*
@@ -1073,50 +1068,7 @@ EXPORT_SYMBOL(iget5_locked);
  */
 struct inode *iget_locked(struct super_block *sb, unsigned long ino)
 {
-	struct hlist_head *head = inode_hashtable + hash(sb, ino);
-	struct inode *inode;
-
-	spin_lock(&inode_hash_lock);
-	inode = find_inode_fast(sb, head, ino);
-	spin_unlock(&inode_hash_lock);
-	if (inode) {
-		wait_on_inode(inode);
-		return inode;
-	}
-
-	inode = alloc_inode(sb);
-	if (inode) {
-		struct inode *old;
-
-		spin_lock(&inode_hash_lock);
-		/* We released the lock, so.. */
-		old = find_inode_fast(sb, head, ino);
-		if (!old) {
-			inode->i_ino = ino;
-			spin_lock(&inode->i_lock);
-			inode->i_state = I_NEW;
-			hlist_add_head(&inode->i_hash, head);
-			spin_unlock(&inode->i_lock);
-			inode_sb_list_add(inode);
-			spin_unlock(&inode_hash_lock);
-
-			/* Return the locked inode with I_NEW set, the
-			 * caller is responsible for filling in the contents
-			 */
-			return inode;
-		}
-
-		/*
-		 * Uhhuh, somebody else created the same inode under
-		 * us. Use the old inode instead of the one we just
-		 * allocated.
-		 */
-		spin_unlock(&inode_hash_lock);
-		destroy_inode(inode);
-		inode = old;
-		wait_on_inode(inode);
-	}
-	return inode;
+	return iget5_locked(sb, ino, test_ino, set_ino, (void *)&ino);
 }
 EXPORT_SYMBOL(iget_locked);
 
@@ -1281,48 +1233,6 @@ struct inode *ilookup(struct super_block *sb, unsigned long ino)
 }
 EXPORT_SYMBOL(ilookup);
 
-int insert_inode_locked(struct inode *inode)
-{
-	struct super_block *sb = inode->i_sb;
-	ino_t ino = inode->i_ino;
-	struct hlist_head *head = inode_hashtable + hash(sb, ino);
-
-	while (1) {
-		struct inode *old = NULL;
-		spin_lock(&inode_hash_lock);
-		hlist_for_each_entry(old, head, i_hash) {
-			if (old->i_ino != ino)
-				continue;
-			if (old->i_sb != sb)
-				continue;
-			spin_lock(&old->i_lock);
-			if (old->i_state & (I_FREEING|I_WILL_FREE)) {
-				spin_unlock(&old->i_lock);
-				continue;
-			}
-			break;
-		}
-		if (likely(!old)) {
-			spin_lock(&inode->i_lock);
-			inode->i_state |= I_NEW;
-			hlist_add_head(&inode->i_hash, head);
-			spin_unlock(&inode->i_lock);
-			spin_unlock(&inode_hash_lock);
-			return 0;
-		}
-		__iget(old);
-		spin_unlock(&old->i_lock);
-		spin_unlock(&inode_hash_lock);
-		wait_on_inode(old);
-		if (unlikely(!inode_unhashed(old))) {
-			iput(old);
-			return -EBUSY;
-		}
-		iput(old);
-	}
-}
-EXPORT_SYMBOL(insert_inode_locked);
-
 int insert_inode_locked4(struct inode *inode, unsigned long hashval,
 		int (*test)(struct inode *, void *), void *data)
 {
@@ -1366,6 +1276,14 @@ int insert_inode_locked4(struct inode *inode, unsigned long hashval,
 }
 EXPORT_SYMBOL(insert_inode_locked4);
 
+int insert_inode_locked(struct inode *inode)
+{
+	unsigned long ino = inode->i_ino;
+	return insert_inode_locked4(inode, ino, test_ino, (void *)&ino);
+}
+EXPORT_SYMBOL(insert_inode_locked);
+
+
 
 int generic_delete_inode(struct inode *inode)
 {
-- 
1.7.1

Re: [PATCH] vfs: remove the unnecessrary code of fs/inode.c

[Gu Zheng]

On 07/01/2013 08:19 PM, Dong Fang wrote:

> These functions, such as find_inode_fast() and find_inode(), iget_lock() and
> iget5_lock(), insert_inode_locked() and insert_inode_locked4(), almost have
> the same code.

Maybe the title "[PATCH] vfs: remove the reduplicate code of fs/inode.c" is more
suitable.

> 
> Signed-off-by: Dong Fang <yp.fangdong@gmail.com>


Reviewed-by: Gu Zheng <guz.fnst@cn.fujitsu.com>

Thanks,
Gu

> ---
>  fs/inode.c |  134 ++++++++++++------------------------------------------------
>  1 files changed, 26 insertions(+), 108 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 00d5fc3..847eee9 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -790,6 +790,22 @@ void prune_icache_sb(struct super_block *sb, int nr_to_scan)
>  }
>  
>  static void __wait_on_freeing_inode(struct inode *inode);
> +
> +
> +static int test_ino(struct inode *inode, void *data)
> +{
> +	unsigned long ino = *(unsigned long *) data;
> +	return inode->i_ino == ino;

Can be more concise:
	return inode->i_ino == *(unsigned long *) data;
,so does the new insert_inode_locked():
	

> +}
> +
> +static int set_ino(struct inode *inode, void *data)
> +{
> +	inode->i_ino = *(unsigned long *) data;
> +	return 0;
> +}
> +
> +
> +
>  /*
>   * Called with the inode lock held.
>   */
> @@ -829,28 +845,7 @@ repeat:
>  static struct inode *find_inode_fast(struct super_block *sb,
>  				struct hlist_head *head, unsigned long ino)
>  {
> -	struct inode *inode = NULL;
> -
> -repeat:
> -	hlist_for_each_entry(inode, head, i_hash) {
> -		spin_lock(&inode->i_lock);
> -		if (inode->i_ino != ino) {
> -			spin_unlock(&inode->i_lock);
> -			continue;
> -		}
> -		if (inode->i_sb != sb) {
> -			spin_unlock(&inode->i_lock);
> -			continue;
> -		}
> -		if (inode->i_state & (I_FREEING|I_WILL_FREE)) {
> -			__wait_on_freeing_inode(inode);
> -			goto repeat;
> -		}
> -		__iget(inode);
> -		spin_unlock(&inode->i_lock);
> -		return inode;
> -	}
> -	return NULL;
> +	return find_inode(sb, head, test_ino, (void *)&ino);
>  }
>  
>  /*
> @@ -1073,50 +1068,7 @@ EXPORT_SYMBOL(iget5_locked);
>   */
>  struct inode *iget_locked(struct super_block *sb, unsigned long ino)
>  {
> -	struct hlist_head *head = inode_hashtable + hash(sb, ino);
> -	struct inode *inode;
> -
> -	spin_lock(&inode_hash_lock);
> -	inode = find_inode_fast(sb, head, ino);
> -	spin_unlock(&inode_hash_lock);
> -	if (inode) {
> -		wait_on_inode(inode);
> -		return inode;
> -	}
> -
> -	inode = alloc_inode(sb);
> -	if (inode) {
> -		struct inode *old;
> -
> -		spin_lock(&inode_hash_lock);
> -		/* We released the lock, so.. */
> -		old = find_inode_fast(sb, head, ino);
> -		if (!old) {
> -			inode->i_ino = ino;
> -			spin_lock(&inode->i_lock);
> -			inode->i_state = I_NEW;
> -			hlist_add_head(&inode->i_hash, head);
> -			spin_unlock(&inode->i_lock);
> -			inode_sb_list_add(inode);
> -			spin_unlock(&inode_hash_lock);
> -
> -			/* Return the locked inode with I_NEW set, the
> -			 * caller is responsible for filling in the contents
> -			 */
> -			return inode;
> -		}
> -
> -		/*
> -		 * Uhhuh, somebody else created the same inode under
> -		 * us. Use the old inode instead of the one we just
> -		 * allocated.
> -		 */
> -		spin_unlock(&inode_hash_lock);
> -		destroy_inode(inode);
> -		inode = old;
> -		wait_on_inode(inode);
> -	}
> -	return inode;
> +	return iget5_locked(sb, ino, test_ino, set_ino, (void *)&ino);
>  }
>  EXPORT_SYMBOL(iget_locked);
>  
> @@ -1281,48 +1233,6 @@ struct inode *ilookup(struct super_block *sb, unsigned long ino)
>  }
>  EXPORT_SYMBOL(ilookup);
>  
> -int insert_inode_locked(struct inode *inode)
> -{
> -	struct super_block *sb = inode->i_sb;
> -	ino_t ino = inode->i_ino;
> -	struct hlist_head *head = inode_hashtable + hash(sb, ino);
> -
> -	while (1) {
> -		struct inode *old = NULL;
> -		spin_lock(&inode_hash_lock);
> -		hlist_for_each_entry(old, head, i_hash) {
> -			if (old->i_ino != ino)
> -				continue;
> -			if (old->i_sb != sb)
> -				continue;
> -			spin_lock(&old->i_lock);
> -			if (old->i_state & (I_FREEING|I_WILL_FREE)) {
> -				spin_unlock(&old->i_lock);
> -				continue;
> -			}
> -			break;
> -		}
> -		if (likely(!old)) {
> -			spin_lock(&inode->i_lock);
> -			inode->i_state |= I_NEW;
> -			hlist_add_head(&inode->i_hash, head);
> -			spin_unlock(&inode->i_lock);
> -			spin_unlock(&inode_hash_lock);
> -			return 0;
> -		}
> -		__iget(old);
> -		spin_unlock(&old->i_lock);
> -		spin_unlock(&inode_hash_lock);
> -		wait_on_inode(old);
> -		if (unlikely(!inode_unhashed(old))) {
> -			iput(old);
> -			return -EBUSY;
> -		}
> -		iput(old);
> -	}
> -}
> -EXPORT_SYMBOL(insert_inode_locked);
> -
>  int insert_inode_locked4(struct inode *inode, unsigned long hashval,
>  		int (*test)(struct inode *, void *), void *data)
>  {
> @@ -1366,6 +1276,14 @@ int insert_inode_locked4(struct inode *inode, unsigned long hashval,
>  }
>  EXPORT_SYMBOL(insert_inode_locked4);
>  
> +int insert_inode_locked(struct inode *inode)
> +{
> +	unsigned long ino = inode->i_ino;
> +	return insert_inode_locked4(inode, ino, test_ino, (void *)&ino);

return insert_inode_locked4(inode, inode->i_ino, test_ino, (void *)&ino);

> +}
> +EXPORT_SYMBOL(insert_inode_locked);
> +
> +
>  
>  int generic_delete_inode(struct inode *inode)
>  {

Re: [PATCH] vfs: remove the unnecessrary code of fs/inode.c

[Al Viro]

On Mon, Jul 01, 2013 at 08:19:03AM -0400, Dong Fang wrote:
> These functions, such as find_inode_fast() and find_inode(), iget_lock() and
> iget5_lock(), insert_inode_locked() and insert_inode_locked4(), almost have
> the same code.

NAK.  These functions exist exactly because the variant with callbacks
costs more.  We walk the hash chain and for each inode on it your
variant would result in
	* call
	* fetching ino from memory
	* comparison (and storing result in general-purpose register)
	* return
	* checking that register and branch on the result of that check
What's more, the whole thing's not fun for branch predictor.

It is a hot enough path to warrant a special-cased variant; if we can't
get away with that, we use the variants with callbacks, but on filesystems
where ->i_ino is sufficient as search key we really want to avoid the
overhead.

Re: [PATCH] vfs: remove the unnecessrary code of fs/inode.c

[Dong Fang]

On 07/02/2013 12:15 AM, Gu Zheng wrote:
> On 07/01/2013 08:19 PM, Dong Fang wrote:
>
>> These functions, such as find_inode_fast() and find_inode(), iget_lock() and
>> iget5_lock(), insert_inode_locked() and insert_inode_locked4(), almost have
>> the same code.
>
> Maybe the title "[PATCH] vfs: remove the reduplicate code of fs/inode.c" is more
> suitable.
>

that's right, thanks for your advice

>>
>> Signed-off-by: Dong Fang <yp.fangdong@gmail.com>
>
>
> Reviewed-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>
> Thanks,
> Gu
>
>> ---
>>   fs/inode.c |  134 ++++++++++++------------------------------------------------
>>   1 files changed, 26 insertions(+), 108 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 00d5fc3..847eee9 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -790,6 +790,22 @@ void prune_icache_sb(struct super_block *sb, int nr_to_scan)
>>   }
>>
>>   static void __wait_on_freeing_inode(struct inode *inode);
>> +
>> +
>> +static int test_ino(struct inode *inode, void *data)
>> +{
>> +	unsigned long ino = *(unsigned long *) data;
>> +	return inode->i_ino == ino;
>
> Can be more concise:
> 	return inode->i_ino == *(unsigned long *) data;
> ,so does the new insert_inode_locked():
> 	
>
>> +}
>> +
>> +static int set_ino(struct inode *inode, void *data)
>> +{
>> +	inode->i_ino = *(unsigned long *) data;
>> +	return 0;
>> +}
>> +
>> +
>> +
>>   /*
>>    * Called with the inode lock held.
>>    */
>> @@ -829,28 +845,7 @@ repeat:
>>   static struct inode *find_inode_fast(struct super_block *sb,
>>   				struct hlist_head *head, unsigned long ino)
>>   {
>> -	struct inode *inode = NULL;
>> -
>> -repeat:
>> -	hlist_for_each_entry(inode, head, i_hash) {
>> -		spin_lock(&inode->i_lock);
>> -		if (inode->i_ino != ino) {
>> -			spin_unlock(&inode->i_lock);
>> -			continue;
>> -		}
>> -		if (inode->i_sb != sb) {
>> -			spin_unlock(&inode->i_lock);
>> -			continue;
>> -		}
>> -		if (inode->i_state & (I_FREEING|I_WILL_FREE)) {
>> -			__wait_on_freeing_inode(inode);
>> -			goto repeat;
>> -		}
>> -		__iget(inode);
>> -		spin_unlock(&inode->i_lock);
>> -		return inode;
>> -	}
>> -	return NULL;
>> +	return find_inode(sb, head, test_ino, (void *)&ino);
>>   }
>>
>>   /*
>> @@ -1073,50 +1068,7 @@ EXPORT_SYMBOL(iget5_locked);
>>    */
>>   struct inode *iget_locked(struct super_block *sb, unsigned long ino)
>>   {
>> -	struct hlist_head *head = inode_hashtable + hash(sb, ino);
>> -	struct inode *inode;
>> -
>> -	spin_lock(&inode_hash_lock);
>> -	inode = find_inode_fast(sb, head, ino);
>> -	spin_unlock(&inode_hash_lock);
>> -	if (inode) {
>> -		wait_on_inode(inode);
>> -		return inode;
>> -	}
>> -
>> -	inode = alloc_inode(sb);
>> -	if (inode) {
>> -		struct inode *old;
>> -
>> -		spin_lock(&inode_hash_lock);
>> -		/* We released the lock, so.. */
>> -		old = find_inode_fast(sb, head, ino);
>> -		if (!old) {
>> -			inode->i_ino = ino;
>> -			spin_lock(&inode->i_lock);
>> -			inode->i_state = I_NEW;
>> -			hlist_add_head(&inode->i_hash, head);
>> -			spin_unlock(&inode->i_lock);
>> -			inode_sb_list_add(inode);
>> -			spin_unlock(&inode_hash_lock);
>> -
>> -			/* Return the locked inode with I_NEW set, the
>> -			 * caller is responsible for filling in the contents
>> -			 */
>> -			return inode;
>> -		}
>> -
>> -		/*
>> -		 * Uhhuh, somebody else created the same inode under
>> -		 * us. Use the old inode instead of the one we just
>> -		 * allocated.
>> -		 */
>> -		spin_unlock(&inode_hash_lock);
>> -		destroy_inode(inode);
>> -		inode = old;
>> -		wait_on_inode(inode);
>> -	}
>> -	return inode;
>> +	return iget5_locked(sb, ino, test_ino, set_ino, (void *)&ino);
>>   }
>>   EXPORT_SYMBOL(iget_locked);
>>
>> @@ -1281,48 +1233,6 @@ struct inode *ilookup(struct super_block *sb, unsigned long ino)
>>   }
>>   EXPORT_SYMBOL(ilookup);
>>
>> -int insert_inode_locked(struct inode *inode)
>> -{
>> -	struct super_block *sb = inode->i_sb;
>> -	ino_t ino = inode->i_ino;
>> -	struct hlist_head *head = inode_hashtable + hash(sb, ino);
>> -
>> -	while (1) {
>> -		struct inode *old = NULL;
>> -		spin_lock(&inode_hash_lock);
>> -		hlist_for_each_entry(old, head, i_hash) {
>> -			if (old->i_ino != ino)
>> -				continue;
>> -			if (old->i_sb != sb)
>> -				continue;
>> -			spin_lock(&old->i_lock);
>> -			if (old->i_state & (I_FREEING|I_WILL_FREE)) {
>> -				spin_unlock(&old->i_lock);
>> -				continue;
>> -			}
>> -			break;
>> -		}
>> -		if (likely(!old)) {
>> -			spin_lock(&inode->i_lock);
>> -			inode->i_state |= I_NEW;
>> -			hlist_add_head(&inode->i_hash, head);
>> -			spin_unlock(&inode->i_lock);
>> -			spin_unlock(&inode_hash_lock);
>> -			return 0;
>> -		}
>> -		__iget(old);
>> -		spin_unlock(&old->i_lock);
>> -		spin_unlock(&inode_hash_lock);
>> -		wait_on_inode(old);
>> -		if (unlikely(!inode_unhashed(old))) {
>> -			iput(old);
>> -			return -EBUSY;
>> -		}
>> -		iput(old);
>> -	}
>> -}
>> -EXPORT_SYMBOL(insert_inode_locked);
>> -
>>   int insert_inode_locked4(struct inode *inode, unsigned long hashval,
>>   		int (*test)(struct inode *, void *), void *data)
>>   {
>> @@ -1366,6 +1276,14 @@ int insert_inode_locked4(struct inode *inode, unsigned long hashval,
>>   }
>>   EXPORT_SYMBOL(insert_inode_locked4);
>>
>> +int insert_inode_locked(struct inode *inode)
>> +{
>> +	unsigned long ino = inode->i_ino;
>> +	return insert_inode_locked4(inode, ino, test_ino, (void *)&ino);
>
> return insert_inode_locked4(inode, inode->i_ino, test_ino, (void *)&ino);
>
>> +}
>> +EXPORT_SYMBOL(insert_inode_locked);
>> +
>> +
>>
>>   int generic_delete_inode(struct inode *inode)
>>   {
>
>
>

Re: [PATCH] vfs: remove the unnecessrary code of fs/inode.c

[Dong Fang]

On 07/02/2013 12:41 AM, Al Viro wrote:
> On Mon, Jul 01, 2013 at 08:19:03AM -0400, Dong Fang wrote:
>> These functions, such as find_inode_fast() and find_inode(), iget_lock() and
>> iget5_lock(), insert_inode_locked() and insert_inode_locked4(), almost have
>> the same code.
>
> NAK.  These functions exist exactly because the variant with callbacks
> costs more.  We walk the hash chain and for each inode on it your
> variant would result in
> 	* call
> 	* fetching ino from memory
> 	* comparison (and storing result in general-purpose register)
> 	* return
> 	* checking that register and branch on the result of that check
> What's more, the whole thing's not fun for branch predictor.
>
> It is a hot enough path to warrant a special-cased variant; if we can't
> get away with that, we use the variants with callbacks, but on filesystems
> where ->i_ino is sufficient as search key we really want to avoid the
> overhead.
>

that's right, i didn't think of it, but i think may be we can remove
the deduplicate codes of iget_lock() and iget5_lock() function, right?

if ok, i will send a new patch later. :)

thx Viro.

Re: [PATCH] vfs: remove the unnecessrary code of fs/inode.c

[Dong Fang]

On 07/02/2013 02:11 AM, Dong Fang wrote:
> On 07/02/2013 12:41 AM, Al Viro wrote:
>> On Mon, Jul 01, 2013 at 08:19:03AM -0400, Dong Fang wrote:
>>> These functions, such as find_inode_fast() and find_inode(),
>>> iget_lock() and
>>> iget5_lock(), insert_inode_locked() and insert_inode_locked4(),
>>> almost have
>>> the same code.
>>
>> NAK.  These functions exist exactly because the variant with callbacks
>> costs more.  We walk the hash chain and for each inode on it your
>> variant would result in
>>     * call
>>     * fetching ino from memory
>>     * comparison (and storing result in general-purpose register)
>>     * return
>>     * checking that register and branch on the result of that check
>> What's more, the whole thing's not fun for branch predictor.
>>
>> It is a hot enough path to warrant a special-cased variant; if we can't
>> get away with that, we use the variants with callbacks, but on
>> filesystems
>> where ->i_ino is sufficient as search key we really want to avoid the
>> overhead.
>>
>
> that's right, i didn't think of it, but i think may be we can remove
> the deduplicate codes of iget_lock() and iget5_lock() function, right?
>
> if ok, i will send a new patch later. :)
>
> thx Viro.

Viro, regard this :)