* Re: [f2fs-dev] [PATCH] f2fs:Add mutex_lock to protect f2fs_stat_list.
[not found] <201301142008133402501@gmail.com>
@ 2013-01-15 2:57 ` Huajun Li
2013-01-15 7:04 ` Jaegeuk Kim
2013-01-15 7:29 ` Jaegeuk Kim
1 sibling, 1 reply; 3+ messages in thread
From: Huajun Li @ 2013-01-15 2:57 UTC (permalink / raw)
To: majianpeng; +Cc: jaegeuk.kim, linux-fsdevel, linux-f2fs-devel
On Mon, Jan 14, 2013 at 8:08 PM, majianpeng <majianpeng@gmail.com> wrote:
> There is an race between umount f2fs and read f2fs/status.
> It will case oops.
> Fox example:
> Thread A Thread B
> umount f2fs cat f2fs/status
> f2fs_destroy_stats() { stat_show() {
> list_for_each_entry_safe(&f2fs_stat_list)
> list_del(&si->stat_list);
> mutex_lock(&si->stat_lock);
> si->sbi = NULL;
> mutex_unlock(&si->stat_lock);
> kfree(sbi->stat_info);
> mutex_lock(&si->stat_lock)
>
> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
> ---
> fs/f2fs/debug.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
> index 0e0380a..ec6d004 100644
> --- a/fs/f2fs/debug.c
> +++ b/fs/f2fs/debug.c
> @@ -26,6 +26,7 @@
>
> static LIST_HEAD(f2fs_stat_list);
> static struct dentry *debugfs_root;
> +static DEFINE_MUTEX(f2fs_stat_mutex);
>
> static void update_general_status(struct f2fs_sb_info *sbi)
> {
> @@ -180,6 +181,7 @@ static int stat_show(struct seq_file *s, void *v)
> int i = 0;
> int j;
>
> + mutex_lock(&f2fs_stat_mutex);
> list_for_each_entry_safe(si, next, &f2fs_stat_list, stat_list) {
>
> mutex_lock(&si->stat_lock);
> @@ -288,6 +290,7 @@ static int stat_show(struct seq_file *s, void *v)
> si->base_mem >> 10, si->cache_mem >> 10);
> mutex_unlock(&si->stat_lock);
> }
> + mutex_unlock(&f2fs_stat_mutex);
> return 0;
> }
>
> @@ -314,7 +317,10 @@ static int init_stats(struct f2fs_sb_info *sbi)
>
> si = sbi->stat_info;
> mutex_init(&si->stat_lock);
> +
> + mutex_lock(&f2fs_stat_mutex);
> list_add_tail(&si->stat_list, &f2fs_stat_list);
> + mutex_unlock(&f2fs_stat_mutex);
>
> si->all_area_segs = le32_to_cpu(raw_super->segment_count);
> si->sit_area_segs = le32_to_cpu(raw_super->segment_count_sit);
> @@ -347,7 +353,10 @@ void f2fs_destroy_stats(struct f2fs_sb_info *sbi)
> {
> struct f2fs_stat_info *si = sbi->stat_info;
>
> + mutex_lock(&f2fs_stat_mutex);
> list_del(&si->stat_list);
> + mutex_unlock(&f2fs_stat_mutex);
> +
Hi Jianpeng,
Is it possible to fix the issue by holding si->stat_lock while
executing list_del(&si->stat_list) ? this can avoid introducing new
variable.
> mutex_lock(&si->stat_lock);
> si->sbi = NULL;
> mutex_unlock(&si->stat_lock);
> --
> 1.7.9.5
> ------------------------------------------------------------------------------
> Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
> MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
> with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
> MVPs and experts. SALE $99.99 this month only -- learn more at:
> http://p.sf.net/sfu/learnmore_122412
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs:Add mutex_lock to protect f2fs_stat_list.
2013-01-15 2:57 ` [f2fs-dev] [PATCH] f2fs:Add mutex_lock to protect f2fs_stat_list Huajun Li
@ 2013-01-15 7:04 ` Jaegeuk Kim
0 siblings, 0 replies; 3+ messages in thread
From: Jaegeuk Kim @ 2013-01-15 7:04 UTC (permalink / raw)
To: Huajun Li; +Cc: majianpeng, linux-fsdevel, linux-f2fs-devel
[-- Attachment #1: Type: text/plain, Size: 3325 bytes --]
Hi,
2013-01-15 (화), 10:57 +0800, Huajun Li:
> On Mon, Jan 14, 2013 at 8:08 PM, majianpeng <majianpeng@gmail.com> wrote:
> > There is an race between umount f2fs and read f2fs/status.
> > It will case oops.
> > Fox example:
> > Thread A Thread B
> > umount f2fs cat f2fs/status
> > f2fs_destroy_stats() { stat_show() {
> > list_for_each_entry_safe(&f2fs_stat_list)
> > list_del(&si->stat_list);
> > mutex_lock(&si->stat_lock);
> > si->sbi = NULL;
> > mutex_unlock(&si->stat_lock);
> > kfree(sbi->stat_info);
> > mutex_lock(&si->stat_lock)
> >
Nice catch.
Actually, &si->stat_lock was introduced to cope with this issue.
So, we need to remove the existing &si->stat_lock, and add your global
f2fs_stat_mutex.
Then, we're able to remove "si->sbi = NULL"-related stuffs too.
> > Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
> > ---
> > fs/f2fs/debug.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
> > index 0e0380a..ec6d004 100644
> > --- a/fs/f2fs/debug.c
> > +++ b/fs/f2fs/debug.c
> > @@ -26,6 +26,7 @@
> >
> > static LIST_HEAD(f2fs_stat_list);
> > static struct dentry *debugfs_root;
> > +static DEFINE_MUTEX(f2fs_stat_mutex);
> >
> > static void update_general_status(struct f2fs_sb_info *sbi)
> > {
> > @@ -180,6 +181,7 @@ static int stat_show(struct seq_file *s, void *v)
> > int i = 0;
> > int j;
> >
> > + mutex_lock(&f2fs_stat_mutex);
> > list_for_each_entry_safe(si, next, &f2fs_stat_list, stat_list) {
> >
> > mutex_lock(&si->stat_lock);
> > @@ -288,6 +290,7 @@ static int stat_show(struct seq_file *s, void *v)
> > si->base_mem >> 10, si->cache_mem >> 10);
> > mutex_unlock(&si->stat_lock);
> > }
> > + mutex_unlock(&f2fs_stat_mutex);
> > return 0;
> > }
> >
> > @@ -314,7 +317,10 @@ static int init_stats(struct f2fs_sb_info *sbi)
> >
> > si = sbi->stat_info;
> > mutex_init(&si->stat_lock);
> > +
> > + mutex_lock(&f2fs_stat_mutex);
> > list_add_tail(&si->stat_list, &f2fs_stat_list);
> > + mutex_unlock(&f2fs_stat_mutex);
> >
> > si->all_area_segs = le32_to_cpu(raw_super->segment_count);
> > si->sit_area_segs = le32_to_cpu(raw_super->segment_count_sit);
> > @@ -347,7 +353,10 @@ void f2fs_destroy_stats(struct f2fs_sb_info *sbi)
> > {
> > struct f2fs_stat_info *si = sbi->stat_info;
> >
> > + mutex_lock(&f2fs_stat_mutex);
> > list_del(&si->stat_list);
> > + mutex_unlock(&f2fs_stat_mutex);
> > +
>
> Hi Jianpeng,
> Is it possible to fix the issue by holding si->stat_lock while
> executing list_del(&si->stat_list) ?
I think it cannot fix the problem.
Still the above errorneous scenario is able to be occurred.
Thank you,
--
Jaegeuk Kim
Samsung
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] f2fs:Add mutex_lock to protect f2fs_stat_list.
[not found] <201301142008133402501@gmail.com>
2013-01-15 2:57 ` [f2fs-dev] [PATCH] f2fs:Add mutex_lock to protect f2fs_stat_list Huajun Li
@ 2013-01-15 7:29 ` Jaegeuk Kim
1 sibling, 0 replies; 3+ messages in thread
From: Jaegeuk Kim @ 2013-01-15 7:29 UTC (permalink / raw)
To: majianpeng; +Cc: linux-f2fs-devel, linux-fsdevel
[-- Attachment #1: Type: text/plain, Size: 3683 bytes --]
Hi Jianpeng, again. :)
How about this patch based on your original patch?
Could you test this?
Thanks,
From 74c4159102ff4808cc59ea8513bd276605c30972 Mon Sep 17 00:00:00 2001
From: majianpeng <majianpeng@gmail.com>
Date: Mon, 14 Jan 2013 20:08:16 +0800
Subject: [PATCH] f2fs: add global mutex_lock to protect f2fs_stat_list
There is an race condition between umounting f2fs and reading
f2fs/status, which results in oops.
Fox example:
Thread A Thread B
umount f2fs cat f2fs/status
f2fs_destroy_stats() { stat_show() {
list_for_each_entry_safe(&f2fs_stat_list)
list_del(&si->stat_list);
mutex_lock(&si->stat_lock);
si->sbi = NULL;
mutex_unlock(&si->stat_lock);
kfree(sbi->stat_info);
} mutex_lock(&si->stat_lock) <- si is gone.
...
}
Solution with a global lock: f2fs_stat_mutex:
Thread A Thread B
umount f2fs cat f2fs/status
f2fs_destroy_stats() { stat_show() {
mutex_lock(&f2fs_stat_mutex);
list_del(&si->stat_list);
mutex_unlock(&f2fs_stat_mutex);
kfree(sbi->stat_info); mutex_lock(&f2fs_stat_mutex);
} list_for_each_entry_safe(&f2fs_stat_list)
...
mutex_unlock(&f2fs_stat_mutex);
}
Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
[jaegeuk.kim@samsung.com: fix typos, description, and remove the
existing lock]
Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
fs/f2fs/debug.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
index 0c374d1..818f2ed 100644
--- a/fs/f2fs/debug.c
+++ b/fs/f2fs/debug.c
@@ -26,6 +26,7 @@
static LIST_HEAD(f2fs_stat_list);
static struct dentry *debugfs_root;
+static DEFINE_MUTEX(f2fs_stat_mutex);
static void update_general_status(struct f2fs_sb_info *sbi)
{
@@ -180,13 +181,9 @@ static int stat_show(struct seq_file *s, void *v)
int i = 0;
int j;
+ mutex_lock(&f2fs_stat_mutex);
list_for_each_entry_safe(si, next, &f2fs_stat_list, stat_list) {
- mutex_lock(&si->stat_lock);
- if (!si->sbi) {
- mutex_unlock(&si->stat_lock);
- continue;
- }
update_general_status(si->sbi);
seq_printf(s, "\n=====[ partition info. #%d ]=====\n", i++);
@@ -286,8 +283,8 @@ static int stat_show(struct seq_file *s, void *v)
seq_printf(s, "\nMemory: %u KB = static: %u + cached: %u\n",
(si->base_mem + si->cache_mem) >> 10,
si->base_mem >> 10, si->cache_mem >> 10);
- mutex_unlock(&si->stat_lock);
}
+ mutex_unlock(&f2fs_stat_mutex);
return 0;
}
@@ -313,9 +310,6 @@ static int init_stats(struct f2fs_sb_info *sbi)
return -ENOMEM;
si = sbi->stat_info;
- mutex_init(&si->stat_lock);
- list_add_tail(&si->stat_list, &f2fs_stat_list);
-
si->all_area_segs = le32_to_cpu(raw_super->segment_count);
si->sit_area_segs = le32_to_cpu(raw_super->segment_count_sit);
si->nat_area_segs = le32_to_cpu(raw_super->segment_count_nat);
@@ -325,6 +319,11 @@ static int init_stats(struct f2fs_sb_info *sbi)
si->main_area_zones = si->main_area_sections /
le32_to_cpu(raw_super->secs_per_zone);
si->sbi = sbi;
+
+ mutex_lock(&f2fs_stat_mutex);
+ list_add_tail(&si->stat_list, &f2fs_stat_list);
+ mutex_unlock(&f2fs_stat_mutex);
+
return 0;
}
@@ -348,10 +347,10 @@ void f2fs_destroy_stats(struct f2fs_sb_info *sbi)
{
struct f2fs_stat_info *si = sbi->stat_info;
+ mutex_lock(&f2fs_stat_mutex);
list_del(&si->stat_list);
- mutex_lock(&si->stat_lock);
- si->sbi = NULL;
- mutex_unlock(&si->stat_lock);
+ mutex_unlock(&f2fs_stat_mutex);
+
kfree(sbi->stat_info);
}
--
1.8.0.1.250.gb7973fb
--
Jaegeuk Kim
Samsung
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-01-15 7:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <201301142008133402501@gmail.com>
2013-01-15 2:57 ` [f2fs-dev] [PATCH] f2fs:Add mutex_lock to protect f2fs_stat_list Huajun Li
2013-01-15 7:04 ` Jaegeuk Kim
2013-01-15 7:29 ` Jaegeuk Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).