linux-kernel - Re: [PATCH v5 1/2] tmpfs: Add per-superblock i

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 6 Jan 2020 08:41:03 +0200
From:   Amir Goldstein <amir73il@...il.com>
To:     "zhengbin (A)" <zhengbin13@...wei.com>
Cc:     Chris Down <chris@...isdown.name>, Linux MM <linux-mm@...ck.org>,
        Hugh Dickins <hughd@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Matthew Wilcox <willy@...radead.org>,
        Jeff Layton <jlayton@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Tejun Heo <tj@...nel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>, kernel-team@...com
Subject: Re: [PATCH v5 1/2] tmpfs: Add per-superblock i_ino support

On Mon, Jan 6, 2020 at 4:04 AM zhengbin (A) <zhengbin13@...wei.com> wrote:
>
>
> On 2020/1/5 20:06, Chris Down 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@...isdown.name>
> > Reviewed-by: Amir Goldstein <amir73il@...il.com>
> > Cc: Hugh Dickins <hughd@...gle.com>
> > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > Cc: Al Viro <viro@...iv.linux.org.uk>
> > Cc: Matthew Wilcox <willy@...radead.org>
> > Cc: Jeff Layton <jlayton@...nel.org>
> > Cc: Johannes Weiner <hannes@...xchg.org>
> > Cc: Tejun Heo <tj@...nel.org>
> > Cc: linux-mm@...ck.org
> > Cc: linux-fsdevel@...r.kernel.org
> > Cc: linux-kernel@...r.kernel.org
> > Cc: kernel-team@...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.