include/linux/fs.h | 78 ++++++++++++++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 40 deletions(-)
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@lst.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- Add missing argument names to functions that lacked them
- Reflow some awkward indentation
- Link to v1: https://lore.kernel.org/r/20260306-iino-u64-v1-1-116b53b5ca42@kernel.org
---
include/linux/fs.h | 78 ++++++++++++++++++++++++++----------------------------
1 file changed, 38 insertions(+), 40 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 097443bf12e289c347651e5f3da5b67eb6b53121..9be701267c3820b2a38fe8de27073c98b78c0d8e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2912,65 +2912,63 @@ static inline bool name_contains_dotdot(const char *name)
#include <linux/err.h>
/* needed for stackable file system support */
-extern loff_t default_llseek(struct file *file, loff_t offset, int whence);
+loff_t default_llseek(struct file *file, loff_t offset, int whence);
-extern loff_t vfs_llseek(struct file *file, loff_t offset, int whence);
+loff_t vfs_llseek(struct file *file, loff_t offset, int whence);
-extern int inode_init_always_gfp(struct super_block *, struct inode *, gfp_t);
+int inode_init_always_gfp(struct super_block *sb, struct inode *inode, gfp_t gfp);
static inline int inode_init_always(struct super_block *sb, struct inode *inode)
{
return inode_init_always_gfp(sb, inode, GFP_NOFS);
}
-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);
+int inode_needs_sync(struct inode *inode);
+int inode_just_drop(struct inode *inode);
static inline int inode_generic_drop(struct inode *inode)
{
return !inode->i_nlink || inode_unhashed(inode);
}
-extern void d_mark_dontcache(struct inode *inode);
+void d_mark_dontcache(struct inode *inode);
-extern struct inode *ilookup5_nowait(struct super_block *sb,
+struct inode *ilookup5_nowait(struct super_block *sb,
u64 hashval, int (*test)(struct inode *, void *),
void *data, bool *isnew);
-extern struct inode *ilookup5(struct super_block *sb, u64 hashval,
+struct inode *ilookup5(struct super_block *sb, u64 hashval,
int (*test)(struct inode *, void *), void *data);
-extern struct inode *ilookup(struct super_block *sb, u64 ino);
+struct inode *ilookup(struct super_block *sb, u64 ino);
-extern struct inode *inode_insert5(struct inode *inode, u64 hashval,
+struct inode *inode_insert5(struct inode *inode, u64 hashval,
int (*test)(struct inode *, void *),
int (*set)(struct inode *, void *),
void *data);
-struct inode *iget5_locked(struct super_block *, u64,
- int (*test)(struct inode *, void *),
- int (*set)(struct inode *, void *), void *);
-struct inode *iget5_locked_rcu(struct super_block *, u64,
- int (*test)(struct inode *, void *),
- int (*set)(struct inode *, void *), void *);
-extern struct inode *iget_locked(struct super_block *, u64);
-extern struct inode *find_inode_nowait(struct super_block *,
- u64,
- int (*match)(struct inode *,
- u64, void *),
- void *data);
-extern struct inode *find_inode_rcu(struct super_block *, u64,
- int (*)(struct inode *, void *), void *);
-extern struct inode *find_inode_by_ino_rcu(struct super_block *, u64);
-extern int insert_inode_locked4(struct inode *, u64, int (*test)(struct inode *, void *), void *);
-extern int insert_inode_locked(struct inode *);
+struct inode *iget5_locked(struct super_block *sb, u64 hashval,
+ int (*test)(struct inode *, void *),
+ int (*set)(struct inode *, void *), void *data);
+struct inode *iget5_locked_rcu(struct super_block *sb, u64 hashval,
+ int (*test)(struct inode *, void *),
+ int (*set)(struct inode *, void *), void *data);
+struct inode *iget_locked(struct super_block *sb, u64 ino);
+struct inode *find_inode_nowait(struct super_block *sb, u64 hashval,
+ int (*match)(struct inode *, u64, void *), void *data);
+struct inode *find_inode_rcu(struct super_block *sb, u64 hashval,
+ int (*test)(struct inode *, void *), void *data);
+struct inode *find_inode_by_ino_rcu(struct super_block *sb, u64 ino);
+int insert_inode_locked4(struct inode *inode, u64 hashval,
+ int (*test)(struct inode *, void *), void *data);
+int insert_inode_locked(struct inode *inode);
#ifdef CONFIG_DEBUG_LOCK_ALLOC
-extern void lockdep_annotate_inode_mutex_key(struct inode *inode);
+void lockdep_annotate_inode_mutex_key(struct inode *inode);
#else
static inline void lockdep_annotate_inode_mutex_key(struct inode *inode) { };
#endif
-extern void unlock_new_inode(struct inode *);
-extern void discard_new_inode(struct inode *);
-extern unsigned int get_next_ino(void);
-extern void evict_inodes(struct super_block *sb);
+void unlock_new_inode(struct inode *inode);
+void discard_new_inode(struct inode *inode);
+unsigned int get_next_ino(void);
+void evict_inodes(struct super_block *sb);
void dump_mapping(const struct address_space *);
/*
@@ -3015,21 +3013,21 @@ int setattr_should_drop_sgid(struct mnt_idmap *idmap,
*/
#define alloc_inode_sb(_sb, _cache, _gfp) kmem_cache_alloc_lru(_cache, &_sb->s_inode_lru, _gfp)
-extern void __insert_inode_hash(struct inode *, u64 hashval);
+void __insert_inode_hash(struct inode *inode, u64 hashval);
static inline void insert_inode_hash(struct inode *inode)
{
__insert_inode_hash(inode, inode->i_ino);
}
-extern void __remove_inode_hash(struct inode *);
+void __remove_inode_hash(struct inode *inode);
static inline void remove_inode_hash(struct inode *inode)
{
if (!inode_unhashed(inode) && !hlist_fake(&inode->i_hash))
__remove_inode_hash(inode);
}
-extern void inode_sb_list_add(struct inode *inode);
-extern void inode_lru_list_add(struct inode *inode);
+void inode_sb_list_add(struct inode *inode);
+void inode_lru_list_add(struct inode *inode);
int generic_file_mmap(struct file *, struct vm_area_struct *);
int generic_file_mmap_prepare(struct vm_area_desc *desc);
---
base-commit: 1b626cfc5c2e16d02f0f7516b68b00aaa0a186cf
change-id: 20260306-iino-u64-18ba046632a0
Best regards,
--
Jeff Layton <jlayton@kernel.org>On Sat, Mar 07, 2026 at 02:54:31PM -0500, 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@lst.de> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- As I said I had already dropped those externs in your original patch but thanks for sending this. I've taken it. Fwiw, one or two cycles ago I started with the fs.h header split. Most of the superblock stuff has been moved from fs.h into: include/linux/fs/super.h include/linux/fs/super_types.h The same thing should ultimately be done for inodes I'm pretty sure. So I'm happy to take patches for that as well.
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@lst.de> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jeff Layton <jlayton@kernel.org> ... > -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. Otherwise I like the changes in this patch so feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
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@lst.de> > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > ... > > -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. > Otherwise I like the changes in this patch so feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > Thanks! -- Jeff Layton <jlayton@kernel.org>
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@lst.de> > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > ... > > > -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... Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
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@lst.de>
> > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ...
> > > > -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.
First, here was Claude's analysis of this piece. (FWIW, we didn't
change ino_t type since that could have had userland implications):
-----------------------8<------------------------
## Phase 5: Related Type Updates
### 5.1 `get_next_ino()` (`fs/inode.c:1145`)
Returns `unsigned int` (32-bit). Used by pseudo-filesystems (tmpfs, sysfs,
debugfs, etc.). This is deliberately limited to 32 bits to avoid EOVERFLOW from
`stat()` on 32-bit userspace. **No change needed** — widening to `u64` storage
is fine; the values still fit.
### 5.2 `iunique()` (`fs/inode.c:1556`)
Uses `static unsigned int counter`. Returns `ino_t`. Same consideration as
`get_next_ino()` — keeps values in 32-bit range for compat. **No change needed**
for the counter, but return type should follow `ino_t` (which may or may not
change per Phase 1.2).
### 5.3 `is_zero_ino()` (`include/linux/fs.h:2986`)
Already casts to `(u32)` explicitly. **No change needed.**
-----------------------8<------------------------
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.
Still, I think we'll want to tread carefully here. Maybe we could add
64-bit versions of these functions that are lockless, and add a kernel
command-line switch that makes them fall back to the older functions in
case there are issues?
--
Jeff Layton <jlayton@kernel.org>
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@lst.de>
> > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ...
> > > > > -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@suse.com>
SUSE Labs, CR
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@lst.de>
> > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > ...
> > > > > > -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.
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).
--
Jeff Layton <jlayton@kernel.org>
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@lst.de>
> > > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > > ...
> > > > > > > -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@suse.com>
SUSE Labs, CR
© 2016 - 2026 Red Hat, Inc.