Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-10-14 at 17:44 +1100, Burn Alting wrote:
>  
> 
> 
> 
> 
>  
> 
> 
> 
>  
> 
> 
> 
> On 13/10/24 21:17, Jeff Layton wrote:
>  
> 
> 
> 
>  
> 
> 
> 
> >  
> > 
> > 
> > 
> > On Fri, 2024-10-11 at 17:30 +0200, Mickaël Salaün wrote:
> >  
> > 
> > 
> > 
> > >  
> > > 
> > > 
> > > 
> > > On Fri, Oct 11, 2024 at 07:39:33AM -0700, Christoph Hellwig wrote:
> > >  
> > > 
> > > 
> > > 
> > > >  
> > > > 
> > > > 
> > > > 
> > > > On Fri, Oct 11, 2024 at 03:52:42PM +0200, Mickaël Salaün wrote:
> > > >  
> > > > 
> > > > 
> > > > 
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > >  
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > >  
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Yes, but how do you call getattr() without a path?
> > > > > > >  
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > >  
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > You don't because inode numbers are irrelevant without the path.
> > > > > >  
> > > > > > 
> > > > > > 
> > > > > > 
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > They are for kernel messages and audit logs.  Please take a look at the
> > > > > use cases with the other patches.
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > >  
> > > > 
> > > > 
> > > > 
> > > > It still is useless.  E.g. btrfs has duplicate inode numbers due to
> > > > subvolumes.
> > > >  
> > > > 
> > > > 
> > > > 
> > >  
> > > 
> > > 
> > > 
> > > At least it reflects what users see.
> > > 
> > >  
> > > 
> > > 
> > > 
> > > >  
> > > > 
> > > > 
> > > > 
> > > > If you want a better pretty but not useful value just work on making
> > > > i_ino 64-bits wide, which is long overdue.
> > > >  
> > > > 
> > > > 
> > > > 
> > >  
> > > 
> > > 
> > > 
> > > That would require too much work for me, and this would be a pain to
> > > backport to all stable kernels.
> > > 
> > >  
> > > 
> > > 
> > > 
> >  
> > 
> > 
> > 
> > Would it though? Adding this new inode operation seems sub-optimal.
> > 
> > Inode numbers are static information. Once an inode number is set on an
> > inode it basically never changes.  This patchset will turn all of those
> > direct inode->i_ino fetches into a pointer chase for the new inode
> > operation, which will then almost always just result in a direct fetch.
> > 
> > A better solution here would be to make inode->i_ino a u64, and just
> > fix up all of the places that touch it to expect that. Then, just
> > ensure that all of the filesystems set it properly when instantiating
> > new inodes. Even on 32-bit arch, you likely wouldn't need a seqcount
> > loop or anything to fetch this since the chance of a torn read there is
> > basically zero.
> > 
> > If there are places where we need to convert i_ino down to 32-bits,
> > then we can just use some scheme like nfs_fattr_to_ino_t(), or just
> > cast i_ino to a u32.
> > 
> > Yes, it'd be a larger patchset, but that seems like it would be a
> > better solution.
> >  
> > 
> > 
> > 
>  
> 
> 
> 
> As someone who lives in the analytical user space of Linux audit,  I take it that for large (say >200TB) file systems, the inode reported in audit PATH records is no longer forensically defensible and it's use as a correlation item is of questionable value now?
>  
> 
> 
> 

It's been a while since I worked in audit, but if audit is only
reporting 32-bit inode numbers in its records, then that could be
ambiguous. It's easily possible on larger filesystems to generate more
than 2^32 inodes.

-- 
Jeff Layton <jlayton@kernel.org>