linux-kernel - Re: [PATCH -v2] f2fs: Remove lock from check_valid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 8 Sep 2014 22:13:03 -0700
From:	Jaegeuk Kim <jaegeuk@...nel.org>
To:	huang ying <huang.ying.caritas@...il.com>
Cc:	Huang Ying <ying.huang@...el.com>,
	Changman Lee <cm224.lee@...sung.com>,
	linux-f2fs-devel@...ts.sourceforge.net,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -v2] f2fs: Remove lock from check_valid_map

Hi Huang,

On Mon, Sep 08, 2014 at 03:36:35PM +0800, huang ying wrote:
> Hi, Jaegeuk,
> 
> On Mon, Sep 8, 2014 at 11:50 AM, Jaegeuk Kim <jaegeuk@...nel.org> wrote:
> 
> > Hi,
> >
> > On Sun, Sep 07, 2014 at 11:38:30AM +0800, Huang Ying wrote:
> > > Only one bit is read in check_valid_map, holding a lock to do that
> > > doesn't help anything except decreasing performance.
> > >
> > > Signed-off-by: Huang, Ying <ying.huang@...el.com>
> > > ---
> > >
> > > v2: Fixed a build warning.
> > >
> > > ---
> > >  fs/f2fs/gc.c |    3 ---
> > >  1 file changed, 3 deletions(-)
> > >
> > > --- a/fs/f2fs/gc.c
> > > +++ b/fs/f2fs/gc.c
> > > @@ -378,14 +378,11 @@ static void put_gc_inode(struct list_hea
> > >  static int check_valid_map(struct f2fs_sb_info *sbi,
> > >                               unsigned int segno, int offset)
> > >  {
> > > -     struct sit_info *sit_i = SIT_I(sbi);
> > >       struct seg_entry *sentry;
> > >       int ret;
> > >
> > > -     mutex_lock(&sit_i->sentry_lock);
> > >       sentry = get_seg_entry(sbi, segno);
> > >       ret = f2fs_test_bit(offset, sentry->cur_valid_map);
> > > -     mutex_unlock(&sit_i->sentry_lock);
> > >       return ret;
> >
> > The f2fs_test_bit is not atomic, so I'm not sure this is a good approach.
> > How about introducing rw_semaphore?
> >
> 
> IMO, f2fs_test_bit just read a global variable (a byte in cur_valid_map),
> then check its value. The byte may be changed in another CPU concurrently.
> But even protected with a mutex, it can be changed in another CPU
> immediately after mutex_unlock.  So mutex does not help  here.  Here we
> just read a global variable, not read/modify/write, so, we don't need
> atomic too.

Hmm. This is a pretty hard corner case to allow the mutex removal under the
following assumption.

1. All the sit entries are cached in a global array, which means that it never
happens that any sit entry pointers are changed.

2. I agree that f2fs_gc tries to conduct the cleaning with best effort, and
it triggers again when it detects there is something to do more.
So, check_valid_bitmap doesn't need to make a precise decision.

But, what I concern is the consistent policy to use such the mutex.
If we break the rule, it becomes harder to debug potential bugs.

Anyway, have you been facing with such the lock contention?

Thanks,

> 
> Best Regards,
> Huang, Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/