[git pull] fixes for 3.12-final

[git pull] fixes for 3.12-final

[Al Viro]

	Exportfs fix from bfields.  Please, pull from the usual place -
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus

Shortlog:
J. Bruce Fields (2):
      vfs: split out vfs_getattr_nosec
      exportfs: fix 32-bit nfsd handling of 64-bit inode numbers

Diffstat:
 fs/exportfs/expfs.c |   18 ++++++++++++++++--
 fs/stat.c           |   31 +++++++++++++++++++++++++------
 include/linux/fs.h  |    1 +
 3 files changed, 42 insertions(+), 8 deletions(-)

Re: [git pull] fixes for 3.12-final

[Linus Torvalds]

Ugh.

This is too late since I want to do 3.12 today, and it seems to not be
a regression - from what I can tell, the problem has always existed,
no?

So I'll consider this post-3.12. However, it also strikes me that we
should just clean things up, and make "i_ino" be an u64, and be able
to do this without the vfs_getattr_nosec() games.

Hmm?

             Linus

On Sat, Nov 2, 2013 at 6:58 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>         Exportfs fix from bfields.  Please, pull from the usual place -
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus
>
> Shortlog:
> J. Bruce Fields (2):
>       vfs: split out vfs_getattr_nosec
>       exportfs: fix 32-bit nfsd handling of 64-bit inode numbers
>
> Diffstat:
>  fs/exportfs/expfs.c |   18 ++++++++++++++++--
>  fs/stat.c           |   31 +++++++++++++++++++++++++------
>  include/linux/fs.h  |    1 +
>  3 files changed, 42 insertions(+), 8 deletions(-)

Re: [git pull] fixes for 3.12-final

[Al Viro]

On Sun, Nov 03, 2013 at 10:25:32AM -0800, Linus Torvalds wrote:
> Ugh.
> 
> This is too late since I want to do 3.12 today, and it seems to not be
> a regression - from what I can tell, the problem has always existed,
> no?
> 
> So I'll consider this post-3.12. However, it also strikes me that we
> should just clean things up, and make "i_ino" be an u64, and be able
> to do this without the vfs_getattr_nosec() games.

IIRC, at some point such an attempt has seriously hurt iget() on 32bit
boxen, so we ended up deciding not to go there.  Had been years ago,
though...

Re: [git pull] fixes for 3.12-final

[Linus Torvalds]

On Sun, Nov 3, 2013 at 11:54 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> IIRC, at some point such an attempt has seriously hurt iget() on 32bit
> boxen, so we ended up deciding not to go there.  Had been years ago,
> though...

Yeah, I think the circumstances have changed. 32-bit is less
important, and iget() is much less critical than it used to be (all
*normal* inode lookups are through the direct dentry pointer).

Sure, ARM is a few years away from 64-bit being common, but it's
happening. And I suspect even 32-bit ARM doesn't have the annoying
issues that x86-32 had with 64-bit values (namely using up a lot of
the register space).

So unless there's something hidden that makes it really nasty, I do
suspect that a "u64 i_ino" would just be the right thing to do. Rather
than adding workarounds for our current odd situation on 32-bit
kernels (and just wasting time on 64-bit kernels).

          Linus

Re: [git pull] fixes for 3.12-final

[Al Viro]

On Sun, Nov 03, 2013 at 03:39:14PM -0800, Linus Torvalds wrote:
> On Sun, Nov 3, 2013 at 11:54 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > IIRC, at some point such an attempt has seriously hurt iget() on 32bit
> > boxen, so we ended up deciding not to go there.  Had been years ago,
> > though...
> 
> Yeah, I think the circumstances have changed. 32-bit is less
> important, and iget() is much less critical than it used to be (all
> *normal* inode lookups are through the direct dentry pointer).
>
> Sure, ARM is a few years away from 64-bit being common, but it's
> happening. And I suspect even 32-bit ARM doesn't have the annoying
> issues that x86-32 had with 64-bit values (namely using up a lot of
> the register space).
> 
> So unless there's something hidden that makes it really nasty, I do
> suspect that a "u64 i_ino" would just be the right thing to do. Rather
> than adding workarounds for our current odd situation on 32-bit
> kernels (and just wasting time on 64-bit kernels).

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).

Re: [git pull] fixes for 3.12-final

[Bruce Fields]

On Sun, Nov 03, 2013 at 03:39:14PM -0800, Linus Torvalds wrote:
> On Sun, Nov 3, 2013 at 11:54 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > IIRC, at some point such an attempt has seriously hurt iget() on 32bit
> > boxen, so we ended up deciding not to go there.  Had been years ago,
> > though...
> 
> Yeah, I think the circumstances have changed. 32-bit is less
> important, and iget() is much less critical than it used to be (all
> *normal* inode lookups are through the direct dentry pointer).
> 
> Sure, ARM is a few years away from 64-bit being common, but it's
> happening. And I suspect even 32-bit ARM doesn't have the annoying
> issues that x86-32 had with 64-bit values (namely using up a lot of
> the register space).
> 
> So unless there's something hidden that makes it really nasty, I do
> suspect that a "u64 i_ino" would just be the right thing to do. Rather
> than adding workarounds for our current odd situation on 32-bit
> kernels (and just wasting time on 64-bit kernels).

I'm all for it, though I'm worried about:

	$ git grep '\bi_ino\b'|wc -l
	1746

so would prefer a more modest (and possibly stable-appropriate) fix for
3.13 while we sort out what i_ino's being used for.

--b.

Re: [git pull] fixes for 3.12-final

[Al Viro]

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...

Re: [git pull] fixes for 3.12-final

[Bruce Fields]

On Wed, Nov 06, 2013 at 03:10:04PM +0000, Al Viro wrote:
> 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...

I believe we also want that in stable?

950ee9566a5b6cc45d15f5fe044bab4f1e8b62cb "exportfs: fix 32-bit nfsd
handling of 64-bit inode numbers"

--b.

Re: [git pull] fixes for 3.12-final

[Bruce Fields]

(Argh, sorry, with the right stable address cc'd this time I hope.)

On Wed, Nov 06, 2013 at 03:10:04PM +0000, Al Viro wrote:
> 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...

I believe we also want that in stable?

950ee9566a5b6cc45d15f5fe044bab4f1e8b62cb "exportfs: fix 32-bit nfsd
handling of 64-bit inode numbers"

--b.

Re: [git pull] fixes for 3.12-final

[Greg KH]

On Wed, Nov 13, 2013 at 10:16:55AM -0500, Bruce Fields wrote:
> (Argh, sorry, with the right stable address cc'd this time I hope.)
> 
> On Wed, Nov 06, 2013 at 03:10:04PM +0000, Al Viro wrote:
> > 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...
> 
> I believe we also want that in stable?
> 
> 950ee9566a5b6cc45d15f5fe044bab4f1e8b62cb "exportfs: fix 32-bit nfsd
> handling of 64-bit inode numbers"

It breaks the build in 3.12 and others so if it is needed, please
provide a backport that works properly to stable@vger.kernel.org.

thanks,

greg k-h

Re: [git pull] fixes for 3.12-final

[Bruce Fields]

On Mon, Nov 18, 2013 at 08:32:43AM -0800, Greg KH wrote:
> On Wed, Nov 13, 2013 at 10:16:55AM -0500, Bruce Fields wrote:
> > (Argh, sorry, with the right stable address cc'd this time I hope.)
> > 
> > On Wed, Nov 06, 2013 at 03:10:04PM +0000, Al Viro wrote:
> > > 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...
> > 
> > I believe we also want that in stable?
> > 
> > 950ee9566a5b6cc45d15f5fe044bab4f1e8b62cb "exportfs: fix 32-bit nfsd
> > handling of 64-bit inode numbers"
> 
> It breaks the build in 3.12 and others so if it is needed, please
> provide a backport that works properly to stable@vger.kernel.org.

Oops--there was a prerequisite patch that I forgot.  So we actually want

	git cherry-pick b7a6ec52dd4eced4a9bcda9ca85b3c8af84d3c90
	git cherry-pick 950ee9566a5b6cc45d15f5fe044bab4f1e8b62cb

--b.

Re: [git pull] fixes for 3.12-final

[Greg KH]

On Wed, Dec 18, 2013 at 02:40:34PM -0500, Bruce Fields wrote:
> On Mon, Nov 18, 2013 at 08:32:43AM -0800, Greg KH wrote:
> > On Wed, Nov 13, 2013 at 10:16:55AM -0500, Bruce Fields wrote:
> > > (Argh, sorry, with the right stable address cc'd this time I hope.)
> > > 
> > > On Wed, Nov 06, 2013 at 03:10:04PM +0000, Al Viro wrote:
> > > > 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...
> > > 
> > > I believe we also want that in stable?
> > > 
> > > 950ee9566a5b6cc45d15f5fe044bab4f1e8b62cb "exportfs: fix 32-bit nfsd
> > > handling of 64-bit inode numbers"
> > 
> > It breaks the build in 3.12 and others so if it is needed, please
> > provide a backport that works properly to stable@vger.kernel.org.
> 
> Oops--there was a prerequisite patch that I forgot.  So we actually want
> 
> 	git cherry-pick b7a6ec52dd4eced4a9bcda9ca85b3c8af84d3c90
> 	git cherry-pick 950ee9566a5b6cc45d15f5fe044bab4f1e8b62cb

Thanks, I'll go take them now.

greg k-h