On Mon, Jan 6, 2020 at 4:04 AM zhengbin (A) [off-list ref] wrote:

On 2020/1/5 20:06, Chris Down wrote:

quoted

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>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: linux-mm@kvack.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               | 30 +++++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

v5: Nothing in code, just resending with correct linux-mm domain.

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index de8e4b71e3ba..7fac91f490dc 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 next_ino;             /* The next per-sb 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 */

diff --git a/mm/shmem.c b/mm/shmem.c
index 8793e8cc1a48..9e97ba972225 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c

@@ -2236,6 +2236,12 @@ 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 this tmpfs is from kern_mount we use get_next_ino, which is global, since
+ * inum churn there is low and this avoids taking locks.
+ */
 static struct inode *shmem_get_inode(struct super_block *sb, const struct inode *dir,
                                   umode_t mode, dev_t dev, unsigned long flags)
 {

@@ -2248,7 +2254,28 @@ 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 (sb->s_flags & SB_KERNMOUNT) {
+                     /*
+                      * __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();
+             } else {
+                     spin_lock(&sbinfo->stat_lock);

Use spin_lock will affect performance, how about define

unsigned long __percpu *last_ino_number; /* Last inode number */
atomic64_t shared_last_ino_number; /* Shared last inode number */
in shmem_sb_info, whose performance will be better?

Please take a look at shmem_reserve_inode().
spin lock is already being taken in shmem_get_inode()
so there is nothing to be gained from complicating next_ino counter.

This fact would have been a lot clearer if next_ino was incremented
inside shmem_reserve_inode() and its value returned to be used by
shmem_get_inode(), but I am also fine with code as it is with the
comment above.

Thanks,
Amir.