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

[Chris Down <chris@chrisdown.name>]

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.