[PATCH v2 0/2] tmpfs: Reduce risk of inum overflow

[PATCH v2 0/2] tmpfs: Reduce risk of inum overflow

[Chris Down]

In Facebook production we are seeing heavy i_ino wraparounds on tmpfs.
On affected tiers, in excess of 10% of hosts show multiple files with
different content and the same inode number, with some servers even
having as many as 150 duplicated inode numbers with differing file
content.

This causes actual, tangible problems in production. For example, we
have complaints from those working on remote caches that their
application is reporting cache corruptions because it uses (device,
inodenum) to establish the identity of a particular cache object, but
because it's not unique any more, the application refuses to continue
and reports cache corruption. Even worse, sometimes applications may not
even detect the corruption but may continue anyway, causing phantom and
hard to debug behaviour.

In general, userspace applications expect that (device, inodenum) should
be enough to be uniquely point to one inode, which seems fair enough.
One might also need to check the generation, but in this case:

1. That's not currently exposed to userspace
   (ioctl(...FS_IOC_GETVERSION...) returns ENOTTY on tmpfs);
2. Even with generation, there shouldn't be two live inodes with the
   same inode number on one device.

In order to mitigate this, we take a two-pronged approach:

1. Moving inum generation from being global to per-sb for tmpfs. This
   itself allows some reduction in i_ino churn. This works on both 64-
   and 32- bit machines.
2. Adding inode{64,32} for tmpfs. This fix is supported on machines with
   64-bit ino_t only: we allow users to mount tmpfs with a new inode64
   option that uses the full width of ino_t, or CONFIG_TMPFS_INODE64.

Chris Down (2):
  tmpfs: Add per-superblock i_ino support
  tmpfs: Support 64-bit inums per-sb

 Documentation/filesystems/tmpfs.txt |  11 +++
 fs/Kconfig                          |  18 +++++
 include/linux/shmem_fs.h            |   2 +
 mm/shmem.c                          | 110 ++++++++++++++++++++++++++--
 4 files changed, 133 insertions(+), 8 deletions(-)

-- 
2.24.1

[PATCH v2 1/2] tmpfs: Add per-superblock i_ino support

[Chris Down]

get_next_ino has a number of problems:

- It uses and returns a uint, which is susceptible to become overflowed
  if a lot of volatile inodes that use get_next_ino are created.
- It's global, with no specificity per-sb or even per-filesystem. This
  means it's not that difficult to cause inode number wraparounds on a
  single device, which can result in having multiple distinct inodes
  with the same inode number.

This patch adds a per-superblock counter that mitigates the second case.
This design also allows us to later have a specific i_ino size
per-device, for example, allowing users to choose whether to use 32- or
64-bit inodes for each tmpfs mount. This is implemented in the next
commit.

Signed-off-by: Chris Down <chris@chrisdown.name>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@fb.com
---
 include/linux/shmem_fs.h |  1 +
 mm/shmem.c               | 55 ++++++++++++++++++++++++++++++++++------
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index de8e4b71e3ba..dec4353cf3b7 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -35,6 +35,7 @@ struct shmem_sb_info {
 	unsigned char huge;	    /* Whether to try for hugepages */
 	kuid_t uid;		    /* Mount uid for root directory */
 	kgid_t gid;		    /* Mount gid for root directory */
+	ino_t last_ino;		    /* The last used per-sb inode number */
 	struct mempolicy *mpol;     /* default memory policy for mappings */
 	spinlock_t shrinklist_lock;   /* Protects shrinklist */
 	struct list_head shrinklist;  /* List of shinkable inodes */
diff --git a/mm/shmem.c b/mm/shmem.c
index 165fa6332993..8af9fb922a96 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2235,8 +2235,18 @@ static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
 	return 0;
 }
 
+/*
+ * shmem_get_inode - reserve, allocate, and initialise a new inode
+ *
+ * If usb_sb_ino is true, we use the per-sb inode allocator to avoid wraparound.
+ * Otherwise, we use get_next_ino, which is global.
+ *
+ * If use_sb_ino is true or max_inodes is greater than 0, we may have to grab
+ * the per-sb stat_lock.
+ */
 static struct inode *shmem_get_inode(struct super_block *sb, const struct inode *dir,
-				     umode_t mode, dev_t dev, unsigned long flags)
+				     umode_t mode, dev_t dev,
+				     unsigned long flags, bool use_sb_ino)
 {
 	struct inode *inode;
 	struct shmem_inode_info *info;
@@ -2247,7 +2257,30 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
 
 	inode = new_inode(sb);
 	if (inode) {
-		inode->i_ino = get_next_ino();
+		if (use_sb_ino) {
+			spin_lock(&sbinfo->stat_lock);
+			inode->i_ino = sbinfo->last_ino++;
+			if (unlikely(inode->i_ino >= UINT_MAX)) {
+				/*
+				 * Emulate get_next_ino uint wraparound for
+				 * compatibility
+				 */
+				pr_warn("%s: inode number overflow on device %d, consider using inode64 mount option\n",
+					__func__, MINOR(sb->s_dev));
+				inode->i_ino = sbinfo->last_ino = 1;
+			}
+			spin_unlock(&sbinfo->stat_lock);
+		} else {
+			/*
+			 * __shmem_file_setup, one of our callers, is lock-free:
+			 * it doesn't hold stat_lock in shmem_reserve_inode
+			 * since max_inodes is always 0, and is called from
+			 * potentially unknown contexts. As such, use the global
+			 * allocator which doesn't require the per-sb stat_lock.
+			 */
+			inode->i_ino = get_next_ino();
+		}
+
 		inode_init_owner(inode, dir, mode);
 		inode->i_blocks = 0;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
@@ -2881,7 +2914,7 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, dev_t dev)
 	struct inode *inode;
 	int error = -ENOSPC;
 
-	inode = shmem_get_inode(dir->i_sb, dir, mode, dev, VM_NORESERVE);
+	inode = shmem_get_inode(dir->i_sb, dir, mode, dev, VM_NORESERVE, true);
 	if (inode) {
 		error = simple_acl_create(dir, inode);
 		if (error)
@@ -2910,7 +2943,7 @@ shmem_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 	struct inode *inode;
 	int error = -ENOSPC;
 
-	inode = shmem_get_inode(dir->i_sb, dir, mode, 0, VM_NORESERVE);
+	inode = shmem_get_inode(dir->i_sb, dir, mode, 0, VM_NORESERVE, true);
 	if (inode) {
 		error = security_inode_init_security(inode, dir,
 						     NULL,
@@ -3106,7 +3139,7 @@ static int shmem_symlink(struct inode *dir, struct dentry *dentry, const char *s
 		return -ENAMETOOLONG;
 
 	inode = shmem_get_inode(dir->i_sb, dir, S_IFLNK | 0777, 0,
-				VM_NORESERVE);
+				VM_NORESERVE, true);
 	if (!inode)
 		return -ENOSPC;
 
@@ -3378,6 +3411,8 @@ enum shmem_param {
 	Opt_nr_inodes,
 	Opt_size,
 	Opt_uid,
+	Opt_inode32,
+	Opt_inode64,
 };
 
 static const struct fs_parameter_spec shmem_param_specs[] = {
@@ -3389,6 +3424,8 @@ static const struct fs_parameter_spec shmem_param_specs[] = {
 	fsparam_string("nr_inodes",	Opt_nr_inodes),
 	fsparam_string("size",		Opt_size),
 	fsparam_u32   ("uid",		Opt_uid),
+	fsparam_flag  ("inode32",	Opt_inode32),
+	fsparam_flag  ("inode64",	Opt_inode64),
 	{}
 };
 
@@ -3690,7 +3727,8 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
 #endif
 	uuid_gen(&sb->s_uuid);
 
-	inode = shmem_get_inode(sb, NULL, S_IFDIR | sbinfo->mode, 0, VM_NORESERVE);
+	inode = shmem_get_inode(sb, NULL, S_IFDIR | sbinfo->mode, 0,
+				VM_NORESERVE, true);
 	if (!inode)
 		goto failed;
 	inode->i_uid = sbinfo->uid;
@@ -4081,7 +4119,8 @@ EXPORT_SYMBOL_GPL(shmem_truncate_range);
 
 #define shmem_vm_ops				generic_file_vm_ops
 #define shmem_file_operations			ramfs_file_operations
-#define shmem_get_inode(sb, dir, mode, dev, flags)	ramfs_get_inode(sb, dir, mode, dev)
+#define shmem_get_inode(sb, dir, mode, dev, flags, use_sb_ino) \
+	ramfs_get_inode(sb, dir, mode, dev)
 #define shmem_acct_size(flags, size)		0
 #define shmem_unacct_size(flags, size)		do {} while (0)
 
@@ -4105,7 +4144,7 @@ static struct file *__shmem_file_setup(struct vfsmount *mnt, const char *name, l
 		return ERR_PTR(-ENOMEM);
 
 	inode = shmem_get_inode(mnt->mnt_sb, NULL, S_IFREG | S_IRWXUGO, 0,
-				flags);
+				flags, false);
 	if (unlikely(!inode)) {
 		shmem_unacct_size(flags, size);
 		return ERR_PTR(-ENOSPC);
-- 
2.24.1

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

[Chris Down]

The default is still set to inode32 for backwards compatibility, but
system administrators can opt in to the new 64-bit inode numbers by
either:

1. Passing inode64 on the command line when mounting, or
2. Configuring the kernel with CONFIG_TMPFS_INODE64=y

Signed-off-by: Chris Down <chris@chrisdown.name>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@fb.com
---
 Documentation/filesystems/tmpfs.txt | 11 ++++++
 fs/Kconfig                          | 18 +++++++++
 include/linux/shmem_fs.h            |  1 +
 mm/shmem.c                          | 57 ++++++++++++++++++++++++++++-
 4 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/tmpfs.txt b/Documentation/filesystems/tmpfs.txt
index 5ecbc03e6b2f..203e12a684c9 100644
--- a/Documentation/filesystems/tmpfs.txt
+++ b/Documentation/filesystems/tmpfs.txt
@@ -136,6 +136,15 @@ These options do not have any effect on remount. You can change these
 parameters with chmod(1), chown(1) and chgrp(1) on a mounted filesystem.
 
 
+tmpfs has a mount option to select whether it will wrap at 32- or 64-bit inode
+numbers:
+
+inode64   Use 64-bit inode numbers
+inode32   Use 32-bit inode numbers
+
+On 64-bit, the default is set by CONFIG_TMPFS_INODE64. On 32-bit, inode64 is
+not legal and will produce an error at mount time.
+
 So 'mount -t tmpfs -o size=10G,nr_inodes=10k,mode=700 tmpfs /mytmpfs'
 will give you tmpfs instance on /mytmpfs which can allocate 10GB
 RAM/SWAP in 10240 inodes and it is only accessible by root.
@@ -147,3 +156,5 @@ Updated:
    Hugh Dickins, 4 June 2007
 Updated:
    KOSAKI Motohiro, 16 Mar 2010
+Updated:
+   Chris Down, 2 Jan 2020
diff --git a/fs/Kconfig b/fs/Kconfig
index 7b623e9fc1b0..af2048ae71eb 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -199,6 +199,24 @@ config TMPFS_XATTR
 
 	  If unsure, say N.
 
+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.
+
+	  To override this default, use the inode32 or inode64 mount options.
+
+	  If unsure, say N.
+
 config HUGETLBFS
 	bool "HugeTLB file system support"
 	depends on X86 || IA64 || SPARC64 || (S390 && 64BIT) || \
diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index dec4353cf3b7..0e645ecd6451 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -35,6 +35,7 @@ struct shmem_sb_info {
 	unsigned char huge;	    /* Whether to try for hugepages */
 	kuid_t uid;		    /* Mount uid for root directory */
 	kgid_t gid;		    /* Mount gid for root directory */
+	bool full_inums;	    /* If i_ino should be uint or ino_t */
 	ino_t last_ino;		    /* The last used per-sb inode number */
 	struct mempolicy *mpol;     /* default memory policy for mappings */
 	spinlock_t shrinklist_lock;   /* Protects shrinklist */
diff --git a/mm/shmem.c b/mm/shmem.c
index 8af9fb922a96..fd2542e5ada9 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -115,11 +115,13 @@ struct shmem_options {
 	kuid_t uid;
 	kgid_t gid;
 	umode_t mode;
+	bool full_inums;
 	int huge;
 	int seen;
 #define SHMEM_SEEN_BLOCKS 1
 #define SHMEM_SEEN_INODES 2
 #define SHMEM_SEEN_HUGE 4
+#define SHMEM_SEEN_INUMS 8
 };
 
 #ifdef CONFIG_TMPFS
@@ -2260,7 +2262,8 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
 		if (use_sb_ino) {
 			spin_lock(&sbinfo->stat_lock);
 			inode->i_ino = sbinfo->last_ino++;
-			if (unlikely(inode->i_ino >= UINT_MAX)) {
+			if (unlikely(!sbinfo->full_inums &&
+				     inode->i_ino >= UINT_MAX)) {
 				/*
 				 * Emulate get_next_ino uint wraparound for
 				 * compatibility
@@ -2277,6 +2280,12 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
 			 * since max_inodes is always 0, and is called from
 			 * potentially unknown contexts. As such, use the global
 			 * allocator which doesn't require the per-sb stat_lock.
+			 *
+			 * No special behaviour is needed for
+			 * sbinfo->full_inums, because it's not possible to
+			 * manually set on callers of this type, and
+			 * CONFIG_TMPFS_INODE64 only applies to user-visible
+			 * mounts.
 			 */
 			inode->i_ino = get_next_ino();
 		}
@@ -3450,6 +3459,7 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
 	unsigned long long size;
 	char *rest;
 	int opt;
+	const char *err;
 
 	opt = fs_parse(fc, &shmem_fs_parameters, param, &result);
 	if (opt < 0)
@@ -3511,6 +3521,18 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
 			break;
 		}
 		goto unsupported_parameter;
+	case Opt_inode32:
+		ctx->full_inums = false;
+		ctx->seen |= SHMEM_SEEN_INUMS;
+		break;
+	case Opt_inode64:
+		if (sizeof(ino_t) < 8) {
+			err = "Cannot use inode64 with <64bit inums in kernel";
+			goto err_msg;
+		}
+		ctx->full_inums = true;
+		ctx->seen |= SHMEM_SEEN_INUMS;
+		break;
 	}
 	return 0;
 
@@ -3518,6 +3540,8 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
 	return invalf(fc, "tmpfs: Unsupported parameter '%s'", param->key);
 bad_value:
 	return invalf(fc, "tmpfs: Bad value for '%s'", param->key);
+err_msg:
+	return invalf(fc, "tmpfs: %s", err);
 }
 
 static int shmem_parse_options(struct fs_context *fc, void *data)
@@ -3602,6 +3626,12 @@ static int shmem_reconfigure(struct fs_context *fc)
 		}
 	}
 
+	if ((ctx->seen & SHMEM_SEEN_INUMS) && !ctx->full_inums &&
+	    sbinfo->last_ino > UINT_MAX) {
+		err = "Current inum too high to switch to 32-bit inums";
+		goto out;
+	}
+
 	if (ctx->seen & SHMEM_SEEN_HUGE)
 		sbinfo->huge = ctx->huge;
 	if (ctx->seen & SHMEM_SEEN_BLOCKS)
@@ -3643,6 +3673,29 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
 				from_kgid_munged(&init_user_ns, sbinfo->gid));
+
+	/*
+	 * 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)
+		seq_printf(seq, ",inode%d", (sbinfo->full_inums ? 64 : 32));
 #ifdef CONFIG_TRANSPARENT_HUGE_PAGECACHE
 	/* Rightly or wrongly, show huge mount option unmasked by shmem_huge */
 	if (sbinfo->huge)
@@ -3702,6 +3755,7 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
 	sbinfo->free_inodes = sbinfo->max_inodes = ctx->inodes;
 	sbinfo->uid = ctx->uid;
 	sbinfo->gid = ctx->gid;
+	sbinfo->full_inums = ctx->full_inums;
 	sbinfo->mode = ctx->mode;
 	sbinfo->huge = ctx->huge;
 	sbinfo->mpol = ctx->mpol;
@@ -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);
 
 	fc->fs_private = ctx;
 	fc->ops = &shmem_fs_context_ops;
-- 
2.24.1

Re: [PATCH v2 1/2] tmpfs: Add per-superblock i_ino support

[Amir Goldstein]

On Thu, Jan 2, 2020 at 8:49 PM Chris Down <chris@chrisdown.name> wrote:
>
> get_next_ino has a number of problems:
>
> - It uses and returns a uint, which is susceptible to become overflowed
>   if a lot of volatile inodes that use get_next_ino are created.
> - It's global, with no specificity per-sb or even per-filesystem. This
>   means it's not that difficult to cause inode number wraparounds on a
>   single device, which can result in having multiple distinct inodes
>   with the same inode number.
>
> This patch adds a per-superblock counter that mitigates the second case.
> This design also allows us to later have a specific i_ino size
> per-device, for example, allowing users to choose whether to use 32- or
> 64-bit inodes for each tmpfs mount. This is implemented in the next
> commit.
>
> Signed-off-by: Chris Down <chris@chrisdown.name>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@fb.com
> ---
>  include/linux/shmem_fs.h |  1 +
>  mm/shmem.c               | 55 ++++++++++++++++++++++++++++++++++------
>  2 files changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index de8e4b71e3ba..dec4353cf3b7 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -35,6 +35,7 @@ struct shmem_sb_info {
>         unsigned char huge;         /* Whether to try for hugepages */
>         kuid_t uid;                 /* Mount uid for root directory */
>         kgid_t gid;                 /* Mount gid for root directory */
> +       ino_t last_ino;             /* The last used per-sb inode number */
>         struct mempolicy *mpol;     /* default memory policy for mappings */
>         spinlock_t shrinklist_lock;   /* Protects shrinklist */
>         struct list_head shrinklist;  /* List of shinkable inodes */
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 165fa6332993..8af9fb922a96 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2235,8 +2235,18 @@ static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
>         return 0;
>  }
>
> +/*
> + * shmem_get_inode - reserve, allocate, and initialise a new inode
> + *
> + * If usb_sb_ino is true, we use the per-sb inode allocator to avoid wraparound.
> + * Otherwise, we use get_next_ino, which is global.
> + *
> + * If use_sb_ino is true or max_inodes is greater than 0, we may have to grab
> + * the per-sb stat_lock.

Wouldn't it be easier to check max_inodes instead of passing this
use_sb_ino arg?
Is there any case where they *need* to differ?

> + */
>  static struct inode *shmem_get_inode(struct super_block *sb, const struct inode *dir,
> -                                    umode_t mode, dev_t dev, unsigned long flags)
> +                                    umode_t mode, dev_t dev,
> +                                    unsigned long flags, bool use_sb_ino)
>  {
>         struct inode *inode;
>         struct shmem_inode_info *info;
> @@ -2247,7 +2257,30 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
>
>         inode = new_inode(sb);
>         if (inode) {
> -               inode->i_ino = get_next_ino();
> +               if (use_sb_ino) {
> +                       spin_lock(&sbinfo->stat_lock);
> +                       inode->i_ino = sbinfo->last_ino++;
> +                       if (unlikely(inode->i_ino >= UINT_MAX)) {
> +                               /*
> +                                * Emulate get_next_ino uint wraparound for
> +                                * compatibility
> +                                */
> +                               pr_warn("%s: inode number overflow on device %d, consider using inode64 mount option\n",
> +                                       __func__, MINOR(sb->s_dev));
> +                               inode->i_ino = sbinfo->last_ino = 1;
> +                       }
> +                       spin_unlock(&sbinfo->stat_lock);
> +               } else {
> +                       /*
> +                        * __shmem_file_setup, one of our callers, is lock-free:
> +                        * it doesn't hold stat_lock in shmem_reserve_inode
> +                        * since max_inodes is always 0, and is called from
> +                        * potentially unknown contexts. As such, use the global
> +                        * allocator which doesn't require the per-sb stat_lock.
> +                        */
> +                       inode->i_ino = get_next_ino();
> +               }
> +
>                 inode_init_owner(inode, dir, mode);
>                 inode->i_blocks = 0;
>                 inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> @@ -2881,7 +2914,7 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, dev_t dev)
>         struct inode *inode;
>         int error = -ENOSPC;
>
> -       inode = shmem_get_inode(dir->i_sb, dir, mode, dev, VM_NORESERVE);
> +       inode = shmem_get_inode(dir->i_sb, dir, mode, dev, VM_NORESERVE, true);
>         if (inode) {
>                 error = simple_acl_create(dir, inode);
>                 if (error)
> @@ -2910,7 +2943,7 @@ shmem_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
>         struct inode *inode;
>         int error = -ENOSPC;
>
> -       inode = shmem_get_inode(dir->i_sb, dir, mode, 0, VM_NORESERVE);
> +       inode = shmem_get_inode(dir->i_sb, dir, mode, 0, VM_NORESERVE, true);
>         if (inode) {
>                 error = security_inode_init_security(inode, dir,
>                                                      NULL,
> @@ -3106,7 +3139,7 @@ static int shmem_symlink(struct inode *dir, struct dentry *dentry, const char *s
>                 return -ENAMETOOLONG;
>
>         inode = shmem_get_inode(dir->i_sb, dir, S_IFLNK | 0777, 0,
> -                               VM_NORESERVE);
> +                               VM_NORESERVE, true);
>         if (!inode)
>                 return -ENOSPC;
>
> @@ -3378,6 +3411,8 @@ enum shmem_param {
>         Opt_nr_inodes,
>         Opt_size,
>         Opt_uid,
> +       Opt_inode32,
> +       Opt_inode64,

Does not belong to this patch..

>  };
>
>  static const struct fs_parameter_spec shmem_param_specs[] = {
> @@ -3389,6 +3424,8 @@ static const struct fs_parameter_spec shmem_param_specs[] = {
>         fsparam_string("nr_inodes",     Opt_nr_inodes),
>         fsparam_string("size",          Opt_size),
>         fsparam_u32   ("uid",           Opt_uid),
> +       fsparam_flag  ("inode32",       Opt_inode32),
> +       fsparam_flag  ("inode64",       Opt_inode64),

Ditto

>         {}
>  };
>
> @@ -3690,7 +3727,8 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
>  #endif
>         uuid_gen(&sb->s_uuid);
>
> -       inode = shmem_get_inode(sb, NULL, S_IFDIR | sbinfo->mode, 0, VM_NORESERVE);
> +       inode = shmem_get_inode(sb, NULL, S_IFDIR | sbinfo->mode, 0,
> +                               VM_NORESERVE, true);

Should usb_sb_ino be true for the kern_mount??
In any case, it wouldn't matter if it was false, hence no need to pass
an argument
and can either check for sbinfo->max_inodes or the SB_KERNMOUNT flag in
shmem_get_inode().

Thanks,
Amir.

Re: [PATCH v2 1/2] tmpfs: Add per-superblock i_ino support

[Chris Down]

Amir Goldstein writes:
>Wouldn't it be easier to check max_inodes instead of passing this
>use_sb_ino arg?
>Is there any case where they *need* to differ?

Hmm, I suppose probably not? In that case should I just check against 
SB_KERNMOUNT, since max_inodes can only be 0 in that case?

>> @@ -3378,6 +3411,8 @@ enum shmem_param {
>>         Opt_nr_inodes,
>>         Opt_size,
>>         Opt_uid,
>> +       Opt_inode32,
>> +       Opt_inode64,
>
>Does not belong to this patch..
>
>>  };
>>
>>  static const struct fs_parameter_spec shmem_param_specs[] = {
>> @@ -3389,6 +3424,8 @@ static const struct fs_parameter_spec shmem_param_specs[] = {
>>         fsparam_string("nr_inodes",     Opt_nr_inodes),
>>         fsparam_string("size",          Opt_size),
>>         fsparam_u32   ("uid",           Opt_uid),
>> +       fsparam_flag  ("inode32",       Opt_inode32),
>> +       fsparam_flag  ("inode64",       Opt_inode64),
>
>Ditto

Bleh, I'll fix this and send v3.

>>         {}
>>  };
>>
>> @@ -3690,7 +3727,8 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
>>  #endif
>>         uuid_gen(&sb->s_uuid);
>>
>> -       inode = shmem_get_inode(sb, NULL, S_IFDIR | sbinfo->mode, 0, VM_NORESERVE);
>> +       inode = shmem_get_inode(sb, NULL, S_IFDIR | sbinfo->mode, 0,
>> +                               VM_NORESERVE, true);
>
>Should usb_sb_ino be true for the kern_mount??
>In any case, it wouldn't matter if it was false, hence no need to pass
>an argument
>and can either check for sbinfo->max_inodes or the SB_KERNMOUNT flag in
>shmem_get_inode().

Ok, cool. I'll fix these up and send v3. Thanks!

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

[Amir Goldstein]

On Thu, Jan 2, 2020 at 8:50 PM Chris Down <chris@chrisdown.name> wrote:
>
> The default is still set to inode32 for backwards compatibility, but
> system administrators can opt in to the new 64-bit inode numbers by
> either:
>
> 1. Passing inode64 on the command line when mounting, or
> 2. Configuring the kernel with CONFIG_TMPFS_INODE64=y
>
> Signed-off-by: Chris Down <chris@chrisdown.name>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@fb.com
> ---
>  Documentation/filesystems/tmpfs.txt | 11 ++++++
>  fs/Kconfig                          | 18 +++++++++
>  include/linux/shmem_fs.h            |  1 +
>  mm/shmem.c                          | 57 ++++++++++++++++++++++++++++-
>  4 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/filesystems/tmpfs.txt b/Documentation/filesystems/tmpfs.txt
> index 5ecbc03e6b2f..203e12a684c9 100644
> --- a/Documentation/filesystems/tmpfs.txt
> +++ b/Documentation/filesystems/tmpfs.txt
> @@ -136,6 +136,15 @@ These options do not have any effect on remount. You can change these
>  parameters with chmod(1), chown(1) and chgrp(1) on a mounted filesystem.
>
>
> +tmpfs has a mount option to select whether it will wrap at 32- or 64-bit inode
> +numbers:
> +
> +inode64   Use 64-bit inode numbers
> +inode32   Use 32-bit inode numbers
> +
> +On 64-bit, the default is set by CONFIG_TMPFS_INODE64. On 32-bit, inode64 is
> +not legal and will produce an error at mount time.
> +
>  So 'mount -t tmpfs -o size=10G,nr_inodes=10k,mode=700 tmpfs /mytmpfs'
>  will give you tmpfs instance on /mytmpfs which can allocate 10GB
>  RAM/SWAP in 10240 inodes and it is only accessible by root.
> @@ -147,3 +156,5 @@ Updated:
>     Hugh Dickins, 4 June 2007
>  Updated:
>     KOSAKI Motohiro, 16 Mar 2010
> +Updated:
> +   Chris Down, 2 Jan 2020
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 7b623e9fc1b0..af2048ae71eb 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -199,6 +199,24 @@ config TMPFS_XATTR
>
>           If unsure, say N.
>
> +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.

> +
> +         To override this default, use the inode32 or inode64 mount options.
> +
> +         If unsure, say N.
> +
>  config HUGETLBFS
>         bool "HugeTLB file system support"
>         depends on X86 || IA64 || SPARC64 || (S390 && 64BIT) || \
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index dec4353cf3b7..0e645ecd6451 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -35,6 +35,7 @@ struct shmem_sb_info {
>         unsigned char huge;         /* Whether to try for hugepages */
>         kuid_t uid;                 /* Mount uid for root directory */
>         kgid_t gid;                 /* Mount gid for root directory */
> +       bool full_inums;            /* If i_ino should be uint or ino_t */
>         ino_t last_ino;             /* The last used per-sb inode number */
>         struct mempolicy *mpol;     /* default memory policy for mappings */
>         spinlock_t shrinklist_lock;   /* Protects shrinklist */
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 8af9fb922a96..fd2542e5ada9 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -115,11 +115,13 @@ struct shmem_options {
>         kuid_t uid;
>         kgid_t gid;
>         umode_t mode;
> +       bool full_inums;
>         int huge;
>         int seen;
>  #define SHMEM_SEEN_BLOCKS 1
>  #define SHMEM_SEEN_INODES 2
>  #define SHMEM_SEEN_HUGE 4
> +#define SHMEM_SEEN_INUMS 8
>  };
>
>  #ifdef CONFIG_TMPFS
> @@ -2260,7 +2262,8 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
>                 if (use_sb_ino) {
>                         spin_lock(&sbinfo->stat_lock);
>                         inode->i_ino = sbinfo->last_ino++;
> -                       if (unlikely(inode->i_ino >= UINT_MAX)) {
> +                       if (unlikely(!sbinfo->full_inums &&
> +                                    inode->i_ino >= UINT_MAX)) {
>                                 /*
>                                  * Emulate get_next_ino uint wraparound for
>                                  * compatibility
> @@ -2277,6 +2280,12 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
>                          * since max_inodes is always 0, and is called from
>                          * potentially unknown contexts. As such, use the global
>                          * allocator which doesn't require the per-sb stat_lock.
> +                        *
> +                        * No special behaviour is needed for
> +                        * sbinfo->full_inums, because it's not possible to
> +                        * manually set on callers of this type, and
> +                        * CONFIG_TMPFS_INODE64 only applies to user-visible
> +                        * mounts.
>                          */
>                         inode->i_ino = get_next_ino();
>                 }
> @@ -3450,6 +3459,7 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
>         unsigned long long size;
>         char *rest;
>         int opt;
> +       const char *err;
>
>         opt = fs_parse(fc, &shmem_fs_parameters, param, &result);
>         if (opt < 0)
> @@ -3511,6 +3521,18 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
>                         break;
>                 }
>                 goto unsupported_parameter;
> +       case Opt_inode32:
> +               ctx->full_inums = false;
> +               ctx->seen |= SHMEM_SEEN_INUMS;
> +               break;
> +       case Opt_inode64:
> +               if (sizeof(ino_t) < 8) {
> +                       err = "Cannot use inode64 with <64bit inums in kernel";
> +                       goto err_msg;
> +               }
> +               ctx->full_inums = true;
> +               ctx->seen |= SHMEM_SEEN_INUMS;
> +               break;
>         }
>         return 0;
>
> @@ -3518,6 +3540,8 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
>         return invalf(fc, "tmpfs: Unsupported parameter '%s'", param->key);
>  bad_value:
>         return invalf(fc, "tmpfs: Bad value for '%s'", param->key);
> +err_msg:
> +       return invalf(fc, "tmpfs: %s", err);
>  }
>
>  static int shmem_parse_options(struct fs_context *fc, void *data)
> @@ -3602,6 +3626,12 @@ static int shmem_reconfigure(struct fs_context *fc)
>                 }
>         }
>
> +       if ((ctx->seen & SHMEM_SEEN_INUMS) && !ctx->full_inums &&
> +           sbinfo->last_ino > UINT_MAX) {
> +               err = "Current inum too high to switch to 32-bit inums";
> +               goto out;
> +       }
> +
>         if (ctx->seen & SHMEM_SEEN_HUGE)
>                 sbinfo->huge = ctx->huge;
>         if (ctx->seen & SHMEM_SEEN_BLOCKS)
> @@ -3643,6 +3673,29 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
>         if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>                 seq_printf(seq, ",gid=%u",
>                                 from_kgid_munged(&init_user_ns, sbinfo->gid));
> +
> +       /*
> +        * 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)

> +               seq_printf(seq, ",inode%d", (sbinfo->full_inums ? 64 : 32));
>  #ifdef CONFIG_TRANSPARENT_HUGE_PAGECACHE
>         /* Rightly or wrongly, show huge mount option unmasked by shmem_huge */
>         if (sbinfo->huge)
> @@ -3702,6 +3755,7 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
>         sbinfo->free_inodes = sbinfo->max_inodes = ctx->inodes;
>         sbinfo->uid = ctx->uid;
>         sbinfo->gid = ctx->gid;
> +       sbinfo->full_inums = ctx->full_inums;
>         sbinfo->mode = ctx->mode;
>         sbinfo->huge = ctx->huge;
>         sbinfo->mpol = ctx->mpol;
> @@ -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.

Thanks,
Amir.

Re: [PATCH v2 1/2] tmpfs: Add per-superblock i_ino support

[Amir Goldstein]

On Thu, Jan 2, 2020 at 10:00 PM Chris Down <chris@chrisdown.name> wrote:
>
> Amir Goldstein writes:
> >Wouldn't it be easier to check max_inodes instead of passing this
> >use_sb_ino arg?
> >Is there any case where they *need* to differ?
>
> Hmm, I suppose probably not? In that case should I just check against
> SB_KERNMOUNT, since max_inodes can only be 0 in that case?
>

Yes, I think that would be best.

Thanks,
Amir.

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.