From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Stroetmann Subject: Re: [PATCH] fs: kill get_new_inode{,_fast} Date: Sun, 24 Oct 2010 02:07:18 +0200 Message-ID: <4CC378B6.7020207@ontolinux.com> References: <20101023174440.GA2805@lst.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: linux-fsdevel To: Christoph Hellwig Return-path: Received: from moutng.kundenserver.de ([212.227.126.171]:58611 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758519Ab0JXAIy (ORCPT ); Sat, 23 Oct 2010 20:08:54 -0400 In-Reply-To: <20101023174440.GA2805@lst.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 > > 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