OpenVZ Forum - RDF feed
type: channel
items: [Q] missing unused dentry in prune_dcache()?
items: Re: [Q] missing unused dentry in prune_dcache()?
items: [PATCH 2.6.19-rc3] VFS: missing unused dentry in prune_dcache()
items: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
items: Re: [PATCH 2.6.19-rc3] VFS: missing unused dentry in prune_dcache()
items: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
items: Re: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
items: [Q] missing ->d_delete() in shrink_dcache_for_umount()?
items: Re: [Q] missing ->d_delete() in shrink_dcache_for_umount()?

[Q] missing unused dentry in prune_dcache()?
type: item
description: Hello folks, I would like to ask you clarify me one question in the the following patch: http://linux.bkbits.net:8080/linux-2.6/gnupatch@449b144ecSF1 rYskg3q-SeR2vf88zg # ChangeSet # 2006/06/22 15:05:57-07:00 neilb@suse.de # [PATCH] Fix dcache race during umount # If prune_dcache finds a dentry that it cannot free, it leaves it where it # is (at the tail of the list) and exits, on the assumption that some other # thread will be removing that dentry soon. However as far as I see this comment is not correct: when we cannot take s_umount rw_semaphore (for example because it was taken in do_remount) this dentry is already extracted from dentry_unused list and we do not add it into the list again. Therefore dentry will not be found by prune_dcache() and shrink_dcache_sb() and will leave in memory very long time until the partition will be unmounted. Am I probably err? The patch adds this dentry into tail of the dentry_unused list. Signed-off-by: Vasily Averin <vvs@sw.ru> --- linux-2.6.19-rc3/fs/dcache.c.prdch 2006-10-25 16:09:19.000000000 +0400 +++ linux-2.6.19-rc3/fs/dcache.c 2006-10-25 16:08:20.000000000 +0400 @@ -477,6 +477,8 @@ static void prune_dcache(int count, stru } up_read(s_umount); } + list_add_tail(&dentry->d_lru, &dentry_unused); + dentry_stat.nr_unused++; spin_unlock(&dentry->d_lock); /* Cannot remove the first dentry, and it isn't appropriate * to move it to the head of the list, so give up, and try
creator: vaverin
date: 2006-10-25T12:30:51-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > # If prune_dcache finds a dentry that it cannot free, it leaves it where it > # is (at the tail of the list) and exits, on the assumption that some other > # thread will be removing that dentry soon. > > However as far as I see this comment is not correct: when we cannot take > s_umount rw_semaphore (for example because it was taken in do_remount) this > dentry is already extracted from dentry_unused list and we do not add it into > the list again. You would seem to be correct. > Therefore dentry will not be found by prune_dcache() and shrink_dcache_sb() > and will leave in memory very long time until the partition will be > unmounted. And here too:-/ > Am I probably err? Unfortunately not. I wonder if remount should be getting a writelock on the s_umount sem, but I don't see why not. grab_super() also gets a writelock on it, and so that could cause problems too. shrink_dcache_for_umount_subtree() doesn't care because it doesn't scan the dcache_unused list, but as you say, other things are affected. > The patch adds this dentry into tail of the dentry_unused list. I think that's reasonable. I wonder if we can avoid removing it from the list in the first place, but I suspect it's less optimal. Acked-By: David Howells <dhowells@redhat.com>
creator: David Howells
date: 2006-10-25T13:51:04-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: David Howells wrote: > Vasily Averin <vvs@sw.ru> wrote: >> The patch adds this dentry into tail of the dentry_unused list. > > I think that's reasonable. I wonder if we can avoid removing it from the list > in the first place, but I suspect it's less optimal. Could you please explain this place in details, I do not understand why tail of the list is better than head. Also I do not understand why we should go to out in this case. Why we cannot use next dentry in the list instead? > Acked-By: David Howells <dhowells@redhat.com> Thank you, Vasily Averin
creator: vaverin
date: 2006-10-25T13:58:40-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > >> The patch adds this dentry into tail of the dentry_unused list. > > > > I think that's reasonable. I wonder if we can avoid removing it from the > > list in the first place, but I suspect it's less optimal. > > Could you please explain this place in details, I do not understand why tail > of the list is better than head. Also I do not understand why we should go > to out in this case. Why we cannot use next dentry in the list instead? I meant adding it back into the list is reasonable; I didn't actually consider where you were adding it back. However, you've just raised a good point: moving a dentry to the head of the list is what happens when we see it's been referenced recently. The most recently used entry is at the head and the least at the tail. If you look at the list scan algorithm, you can see it works from the tail towards the head. We could re-insert the dentry at the position from which we removed it, but that would mean retaining a pointer to one of its neighbouring dentries. We can do that - it's just stack space after all... I think the main point though, is generally that the superblock that owns the dentry is very busy in some critical way; either it's being unmounted (in which case we may as well put the dentry at the MRU end), or it's being remounted (in which case, it's probably best putting it back where we found it). You also have to consider how often superblocks are going to be unmounted or remounted vs the dcache being pruned... There is also grab_super(), but that's only used by sget() when a superblock is being mounted again - also unlikely. So, given that the three ops that affect this are very unlikely to be happening at any one time, it's probably worth just slapping it at the head and ignoring it. The main thing is that it's reinserted somewhere. Hope that helps... David
creator: David Howells
date: 2006-10-25T14:23:29-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Hello David David Howells wrote: > Vasily Averin <vvs@sw.ru> wrote: >>>> The patch adds this dentry into tail of the dentry_unused list. >>> I think that's reasonable. I wonder if we can avoid removing it from the >>> list in the first place, but I suspect it's less optimal. >> Could you please explain this place in details, I do not understand why tail >> of the list is better than head. Also I do not understand why we should go >> to out in this case. Why we cannot use next dentry in the list instead? > > I meant adding it back into the list is reasonable; I didn't actually consider > where you were adding it back. > > So, given that the three ops that affect this are very unlikely to be happening > at any one time, it's probably worth just slapping it at the head and ignoring > it. The main thing is that it's reinserted somewhere. I don't like to insert this dentry into tail of list because of it prevents shrink_dcache_memory. It finds "skipped" dentry at the tail of the list, does not free it and goes to out without any progress. Therefore I've removed break of cycle and insert this dentry to head of the list. Theoretically it can lead to the second using of the same dentry, however I do not think that it is a big problem. Signed-off-by: Vasily Averin <vvs@sw.ru> --- linux-2.6.19-rc3/fs/dcache.c.prdch 2006-10-25 16:09:19.000000000 +0400 +++ linux-2.6.19-rc3/fs/dcache.c 2006-10-26 15:14:51.000000000 +0400 @@ -478,11 +478,9 @@ static void prune_dcache(int count, stru up_read(s_umount); } spin_unlock(&dentry->d_lock); - /* Cannot remove the first dentry, and it isn't appropriate - * to move it to the head of the list, so give up, and try - * later - */ - break; + /* Inserting dentry to tail of the list leads to cycle */ + list_add(&dentry->d_lru, &dentry_unused); + dentry_stat.nr_unused++; } spin_unlock(&dcache_lock); }
creator: vaverin
date: 2006-10-26T11:36:53-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Hello David, I've noticed one more minor issue in your patch: in shrink_dcache_for_umount_subtree() function you decrement dentry_stat.nr_dentry without dcache_lock. Thank you, Vasily Averin
creator: vaverin
date: 2006-10-26T11:49:25-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > I've noticed one more minor issue in your patch: in > shrink_dcache_for_umount_subtree() function you decrement > dentry_stat.nr_dentry without dcache_lock. Hmmm... that's a very good point:-/ I wonder if I can batch them. David
creator: David Howells
date: 2006-10-26T12:33:58-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > I've noticed one more minor issue in your patch: in > shrink_dcache_for_umount_subtree() function you decrement > dentry_stat.nr_dentry without dcache_lock. How about the attached patch? David --- VFS: Fix an error in unused dentry counting From: David Howells <dhowells@redhat.com> Fix an error in unused dentry counting in shrink_dcache_for_umount_subtree() in which the count is modified without the dcache_lock held. Signed-Off-By: David Howells <dhowells@redhat.com> --- fs/dcache.c | 22 +++++++++++++++------- 1 files changed, 15 insertions(+), 7 deletions(-) diff --git a/fs/dcache.c b/fs/dcache.c index a1ff91e..eab1bf4 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -553,16 +553,17 @@ repeat: * - see the comments on shrink_dcache_for_umount() for a description of the * locking */ -static void shrink_dcache_for_umount_subtree(struct dentry *dentry) +static unsigned shrink_dcache_for_umount_subtree(struct dentry *dentry) { struct dentry *parent; + unsigned detached = 0; BUG_ON(!IS_ROOT(dentry)); /* detach this root from the system */ spin_lock(&dcache_lock); if (!list_empty(&dentry->d_lru)) { - dentry_stat.nr_unused--; + detached++; list_del_init(&dentry->d_lru); } __d_drop(dentry); @@ -579,7 +580,7 @@ static void shrink_dcache_for_umount_sub list_for_each_entry(loop, &dentry->d_subdirs, d_u.d_child) { if (!list_empty(&loop->d_lru)) { - dentry_stat.nr_unused--; + detached++; list_del_init(&loop->d_lru); } @@ -620,7 +621,7 @@ static void shrink_dcache_for_umount_sub atomic_dec(&parent->d_count); list_del(&dentry->d_u.d_child); - dentry_stat.nr_dentry--; /* For d_free, below */ + detached++; /* For d_free, below */ inode = dentry->d_inode; if (inode) { @@ -638,7 +639,7 @@ static void shrink_dcache_for_umount_sub * otherwise we ascend to the parent and move to the * next sibling if there is one */ if (!parent) - return; + return detached; dentry = parent; @@ -663,6 +664,7 @@ static void shrink_dcache_for_umount_sub void shrink_dcache_for…
creator: David Howells
date: 2006-10-26T13:23:35-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > Therefore I've removed break of cycle and insert this dentry to head of the > list. Theoretically it can lead to the second using of the same dentry, > however I do not think that it is a big problem. Hmmm... Or maybe it could be a problem. If whenever we find the dentry we stick it back on the head of the list, this could be a problem as we're traversing the list from tail to head. David
creator: David Howells
date: 2006-10-26T13:25:29-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: David Howells wrote: > Vasily Averin <vvs@sw.ru> wrote: > >> I've noticed one more minor issue in your patch: in >> shrink_dcache_for_umount_subtree() function you decrement >> dentry_stat.nr_dentry without dcache_lock. > > How about the attached patch? I would note that in first two hunks you have decremented dentry_stat.nr_unused correctly, under dcache_lock. Probably it's better to count freed dentries only in third case and corrects dentry_stat.nr_unused value inside shrink_dcache_for_umount_subtree() function before return. Thank you, Vasily Averin > David > --- > VFS: Fix an error in unused dentry counting > > From: David Howells <dhowells@redhat.com> > > Fix an error in unused dentry counting in shrink_dcache_for_umount_subtree() in > which the count is modified without the dcache_lock held. > > Signed-Off-By: David Howells <dhowells@redhat.com> > --- > > fs/dcache.c | 22 +++++++++++++++------- > 1 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/fs/dcache.c b/fs/dcache.c > index a1ff91e..eab1bf4 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -553,16 +553,17 @@ repeat: > * - see the comments on shrink_dcache_for_umount() for a description of the > * locking > */ > -static void shrink_dcache_for_umount_subtree(struct dentry *dentry) > +static unsigned shrink_dcache_for_umount_subtree(struct dentry *dentry) > { > struct dentry *parent; > + unsigned detached = 0; > > BUG_ON(!IS_ROOT(dentry)); > > /* detach this root from the system */ > spin_lock(&dcache_lock); > if (!list_empty(&dentry->d_lru)) { > - dentry_stat.nr_unused--; > + detached++; > list_del_init(&dentry->d_lru); > } > __d_drop(dentry); > @@ -579,7 +580,7 @@ static void shrink_dcache_for_umount_sub > list_for_each_entry(loop, &dentry->d_subdirs, > d_u.d_child) { > if (!list_empty(&loop->d_lru)) { > - dentry_stat.nr_unused--; > + detached++; > list_del_init(&loop->d_lru); > } > > @@ -620,7 +621,7 @@ static void shrink_dcache_for_umount_sub > atomic_dec(&parent->d_count); > > list_d…
creator: vaverin
date: 2006-10-26T13:58:20-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > I would note that in first two hunks you have decremented > dentry_stat.nr_unused correctly, under dcache_lock. Probably it's better to > count freed dentries only in third case and corrects dentry_stat.nr_unused > value inside shrink_dcache_for_umount_subtree() function before return. Maybe, maybe not. This way we only touch the dentry_stat cacheline once, if at all. I'm not sure it'd make much difference though. David
creator: David Howells
date: 2006-10-26T14:07:32-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: David Howells wrote: > Vasily Averin <vvs@sw.ru> wrote: > >> I've noticed one more minor issue in your patch: in >> shrink_dcache_for_umount_subtree() function you decrement >> dentry_stat.nr_dentry without dcache_lock. > > How about the attached patch? I'm sorry, but your patch is wrong: you have mixed calculation of 2 variables: dentry_stat.nr_unused -- were correct, it was decremented under dcache_lock. dentry_stat.nr_dentry -- were incorrect, it was decremented without dcache_lock. You should correct dentry_stat.nr_dentry, but instead you broke calculation of dentry_stat.nr_unused. I've fixed this issue by following patch. Thank you, Vasily Averin --- VFS: Fix an error in dentry_stat.nr_dentry counting From: Vasily Averin <vvs@sw.ru> Fix an error in dentry_stat.nr_dentry counting in shrink_dcache_for_umount_subtree() in which the count is modified without the dcache_lock held. Signed-Off-By: Vasily Averin <vvs@sw.ru> --- linux-2.6.19-rc3/fs/dcache.c.nrdntr 2006-10-26 15:14:51.000000000 +0400 +++ linux-2.6.19-rc3/fs/dcache.c 2006-10-27 10:24:07.000000000 +0400 @@ -554,6 +554,7 @@ repeat: static void shrink_dcache_for_umount_subtree(struct dentry *dentry) { struct dentry *parent; + unsigned detached = 0; BUG_ON(!IS_ROOT(dentry)); @@ -618,7 +619,7 @@ static void shrink_dcache_for_umount_sub atomic_dec(&parent->d_count); list_del(&dentry->d_u.d_child); - dentry_stat.nr_dentry--; /* For d_free, below */ + detached++; inode = dentry->d_inode; if (inode) { @@ -635,8 +636,8 @@ static void shrink_dcache_for_umount_sub /* finished when we fall off the top of the tree, * otherwise we ascend to the parent and move to the * next sibling if there is one */ - if (!parent) - return; + if (!parent) + goto out; dentry = parent; @@ -645,6 +646,11 @@ static void shrink_dcache_for_umount_sub dentry = list_entry(dentry->d_subdirs.next, struct dentry, d_u.d_child); } +out: + /* several dentries were freed, need to correct nr_dentry */ + spin_lock(&dcache_lock); + dentry_stat.nr_dentry…
creator: vaverin
date: 2006-10-27T06:32:35-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin wrote: > David Howells wrote: >> Vasily Averin <vvs@sw.ru> wrote: >> >>> I've noticed one more minor issue in your patch: in >>> shrink_dcache_for_umount_subtree() function you decrement >>> dentry_stat.nr_dentry without dcache_lock. >> How about the attached patch? > I'm sorry, but your patch is wrong: > you have mixed calculation of 2 variables: > dentry_stat.nr_unused -- were correct, it was decremented under dcache_lock. > dentry_stat.nr_dentry -- were incorrect, it was decremented without dcache_lock. > > You should correct dentry_stat.nr_dentry, but instead you broke calculation of > dentry_stat.nr_unused. > > I've fixed this issue by following patch. corrected version, extra space were removed Thank you, Vasily Averin --- VFS: Fix an error in dentry_stat.nr_dentry counting From: Vasily Averin <vvs@sw.ru> Fix an error in dentry_stat.nr_dentry counting in shrink_dcache_for_umount_subtree() in which the count is modified without the dcache_lock held. Signed-Off-By: Vasily Averin <vvs@sw.ru> --- linux-2.6.19-rc3/fs/dcache.c.nrdntr 2006-10-26 15:14:51.000000000 +0400 +++ linux-2.6.19-rc3/fs/dcache.c 2006-10-27 10:45:11.000000000 +0400 @@ -554,6 +554,7 @@ repeat: static void shrink_dcache_for_umount_subtree(struct dentry *dentry) { struct dentry *parent; + unsigned detached = 0; BUG_ON(!IS_ROOT(dentry)); @@ -618,7 +619,7 @@ static void shrink_dcache_for_umount_sub atomic_dec(&parent->d_count); list_del(&dentry->d_u.d_child); - dentry_stat.nr_dentry--; /* For d_free, below */ + detached++; inode = dentry->d_inode; if (inode) { @@ -636,7 +637,7 @@ static void shrink_dcache_for_umount_sub * otherwise we ascend to the parent and move to the * next sibling if there is one */ if (!parent) - return; + goto out; dentry = parent; @@ -645,6 +646,11 @@ static void shrink_dcache_for_umount_sub dentry = list_entry(dentry->d_subdirs.next, struct dentry, d_u.d_child); } +out: + /* several dentries were freed, need to correct nr_dentry */ + spin_lock(&dcache_lock); +…
creator: vaverin
date: 2006-10-27T06:50:45-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: David Howells wrote: > Vasily Averin <vvs@sw.ru> wrote: > >> Therefore I've removed break of cycle and insert this dentry to head of the >> list. Theoretically it can lead to the second using of the same dentry, >> however I do not think that it is a big problem. > > Hmmm... Or maybe it could be a problem. If whenever we find the dentry we > stick it back on the head of the list, this could be a problem as we're > traversing the list from tail to head. I still do not think that it could be a problem: count argument of prune_dcache is 128 if prune_dcache is called from shrink_dcache_memory() function and even lesser if prune_dcache is called from shrink_dcache_parent() function. I think usually the size of unused_dentry list (nr_usnused) is much greater and may be compared with these values only in case of very hard memory shortage. From my point of view it is very rare situation and I even not sure that it can really happen at all. However even if this situation will happen I do not see here any seriously troubles: Yes, we will try to free the same dentries, much probably without success again, but why it is bad in case of hard memory shortage? If my arguments are not convincing, I can protect second use of the same dentry: we can compare counter of the skipped dentries with nr_unused value. And what the alternatives we have? 1) We can move this dentry to end of list? But it is bad because it will prevent shrink_dcache_memory(). 2) we can move these dentries into some temporal list, and insert it back to unused_list later? But it is bad because of we can be rescheduled and drop dcache_lock and may confuse someone who will assume that these dentries are in unused_list. Therefore I believe that my patch is optimal solution. Thank you, Vasily Averin
creator: vaverin
date: 2006-10-27T08:05:54-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > > You should correct dentry_stat.nr_dentry, but instead you broke > > calculation of dentry_stat.nr_unused. > > > > I've fixed this issue by following patch. > corrected version, extra space were removed Oops. You're right. Acked-By: David Howells <dhowells@redhat.com>
creator: David Howells
date: 2006-10-27T09:36:37-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > Therefore I believe that my patch is optimal solution. I'm not sure that prune_dcache() is particularly optimal. If we're looking to prune for a specific superblock, it may scan most of the dentry_unused list several times, once for each dentry it eliminates. Imagine the list with a million dentries on it. Imagine further that all the dentries you're trying to eliminate are up near the head end: you're going to have to scan most of the list several times unnecessarily; if you're asked to kill 128 dentries, you might wind up examining on the order of 100,000,000 dentries, 99% of which you scan 128 times. I wonder if this could be improved by making the assumption that there won't be any entries inserted tailwards of where we've just looked. The problem is that if dcache_lock is dropped, we've no way of keeping track of the current position without inserting a marker into the list. Now we could do the marker thing quite easily. We'd have to insert a dummy dcache entry, probably with d_sb pointing to some special location that is recognised as saying "that dentry is a marker". We could do something like the attached patch, for example. Note that the patch compiles, but I haven't tested it. It also uses a big chunk of stack space for the marker. It ought to be safe enough with respect to the other functions that touch that list - all of those deal with specific dentries or look for dentries by superblock. David --- diff --git a/fs/dcache.c b/fs/dcache.c index eab1bf4..a1cae74 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -395,18 +395,27 @@ static void prune_one_dentry(struct dent * This function may fail to free any resources if * all the dentries are in use. */ - + static void prune_dcache(int count, struct super_block *sb) { + struct dentry marker = { + .d_sb = (struct super_block *) &prune_dcache, + }; + + struct list_head *tmp; + spin_lock(&dcache_lock); + list_add_tail(&marker.d_lru, &dentry_unused); + for (; count ; count--) { st…
creator: David Howells
date: 2006-10-27T10:42:49-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: David, David Howells wrote: > Vasily Averin <vvs@sw.ru> wrote: >> Therefore I believe that my patch is optimal solution. > I'm not sure that prune_dcache() is particularly optimal. I means that my patch is optimal for problem in subject. I would like to ask you to approve it and we will go to next issue. > If we're looking to > prune for a specific superblock, it may scan most of the dentry_unused list > several times, once for each dentry it eliminates. > > Imagine the list with a million dentries on it. Imagine further that all the > dentries you're trying to eliminate are up near the head end: you're going to > have to scan most of the list several times unnecessarily; if you're asked to > kill 128 dentries, you might wind up examining on the order of 100,000,000 > dentries, 99% of which you scan 128 times. I would note that we (Virtuozzo/OpenVZ team) have seen similar issue on praxis. We have kernel that handles a few dozens Virtual servers, and each of them have the several isolated filesystems. We have seen that umount (and remount) can work very slowly, it was cycled inside shrink_dcache_sb() up to several hours with taken s_umount semaphore. We are trying to resolve this issue by using per-sb lru list. I'm preparing the patch for 2.6.19-rc3 right now and going to send it soon. thank you, Vasily Averin
creator: vaverin
date: 2006-10-27T11:50:59-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > > Vasily Averin <vvs@sw.ru> wrote: > >> Therefore I believe that my patch is optimal solution. > > I'm not sure that prune_dcache() is particularly optimal. > > I means that my patch is optimal for problem in subject. I didn't say it wasn't. > I would like to ask you to approve it and we will go to next issue. I did ack it didn't I? I must fix my mail client so that it doesn't automatically remove my email address from the To/Cc fields when I'm replying to a message:-/ > We have seen that umount (and remount) can work very slowly, it was cycled > inside shrink_dcache_sb() up to several hours with taken s_umount semaphore. umount at least should be fixed as that should no longer use shrink_dcache_sb(). > We are trying to resolve this issue by using per-sb lru list. I'm preparing > the patch for 2.6.19-rc3 right now and going to send it soon. That sounds tricky; you have to check all your LRU lists to find the LRU dentry. David
creator: David Howells
date: 2006-10-27T12:11:21-00:00

[PATCH 2.6.19-rc3] VFS: missing unused dentry in prune_dcache()
type: item
description: Andrew, could you please use this patch instead of missing-unused-dentry-in-prune_dcache.patch As far as I understand David Howells have not any objections Thank you, Vasily Averin --- VFS: missing unused dentry in prune_dcache() From: Vasily Averin <vvs@sw.ru> If we cannot delete dentry we should insert it back to the dentry_unused list. To prevent cycle and do not blocks prune_dcache() call from shrink_dcache_memory() we adding this dentry to head of list. Signed-off-by: Vasily Averin <vvs@sw.ru> --- linux-2.6.19-rc3/fs/dcache.c.prdch 2006-10-25 16:09:19.000000000 +0400 +++ linux-2.6.19-rc3/fs/dcache.c 2006-10-26 15:14:51.000000000 +0400 @@ -478,11 +478,9 @@ static void prune_dcache(int count, stru up_read(s_umount); } spin_unlock(&dentry->d_lock); - /* Cannot remove the first dentry, and it isn't appropriate - * to move it to the head of the list, so give up, and try - * later - */ - break; + /* Inserting dentry to tail of the list leads to cycle */ + list_add(&dentry->d_lru, &dentry_unused); + dentry_stat.nr_unused++; } spin_unlock(&dcache_lock); }
creator: vaverin
date: 2006-10-27T13:42:03-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: David Howells wrote: > Vasily Averin <vvs@sw.ru> wrote: >> I would like to ask you to approve it and we will go to next issue. > > I did ack it didn't I? I must fix my mail client so that it doesn't > automatically remove my email address from the To/Cc fields when I'm replying > to a message:-/ To prevent any mistake I've resend this patch again to you and akpm@. >> We have seen that umount (and remount) can work very slowly, it was cycled >> inside shrink_dcache_sb() up to several hours with taken s_umount semaphore. > > umount at least should be fixed as that should no longer use > shrink_dcache_sb(). Umount calls shrink_dcache_sb in "Special case for "unmounting" root". Usually it happen only once, but in case OpenVZ it happens every time when any Virtual server is stopped, each of them have own isolated root partition. >> We are trying to resolve this issue by using per-sb lru list. I'm preparing >> the patch for 2.6.19-rc3 right now and going to send it soon. Please wait until my following email, it will be ready soon thank you, Vasily Averin
creator: vaverin
date: 2006-10-27T13:47:00-00:00

[PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: From: Vasily Averin <vvs@sw.ru> Virtuozzo/OpenVZ linux kernel team has discovered that umount/remount can last for hours looping in shrink_dcache_sb() without much successes. Since during shrinking s_umount semaphore is taken lots of other unrelated operations like sync can stop working until shrink finished. It happens due to very long unused dentries list (>1 million of dentries), so that it takes shrink_dcache_sb() longer then 1/HZ seconds to get to the required dentry and after freeing this single dentry it reschedules and restarts. The proposed fix prevents this issue by using per-sb dentry LRU list. It provides very quickly search for the unused dentries for given super block thus forcing shrinking always making good progress. It was well tested on 2.6.9-based Virtuozzo/OpenVZ kernels, but the port to the latest mainstream kernel is not tested. Signed-off-by: Kirill Korotaev <dev@openvz.org> Signed-off-by: Vasily Averin <vvs@sw.ru> --- linux-2.6.19-rc3/fs/dcache.c.shrsb 2006-10-27 10:45:11.000000000 +0400 +++ linux-2.6.19-rc3/fs/dcache.c 2006-10-27 15:49:54.000000000 +0400 @@ -173,6 +173,7 @@ repeat: if (list_empty(&dentry->d_lru)) { dentry->d_flags |= DCACHE_REFERENCED; list_add(&dentry->d_lru, &dentry_unused); + list_add(&dentry->d_sb_lru, &dentry->d_sb->s_dentry_unused); dentry_stat.nr_unused++; } spin_unlock(&dentry->d_lock); @@ -190,6 +191,7 @@ kill_it: { */ if (!list_empty(&dentry->d_lru)) { list_del(&dentry->d_lru); + list_del(&dentry->d_sb_lru); dentry_stat.nr_unused--; } list_del(&dentry->d_u.d_child); @@ -270,6 +272,7 @@ static inline struct dentry * __dget_loc if (!list_empty(&dentry->d_lru)) { dentry_stat.nr_unused--; list_del_init(&dentry->d_lru); + list_del_init(&dentry->d_sb_lru); } return dentry; } @@ -398,6 +401,10 @@ static void prune_one_dentry(struct dent static void prune_dcache(int count, struct super_block *sb) { + struct list_head *lru_head; + + lru_head = sb ? &sb->s_dentry_unused : &dentry_unused; + spin_lock(&dcache_lock); for (; cou…
creator: vaverin
date: 2006-10-27T14:05:50-00:00

Re: [PATCH 2.6.19-rc3] VFS: missing unused dentry in prune_dcache()
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > + /* Inserting dentry to tail of the list leads to cycle */ > + list_add(&dentry->d_lru, &dentry_unused); > + dentry_stat.nr_unused++; I'd phrase that comment differently: "Insert dentry at the head of the list as inserting at the tail leads to a cycle". But other than that: Acked-By: David Howells <dhowells@redhat.com>
creator: David Howells
date: 2006-10-27T14:24:33-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > Umount calls shrink_dcache_sb in "Special case for "unmounting" root". > Usually it happen only once, but in case OpenVZ it happens every time when any > Virtual server is stopped, each of them have own isolated root partition. Where is that? Are you talking about do_remount_sb()? David
creator: David Howells
date: 2006-10-27T14:29:05-00:00

Re: [Q] missing unused dentry in prune_dcache()?
type: item
description: > Vasily Averin <vvs@sw.ru> wrote: > > >>Umount calls shrink_dcache_sb in "Special case for "unmounting" root". >>Usually it happen only once, but in case OpenVZ it happens every time when any >>Virtual server is stopped, each of them have own isolated root partition. > > > Where is that? Are you talking about do_remount_sb()? AFAIR yes. Kirill
creator: dev
date: 2006-10-27T14:34:12-00:00

Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: On Fri, 27 Oct 2006 18:05:50 +0400 Vasily Averin <vvs@sw.ru> wrote: > Virtuozzo/OpenVZ linux kernel team has discovered that umount/remount can last > for hours looping in shrink_dcache_sb() without much successes. Since during > shrinking s_umount semaphore is taken lots of other unrelated operations like > sync can stop working until shrink finished. Did you consider altering shrink_dcache_sb() so that it holds onto dcache_lock and moves all the to-be-pruned dentries onto a private list in a single pass, then prunes them all outside the lock?
creator: Andrew Morton
date: 2006-10-27T18:06:45-00:00

Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: On Fri, Oct 27, 2006 at 06:05:50PM +0400, Vasily Averin wrote: > > The proposed fix prevents this issue by using per-sb dentry LRU list. It > provides very quickly search for the unused dentries for given super block thus > forcing shrinking always making good progress. We've been down this path before: http://marc.theaimsgroup.com/?l=linux-kernel&m=114861109 717260&w=2 A lot of comments on per-sb unused dentry lists were made in the threads associated with the above. other solutions were found to the problem that the above patch addressed, but I don't think any of them have made it to mainline yet. You might want to have a bit of a read of these threads first... Cheers, Dave. -- Dave Chinner Principal Engineer SGI Australian Software Group
creator: David Chinner
date: 2006-10-30T04:24:19-00:00

Re: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: David, >>The proposed fix prevents this issue by using per-sb dentry LRU list. It >>provides very quickly search for the unused dentries for given super block thus >>forcing shrinking always making good progress. > > > We've been down this path before: > > http://marc.theaimsgroup.com/?l=linux-kernel&m=114861109 717260&w=2 > > A lot of comments on per-sb unused dentry lists were made in > the threads associated with the above. other solutions were > found to the problem that the above patch addressed, but I don't > think any of them have made it to mainline yet. You might want > to have a bit of a read of these threads first... The major difference between our patch and the one discussed in the link it that we keep both global and per-sb dentry LRU lists. Thus, when needed normal LRU is used and prune logic is unchanged, while umount/remount use per-sb list and do its job faster. Thanks, Kirill
creator: dev
date: 2006-10-30T06:22:14-00:00

Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: Andrew Morton wrote: > On Fri, 27 Oct 2006 18:05:50 +0400 > Vasily Averin <vvs@sw.ru> wrote: > >> Virtuozzo/OpenVZ linux kernel team has discovered that umount/remount can last >> for hours looping in shrink_dcache_sb() without much successes. Since during >> shrinking s_umount semaphore is taken lots of other unrelated operations like >> sync can stop working until shrink finished. > > Did you consider altering shrink_dcache_sb() so that it holds onto > dcache_lock and moves all the to-be-pruned dentries onto a private list in > a single pass, then prunes them all outside the lock? At the first glance it is wrong because of 2 reasons: 1) it continues to check the whole global LRU list (we propose to use per-sb LRU, it will provide very quick search) 2) we have not any guarantee that someone will add new unused dentries to the list when we prune it outside the lock. And to the contrary, some of unused dentries can be used again. As far as I understand we should hold dcache_lock beginning at the removing dentry from unused_list until dentry_iput() call. David did it inside shrink_dcache_for_umount() just because it have guarantee that all the filesystem operations are finished and new ones cannot be started. Thank you, Vasily Averin
creator: vaverin
date: 2006-10-30T14:24:22-00:00

Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: Andrew, >>Virtuozzo/OpenVZ linux kernel team has discovered that umount/remount can last >>for hours looping in shrink_dcache_sb() without much successes. Since during >>shrinking s_umount semaphore is taken lots of other unrelated operations like >>sync can stop working until shrink finished. > > > Did you consider altering shrink_dcache_sb() so that it holds onto > dcache_lock and moves all the to-be-pruned dentries onto a private list in > a single pass, then prunes them all outside the lock? moving dentries from global list to the local one can take arbitrary number of milliseconds (with huge amount of memory), so nothing good here from latency view point. Thanks, Kirill
creator: dev
date: 2006-10-30T15:06:48-00:00

Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: On Monday 30 October 2006 15:24, Vasily Averin wrote: > Andrew Morton wrote: > > On Fri, 27 Oct 2006 18:05:50 +0400 > > > > Vasily Averin <vvs@sw.ru> wrote: > >> Virtuozzo/OpenVZ linux kernel team has discovered that umount/remount > >> can last for hours looping in shrink_dcache_sb() without much successes. > >> Since during shrinking s_umount semaphore is taken lots of other > >> unrelated operations like sync can stop working until shrink finished. > > > > Did you consider altering shrink_dcache_sb() so that it holds onto > > dcache_lock and moves all the to-be-pruned dentries onto a private list > > in a single pass, then prunes them all outside the lock? > > At the first glance it is wrong because of 2 reasons: > 1) it continues to check the whole global LRU list (we propose to use > per-sb LRU, it will provide very quick search) Quick search maybe, but your patch adds 2 pointers to each dentry in the system... That's pretty expensive, as dentries are already using a *lot* of ram. Maybe an alternative would be to not have anymore a global dentry_unused, but only per-sb unused dentries lists ? > 2) we have not any guarantee that someone will add new unused dentries to > the list when we prune it outside the lock. And to the contrary, some of > unused dentries can be used again. As far as I understand we should hold > dcache_lock beginning at the removing dentry from unused_list until > dentry_iput() call. > > David did it inside shrink_dcache_for_umount() just because it have > guarantee that all the filesystem operations are finished and new ones > cannot be started.
creator: Eric Dumazet
date: 2006-10-30T15:08:25-00:00

Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: > Quick search maybe, but your patch adds 2 pointers to each dentry in the > system... That's pretty expensive, as dentries are already using a *lot* of > ram. I don't see much problems with it... it is cache and it can be pruned if needed. Some time ago, for example, my patch introducing the same list for inodes was commited. > Maybe an alternative would be to not have anymore a global dentry_unused, but > only per-sb unused dentries lists ? I don't know global LRU implementation based on per-sb lists, do you? If someone suggest the algorithm for more or less fair global LRU based on non-global list we will implement it. However, so far, AFAICS there were problems with it. Thanks, Kirill
creator: dev
date: 2006-10-30T15:27:43-00:00

Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: On Monday 30 October 2006 16:34, Kirill Korotaev wrote: > > Quick search maybe, but your patch adds 2 pointers to each dentry in the > > system... That's pretty expensive, as dentries are already using a *lot* > > of ram. > > I don't see much problems with it... it is cache and it can be pruned if > needed. Some time ago, for example, my patch introducing the same list for > inodes was commited. The ratio of dentries/PAGE is higher than ration of inodes/PAGE. (On a x86_64 machine, 19 dentries per PAGE, 5 ext3 inodes per PAGE= Therefore I suspect that the number of pages locked in dentry_cache because of one inuse dentry is higher. > > > Maybe an alternative would be to not have anymore a global dentry_unused, > > but only per-sb unused dentries lists ? > > I don't know global LRU implementation based on per-sb lists, do you? > If someone suggest the algorithm for more or less fair global LRU > based on non-global list we will implement it. However, so far, > AFAICS there were problems with it. Using 32 bytes per dentry for unused LRUs sounds too much, maybe we should revert LRUS handling to timestamps or things like that.
creator: Eric Dumazet
date: 2006-10-30T16:09:31-00:00

Re: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: On Monday October 30, dev@sw.ru wrote: > David, > > >>The proposed fix prevents this issue by using per-sb dentry LRU list. It > >>provides very quickly search for the unused dentries for given super block thus > >>forcing shrinking always making good progress. > > > > > > We've been down this path before: > > > > http://marc.theaimsgroup.com/?l=linux-kernel&m=114861109 717260&w=2 > > > > A lot of comments on per-sb unused dentry lists were made in > > the threads associated with the above. other solutions were > > found to the problem that the above patch addressed, but I don't > > think any of them have made it to mainline yet. You might want > > to have a bit of a read of these threads first... > The major difference between our patch and the one discussed in the link > it that we keep both global and per-sb dentry LRU lists. > Thus, when needed normal LRU is used and prune logic is unchanged, > while umount/remount use per-sb list and do its job faster. Yes, we have been down this path before - several times I think. Below is the patch that I like (not tested recently - just rediffed and reviewed). NeilBrown Subject: Reduce contention in dentry_unused when unmounting. When we unmount a filesystem we need to release all dentries. We currently - move a collection of dentries to the end of the dentry_unused list - call prune_dcache to prune that number of dentries. If lots of other dentries are added to the end of the list while the prune_dcache proceeds (e.g. another filesystem is unmounted), this can involve a lot of wasted time wandering through the list looking for dentries that we had previously found. This patch allows the dentry_unused list to temporarily be multiple lists. When unmounting, dentries that are found to require pruning are moved to a temporary list, but accounted as though they were on dentry_unused. Then this list is passed to prune_dcache for freeing. Any entries that are not pruned for whatever reason are added to the end of dentry_unused. A…
creator: Neil Brown
date: 2006-10-31T04:38:58-00:00

Re: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: Neil Brown <neilb@suse.de> wrote: > When we unmount a filesystem we need to release all dentries. > We currently > - move a collection of dentries to the end of the dentry_unused list > - call prune_dcache to prune that number of dentries. This is not true anymore. David
creator: David Howells
date: 2006-10-31T10:40:14-00:00

Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: Hello Neil, Neil Brown wrote: >>> We've been down this path before: >>> >>> http://marc.theaimsgroup.com/?l=linux-kernel&m=114861109 717260&w=2 >>> >>> A lot of comments on per-sb unused dentry lists were made in >>> the threads associated with the above. other solutions were >>> found to the problem that the above patch addressed, but I don't >>> think any of them have made it to mainline yet. You might want >>> to have a bit of a read of these threads first... >> The major difference between our patch and the one discussed in the link >> it that we keep both global and per-sb dentry LRU lists. >> Thus, when needed normal LRU is used and prune logic is unchanged, >> while umount/remount use per-sb list and do its job faster. > > Yes, we have been down this path before - several times I think. > Below is the patch that I like (not tested recently - just rediffed > and reviewed). I've reviewed your patch, in general it looks great and I'm very like an idea to have only one place to free the unused dentries. I have several minor notes and one big question: > @@ -485,68 +483,25 @@ static void prune_dcache(int count, stru > list_add(&dentry->d_lru, &dentry_unused); > dentry_stat.nr_unused++; > } > + /* split any remaining entries back onto dentry_unused */ > + if (have_list) > + list_splice(list, dentry_unused.prev); > spin_unlock(&dcache_lock); > } - It seems for me this hunk is not required here. You are knows how many entries are present in the private list and nobody can add new dentries to it. Therefore I think your list should be empty on exit. - I do not like idea to split dentry_unused list, IMHO it broke LRU slightly (if somebody on the other CPU's shrinks dcache). However on the other hand this split is temporal and therefore it should not lead to any problems. Unfortunately I'm not sure that it can help us against long remount. As far as I understand the work time of shrink_dcache_sb() function depends on the subtree size, and you need to check whole tree sev…
creator: vaverin
date: 2006-10-31T13:08:03-00:00

[Q] missing ->d_delete() in shrink_dcache_for_umount()?
type: item
description: David, Vasily Averin wrote: > Hello folks, > > I would like to ask you clarify me one question in the the following patch: > http://linux.bkbits.net:8080/linux-2.6/gnupatch@449b144ecSF1 rYskg3q-SeR2vf88zg > # ChangeSet > # 2006/06/22 15:05:57-07:00 neilb@suse.de > # [PATCH] Fix dcache race during umount > > # If prune_dcache finds a dentry that it cannot free, it leaves it where it > # is (at the tail of the list) and exits, on the assumption that some other > # thread will be removing that dentry soon. It looks like I've noticed yet one suspicious place in your patch. As far as I see you have removed dput(root) call in shrink_dcache_for_umount() function. However I would note that dput contains ->d_delete() call that is missing in your function: if (dentry->d_op && dentry->d_op->d_delete) { if (dentry->d_op->d_delete(dentry)) goto unhash_it; I'm not sure but it seems to me some (probably out-of-tree) filesystems can do something useful in this place. Thank you, Vasily Averin
creator: vaverin
date: 2006-10-31T13:24:36-00:00

Re: [Q] missing ->d_delete() in shrink_dcache_for_umount()?
type: item
description: Vasily Averin <vvs@sw.ru> wrote: > It looks like I've noticed yet one suspicious place in your patch. As far as I > see you have removed dput(root) call in shrink_dcache_for_umount() function. > However I would note that dput contains ->d_delete() call that is missing in > your function: > > if (dentry->d_op && dentry->d_op->d_delete) { > if (dentry->d_op->d_delete(dentry)) > goto unhash_it; > > I'm not sure but it seems to me some (probably out-of-tree) filesystems can do > something useful in this place. Can they though? I'm not so sure. I've added Al to the To list to get his opinion. It seems to me that d_op->d_delete() is asking a question of whether the dentry should be discarded immediately upon dput() reducing the usage count to 0. The documentation in vfs.txt isn't entirely clear on this point, but what it does say is that that op isn't allowed to sleep, so there's a limit as to what it can do. Furthermore, d_op->d_delete() is probably the wrong hook to call. It returns an indication of whether the dentry should be retained or not, but we aren't interested in that at this point: we have to get rid of the dentry anyway, so the fs's opinion is irrelevant. Finally, we do still call d_op->d_release() by way of d_free(), so it's not as if the fs doesn't get a chance to clean up the dentry. David
creator: David Howells
date: 2006-10-31T15:06:57-00:00

Re: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: On Tuesday October 31, dhowells@redhat.com wrote: > Neil Brown <neilb@suse.de> wrote: > > > When we unmount a filesystem we need to release all dentries. > > We currently > > - move a collection of dentries to the end of the dentry_unused list > > - call prune_dcache to prune that number of dentries. > > This is not true anymore. True. That should read: When we remount a filesystem or invalidate a block device which has a mounted filesystem we call shrink dcache_sb which currently: - moves a collection of dentries to the end of the dentry_unused list - calls prune_dcache to prune that number of dentries. but the patch is still valid. Any objections to it going in to -mm and maybe .20 ?? Thanks, NeilBrown
creator: Neil Brown
date: 2006-11-01T06:32:54-00:00

Re: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: Neil Brown wrote: > On Monday October 30, dev@sw.ru wrote: > >>David, >> >> >>>>The proposed fix prevents this issue by using per-sb dentry LRU list. It >>>>provides very quickly search for the unused dentries for given super block thus >>>>forcing shrinking always making good progress. >>> >>> >>>We've been down this path before: >>> >>> http://marc.theaimsgroup.com/?l=linux-kernel&m=114861109 717260&w=2 >>> >>>A lot of comments on per-sb unused dentry lists were made in >>>the threads associated with the above. other solutions were >>>found to the problem that the above patch addressed, but I don't >>>think any of them have made it to mainline yet. You might want >>>to have a bit of a read of these threads first... >> >>The major difference between our patch and the one discussed in the link >>it that we keep both global and per-sb dentry LRU lists. >>Thus, when needed normal LRU is used and prune logic is unchanged, >>while umount/remount use per-sb list and do its job faster. > > > Yes, we have been down this path before - several times I think. > Below is the patch that I like (not tested recently - just rediffed > and reviewed). > > NeilBrown > > Subject: Reduce contention in dentry_unused when unmounting. > > When we unmount a filesystem we need to release all dentries. > We currently > - move a collection of dentries to the end of the dentry_unused list > - call prune_dcache to prune that number of dentries. > > If lots of other dentries are added to the end of the list while > the prune_dcache proceeds (e.g. another filesystem is unmounted), > this can involve a lot of wasted time wandering through the > list looking for dentries that we had previously found. > > This patch allows the dentry_unused list to temporarily be multiple > lists. > When unmounting, dentries that are found to require pruning are moved > to a temporary list, but accounted as though they were on dentry_unused. > Then this list is passed to prune_dcache for freeing. Any entries > that …
creator: dev
date: 2006-11-01T10:48:06-00:00

Re: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: Hello Neil, Neil Brown wrote: > On Tuesday October 31, dhowells@redhat.com wrote: >> Neil Brown <neilb@suse.de> wrote: >> >>> When we unmount a filesystem we need to release all dentries. >>> We currently >>> - move a collection of dentries to the end of the dentry_unused list >>> - call prune_dcache to prune that number of dentries. >> This is not true anymore. > > True. That should read: > > When we remount a filesystem or invalidate a block device which has a > mounted filesystem we call shrink dcache_sb which currently: > - moves a collection of dentries to the end of the dentry_unused list > - calls prune_dcache to prune that number of dentries. > > but the patch is still valid. > > Any objections to it going in to -mm and maybe .20 ?? Currently we have 3 type of functions that works with dentry_unused list: 1) prune_dcache(NULL) -- called from shrink_dcache_memory, frees the memory and requires global LRU. works well in current implementation. 2) prune_dcache(sb) -- called from shrink_dcache_parent(), frees subtree, LRU is not need here. Current implementation uses global LRU for these purposes, it is ineffective, and patch from Neil Brown fixes this issue. 3) shrink_dcache_sb() -- called when we need to free the unused dentries for given super block. Current implementation is not effective too, and per-sb LRU would be the best solution here. On the other hand patch from Neil Brown is much better than current implementation. In general I think that we should approve Neil Brown's patch. We (I and Kirill Korotaev) are ready to acknowledge it when the following remarks fill be fixed: - it seems for me list_splice() is not required inside prune_dcache(), - DCACHE_REFERENCED dentries should not be removed from private list to dentry_unused list, this flag should be ignored if the private list is used, - count argument should be ignored in this case too, we want to free all the dentries in private list, - when we shrink the whole super block we should free per-sb an…
creator: vaverin
date: 2006-11-01T13:32:03-00:00

Re: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: On Wednesday November 1, dev@sw.ru wrote: > <<<< AFAICS, doing so you introduced a leak of anonymous dentries. > > d_alloc_anon() calls d_alloc() with parent == NULL, i.e. dentries have no parent > and are not linked to the sb->s_root... Yep, thanks. > BTW, looking at it, I found that s_anon field on super block is not > used any more. I don't know what you mean by that. It is still used... > we can add BUG_ON(!hlist_empty(&sb->s_anon)) in generic_shutdown_super to avoid such issues like this. > > maybe we can fix it adding something like: > while (!list_empty(&sb->s_anon))) > prune_dcache(MAX_INT, &sb->s_anon); It seems that anon dentries can now have children (I think someone explained that too me - shrink_dcache_for_umount certainly suggests it). Also, in this context we cannot be sure that all dentries can be freed. This is being called a remount time remember, and some stuff might still be in use. We probably need to move the s_anon list to a temporary list and repeatedly: move the top entry back to s_anon and call shrink_dcache_parent on it. Needs more thought. NeilBrown
creator: Neil Brown
date: 2006-11-14T04:51:08-00:00

Re: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: On Wednesday November 1, vvs@sw.ru wrote: > > Currently we have 3 type of functions that works with dentry_unused list: > > 1) prune_dcache(NULL) -- called from shrink_dcache_memory, frees the memory and > requires global LRU. works well in current implementation. > 2) prune_dcache(sb) -- called from shrink_dcache_parent(), frees subtree, LRU > is not need here. Current implementation uses global LRU for these purposes, it > is ineffective, and patch from Neil Brown fixes this issue. > 3) shrink_dcache_sb() -- called when we need to free the unused dentries for > given super block. Current implementation is not effective too, and per-sb LRU > would be the best solution here. On the other hand patch from Neil Brown is much > better than current implementation. > > In general I think that we should approve Neil Brown's patch. We (I and Kirill > Korotaev) are ready to acknowledge it when the following remarks fill be fixed: > > - it seems for me list_splice() is not required inside > prune_dcache(), Yes, the list should be empty when we finish so you are right. > - DCACHE_REFERENCED dentries should not be removed from private list to > dentry_unused list, this flag should be ignored if the private list is used, Agreed. > - count argument should be ignored in this case too, we want to free all the > dentries in private list, Agreed. > - when we shrink the whole super block we should free per-sb anonymous dentries > too (please see Kirill Korotaev's letter) > Yes. Unfortunately I don't think it is as easy as it sounds. I'll have a closer look. > Then I'm going to prepare new patch that will enhance the shrink_dcache_sb() > performance: > - we can add new list head into struct superblock and use it in > shrink_dcache_sb() instead of temporal private list. We will check is it empty > in dput() and add the new unused dentries to per-sb list instead of > dentry_unused list. I think that makes sense. It means that you end up doing less work in select_parent, because the work …
creator: Neil Brown
date: 2006-11-14T05:44:46-00:00

Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: Hello Neil Neil Brown wrote: > On Wednesday November 1, vvs@sw.ru wrote: >> Currently we have 3 type of functions that works with dentry_unused list: >> >> 1) prune_dcache(NULL) -- called from shrink_dcache_memory, frees the memory and >> requires global LRU. works well in current implementation. >> 2) prune_dcache(sb) -- called from shrink_dcache_parent(), frees subtree, LRU >> is not need here. Current implementation uses global LRU for these purposes, it >> is ineffective, and patch from Neil Brown fixes this issue. >> 3) shrink_dcache_sb() -- called when we need to free the unused dentries for >> given super block. Current implementation is not effective too, and per-sb LRU >> would be the best solution here. On the other hand patch from Neil Brown is much >> better than current implementation. >> >> In general I think that we should approve Neil Brown's patch. We (I and Kirill >> Korotaev) are ready to acknowledge it when the following remarks fill be fixed: > > >> - it seems for me list_splice() is not required inside >> prune_dcache(), > > Yes, the list should be empty when we finish so you are right. > >> - DCACHE_REFERENCED dentries should not be removed from private list to >> dentry_unused list, this flag should be ignored if the private list is used, > > Agreed. > >> - count argument should be ignored in this case too, we want to free all the >> dentries in private list, > > Agreed. > >> - when we shrink the whole super block we should free per-sb anonymous dentries >> too (please see Kirill Korotaev's letter) >> > > Yes. Unfortunately I don't think it is as easy as it sounds. > I'll have a closer look. > > >> Then I'm going to prepare new patch that will enhance the shrink_dcache_sb() >> performance: >> - we can add new list head into struct superblock and use it in >> shrink_dcache_sb() instead of temporal private list. We will check is it empty >> in dput() and add the new unused dentries to per-sb list instead of >> dentry_unused list. > > I think th…
creator: vaverin
date: 2006-11-14T06:12:45-00:00

Re: Re: [PATCH 2.6.19-rc3] VFS: per-sb dentry lru list
type: item
description: Neil Brown <neilb@suse.de> wrote: > It seems that anon dentries can now have children (I think someone > explained that too me - shrink_dcache_for_umount certainly suggests > it). That is correct. NFS can have multiple roots in its superblocks, and these take the form of anon dentries with children. David
creator: David Howells
date: 2006-11-14T09:29:42-00:00