From: Al Viro <viro@ZenIV.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Bruce Fields <bfields@fieldses.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [git pull] fixes for 3.12-final
Date: Wed, 6 Nov 2013 15:10:04 +0000 [thread overview]
Message-ID: <20131106151003.GA21425@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20131104005300.GM13318@ZenIV.linux.org.uk>
On Mon, Nov 04, 2013 at 12:53:00AM +0000, Al Viro wrote:
> Maybe... OTOH, that crap really needs doing something only with nfsd on
> filesystems with 64bit inode numbers living on 32bit hosts (i_ino is
> unsigned long, not u32 right now). Hell knows; I'm somewhat concerned about
> setups like e.g. ext2 on VIA C7 mini-itx boxen (and yes, I do have such
> beasts). FWIW, the whole area around iget_locked() needs profiling;
> in particular, I really wonder if this
> 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;
> }
> makes any sense; both ->i_ino and ->i_sb are assign-once and assigned before
> the sucker gets inserted into hash, so inode_hash_lock provides all barriers
> we need here. Sure, we want to grab ->i_lock for this:
> if (inode->i_state & (I_FREEING|I_WILL_FREE)) {
> __wait_on_freeing_inode(inode);
> goto repeat;
> }
> __iget(inode);
> spin_unlock(&inode->i_lock);
> but that's once per find_inode{_fast,}(), not once per inode in hash chain
> being traversed...
>
> And picking them from dentries is fine, but every time we associate an inode
> with dentry, we end up walking the hash chain in icache and the time we
> spend in that loop can get sensitive - we are holding a system-wide lock,
> after all (and the way it's implemented right now, we end up touching
> a cacheline in a bunch of struct inode for no good reason).
FWIW, not taking ->i_lock there definitely looks like a good thing. As for
64bit ->i_ino itself... Looks like the main problem is the shitload of
printks - the actual uses of ->i_ino are fine, but these suckers create
a lot of noise. So for now I'm going with Bruce's variant; 64bit i_ino
doesn't look too bad (even on i386, actually), but it'll have to wait
until 3.14. Too noisy and late in this cycle...
next prev parent reply other threads:[~2013-11-06 15:10 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-03 1:58 [git pull] fixes for 3.12-final Al Viro
2013-11-03 18:25 ` Linus Torvalds
2013-11-03 19:54 ` Al Viro
2013-11-03 23:39 ` Linus Torvalds
2013-11-04 0:53 ` Al Viro
2013-11-06 15:10 ` Al Viro [this message]
2013-11-13 14:43 ` Bruce Fields
2013-11-13 15:16 ` Bruce Fields
2013-11-18 16:32 ` Greg KH
2013-12-18 19:40 ` Bruce Fields
2013-12-18 20:12 ` Greg KH
2013-11-04 22:30 ` Bruce Fields
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131106151003.GA21425@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=bfields@fieldses.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.