Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1692177 > unrolled thread

Re: [PATCH 1/4] fs/dcache: Limit numbers of negative dentries

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/4] fs/dcache: Limit numbers of negative dentries Miklos Szeredi <mszeredi@redhat.com> - 2017-07-19 22:30 +0200
    Re: [PATCH 1/4] fs/dcache: Limit numbers of negative dentries Waiman Long <longman@redhat.com> - 2017-07-19 22:50 +0200
      Re: [PATCH 1/4] fs/dcache: Limit numbers of negative dentries Miklos Szeredi <mszeredi@redhat.com> - 2017-07-20 09:30 +0200
        Re: [PATCH 1/4] fs/dcache: Limit numbers of negative dentries Waiman Long <longman@redhat.com> - 2017-07-20 16:30 +0200
          Re: [PATCH 1/4] fs/dcache: Limit numbers of negative dentries Miklos Szeredi <mszeredi@redhat.com> - 2017-07-20 17:10 +0200
            Re: [PATCH 1/4] fs/dcache: Limit numbers of negative dentries Waiman Long <longman@redhat.com> - 2017-07-20 17:50 +0200

#1692177 — Re: [PATCH 1/4] fs/dcache: Limit numbers of negative dentries

On Mon, Jul 17, 2017 at 3:39 PM, Waiman Long <longman@redhat.com> wrote:
> The number of positive dentries is limited by the number of files
> in the filesystems. The number of negative dentries, however,
> has no limit other than the total amount of memory available in
> the system. So a rogue application that generates a lot of negative
> dentries can potentially exhaust most of the memory available in the
> system impacting performance on other running applications.
>
> To prevent this from happening, the dcache code is now updated to limit
> the amount of the negative dentries in the LRU lists that can be kept
> as a percentage of total available system memory. The default is 5%
> and can be changed by specifying the "neg_dentry_pc=" kernel command
> line option.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---

[...]

> @@ -603,7 +698,13 @@ static struct dentry *dentry_kill(struct dentry *dentry)
>
>         if (!IS_ROOT(dentry)) {
>                 parent = dentry->d_parent;
> -               if (unlikely(!spin_trylock(&parent->d_lock))) {
> +               /*
> +                * Force the killing of this negative dentry when
> +                * DCACHE_KILL_NEGATIVE flag is set.
> +                */
> +               if (unlikely(dentry->d_flags & DCACHE_KILL_NEGATIVE)) {
> +                       spin_lock(&parent->d_lock);

This looks like d_lock ordering problem (should be parent first, child
second).  Why is this needed, anyway?

> +               } else if (unlikely(!spin_trylock(&parent->d_lock))) {
>                         if (inode)
>                                 spin_unlock(&inode->i_lock);
>                         goto failed;

Thanks,
Miklos

[toc] | [next] | [standalone]

#1692192

On 07/19/2017 04:24 PM, Miklos Szeredi wrote:
> On Mon, Jul 17, 2017 at 3:39 PM, Waiman Long <longman@redhat.com> wrote:
>> The number of positive dentries is limited by the number of files
>> in the filesystems. The number of negative dentries, however,
>> has no limit other than the total amount of memory available in
>> the system. So a rogue application that generates a lot of negative
>> dentries can potentially exhaust most of the memory available in the
>> system impacting performance on other running applications.
>>
>> To prevent this from happening, the dcache code is now updated to limit
>> the amount of the negative dentries in the LRU lists that can be kept
>> as a percentage of total available system memory. The default is 5%
>> and can be changed by specifying the "neg_dentry_pc=" kernel command
>> line option.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
> [...]
>
>> @@ -603,7 +698,13 @@ static struct dentry *dentry_kill(struct dentry *dentry)
>>
>>         if (!IS_ROOT(dentry)) {
>>                 parent = dentry->d_parent;
>> -               if (unlikely(!spin_trylock(&parent->d_lock))) {
>> +               /*
>> +                * Force the killing of this negative dentry when
>> +                * DCACHE_KILL_NEGATIVE flag is set.
>> +                */
>> +               if (unlikely(dentry->d_flags & DCACHE_KILL_NEGATIVE)) {
>> +                       spin_lock(&parent->d_lock);
> This looks like d_lock ordering problem (should be parent first, child
> second).  Why is this needed, anyway?
>

Yes, that is a bug. I should have used lock_parent() instead.

I have a test program that generate a lot of negative dentries
continuously. Using spin_trylock(), it failed most of the time when that
test program was running. So I need to actually acquire the parent's
d_lock to make sure that the offending negative dentry was really
killed. It was there to protect against the worst case situation. I will
update the patch to correct that.

Thanks for spotting this.

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1692491

On Wed, Jul 19, 2017 at 10:42 PM, Waiman Long <longman@redhat.com> wrote:
> On 07/19/2017 04:24 PM, Miklos Szeredi wrote:
>> On Mon, Jul 17, 2017 at 3:39 PM, Waiman Long <longman@redhat.com> wrote:
>>> The number of positive dentries is limited by the number of files
>>> in the filesystems. The number of negative dentries, however,
>>> has no limit other than the total amount of memory available in
>>> the system. So a rogue application that generates a lot of negative
>>> dentries can potentially exhaust most of the memory available in the
>>> system impacting performance on other running applications.
>>>
>>> To prevent this from happening, the dcache code is now updated to limit
>>> the amount of the negative dentries in the LRU lists that can be kept
>>> as a percentage of total available system memory. The default is 5%
>>> and can be changed by specifying the "neg_dentry_pc=" kernel command
>>> line option.
>>>
>>> Signed-off-by: Waiman Long <longman@redhat.com>
>>> ---
>> [...]
>>
>>> @@ -603,7 +698,13 @@ static struct dentry *dentry_kill(struct dentry *dentry)
>>>
>>>         if (!IS_ROOT(dentry)) {
>>>                 parent = dentry->d_parent;
>>> -               if (unlikely(!spin_trylock(&parent->d_lock))) {
>>> +               /*
>>> +                * Force the killing of this negative dentry when
>>> +                * DCACHE_KILL_NEGATIVE flag is set.
>>> +                */
>>> +               if (unlikely(dentry->d_flags & DCACHE_KILL_NEGATIVE)) {
>>> +                       spin_lock(&parent->d_lock);
>> This looks like d_lock ordering problem (should be parent first, child
>> second).  Why is this needed, anyway?
>>
>
> Yes, that is a bug. I should have used lock_parent() instead.

lock_parent() can release dentry->d_lock, which means it's perfectly
useless for this.

I still feel forcing  free is wrong here.  Why not just block until
the number of negatives goes below the limit (start reclaim if not
already doing so, etc...)?

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1692976

On 07/20/2017 03:20 AM, Miklos Szeredi wrote:
> On Wed, Jul 19, 2017 at 10:42 PM, Waiman Long <longman@redhat.com> wrote:
>>
>>>> @@ -603,7 +698,13 @@ static struct dentry *dentry_kill(struct dentry *dentry)
>>>>
>>>>         if (!IS_ROOT(dentry)) {
>>>>                 parent = dentry->d_parent;
>>>> -               if (unlikely(!spin_trylock(&parent->d_lock))) {
>>>> +               /*
>>>> +                * Force the killing of this negative dentry when
>>>> +                * DCACHE_KILL_NEGATIVE flag is set.
>>>> +                */
>>>> +               if (unlikely(dentry->d_flags & DCACHE_KILL_NEGATIVE)) {
>>>> +                       spin_lock(&parent->d_lock);
>>> This looks like d_lock ordering problem (should be parent first, child
>>> second).  Why is this needed, anyway?
>>>
>> Yes, that is a bug. I should have used lock_parent() instead.
> lock_parent() can release dentry->d_lock, which means it's perfectly
> useless for this.

As the reference count is kept at 1 in dentry_kill(), the dentry won't
go away even if the dentry lock is temporarily released.

> I still feel forcing  free is wrong here.  Why not just block until
> the number of negatives goes below the limit (start reclaim if not
> already doing so, etc...)?

Force freeing is the simplest. Any other ways will require adding more
code and increasing code complexity.

One reason why I prefer this is to avoid adding unpredictable latency to
the regular directory lookup and other dentry related operations. We can
always change the code later on if there is a better way of doing it.

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1693012

On Thu, Jul 20, 2017 at 4:21 PM, Waiman Long <longman@redhat.com> wrote:
> On 07/20/2017 03:20 AM, Miklos Szeredi wrote:
>> On Wed, Jul 19, 2017 at 10:42 PM, Waiman Long <longman@redhat.com> wrote:
>>>
>>>>> @@ -603,7 +698,13 @@ static struct dentry *dentry_kill(struct dentry *dentry)
>>>>>
>>>>>         if (!IS_ROOT(dentry)) {
>>>>>                 parent = dentry->d_parent;
>>>>> -               if (unlikely(!spin_trylock(&parent->d_lock))) {
>>>>> +               /*
>>>>> +                * Force the killing of this negative dentry when
>>>>> +                * DCACHE_KILL_NEGATIVE flag is set.
>>>>> +                */
>>>>> +               if (unlikely(dentry->d_flags & DCACHE_KILL_NEGATIVE)) {
>>>>> +                       spin_lock(&parent->d_lock);
>>>> This looks like d_lock ordering problem (should be parent first, child
>>>> second).  Why is this needed, anyway?
>>>>
>>> Yes, that is a bug. I should have used lock_parent() instead.
>> lock_parent() can release dentry->d_lock, which means it's perfectly
>> useless for this.
>
> As the reference count is kept at 1 in dentry_kill(), the dentry won't
> go away even if the dentry lock is temporarily released.

It won't go away, but anything else might happen to it (ref grabbed by
somebody else, instantiated, etc).  Don't see how it's going to be
better than the existing trylock.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1693042

On 07/20/2017 11:08 AM, Miklos Szeredi wrote:
> On Thu, Jul 20, 2017 at 4:21 PM, Waiman Long <longman@redhat.com> wrote:
>> On 07/20/2017 03:20 AM, Miklos Szeredi wrote:
>>> On Wed, Jul 19, 2017 at 10:42 PM, Waiman Long <longman@redhat.com> wrote:
>>>>>> @@ -603,7 +698,13 @@ static struct dentry *dentry_kill(struct dentry *dentry)
>>>>>>
>>>>>>         if (!IS_ROOT(dentry)) {
>>>>>>                 parent = dentry->d_parent;
>>>>>> -               if (unlikely(!spin_trylock(&parent->d_lock))) {
>>>>>> +               /*
>>>>>> +                * Force the killing of this negative dentry when
>>>>>> +                * DCACHE_KILL_NEGATIVE flag is set.
>>>>>> +                */
>>>>>> +               if (unlikely(dentry->d_flags & DCACHE_KILL_NEGATIVE)) {
>>>>>> +                       spin_lock(&parent->d_lock);
>>>>> This looks like d_lock ordering problem (should be parent first, child
>>>>> second).  Why is this needed, anyway?
>>>>>
>>>> Yes, that is a bug. I should have used lock_parent() instead.
>>> lock_parent() can release dentry->d_lock, which means it's perfectly
>>> useless for this.
>> As the reference count is kept at 1 in dentry_kill(), the dentry won't
>> go away even if the dentry lock is temporarily released.
> It won't go away, but anything else might happen to it (ref grabbed by
> somebody else, instantiated, etc).  Don't see how it's going to be
> better than the existing trylock.
>
> Thanks,
> Miklos

In the unlikely event that the reference count or the d_flags changes,
we can abort the killing.

Cheers,
Longman

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web