[PATCH] f2fs: Remove lock from check_valid_map

[PATCH] f2fs: Remove lock from check_valid_map

[Huang Ying]

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>
---
 fs/f2fs/gc.c |    2 --
 1 file changed, 2 deletions(-)

--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -382,10 +382,8 @@ static int check_valid_map(struct f2fs_s
 	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;
 }
 

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

[PATCH -v2] f2fs: Remove lock from check_valid_map

[Huang Ying]

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;
 }
 

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

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

[Jaegeuk Kim]

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?

>  }
>  

------------------------------------------------------------------------------
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk

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

[Jaegeuk Kim]

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.

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

Thanks,

> 
> Best Regards,
> Huang, Ying

------------------------------------------------------------------------------
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk

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

[Huang Ying]

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.  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



------------------------------------------------------------------------------
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce.
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk

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

[Jaegeuk Kim]

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.

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

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

[Huang Ying]

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