Re: [PATCH v2] vfs: remove externs from fs.h on functions modified by i_ino widening

[Jan Kara]

On Mon 09-03-26 10:56:02, Jeff Layton wrote:
> On Mon, 2026-03-09 at 15:46 +0100, Jan Kara wrote:
> > On Mon 09-03-26 09:27:47, Jeff Layton wrote:
> > > On Mon, 2026-03-09 at 13:27 +0100, Jan Kara wrote:
> > > > On Mon 09-03-26 07:53:51, Jeff Layton wrote:
> > > > > On Mon, 2026-03-09 at 11:02 +0100, Jan Kara wrote:
> > > > > > On Sat 07-03-26 14:54:31, Jeff Layton wrote:
> > > > > > > Christoph says, in response to one of the patches in the i_ino widening
> > > > > > > series, which changes the prototype of several functions in fs.h:
> > > > > > > 
> > > > > > >     "Can you please drop all these pointless externs while you're at it?"
> > > > > > > 
> > > > > > > Remove extern keyword from functions touched by that patch (and a few
> > > > > > > that happened to be nearby). Also add missing argument names to
> > > > > > > declarations that lacked them.
> > > > > > > 
> > > > > > > Suggested-by: Christoph Hellwig <hch@xxxxxx>
> > > > > > > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > > > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > > > ...
> > > > > > > -extern void inode_init_once(struct inode *);
> > > > > > > -extern void address_space_init_once(struct address_space *mapping);
> > > > > > > -extern struct inode * igrab(struct inode *);
> > > > > > > -extern ino_t iunique(struct super_block *, ino_t);
> > > > > > > -extern int inode_needs_sync(struct inode *inode);
> > > > > > > -extern int inode_just_drop(struct inode *inode);
> > > > > > > +void inode_init_once(struct inode *inode);
> > > > > > > +void address_space_init_once(struct address_space *mapping);
> > > > > > > +struct inode *igrab(struct inode *inode);
> > > > > > > +ino_t iunique(struct super_block *sb, ino_t max_reserved);
> > > > > > 
> > > > > > I've just noticed that we probably forgot to convert iunique() to use u64
> > > > > > for inode numbers... Although the iunique() number allocator might prefer
> > > > > > to stay within 32 bits, the interfaces should IMO still use u64 for
> > > > > > consistency.
> > > > > > 
> > > > > 
> > > > > I went back and forth on that one, but I left iunique() changes off
> > > > > since they weren't strictly required. Most filesystems that use it
> > > > > won't have more than 2^32 inodes anyway.
> > > > > 
> > > > > If they worked before with iunique() limited to 32-bit values, they
> > > > > should still after this. After the i_ino widening we could certainly
> > > > > change it to return a u64 though.
> > > > 
> > > > Yes, it won't change anything wrt functionality. I just think that if we go
> > > > for "ino_t is the userspace API type and kernel-internal inode numbers
> > > > (i.e.  what gets stored in inode->i_ino) are passed as u64", then this
> > > > place should logically have u64...
> > > > 
> > > 
> > > I think we'll need a real plan for this.
> > 
> > <snip claude analysis>
> >  
> > > It certainly wouldn't hurt to make these functions return a u64 type,
> > > but I worry a little about letting them return values bigger than
> > > UINT_MAX:
> > > 
> > > One of my very first patches was 866b04fccbf1 ("inode numbering: make
> > > static counters in new_inode and iunique be 32 bits"), and it made
> > > get_next_ino() and iunique() always return 32 bit values. 
> > > 
> > > At the time, 32-bit machines and legacy binaries were a lot more
> > > prevalent than they are today and this was real problem. I'm guessing
> > > it's not so much today, so we could probably make them return real 64-
> > > bit values. That might also obviate the need for locking in these
> > > functions too.
> > 
> > Hum, I think I still didn't express myself clearly enough. I *don't* want
> > to change values returned from get_next_ino() or iunique(). I would leave
> > that for the moment when someone comes with a need for more than 4 billions
> > of inodes in a filesystem using these (which I think is still quite a few
> > years away). All I want is that in-kernel inode number is consistently
> > passed as u64 and not as ino_t. So all I ask for is this diff:
> > 
> > - ino_t iunique(struct super_block *sb, ino_t max_reserved)
> > + u64 iunique(struct super_block *sb u64 max_reserved)
> > ...
> >  	static unsigned int counter;
> > -	ino_t res;
> > +	u64 res;
> > 
> > and that's it. I.e., the 'counter' variable that determines max value of
> > returned number stays as unsigned int.
> 
> Sure, that's simple enough, though I wonder whether it's the right
> thing to do:
> 
> Both these functions return a max values of UINT_MAX, so maybe they
> should return u32 instead? If you make them return u64, then this
> limitation isn't evident unless you look at the function itself.

Well, that limitation wasn't evident with ino_t either :). The kernel doc
comment for the function should mention this, that's true. I don't think
the type of the return value is a very explicit way of expressing that
either (I'd say you often don't notice). If you want to make that more
explicit besides the kernel doc comment, then I'd suggest to call the
function iunique32().

> I'm also wondering whether we should just go ahead and revamp them to
> return real 64-bit values. When the collision risk goes away, then we
> don't need the spinlock in iunique(), for instance. We could just make
> that use an atomic64_t (or do something creative with percpu values).

Yes, that would be interesting optimization, I think someone was already
wanting to do something like that, but I agree we need to be careful about
userspace regressions there and probably transition users one by one.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR