Message-ID: <20201030104823.GA19757@quack2.suse.cz>
Date:   Fri, 30 Oct 2020 11:48:23 +0100
From:   Jan Kara <jack@...e.cz>
To:     Waiman Long <longman@...hat.com>
Cc:     Amir Goldstein <amir73il@...il.com>, Jan Kara <jack@...e.cz>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Luca BRUNO <lucab@...hat.com>
Subject: Re: [PATCH v2] inotify: Increase default inotify.max_user_watches
 limit to 1048576

On Thu 29-10-20 15:04:56, Waiman Long wrote:
> On 10/29/20 2:46 PM, Amir Goldstein wrote:
> > On Thu, Oct 29, 2020 at 8:05 PM Waiman Long <longman@...hat.com> wrote:
> > > On 10/29/20 1:27 PM, Amir Goldstein wrote:
> > > > On Thu, Oct 29, 2020 at 5:46 PM Waiman Long <longman@...hat.com> wrote:
> > > > > The default value of inotify.max_user_watches sysctl parameter was set
> > > > > to 8192 since the introduction of the inotify feature in 2005 by
> > > > > commit 0eeca28300df ("[PATCH] inotify"). Today this value is just too
> > > > > small for many modern usage. As a result, users have to explicitly set
> > > > > it to a larger value to make it work.
> > > > > 
> > > > > After some searching around the web, these are the
> > > > > inotify.max_user_watches values used by some projects:
> > > > >    - vscode:  524288
> > > > >    - dropbox support: 100000
> > > > >    - users on stackexchange: 12228
> > > > >    - lsyncd user: 2000000
> > > > >    - code42 support: 1048576
> > > > >    - monodevelop: 16384
> > > > >    - tectonic: 524288
> > > > >    - openshift origin: 65536
> > > > > 
> > > > > Each watch point adds an inotify_inode_mark structure to an inode to
> > > > > be watched. It also pins the watched inode as well as an inotify fdinfo
> > > > > procfs file.
> > > > > 
> > > > > Modeled after the epoll.max_user_watches behavior to adjust the default
> > > > > value according to the amount of addressable memory available, make
> > > > > inotify.max_user_watches behave in a similar way to make it use no more
> > > > > than 1% of addressable memory within the range [8192, 1048576].
> > > > > 
> > > > > For 64-bit archs, inotify_inode_mark plus 2 inode have a size close
> > > > > to 2 kbytes. That means a system with 196GB or more memory should have
> > > > > the maximum value of 1048576 for inotify.max_user_watches. This default
> > > > > should be big enough for most use cases.
> > > > > 
> > > > > With my x86-64 config, the size of xfs_inode, proc_inode and
> > > > > inotify_inode_mark is 1680 bytes. The estimated INOTIFY_WATCH_COST is
> > > > > 1760 bytes.
> > > > > 
> > > > > [v2: increase inotify watch cost as suggested by Amir and Honza]
> > > > > 
> > > > > Signed-off-by: Waiman Long <longman@...hat.com>
> > > > > ---
> > > > >    fs/notify/inotify/inotify_user.c | 24 +++++++++++++++++++++++-
> > > > >    1 file changed, 23 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> > > > > index 186722ba3894..37d9f09c226f 100644
> > > > > --- a/fs/notify/inotify/inotify_user.c
> > > > > +++ b/fs/notify/inotify/inotify_user.c
> > > > > @@ -37,6 +37,16 @@
> > > > > 
> > > > >    #include <asm/ioctls.h>
> > > > > 
> > > > > +/*
> > > > > + * An inotify watch requires allocating an inotify_inode_mark structure as
> > > > > + * well as pinning the watched inode and adding inotify fdinfo procfs file.
> > > > Maybe you misunderstood me.
> > > > There is no procfs file per watch.
> > > > There is a procfs file per inotify_init() fd.
> > > > The fdinfo of that procfile lists all the watches of that inotify instance.
> > > Thanks for the clarification. Yes, I probably had misunderstood you
> > > because of the 2 * sizeof(inode) figure you provided.
> > > > > + * The increase in size of a filesystem inode versus a VFS inode varies
> > > > > + * depending on the filesystem. An extra 512 bytes is added as rough
> > > > > + * estimate of the additional filesystem inode cost.
> > > > > + */
> > > > > +#define INOTIFY_WATCH_COST     (sizeof(struct inotify_inode_mark) + \
> > > > > +                                2 * sizeof(struct inode) + 512)
> > > > > +
> > > > I would consider going with double the sizeof inode as rough approximation for
> > > > filesystem inode size.
> > > > 
> > > > It is a bit less arbitrary than 512 and it has some rationale behind it -
> > > > Some kernel config options will grow struct inode (debug, smp)
> > > > The same config options may also grow the filesystem part of the inode.
> > > > 
> > > > And this approximation can be pretty accurate at times.
> > > > For example, on Ubuntu 18.04 kernel 5.4.0:
> > > > inode_cache        608
> > > > nfs_inode_cache      1088
> > > > btrfs_inode            1168
> > > > xfs_inode              1024
> > > > ext4_inode_cache   1096
> > > Just to clarify, is your original 2 * sizeof(struct inode) figure
> > > include the filesystem inode overhead or there is an additional inode
> > > somewhere that I needs to go to 4 * sizeof(struct inode)?
> > No additional inode.
> > 
> > #define INOTIFY_WATCH_COST     (sizeof(struct inotify_inode_mark) + \
> >                                                        2 * sizeof(struct inode))
> > 
> > Not sure if the inotify_inode_mark part matters, but it doesn't hurt.
> > Do note that Jan had a different proposal for fs inode size estimation (1K).
> > I have no objection to this estimation if Jan insists.
> > 
> > Thanks,
> > Amir.
> > 
> Thanks for the confirmation. 2*sizeof(struct inode) is more than 1k. Besides
> with debugging turned on, the size will increase more. So that figure is
> good enough.

Yeah, the 2*sizeof(struct inode) is fine by me as well. Please don't forget
to update the comment explaining INOTIFY_WATCH_COST. Thanks!

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists