[PATCH] generic/084: check inotify limit before tail many files

[PATCH] generic/084: check inotify limit before tail many files

[Zorro Lang]

generic/084 try to run 'tail' command, tail will use
inotify, and there're some limit about inotify. I think
the most important is fs.inotify.max_user_instances, then
fs.inotify.max_user_watches is importand too.

When I test on a machine with 154 cpu cores, this case
run failed, and hit many warning likes:

    tail: inotify cannot be used, reverting to polling: Too many
    open files

Because the fs.inotify.max_user_instances is 128, so if
we try to tail 154 files, it will be failed.

Of course, open files limit will effect this too. But generally
max_user_instances is the minimum number, so I don't check open
file limit.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 tests/generic/084 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tests/generic/084 b/tests/generic/084
index 3fec6c2..f34d4b2 100755
--- a/tests/generic/084
+++ b/tests/generic/084
@@ -63,6 +63,15 @@ link_unlink_storm()
 
 rm -f $seqres.full
 nr_cpu=`$here/src/feature -o`
+user_watches=`sysctl -n fs.inotify.max_user_watches`
+user_instances=`sysctl -n fs.inotify.max_user_instances`
+
+user_limit=$((user_watches > user_instances ? user_instances : user_watches))
+if [ $nr_cpu -ge $((user_limit/2)) ]
+then
+	nr_cpu=$((user_limit/2))
+fi
+
 echo "Silence is golden"
 
 _scratch_mkfs >>$seqres.full 2>&1
-- 
1.9.3

Re: [PATCH] generic/084: check inotify limit before tail many files

[Eryu Guan]

On Fri, Aug 14, 2015 at 12:16:32AM +0800, Zorro Lang wrote:
> generic/084 try to run 'tail' command, tail will use
> inotify, and there're some limit about inotify. I think
> the most important is fs.inotify.max_user_instances, then
> fs.inotify.max_user_watches is importand too.

max_user_watches is much larger (usually 8k?) than max_user_instances
(128), I think we can skip the max_user_watches check here.

> 
> When I test on a machine with 154 cpu cores, this case
> run failed, and hit many warning likes:
> 
>     tail: inotify cannot be used, reverting to polling: Too many
>     open files

No need to wrap lines for quoted logs.

> 
> Because the fs.inotify.max_user_instances is 128, so if
> we try to tail 154 files, it will be failed.
> 
> Of course, open files limit will effect this too. But generally
> max_user_instances is the minimum number, so I don't check open
> file limit.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
>  tests/generic/084 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tests/generic/084 b/tests/generic/084
> index 3fec6c2..f34d4b2 100755
> --- a/tests/generic/084
> +++ b/tests/generic/084
> @@ -63,6 +63,15 @@ link_unlink_storm()
>  
>  rm -f $seqres.full
>  nr_cpu=`$here/src/feature -o`
> +user_watches=`sysctl -n fs.inotify.max_user_watches`
> +user_instances=`sysctl -n fs.inotify.max_user_instances`

I think we need some comments in the code too.

> +
> +user_limit=$((user_watches > user_instances ? user_instances : user_watches))

Please use plain if:then

> +if [ $nr_cpu -ge $((user_limit/2)) ]
> +then

if []; then
fi

this is the preferred format in xfstests

> +	nr_cpu=$((user_limit/2))

Need comments to explain why divide by 2.

Thanks,
Eryu
> +fi
> +
>  echo "Silence is golden"
>  
>  _scratch_mkfs >>$seqres.full 2>&1
> -- 
> 1.9.3
> 

Re: [PATCH] generic/084: check inotify limit before tail many files

[Dave Chinner]

On Fri, Aug 14, 2015 at 12:16:32AM +0800, Zorro Lang wrote:
> generic/084 try to run 'tail' command, tail will use
> inotify, and there're some limit about inotify. I think
> the most important is fs.inotify.max_user_instances, then
> fs.inotify.max_user_watches is importand too.
>
> When I test on a machine with 154 cpu cores, this case
> run failed, and hit many warning likes:
> 
>     tail: inotify cannot be used, reverting to polling: Too many
>     open files
> 
> Because the fs.inotify.max_user_instances is 128, so if
> we try to tail 154 files, it will be failed.

We use 'tail' all over the place in xfstests, so why is only
generic/084 affected?

And really, this seems more like a distro/environment bug and
doesn't need xfstests help to work around. i.e. changing the
sysctl before starting xfstests seems much more appropriate than
hacking it a random test. Especially as there may be more than one
test that is affected by this, and when run in a random order this
would cause those other tests to pass/fail depending on whether
generic/084 had already been run on that machine....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic/084: check inotify limit before tail many files

[Zirong Lang]

Hi Dave,

Thanks for your reply.

----- 原始邮件 -----
> 发件人: "Dave Chinner" <david@fromorbit.com>
> 收件人: "Zorro Lang" <zlang@redhat.com>
> 抄送: fstests@vger.kernel.org, eguan@redhat.com
> 发送时间: 星期一, 2015年 8 月 17日 上午 8:03:36
> 主题: Re: [PATCH] generic/084: check inotify limit before tail many files
> 
> On Fri, Aug 14, 2015 at 12:16:32AM +0800, Zorro Lang wrote:
> > generic/084 try to run 'tail' command, tail will use
> > inotify, and there're some limit about inotify. I think
> > the most important is fs.inotify.max_user_instances, then
> > fs.inotify.max_user_watches is importand too.
> >
> > When I test on a machine with 154 cpu cores, this case
> > run failed, and hit many warning likes:
> > 
> >     tail: inotify cannot be used, reverting to polling: Too many
> >     open files
> > 
> > Because the fs.inotify.max_user_instances is 128, so if
> > we try to tail 154 files, it will be failed.
> 
> We use 'tail' all over the place in xfstests, so why is only
> generic/084 affected?

Because generic/084 use try to create $nr_cpu tail processes:
for i in `seq 1 $nr_cpu`; do
    ...
    tail -f $testfile &
    ...
done

And nr_cpu=`$here/src/feature -o`.

Generally fs.inotify.max_user_instances is 128, when a machine
have more than(or nearly the same) this number, this test will
failed.

Maybe other cases don't try to create so many tail processes, so
they passed.

> 
> And really, this seems more like a distro/environment bug and
> doesn't need xfstests help to work around. i.e. changing the
> sysctl before starting xfstests seems much more appropriate than
> hacking it a random test. Especially as there may be more than one
> test that is affected by this, and when run in a random order this
> would cause those other tests to pass/fail depending on whether
> generic/084 had already been run on that machine....

As this situation generally won't effect other cases, so I try to
make the patch don't effect more other cases. Because I don't know
if someone try to test fs.inotify...

But if maintainer think it'll be better, I will change the patch:)

Thanks,
Zorro Lang

> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH] generic/084: check inotify limit before tail many files

[Dave Chinner]

On Sun, Aug 16, 2015 at 09:05:19PM -0400, Zirong Lang wrote:
> Hi Dave,
> 
> Thanks for your reply.
> 
> ----- 原始邮件 -----
> > 发件人: "Dave Chinner" <david@fromorbit.com>
> > 收件人: "Zorro Lang" <zlang@redhat.com>
> > 抄送: fstests@vger.kernel.org, eguan@redhat.com
> > 发送时间: 星期一, 2015年 8 月 17日 上午 8:03:36
> > 主题: Re: [PATCH] generic/084: check inotify limit before tail many files
> > 
> > On Fri, Aug 14, 2015 at 12:16:32AM +0800, Zorro Lang wrote:
> > > generic/084 try to run 'tail' command, tail will use
> > > inotify, and there're some limit about inotify. I think
> > > the most important is fs.inotify.max_user_instances, then
> > > fs.inotify.max_user_watches is importand too.
> > >
> > > When I test on a machine with 154 cpu cores, this case
> > > run failed, and hit many warning likes:
> > > 
> > >     tail: inotify cannot be used, reverting to polling: Too many
> > >     open files
> > > 
> > > Because the fs.inotify.max_user_instances is 128, so if
> > > we try to tail 154 files, it will be failed.
> > 
> > We use 'tail' all over the place in xfstests, so why is only
> > generic/084 affected?
> 
> Because generic/084 use try to create $nr_cpu tail processes:
> for i in `seq 1 $nr_cpu`; do
>     ...
>     tail -f $testfile &
>     ...
> done
> 
> And nr_cpu=`$here/src/feature -o`.
> 
> Generally fs.inotify.max_user_instances is 128, when a machine
> have more than(or nearly the same) this number, this test will
> failed.

This information should have been in the patch description - it's
concurrently run tail commands that are the problem, not a single
execution of tail...

> Maybe other cases don't try to create so many tail processes, so
> they passed.

Exactly. The answer is obvious when you explain it fully :)

So, we have other tests that use hundreds of open unlinked files and
they don't have this problem. That means the issue is how
generic/084 is creating the unlinked files, not an issue with
inotify config.

Go look at src/multi_open_unlink.c and tests/xfs/1[28]1, and then
rewrite generic/084 to use multi_open_unlink to create and hold 
open unlinked files for a specified amount of time.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic/084: check inotify limit before tail many files

[Zirong Lang]

----- 原始邮件 -----
> 发件人: "Dave Chinner" <david@fromorbit.com>
> 收件人: "Zirong Lang" <zlang@redhat.com>
> 抄送: fstests@vger.kernel.org, eguan@redhat.com
> 发送时间: 星期一, 2015年 8 月 17日 下午 1:06:12
> 主题: Re: [PATCH] generic/084: check inotify limit before tail many files
> 
> On Sun, Aug 16, 2015 at 09:05:19PM -0400, Zirong Lang wrote:
> > Hi Dave,
> > 
> > Thanks for your reply.
> > 
> > ----- 原始邮件 -----
> > > 发件人: "Dave Chinner" <david@fromorbit.com>
> > > 收件人: "Zorro Lang" <zlang@redhat.com>
> > > 抄送: fstests@vger.kernel.org, eguan@redhat.com
> > > 发送时间: 星期一, 2015年 8 月 17日 上午 8:03:36
> > > 主题: Re: [PATCH] generic/084: check inotify limit before tail many files
> > > 
> > > On Fri, Aug 14, 2015 at 12:16:32AM +0800, Zorro Lang wrote:
> > > > generic/084 try to run 'tail' command, tail will use
> > > > inotify, and there're some limit about inotify. I think
> > > > the most important is fs.inotify.max_user_instances, then
> > > > fs.inotify.max_user_watches is importand too.
> > > >
> > > > When I test on a machine with 154 cpu cores, this case
> > > > run failed, and hit many warning likes:
> > > > 
> > > >     tail: inotify cannot be used, reverting to polling: Too many
> > > >     open files
> > > > 
> > > > Because the fs.inotify.max_user_instances is 128, so if
> > > > we try to tail 154 files, it will be failed.
> > > 
> > > We use 'tail' all over the place in xfstests, so why is only
> > > generic/084 affected?
> > 
> > Because generic/084 use try to create $nr_cpu tail processes:
> > for i in `seq 1 $nr_cpu`; do
> >     ...
> >     tail -f $testfile &
> >     ...
> > done
> > 
> > And nr_cpu=`$here/src/feature -o`.
> > 
> > Generally fs.inotify.max_user_instances is 128, when a machine
> > have more than(or nearly the same) this number, this test will
> > failed.
> 
> This information should have been in the patch description - it's
> concurrently run tail commands that are the problem, not a single
> execution of tail...
> 
> > Maybe other cases don't try to create so many tail processes, so
> > they passed.
> 
> Exactly. The answer is obvious when you explain it fully :)
> 
> So, we have other tests that use hundreds of open unlinked files and
> they don't have this problem. That means the issue is how
> generic/084 is creating the unlinked files, not an issue with
> inotify config.
> 
> Go look at src/multi_open_unlink.c and tests/xfs/1[28]1, and then
> rewrite generic/084 to use multi_open_unlink to create and hold
> open unlinked files for a specified amount of time.

You're Great! This's a good idea to fix this problem. Thanks very much.
I have sent the V3 patch, and tested on an old kernel to sure
the bug still can be reproduced after I changed.

Thanks,
Zorro Lang

> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>