From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Layton Subject: Re: [RFC][PATCH] ensure i_ino uniqueness in filesystems without permanent inode numbers (via idr) Date: Thu, 16 Nov 2006 09:34:21 -0500 Message-ID: <1163687661.4468.42.camel@dantu.rdu.redhat.com> References: <1163535728.15846.21.camel@dantu.rdu.redhat.com> <20061114202625.GY29920@ftp.linux.org.uk> <1163608958.7571.8.camel@tleilax.poochiereds.net> <20061116140621.GG29920@ftp.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org Return-path: Received: from mx1.redhat.com ([66.187.233.31]:60649 "EHLO mx1.redhat.com") by vger.kernel.org with ESMTP id S933513AbWKPOe2 (ORCPT ); Thu, 16 Nov 2006 09:34:28 -0500 To: Al Viro In-Reply-To: <20061116140621.GG29920@ftp.linux.org.uk> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Thu, 2006-11-16 at 14:06 +0000, Al Viro wrote: > On Wed, Nov 15, 2006 at 11:42:38AM -0500, Jeff Layton wrote: > > +{ > > + int rv; > > + > > + rv = idr_pre_get(&inode->i_sb->s_inode_ids, GFP_KERNEL); > > + if (! rv) > > + return -ENOMEM; > > + > > + lock_super(inode->i_sb); > > ?!#!@#!??? > > Please, use something saner. Use of lock_super() for anything generic > is wrong; using it for something that'd better be fast is even dumber... > Well, I considered the inode_lock here, but since all of this stuff is per-sb, I thought s_lock would be a better choice. If that's not suitable, what do you suggest? A new spinlock to protect the new sb fields? > > @@ -1025,6 +1055,7 @@ void generic_delete_inode(struct inode * > > spin_lock(&inode_lock); > > hlist_del_init(&inode->i_hash); > > spin_unlock(&inode_lock); > > + iunique_unregister(inode); > > Unconditional? Hitting every fs out there? With that kind of locking? > I'm not sure what condition I would base this on. That said, I don't think the impact would be too bad here though. Presumably, those filesystems that don't use iunique_register will have empty idr hashes and would return quickly. > > > if (inode->i_data.nrpages) > > truncate_inode_pages(&inode->i_data, 0); > > clear_inode(inode); > > diff --git a/fs/pipe.c b/fs/pipe.c > > index b1626f2..d74ae65 100644 > > --- a/fs/pipe.c > > +++ b/fs/pipe.c > > @@ -845,6 +845,9 @@ static struct inode * get_pipe_inode(voi > > if (!inode) > > goto fail_inode; > > > > + if (iunique_register(inode, 0)) > > + goto fail_iput; > > + > > Humm... I wonder what the overhead of that is going to be. There > easily can be shitloads of pipes on the box, with all sorts of > lifetimes. And we'd better be fast on that codepath... IDR is supposedly quick for this sort of thing though I don't have any numbers as of yet. Still, getting i_ino uniqueness isn't going to come for free. There will be some performance impact regardless of what scheme we use. -- Jeff