Re: [f2fs-dev] [PATCH] f2fs:Add mutex_lock to protect f2fs_stat_list.

[Jaegeuk Kim <jaegeuk.kim@samsung.com>]

[-- 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 --]