Re: [PATCH v2 2/2] tmpfs: Support 64-bit inums per-sb

[Chris Down]

Amir Goldstein writes:
> > +config TMPFS_INODE64
> > +       bool "Use 64-bit ino_t by default in tmpfs"
> > +       depends on TMPFS && 64BIT
> > +       default n
> > +       help
> > +         tmpfs has historically used only inode numbers as wide as an unsigned
> > +         int. In some cases this can cause wraparound, potentially resulting in
> > +         multiple files with the same inode number on a single device. This option
> > +         makes tmpfs use the full width of ino_t by default, similarly to the
> > +         inode64 mount option.
> > +
> > +         tmpfs mounts that are used privately by the kernel and are not visible to
> > +         users are unaffected.
>
> Admins won't know what the line above means and they shouldn't care.
> It adds no information, so better remove it.

Sure thing.

> > +
> > +       /*
> > +        * Showing inode{64,32} might be useful even if it's the system default,
> > +        * since then people don't have to resort to checking both here and
> > +        * /proc/config.gz to confirm 64-bit inums were successfully applied
> > +        * (which may not even exist if IKCONFIG_PROC isn't enabled).
> > +        *
> > +        * We hide it when inode64 isn't the default and we are using 32-bit
> > +        * inodes, since that probably just means the feature isn't even under
> > +        * consideration.
> > +        *
> > +        * As such:
> > +        *
> > +        *                     +-----------------+-----------------+
> > +        *                     | TMPFS_INODE64=y | TMPFS_INODE64=n |
> > +        *  +------------------+-----------------+-----------------+
> > +        *  | full_inums=true  | show            | show            |
> > +        *  | full_inums=false | show            | hide            |
> > +        *  +------------------+-----------------+-----------------+
> > +        *
> > +        */
> > +       if (IS_ENABLED(CONFIG_TMPFS_INODE64) || !sbinfo->full_inums)
>
> Condition does not match comment - should be || sbinfo->full_inums)

Good catch! Thanks.

> > @@ -3915,6 +3969,7 @@ int shmem_init_fs_context(struct fs_context *fc)
> >         ctx->mode = 0777 | S_ISVTX;
> >         ctx->uid = current_fsuid();
> >         ctx->gid = current_fsgid();
> > +       ctx->full_inums = IS_ENABLED(CONFIG_TMPFS_INODE64);
>
> This is the wrong place for this - it is also being set for the kern_mount.
> Follow the lead of shmem_default_max_inodes.

Hmm, full_inums is intended to be simply ignored for SB_KERNMOUNT though, so it 
seems harmless, but I agree maybe it makes the intent of the code clearer to 
move it to a more specific place.

Thanks! I'll fix these up for v3.