Message-ID: <53f56207-28d8-5a87-af3d-f6c7a40a1907@redhat.com>
Date:   Mon, 24 Jul 2017 11:54:44 -0400
From:   Waiman Long <longman@...hat.com>
To:     James Bottomley <James.Bottomley@...senPartnership.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Jonathan Corbet <corbet@....net>
Cc:     linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        Miklos Szeredi <mszeredi@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Larry Woodman <lwoodman@...hat.com>
Subject: Re: [PATCH v2 3/4] fs/dcache: Enable automatic pruning of negative
 dentries

On 07/21/2017 07:07 PM, James Bottomley wrote:
> On Fri, 2017-07-21 at 16:17 -0400, Waiman Long wrote:
>> On 07/21/2017 03:30 PM, James Bottomley wrote:
>>> On Fri, 2017-07-21 at 09:43 -0400, Waiman Long wrote:
>>>> Having a limit for the number of negative dentries does have an
>>>> undesirable side effect that no new negative dentries will be
>>>> allowed when the limit is reached. This will have performance
>>>> implication for some types of workloads.
>>> This really seems like a significant problem: negative dentries
>>> should be released in strict lru order because the chances are no-
>>> one cares about the least recently used one, but they may care
>>> about having the most recently created one.
>> This should not happen under normal circumstances as the asynchronous
>> shrinker should be able to keep enough free negative dentry available
>> in the pool that direct negative dentry killing will rarely happen.
> But that's an argument for not bothering with the patch set at all: if
> it's a corner case that rarely occurs.
>
> Perhaps we should start with why the series is important and then get
> back to what we do if the condition is hit?

There is concern that unlimited negative dentry growth can be used as a
kind of DoS attack by robbing system of all its memory and triggering
massive memory reclaim or even OOM kill. This patchset is aimed to
prevent this kind of worst case situation from happening.

>>> [...]
>>>> @@ -323,6 +329,16 @@ static void __neg_dentry_inc(struct dentry
>>>> *dentry)
>>>>  	 */
>>>>  	if (!cnt)
>>>>  		dentry->d_flags |= DCACHE_KILL_NEGATIVE;
>>>> +
>>>> +	/*
>>>> +	 * Initiate negative dentry pruning if free pool has
>>>> less
>>>> than
>>>> +	 * 1/4 of its initial value.
>>>> +	 */
>>>> +	if (READ_ONCE(ndblk.nfree) < neg_dentry_nfree_init/4) {
>>>> +		WRITE_ONCE(ndblk.prune_sb, dentry->d_sb);
>>>> +		schedule_delayed_work(&prune_neg_dentry_work,
>>>> +				      NEG_PRUNING_DELAY);
>>>> +	}
>>> So here, why not run the negative dentry shrinker synchronously to
>>> see if we can shrink the cache and avoid killing the current
>>> negative dentry.  If there are context problems doing that, we
>>> should at least make the effort to track down the least recently
>>> used negative dentry and mark that for killing instead.
>> Only one CPU will be calling the asynchronous shrinker. So its effect
>> on the overall performance of the system should be negligible.
>>
>> Allowing all CPUs to potentially do synchronous shrinking can cause a
>> lot of lock and cacheline contention.
> Does that matter on something you thought was a rare occurrence above?
>  Plus, if the only use case is malicious user, as you say below, then
> they get to pay the biggest overhead penalty because they're the ones
> trying to create negative dentries.
>
> Perhaps if the threat model truly is malicious users creating negative
> dentries then a better fix is to add a delay penalty to true negative
> dentry creation (say short msleep) when we get into this situation (if
> you only pay the penalty on true negative dentry creation, not negative
> promotes to positive, then we'll mostly penalise the process trying to
> be malicious).

I can insert a delay loop before killing the negative dentry. Taking a
msleep can be problematic as the callers of dput() may not be in a
sleepable state.

>>  I will look further to see if there is opportunity to do some
>> optimistic synchronous shrinking. If that fails because of a
>> contended lock, for example, we will need to fall back to killing the
>> dentry. That should only happen under the worst case situation, like
>> when a malicious process is running.
> Right, so I can force this by doing a whole load of non existent file
> lookups then what happens:  negative dentries stop working for everyone
> because they're killed as soon as they're created.  Negative dentries
> are useful performance enhancements for things like the usual does
> x.lock exist, if not modify x things that applications do.

I am well aware of the performance benefit offered by negative dentries.
That is why I added this patch to prune the LRU list before it is too
late and is forced to kill negative dentries. However, negative dentry
killing may still happen if the negative dentry generation rate is
faster than the pruning rate.
This can be caused by bugs in the applications or malicious intent. If
this happens, it is likely that negative dentries will consume most of
the system memory without this patchset. Application performance will
suffer somewhat, but it will not as bad as when most of the memory are
consumed by negative dentries.

> It also looks like the dentry never loses DCACHE_KILL_NEGATIVE, so if
> I'm creating the x.lock file, and we're in this situation, it gets a
> positive dentry with the DCACHE_KILL_NEGATIVE flag set (because we
> start the lookup finding a negative dentry which gets the flag and then
> promote it to positive).

Thanks for pointing this out. I am going to fix this bug in my patch.

Cheers,
Longman

Powered by blists - more mailing lists