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

[Jan Kara]

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.

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