From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932100AbaIIFNI (ORCPT ); Tue, 9 Sep 2014 01:13:08 -0400 Received: from mail.kernel.org ([198.145.19.201]:54199 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750827AbaIIFNG (ORCPT ); Tue, 9 Sep 2014 01:13:06 -0400 Date: Mon, 8 Sep 2014 22:13:03 -0700 From: Jaegeuk Kim To: huang ying Cc: Huang Ying , Changman Lee , linux-f2fs-devel@lists.sourceforge.net, LKML Subject: Re: [PATCH -v2] f2fs: Remove lock from check_valid_map Message-ID: <20140909051303.GB24581@jaegeuk-mac02.hsd1.ca.comcast.net> References: <1410059104-9060-1-git-send-email-ying.huang@intel.com> <1410061110-31081-1-git-send-email-ying.huang@intel.com> <20140908035024.GA13009@jaegeuk-mac02.hsd1.ca.comcast.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 > > > --- > > > > > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jaegeuk Kim Subject: Re: [PATCH -v2] f2fs: Remove lock from check_valid_map Date: Mon, 8 Sep 2014 22:13:03 -0700 Message-ID: <20140909051303.GB24581@jaegeuk-mac02.hsd1.ca.comcast.net> References: <1410059104-9060-1-git-send-email-ying.huang@intel.com> <1410061110-31081-1-git-send-email-ying.huang@intel.com> <20140908035024.GA13009@jaegeuk-mac02.hsd1.ca.comcast.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from sog-mx-1.v43.ch3.sourceforge.com ([172.29.43.191] helo=mx.sourceforge.net) by sfs-ml-4.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1XRDjt-00016E-DF for linux-f2fs-devel@lists.sourceforge.net; Tue, 09 Sep 2014 05:13:13 +0000 Received: from mail.kernel.org ([198.145.19.201]) by sog-mx-1.v43.ch3.sourceforge.com with esmtp (Exim 4.76) id 1XRDjr-0002dK-8h for linux-f2fs-devel@lists.sourceforge.net; Tue, 09 Sep 2014 05:13:13 +0000 Content-Disposition: inline In-Reply-To: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linux-f2fs-devel-bounces@lists.sourceforge.net To: huang ying Cc: linux-f2fs-devel@lists.sourceforge.net, 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 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 > > > --- > > > > > > 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