* [PATCH] f2fs: Remove lock from check_valid_map @ 2014-09-07 3:05 ` Huang Ying 0 siblings, 0 replies; 12+ messages in thread From: Huang Ying @ 2014-09-07 3:05 UTC (permalink / raw) To: Jaegeuk Kim, Changman Lee; +Cc: linux-f2fs-devel, linux-kernel, 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; } ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] f2fs: Remove lock from check_valid_map @ 2014-09-07 3:05 ` Huang Ying 0 siblings, 0 replies; 12+ messages in thread From: Huang Ying @ 2014-09-07 3:05 UTC (permalink / raw) To: Jaegeuk Kim, Changman Lee; +Cc: Huang Ying, linux-kernel, linux-f2fs-devel 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/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH -v2] f2fs: Remove lock from check_valid_map 2014-09-07 3:05 ` Huang Ying @ 2014-09-07 3:38 ` Huang Ying -1 siblings, 0 replies; 12+ messages in thread From: Huang Ying @ 2014-09-07 3:38 UTC (permalink / raw) To: Jaegeuk Kim, Changman Lee; +Cc: linux-f2fs-devel, linux-kernel, 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; } ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH -v2] f2fs: Remove lock from check_valid_map @ 2014-09-07 3:38 ` Huang Ying 0 siblings, 0 replies; 12+ messages in thread From: Huang Ying @ 2014-09-07 3:38 UTC (permalink / raw) To: Jaegeuk Kim, Changman Lee; +Cc: Huang Ying, linux-kernel, linux-f2fs-devel 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/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -v2] f2fs: Remove lock from check_valid_map 2014-09-07 3:38 ` Huang Ying @ 2014-09-08 3:50 ` Jaegeuk Kim -1 siblings, 0 replies; 12+ messages in thread From: Jaegeuk Kim @ 2014-09-08 3:50 UTC (permalink / raw) To: Huang Ying; +Cc: Changman Lee, linux-f2fs-devel, linux-kernel 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? > } > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -v2] f2fs: Remove lock from check_valid_map @ 2014-09-08 3:50 ` Jaegeuk Kim 0 siblings, 0 replies; 12+ messages in thread From: Jaegeuk Kim @ 2014-09-08 3:50 UTC (permalink / raw) To: Huang Ying; +Cc: linux-kernel, linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <CAC=cRTOdvOp=zBT986SqGXC2+iRxGzSgKdFYzTQjbAamYsGVsg@mail.gmail.com>]
* Re: [PATCH -v2] f2fs: Remove lock from check_valid_map [not found] ` <CAC=cRTOdvOp=zBT986SqGXC2+iRxGzSgKdFYzTQjbAamYsGVsg@mail.gmail.com> @ 2014-09-09 5:13 ` Jaegeuk Kim 0 siblings, 0 replies; 12+ messages in thread From: Jaegeuk Kim @ 2014-09-09 5:13 UTC (permalink / raw) To: huang ying; +Cc: Huang Ying, Changman Lee, linux-f2fs-devel, LKML 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -v2] f2fs: Remove lock from check_valid_map @ 2014-09-09 5:13 ` Jaegeuk Kim 0 siblings, 0 replies; 12+ messages in thread From: Jaegeuk Kim @ 2014-09-09 5:13 UTC (permalink / raw) To: huang ying; +Cc: linux-f2fs-devel, LKML, Huang Ying 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -v2] f2fs: Remove lock from check_valid_map 2014-09-09 5:13 ` Jaegeuk Kim @ 2014-09-09 5:43 ` Huang Ying -1 siblings, 0 replies; 12+ messages in thread From: Huang Ying @ 2014-09-09 5:43 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: huang ying, Changman Lee, linux-f2fs-devel, LKML 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -v2] f2fs: Remove lock from check_valid_map @ 2014-09-09 5:43 ` Huang Ying 0 siblings, 0 replies; 12+ messages in thread From: Huang Ying @ 2014-09-09 5:43 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: huang ying, LKML, linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -v2] f2fs: Remove lock from check_valid_map 2014-09-09 5:43 ` Huang Ying (?) @ 2014-09-09 7:41 ` Jaegeuk Kim 2014-09-09 8:04 ` Huang Ying -1 siblings, 1 reply; 12+ messages in thread From: Jaegeuk Kim @ 2014-09-09 7:41 UTC (permalink / raw) To: Huang Ying; +Cc: huang ying, Changman Lee, linux-f2fs-devel, LKML 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -v2] f2fs: Remove lock from check_valid_map 2014-09-09 7:41 ` Jaegeuk Kim @ 2014-09-09 8:04 ` Huang Ying 0 siblings, 0 replies; 12+ messages in thread From: Huang Ying @ 2014-09-09 8:04 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: huang ying, Changman Lee, linux-f2fs-devel, LKML 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-09-09 8:04 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-07 3:05 [PATCH] f2fs: Remove lock from check_valid_map Huang Ying
2014-09-07 3:05 ` Huang Ying
2014-09-07 3:38 ` [PATCH -v2] " Huang Ying
2014-09-07 3:38 ` Huang Ying
2014-09-08 3:50 ` Jaegeuk Kim
2014-09-08 3:50 ` Jaegeuk Kim
[not found] ` <CAC=cRTOdvOp=zBT986SqGXC2+iRxGzSgKdFYzTQjbAamYsGVsg@mail.gmail.com>
2014-09-09 5:13 ` Jaegeuk Kim
2014-09-09 5:13 ` Jaegeuk Kim
2014-09-09 5:43 ` Huang Ying
2014-09-09 5:43 ` Huang Ying
2014-09-09 7:41 ` Jaegeuk Kim
2014-09-09 8:04 ` Huang Ying
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.