Re: [RHEL5 PATCH 2/4] VFS: Make inode numbers 64-bits

Re: [RHEL5 PATCH 2/4] VFS: Make inode numbers 64-bits

[Al Viro]

On Mon, Aug 14, 2006 at 10:15:09PM +0100, David Howells wrote:
> Make the in-kernel representations of inode number 64-bit in size at a minimum
> as some network filesystems (eg: NFS) and local filesystems (eg: XFS) provide
> such.
> 
> The 64-bit inode number will be returned through stat64() and getdents64() on
> archs that are currently set up to do so.
> 
> This patch changes __kernel_ino_t to be "unsigned long long" on all archs, but
> changes usages of that in struct stat to be the old type so that the userspace
> interface does not change.  The 64-bit division patch is required to get this
> to link on some archs.
> 
> struct inode::i_ino and struct kstat::ino have been converted to ino_t.
> Neither needs moving within the bounds of its parent structure to make sure
> that they reside on a 64-bit boundary if the structure itself does so.

NAK.  There's no need to touch i_ino and a lot of reasons for not doing
that.  ->getattr() can fill 64bit field just fine without that and there's
zero need to touch every fs out there *and* add cycles on icache lookups.
WTF for?

Filesystems that want 64bit values in st_ino are welcome to
	* set it in ->getattr() and
	* use iget5()

Less PITA for everyone and less intrusive patch that way.

Re: [RHEL5 PATCH 2/4] VFS: Make inode numbers 64-bits

[David Howells]

Al Viro <viro@ftp.linux.org.uk> wrote:

> NAK.  There's no need to touch i_ino and a lot of reasons for not doing
> that.  ->getattr() can fill 64bit field just fine without that and there's
> zero need to touch every fs out there *and* add cycles on icache lookups.
> WTF for?

That doesn't fix getdents64() though...

David

Re: [RHEL5 PATCH 2/4] VFS: Make inode numbers 64-bits

[Al Viro]

On Tue, Aug 15, 2006 at 09:21:11AM +0100, David Howells wrote:
> Al Viro <viro@ftp.linux.org.uk> wrote:
> 
> > NAK.  There's no need to touch i_ino and a lot of reasons for not doing
> > that.  ->getattr() can fill 64bit field just fine without that and there's
> > zero need to touch every fs out there *and* add cycles on icache lookups.
> > WTF for?
> 
> That doesn't fix getdents64() though...

Good grief...  Make filldir() eat u64 explicitly and fix all of a dozen
instances.  End of story.

Re: [RHEL5 PATCH 2/4] VFS: Make inode numbers 64-bits

[David Howells]

Al Viro <viro@ftp.linux.org.uk> wrote:

> NAK.  There's no need to touch i_ino and a lot of reasons for not doing
> that.

Like all those printks that write ambiguous messages because they can't report
the full inode number?  I'm not so worried about those because they're for the
most part debugging messages, but still, they *can* report invalid information
because i_ino is not big enough in error and warning messages.

David

Re: [RHEL5 PATCH 2/4] VFS: Make inode numbers 64-bits

[Al Viro]

On Tue, Aug 15, 2006 at 09:32:57AM +0100, David Howells wrote:
> Al Viro <viro@ftp.linux.org.uk> wrote:
> 
> > NAK.  There's no need to touch i_ino and a lot of reasons for not doing
> > that.
> 
> Like all those printks that write ambiguous messages because they can't report
> the full inode number?  I'm not so worried about those because they're for the
> most part debugging messages, but still, they *can* report invalid information
> because i_ino is not big enough in error and warning messages.

In fs-independent code?  How many of those do we actually have?

Re: [RHEL5 PATCH 2/4] VFS: Make inode numbers 64-bits

[David Howells]

Al Viro <viro@ftp.linux.org.uk> wrote:

> In fs-independent code?  How many of those do we actually have?

At most about half a dozen.  Depends whether you include pipes and eventpoll
in that.  There's an instance in fs/dcache.c and one in fs/fs-writeback.c

find_inode_number() is also a potential problem, though I suppose we have to
say you may not use it if 0 is a valid thing to have in i_ino.

Interestingly, one of these also touches userspace: /proc/locks passes the
inode number out, but will pass the wrong one if i_ino is too short.  Does
anything in userspace actually use that?

David

Re: [RHEL5 PATCH 2/4] VFS: Make inode numbers 64-bits

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

On Tue, 15 Aug 2006 10:25:47 +0100 David Howells <dhowells@redhat.com> wrote:
>
> Interestingly, one of these also touches userspace: /proc/locks passes the
> inode number out, but will pass the wrong one if i_ino is too short.  Does
> anything in userspace actually use that?

As far as I know, /proc/locks is just useful for debugging the locking code.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 0/4] Use 64-bit inode numbers internally in the kernel

[Al Viro]

On Tue, Aug 15, 2006 at 09:21:53AM +0100, David Howells wrote:
> Josh Boyer <jwboyer@gmail.com> wrote:
> 
> > Out of curiosity, is there a performance hit for 32-bit systems?  Have
> > you done any minimal benchmarks to see?
> 
> Yes, I'm sure there is, but we're talking performance vs correctness.

ITYM performance vs. slightly different patch.  Let me put all pieces
in one place:
	* kstat gets u64 ino
	* filesystems that want to report 64bit st_ino do it in their
->getattr(); the rest is unchanged.
	* ino_t is left as-is
	* filldir() callbacks get u64 ino in arguments.  Filesystem may
pass 64bit value if it cares to; otherwise it's left unchanged.
	* filesystem that wants unusual search key can use iget5() (as
it can do right now)
	* filesystem that wants to use the values it'd put into st_ino in
its printks should use appropriate format
	* any printk in generic code that happens to use i_ino should
be hunted down and shot for utter uselessness for too many filesystems
(not sure if we actually _have_ any such printk these days).