Messages in this thread Patch in this message | | | Date | Tue, 24 Jan 2006 16:40:19 +0530 | From | Balbir Singh <> | Subject | Re: [PATCH] shrink_dcache_parent() races against shrink_dcache_memory() |
| |
Hi, Kirill,
On Tue, Jan 24, 2006 at 12:48:13PM +0300, Kirill Korotaev wrote: > I like your idea, but some comments below... I doubt it works. > I will think it over a bit later... >
Thanks. Please find my comments and updated patch below
> Kirill > P.S. it's not easily reproducable. Before my fix it took us 3-6 hours on > automated stress testing to hit this bug. Right now I can't setup it for > testing, maybe in a week or so.
Sure, please test whenever you set it up.
[snip]
> >+ spin_lock(&sb_lock); > <<<< 1. sb_lock doesn't protect atomic_read() anyhow... > <<<< I mean, sb_lock is not required to read its value...
Good point, the sb_lock is not required. I have removed it.
> >+ if (!atomic_read(&dentry->d_sb->s_active)) { > >+ /* > >+ * Race condition, umount and other pruning is > >happening > >+ * in parallel. > >+ */ > >+ if (current->flags & PF_MEMALLOC) { > >+ /* > >+ * let the allocator leave this dentry alone > >+ */ > >+ spin_unlock(&sb_lock); > >+ spin_unlock(&dentry->d_lock); > >+ spin_unlock(&dcache_lock); > >+ return; > <<<< you should not return, but rather 'continue'. otherwise you skip > _all_ dentries, even from active super blocks.
Good point.
> >+ } > >+ } > >+ spin_unlock(&sb_lock); > >+ > <<<< and here, when you drop sb_lock, and dentry->d_lock/dcache_lock in > prune_dentry() it looks to me that we have exactly the same situation as > it was without your patch: > <<<< another CPU can start umount in parallel. > <<<< maybe sb_lock barrier helps this somehow, but I can't see how yet...
From the unmount path, __mntput() is called. It sets s_active to 0 in deactivate_super(), hence our check would prevent us from pruning a dentry that is a part of a super block that is going to go away soon. The idea is to let the unmount do all the work here, the allocator can concentrate on other dentries.
> > <<<< another idea: down_read(&sb->s_umount) probably could help... > <<<< because it will block the whole umount operation... > <<<< but we can't take it under dcache_lock...
Yes, we cannot do a down* under a spinlock
[snip]
How does the modified patch look?
Regards, Balbir
Signed-off-by: Balbir Singh <balbir@in.ibm.com> ---
fs/dcache.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+)
diff -puN fs/dcache.c~dcache_race_fix2 fs/dcache.c --- linux-2.6/fs/dcache.c~dcache_race_fix2 2006-01-24 11:05:46.000000000 +0530 +++ linux-2.6-balbir/fs/dcache.c 2006-01-24 15:49:30.000000000 +0530 @@ -425,6 +425,21 @@ static void prune_dcache(int count) spin_unlock(&dentry->d_lock); continue; } + + if (!atomic_read(&dentry->d_sb->s_active)) { + /* + * Race condition, umount and other pruning is happening + * in parallel. + */ + if (current->flags & PF_MEMALLOC) { + /* + * Ask the allocator leave this dentry alone + */ + spin_unlock(&dentry->d_lock); + continue; + } + } + prune_one_dentry(dentry); } spin_unlock(&dcache_lock); _ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |