Skip to contentSkip to conversation

tmpfs: inode: Reduce risk of inum overflow

PATCH Superseded Chris Down 2 participants 5 messages Jul 13, 2020

Subsystems (4)

v6 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Chris Down

author
#0 lore raw

v6 cover tmpfs: inode: Reduce risk of inum overflow

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.

You can see how this compares to previous related patches which didn't implement this per-superblock:

- https://patchwork.kernel.org/patch/11254001/
- https://patchwork.kernel.org/patch/11023915/

Changes since v5 (it's been a while, eh?):

- Implement percpu batching for SB_KERNMOUNT at Hugh's suggestion.
- Hugh also pointed out that user-exposed tmpfs can also now have max_inodes == 0, so we have to account for that. I just use SB_KERNMOUNT to do that in all the places we used to look at max_inodes.

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

 Documentation/filesystems/tmpfs.rst |  11 +++
 fs/Kconfig                          |  15 ++++
 include/linux/fs.h                  |  15 ++++
 include/linux/shmem_fs.h            |   3 +
 mm/shmem.c                          | 127 ++++++++++++++++++++++++++--
 5 files changed, 166 insertions(+), 5 deletions(-)
Signature · 1 line
2.27.0
author
#1 lore raw

PATCH v6 1/2 tmpfs: Per-superblock i_ino support

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.

For internal shmem mounts which may be less tolerant to spinlock delays, we implement a percpu batching scheme which only takes the stat_lock at each batch boundary.

Cc Amir Goldstein <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Al Viro <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Jeff Layton <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Tejun Heo <[email protected]>
---
 include/linux/fs.h       | 15 +++++++++
 include/linux/shmem_fs.h |  2 ++
 mm/shmem.c               | 66 +++++++++++++++++++++++++++++++++++++---
 3 files changed, 78 insertions(+), 5 deletions(-)
include/linux/fs.h +15 -0
index f15848899945..b70b334f8e16 100644
@@ -2961,6 +2961,21 @@ extern void discard_new_inode(struct inode *);
 extern unsigned int get_next_ino(void);
 extern void evict_inodes(struct super_block *sb);
 
+/*
+ * Userspace may rely on the the inode number being non-zero. For example, glibc
+ * simply ignores files with zero i_ino in unlink() and other places.
+ *
+ * As an additional complication, if userspace was compiled with
+ * _FILE_OFFSET_BITS=32 on a 64-bit kernel we'll only end up reading out the
+ * lower 32 bits, so we need to check that those aren't zero explicitly. With
+ * _FILE_OFFSET_BITS=64, this may cause some harmless false-negatives, but
+ * better safe than sorry.
+ */
+static inline bool is_zero_ino(ino_t ino)
+{
+	return (u32)ino == 0;
+}
+
 extern void __iget(struct inode * inode);
 extern void iget_failed(struct inode *);
 extern void clear_inode(struct inode *);
include/linux/shmem_fs.h +2 -0
index 7a35a6901221..eb628696ec66 100644
@@ -36,6 +36,8 @@ 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 next_ino;		    /* The next per-sb inode number to use */
+	ino_t __percpu *ino_batch;  /* The next per-cpu inode number to use */
 	struct mempolicy *mpol;     /* default memory policy for mappings */
 	spinlock_t shrinklist_lock;   /* Protects shrinklist */
 	struct list_head shrinklist;  /* List of shinkable inodes */
mm/shmem.c +61 -5

111 lines not on this page · raw message

Signature · 1 line
2.27.0
#2 lore raw

Re: [PATCH v6 1/2] tmpfs: Per-superblock i_ino support

Quoted 1 line · Chris Down

+#define SHMEM_INO_BATCH 1024U

...

mm/shmem.c bool vma_is_shmem(struct vm_area_struct *vma) · line 311

Quoted 1 line
mm/shmem.c +1 -0
+		if (unlikely((ino & ~SHMEM_INO_BATCH) == 0)) {

I don't think that works. I think you meant to write ~(SHMEM_INO_BATCH - 1). Or just ino % SHMEM_INO_BATCH which works even for non-power-of-two.

author
#3 lore raw

Re: [PATCH v6 1/2] tmpfs: Per-superblock i_ino support

Quoted 2 lines · Matthew Wilcox

I don't think that works. I think you meant to write ~(SHMEM_INO_BATCH - 1). Or just ino % SHMEM_INO_BATCH which works even for non-power-of-two.

Er, right. I now wonder why I didn't just write ino % SHMEM_INO_BATCH :-)

I'll send again with that fix.

author
#4 lore raw

PATCH v6 2/2 tmpfs: Support 64-bit inums per-sb

In reply to 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

The inode64 and inode32 names are used based on existing precedent from XFS.

---
 Documentation/filesystems/tmpfs.rst | 11 +++++
 fs/Kconfig                          | 15 +++++++
 include/linux/shmem_fs.h            |  1 +
 mm/shmem.c                          | 65 ++++++++++++++++++++++++++++-
 4 files changed, 90 insertions(+), 2 deletions(-)
Documentation/filesystems/tmpfs.rst +11 -0
index 4e95929301a5..47b84ddaa8bb 100644
@@ -150,6 +150,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.
@@ -161,3 +170,5 @@ RAM/SWAP in 10240 inodes and it is only accessible by root.
    Hugh Dickins, 4 June 2007
 :Updated:
    KOSAKI Motohiro, 16 Mar 2010
+Updated:
+   Chris Down, 13 July 2020
fs/Kconfig +15 -0
index ff257b81fde5..64d530ba42f6 100644
@@ -229,6 +229,21 @@ 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.
+
+	  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) || \
include/linux/shmem_fs.h +1 -0
index eb628696ec66..a5a5d1d4d7b1 100644
@@ -36,6 +36,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 next_ino;		    /* The next per-sb inode number to use */
 	ino_t __percpu *ino_batch;  /* The next per-cpu inode number to use */
 	struct mempolicy *mpol;     /* default memory policy for mappings */
mm/shmem.c +63 -2

136 lines not on this page · raw message

Signature · 1 line
2.27.0