Re: [PATCH] fs: kill get_new_inode{,_fast}

[Christian Stroetmann <stroetmann@ontolinux.com>]

  Hola;

Maybe typos

On 23.10.2010 19:44, Christoph Hellwig wrote:
> There's very little point in these helpers.  One caller each
> doing a tailcall with trivial amounts of code before it.
>
> Merging into callers also makes use of the old vs inode variables
> more obvious.
>
> Also fix up kerneldoc comments to focus on what we are doing and
> not how it's done.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
>
> Index: linux-2.6/fs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/inode.c	2010-10-23 19:05:01.114003338 +0200
> +++ linux-2.6/fs/inode.c	2010-10-23 19:40:28.740005644 +0200
> @@ -819,102 +819,6 @@ void unlock_new_inode(struct inode *inod
>   EXPORT_SYMBOL(unlock_new_inode);
>
>   /*
> - * This is called without the inode lock held.. Be careful.
> - *
> - * We no longer cache the sb_flags in i_flags - see fs.h
> - *	-- rmk@arm.uk.linux.org
> - */
> -static struct inode *get_new_inode(struct super_block *sb,
> -				struct hlist_head *head,
> -				int (*test)(struct inode *, void *),
> -				int (*set)(struct inode *, void *),
> -				void *data)
> -{
> -	struct inode *inode;
> -
> -	inode = alloc_inode(sb);
> -	if (inode) {
> -		struct inode *old;
> -
> -		spin_lock(&inode_lock);
> -		/* We released the lock, so.. */
> -		old = find_inode(sb, head, test, data);
> -		if (!old) {
> -			if (set(inode, data))
> -				goto set_failed;
> -
> -			hlist_add_head(&inode->i_hash, head);
> -			__inode_sb_list_add(inode);
> -			inode->i_state = I_NEW;
> -			spin_unlock(&inode_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_lock);
> -		destroy_inode(inode);
> -		inode = old;
> -		wait_on_inode(inode);
> -	}
> -	return inode;
> -
> -set_failed:
> -	spin_unlock(&inode_lock);
> -	destroy_inode(inode);
> -	return NULL;
> -}
> -
> -/*
> - * get_new_inode_fast is the fast path version of get_new_inode, see the
> - * comment at iget_locked for details.
> - */
> -static struct inode *get_new_inode_fast(struct super_block *sb,
> -				struct hlist_head *head, unsigned long ino)
> -{
> -	struct inode *inode;
> -
> -	inode = alloc_inode(sb);
> -	if (inode) {
> -		struct inode *old;
> -
> -		spin_lock(&inode_lock);
> -		/* We released the lock, so.. */
> -		old = find_inode_fast(sb, head, ino);
> -		if (!old) {
> -			inode->i_ino = ino;
> -			hlist_add_head(&inode->i_hash, head);
> -			__inode_sb_list_add(inode);
> -			inode->i_state = I_NEW;
> -			spin_unlock(&inode_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_lock);
> -		destroy_inode(inode);
> -		inode = old;
> -		wait_on_inode(inode);
> -	}
> -	return inode;
> -}
> -
> -/*
>    * search the inode cache for a matching inode number.
>    * If we find one, then the inode number we are trying to
>    * allocate is not unique and so we should not use it.
> @@ -1147,15 +1051,14 @@ EXPORT_SYMBOL(ilookup);
>    * @set:	callback used to initialize a new struct inode
>    * @data:	opaque data pointer to pass to @test and @set
>    *
> - * iget5_locked() uses ifind() to search for the inode specified by @hashval
> - * and @data in the inode cache and if present it is returned with an increased
> - * reference count. This is a generalized version of iget_locked() for file
> - * systems where the inode number is not sufficient for unique identification
> - * of an inode.
> + * Search for the inode specified by @hashval and @data in the inode cache,
> + * and if present return it with an increased reference count.  If the inode
> + * is not in cache, allocate a new inode and returned it hashed and with the
> + * I_NEW flag set.  The file system gets to fill the inode in before unlocking
> + * it via unlock_new_inode().
Maybe:

+ * Search for the inode specified by @hashval and @data in the inode cache,
+ * and if present, then return it with an increased reference count.  If the inode
+ * is not in cache, then allocate a new inode, and return it hashed and with the


>    *
> - * If the inode is not in cache, get_new_inode() is called to allocate a new
> - * inode and this is returned locked, hashed, and with the I_NEW flag set. The
> - * file system gets to fill it in before unlocking it via unlock_new_inode().
> + * This is a generalized version of iget_locked() for file systems where the
> + * inode number is not sufficient for unique identification of an inode.
>    *
>    * Note both @test and @set are called with the inode_lock held, so can't sleep.
>    */
> @@ -1164,16 +1067,48 @@ struct inode *iget5_locked(struct super_
>   		int (*set)(struct inode *, void *), void *data)
>   {
>   	struct hlist_head *head = inode_hashtable + hash(sb, hashval);
> -	struct inode *inode;
> +	struct inode *inode, *old;
> +
> +	old = ifind(sb, head, test, data, 1);
> +	if (old)
> +		return old;
> +
> +	inode = alloc_inode(sb);
> +	if (!inode)
> +		return NULL;
> +
> +	spin_lock(&inode_lock);
> +	/* We released the lock, so.. */
> +	old = find_inode(sb, head, test, data);
> +	if (old) {
> +		/*
> +		 * Uhhuh, somebody else created the same inode under
Uhhuh is indeed funny, but ...
> +		 * us. Use the old inode instead of the one we just
> +		 * allocated.
> +		 */
> +		spin_unlock(&inode_lock);
> +		destroy_inode(inode);
> +		wait_on_inode(old);
> +		return old;
> +	}
> +
> +	if (set(inode, data))
> +		goto set_failed;
> +
> +	hlist_add_head(&inode->i_hash, head);
> +	__inode_sb_list_add(inode);
> +	inode->i_state = I_NEW;
> +	spin_unlock(&inode_lock);
>
> -	inode = ifind(sb, head, test, data, 1);
> -	if (inode)
> -		return inode;
>   	/*
> -	 * get_new_inode() will do the right thing, re-trying the search
> -	 * in case it had to block at any point.
> +	 * Return the locked inode with I_NEW set, the caller is responsible for
> +	 * filling in the contents
missing end point?!
>   	*/
> -	return get_new_inode(sb, head, test, set, data);
> +	return inode;
> +set_failed:
> +	spin_unlock(&inode_lock);
> +	destroy_inode(inode);
> +	return NULL;
>   }
>   EXPORT_SYMBOL(iget5_locked);
>
> @@ -1182,29 +1117,51 @@ EXPORT_SYMBOL(iget5_locked);
>    * @sb:		super block of file system
>    * @ino:	inode number to get
>    *
> - * iget_locked() uses ifind_fast() to search for the inode specified by @ino in
> - * the inode cache and if present it is returned with an increased reference
> - * count. This is for file systems where the inode number is sufficient for
> - * unique identification of an inode.
> - *
> - * If the inode is not in cache, get_new_inode_fast() is called to allocate a
> - * new inode and this is returned locked, hashed, and with the I_NEW flag set.
> - * The file system gets to fill it in before unlocking it via
> + * Search for the inode specified by @ino in the inode cache, and if present
> + * return it with an increased reference count.  If the inode is not in cache,
> + * allocate a new inode and returned it hashed and with the I_NEW flag set.
maybe as above:

+ * Search for the inode specified by @ino in the inode cache, and if present,
+ * then return it with an increased reference count.  If the inode is not in cache,
+ * then allocate a new inode, and return it hashed and with the I_NEW flag set.

> + * The file system gets to fill the inode in before unlocking it via
>    * unlock_new_inode().
>    */
>   struct inode *iget_locked(struct super_block *sb, unsigned long ino)
>   {
>   	struct hlist_head *head = inode_hashtable + hash(sb, ino);
> -	struct inode *inode;
> +	struct inode *inode, *old;
> +
> +	old = ifind_fast(sb, head, ino);
> +	if (old)
> +		return old;
> +
> +	inode = alloc_inode(sb);
> +	if (!inode)
> +		return NULL;
> +
> +	spin_lock(&inode_lock);
> +	/* We released the lock, so.. */
> +	old = find_inode_fast(sb, head, ino);
> +	if (old) {
> +		/*
> +		 * Uhhuh, somebody else created the same inode under
Ohhoh, Uhhuh :-D
> +		 * us. Use the old inode instead of the one we just
> +		 * allocated.
> +		 */
> +		spin_unlock(&inode_lock);
> +		destroy_inode(inode);
> +		wait_on_inode(old);
> +		return old;
> +	}
> +
> +	inode->i_ino = ino;
> +	hlist_add_head(&inode->i_hash, head);
> +	__inode_sb_list_add(inode);
> +	inode->i_state = I_NEW;
> +	spin_unlock(&inode_lock);
>
> -	inode = ifind_fast(sb, head, ino);
> -	if (inode)
> -		return inode;
>   	/*
> -	 * get_new_inode_fast() will do the right thing, re-trying the search
> -	 * in case it had to block at any point.
> +	 * Return the locked inode with I_NEW set, the caller is responsible for
> +	 * filling in the contents
again missing end point?!
>   	*/
> -	return get_new_inode_fast(sb, head, ino);
> +	return inode;
>   }
>   EXPORT_SYMBOL(iget_locked);
>
> Index: linux-2.6/fs/ocfs2/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/ocfs2/inode.c	2010-10-23 19:10:36.043010811 +0200
> +++ linux-2.6/fs/ocfs2/inode.c	2010-10-23 19:10:48.961026523 +0200
> @@ -183,7 +183,7 @@ bail:
>    * here's how inodes get read from disk:
>    * iget5_locked ->  find_actor ->  OCFS2_FIND_ACTOR
>    * found? : return the in-memory inode
> - * not found? : get_new_inode ->  OCFS2_INIT_LOCKED_INODE
> + * not found? : iget5_locked ->  OCFS2_INIT_LOCKED_INODE
>    */
>
>   static int ocfs2_find_actor(struct inode *inode, void *opaque)
> Index: linux-2.6/fs/udf/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/udf/inode.c	2010-10-23 19:10:52.729253443 +0200
> +++ linux-2.6/fs/udf/inode.c	2010-10-23 19:11:12.188005500 +0200
> @@ -1065,9 +1065,9 @@ static void __udf_read_inode(struct inod
>
>   	/*
>   	 * Set defaults, but the inode is still incomplete!
> -	 * Note: get_new_inode() sets the following on a new inode:
> +	 * Note: iget_locked() sets the following on a new inode:
>   	 *      i_sb = sb
> -	 *      i_no = ino
> +	 *      i_ino = ino
>   	 *      i_flags = sb->s_flags
>   	 *      i_state = 0
>   	 * clean_inode(): zero fills and sets

Have fun
Christian Stroetmann