Re: [PATCH -v2] f2fs: Remove lock from check_valid_map

[Huang Ying <ying.huang@intel.com>]

On Tue, 2014-09-09 at 00:41 -0700, Jaegeuk Kim wrote:
> On Tue, Sep 09, 2014 at 01:43:46PM +0800, Huang Ying wrote:
> > On Mon, 2014-09-08 at 22:13 -0700, Jaegeuk Kim wrote:
> > > 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@kernel.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@intel.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.
> > 
> > Yes.  We definitely need a rule.  But I suggest to make a small tweak to
> > the rule.
> 
> I don't think there is enough reason that we should take a small tweak while
> breaking the locking policy. It's related to neither performance issue nor a
> bug case.

I don't want to break the locking rule.  I just propose a suggestion to
tweak the rule itself a little.  To make something like "If we just read
one variable with fixed address, we need not to use a lock to protect
that." to be part of the rule.

But if you think it is better to use a lock here.  That is not a problem
for me.

Best Regards,
Huang, Ying

> Even if f2fs suffers from lock contention here, I think we need to bet on
> rw_semaphore to satisfy the rule and performance at the same time.
> 
> Thanks,
> 
> > If we just read one variable with fixed address, we need not
> > to use a mutex to protect that.
> > 
> > > Anyway, have you been facing with such the lock contention?
> > 
> > No, I just review the code and thinks the mutex is not necessary.
> > 
> > Best Regards,
> > Huang, Ying