| Message ID | 1500644590-6599-4-git-send-email-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. [...] > @@ -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. James
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. > [...] >> @@ -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. 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. Cheers, Longman
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? > > > > [...] > > > > > > @@ -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 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. 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). James
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
diff --git a/fs/dcache.c b/fs/dcache.c index d6d2d2d..c2ea876 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -134,13 +134,19 @@ struct dentry_stat_t dentry_stat = { * Macros and variables to manage and count negative dentries. */ #define NEG_DENTRY_BATCH (1 << 8) +#define NEG_PRUNING_DELAY (HZ/10) static long neg_dentry_percpu_limit __read_mostly; static long neg_dentry_nfree_init __read_mostly; /* Free pool initial value */ static struct { raw_spinlock_t nfree_lock; long nfree; /* Negative dentry free pool */ + struct super_block *prune_sb; /* Super_block for pruning */ + int neg_count, prune_count; /* Pruning counts */ } ndblk ____cacheline_aligned_in_smp; +static void prune_negative_dentry(struct work_struct *work); +static DECLARE_DELAYED_WORK(prune_neg_dentry_work, prune_negative_dentry); + static DEFINE_PER_CPU(long, nr_dentry); static DEFINE_PER_CPU(long, nr_dentry_unused); static DEFINE_PER_CPU(long, nr_dentry_neg); @@ -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); + } } static inline void neg_dentry_inc(struct dentry *dentry) @@ -1320,6 +1336,103 @@ void shrink_dcache_sb(struct super_block *sb) } EXPORT_SYMBOL(shrink_dcache_sb); +/* + * A modified version that attempts to remove a limited number of negative + * dentries as well as some other non-negative dentries at the front. + */ +static enum lru_status dentry_negative_lru_isolate(struct list_head *item, + struct list_lru_one *lru, spinlock_t *lru_lock, void *arg) +{ + struct list_head *freeable = arg; + struct dentry *dentry = container_of(item, struct dentry, d_lru); + enum lru_status status = LRU_SKIP; + + /* + * Stop further list walking for the current node list to limit + * performance impact, but allow further walking in the next node + * list. + */ + if ((ndblk.neg_count >= NEG_DENTRY_BATCH) || + (ndblk.prune_count >= NEG_DENTRY_BATCH)) { + ndblk.prune_count = 0; + return LRU_STOP; + } + + /* + * we are inverting the lru lock/dentry->d_lock here, + * so use a trylock. If we fail to get the lock, just skip + * it + */ + if (!spin_trylock(&dentry->d_lock)) { + ndblk.prune_count++; + return LRU_SKIP; + } + + /* + * Referenced dentries are still in use. If they have active + * counts, just remove them from the LRU. Otherwise give them + * another pass through the LRU. + */ + if (dentry->d_lockref.count) { + d_lru_isolate(lru, dentry); + status = LRU_REMOVED; + goto out; + } + + /* + * Dentries with reference bit on are moved back to the tail + * except for the negative ones. + */ + if ((dentry->d_flags & DCACHE_REFERENCED) && !d_is_negative(dentry)) { + dentry->d_flags &= ~DCACHE_REFERENCED; + status = LRU_ROTATE; + goto out; + } + + status = LRU_REMOVED; + d_lru_shrink_move(lru, dentry, freeable); + if (d_is_negative(dentry)) + ndblk.neg_count++; +out: + spin_unlock(&dentry->d_lock); + ndblk.prune_count++; + return status; +} + +/* + * A workqueue function to prune negative dentry. + * + * The pruning is done gradually over time so as not to have noticeable + * performance impact. + */ +static void prune_negative_dentry(struct work_struct *work) +{ + int freed; + struct super_block *sb = READ_ONCE(ndblk.prune_sb); + LIST_HEAD(dispose); + + if (!sb) + return; + + ndblk.neg_count = ndblk.prune_count = 0; + freed = list_lru_walk(&sb->s_dentry_lru, dentry_negative_lru_isolate, + &dispose, NEG_DENTRY_BATCH); + + if (freed) + shrink_dentry_list(&dispose); + /* + * Continue delayed pruning once every second until negative dentry + * free pool is at least 1/2 of the initial value or the super_block + * has no more negative dentries left at the front. + */ + if (ndblk.neg_count && + (READ_ONCE(ndblk.nfree) < neg_dentry_nfree_init/2)) + schedule_delayed_work(&prune_neg_dentry_work, + NEG_PRUNING_DELAY); + else + WRITE_ONCE(ndblk.prune_sb, NULL); +} + /** * enum d_walk_ret - action to talke during tree walk * @D_WALK_CONTINUE: contrinue walk diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h index fa7fd03..06c9d15 100644 --- a/include/linux/list_lru.h +++ b/include/linux/list_lru.h @@ -22,6 +22,7 @@ enum lru_status { LRU_SKIP, /* item cannot be locked, skip */ LRU_RETRY, /* item not freeable. May drop the lock internally, but has to return locked. */ + LRU_STOP, /* stop walking the list */ }; struct list_lru_one { diff --git a/mm/list_lru.c b/mm/list_lru.c index 7a40fa2..f6e7796 100644 --- a/mm/list_lru.c +++ b/mm/list_lru.c @@ -244,11 +244,13 @@ unsigned long list_lru_count_node(struct list_lru *lru, int nid) */ assert_spin_locked(&nlru->lock); goto restart; + case LRU_STOP: + goto out; default: BUG(); } } - +out: spin_unlock(&nlru->lock); return isolated; }
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. So we need a way to prune the negative dentries so that new ones can be created. This is done by using the workqueue API to do the pruning gradually when a threshold is reached to minimize performance impact on other running tasks. The pruning is done at a frequency of 10 runs per second. Each run scans at most 256 LRU dentries for each node LRU list of a certain superblock. Some non-negative dentries that happen to be at the front of the LRU lists will also be pruned. Signed-off-by: Waiman Long <longman@redhat.com> --- fs/dcache.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/list_lru.h | 1 + mm/list_lru.c | 4 +- 3 files changed, 117 insertions(+), 1 deletion(-)