[09/11] vfs: factor out inode_insert5()

Commit Message

Miklos Szeredi June 11, 2018, 11:32 a.m. UTC

On Mon, Jun 11, 2018 at 11:15:55AM +0200, Miklos Szeredi wrote:
> On Sun, Jun 10, 2018 at 8:02 AM, Al Viro <[e-mail]> wrote:
> > On Sun, Jun 10, 2018 at 06:49:10AM +0100, Al Viro wrote:
> >> On Tue, May 29, 2018 at 04:41:41PM +0200, Miklos Szeredi wrote:
> >> > From: Miklos Szeredi <[e-mail]>
> >> >
> >> > Split out common helper for race free insertion of an already allocated
> >> > inode into the cache.  Use this from iget5_locked() and
> >> > insert_inode_locked4().  Make iget5_locked() use new_inode()/iput() instead
> >> > of alloc_inode()/destroy_inode() directly.
> >>
> >> ... thus hitting the sucker with ->evict_inode(), in condition that is quite
> >> likely to be unfit to be seen by that.
> >>
> >> NAK.
> >
> > To clarify: objection here is against the switch to new_inode/iput.  The rest is
> > sane.  What makes new_inode() better here, anyway?
> 
> Umm, got to look into this.  The patch has already been pulled by
> Linus, but I hope it's salvageable.

Incremental follows.  I think it's cleaner to initialize i_state and i_sb_list
up front (hence the use of new_inode()), but could just as well add to sb list
afterwards.

---

Comments

Al Viro June 11, 2018, 4:43 p.m. UTC | #1

On Mon, Jun 11, 2018 at 01:32:30PM +0200, Miklos Szeredi wrote:

> Incremental follows.  I think it's cleaner to initialize i_state and i_sb_list
> up front (hence the use of new_inode()), but could just as well add to sb list
> afterwards.

> ---
> diff --git a/fs/inode.c b/fs/inode.c
> index 0df41bb77e0f..03c0d7c1296f 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1098,8 +1098,10 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval,
>  
>  		if (new) {
>  			inode = inode_insert5(new, hashval, test, set, data);
> -			if (unlikely(inode != new))
> -				iput(new);
> +			if (unlikely(inode != new)) {
> +				inode_sb_list_del(inode);
> +				destroy_inode(new);
> +			}

The thing is, until you put it into the list, it's invisible to everyone other
than iget5_locked() - no references in any shared data structures.  Which
outweighs the "it's somewhat irregular in not being on the list" considerably,
as far as the complexity of analysis goes, especially since there are inodes
that never get on that list and it's not something exotic - all sockets and
pipes are that way, for starters.  So IMO that should be dealt with in
inode_insert5().

Patch

diff --git a/fs/inode.c b/fs/inode.c
index 0df41bb77e0f..03c0d7c1296f 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1098,8 +1098,10 @@  struct inode *iget5_locked(struct super_block *sb, unsigned long hashval,
 
 		if (new) {
 			inode = inode_insert5(new, hashval, test, set, data);
-			if (unlikely(inode != new))
-				iput(new);
+			if (unlikely(inode != new)) {
+				inode_sb_list_del(inode);
+				destroy_inode(new);
+			}
 		}
 	}
 	return inode;