From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gu Zheng Subject: Re: [PATCH] f2fs: optimize fs_lock for better performance Date: Wed, 11 Sep 2013 13:37:58 +0800 Message-ID: <523001B6.3080506@cn.fujitsu.com> References: <88.C4.11914.9D4A9225@epcpsbge6.samsung.com> <1378774324.2354.103.camel@kjgkr> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from sog-mx-3.v43.ch3.sourceforge.com ([172.29.43.193] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1VJdCb-0004qw-MI for linux-f2fs-devel@lists.sourceforge.net; Wed, 11 Sep 2013 05:42:57 +0000 Received: from [222.73.24.84] (helo=song.cn.fujitsu.com) by sog-mx-3.v43.ch3.sourceforge.com with esmtp (Exim 4.76) id 1VJdCY-0005IF-EQ for linux-f2fs-devel@lists.sourceforge.net; Wed, 11 Sep 2013 05:42:57 +0000 In-Reply-To: <1378774324.2354.103.camel@kjgkr> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linux-f2fs-devel-bounces@lists.sourceforge.net To: jaegeuk.kim@samsung.com Cc: linux-fsdevel@vger.kernel.org, shu.tan@samsung.com, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net SGkgSmFlZ2V1aywgQ2hhbywKCk9uIDA5LzEwLzIwMTMgMDg6NTIgQU0sIEphZWdldWsgS2ltIHdy b3RlOgoKPiBIaSwKPiAKPiBBdCBmaXJzdCwgdGhhbmsgeW91IGZvciB0aGUgcmVwb3J0IGFuZCBw bGVhc2UgZm9sbG93IHRoZSBlbWFpbCB3cml0aW5nCj4gcnVsZXMuIDopCj4gCj4gQW55d2F5LCBJ IGFncmVlIHRvIHRoZSBiZWxvdyBpc3N1ZS4KPiBPbmUgdGhpbmcgdGhhdCBJIGNhbiB0aGluayBv ZiBpcyB0aGF0IHdlIGRvbid0IG5lZWQgdG8gdXNlIHRoZQo+IHNwaW5fbG9jaywgc2luY2Ugd2Ug ZG9uJ3QgY2FyZSBhYm91dCB0aGUgZXhhY3QgbG9jayBudW1iZXIsIGJ1dCBqdXN0Cj4gbmVlZCB0 byBnZXQgYW55IG5vdC1jb2xsaWRlZCBudW1iZXIuCgpJTUhPLCBqdXN0IG1vdmluZyBzYmktPm5l eHRfbG9ja19udW0rKyBiZWZvcmUgbXV0ZXhfbG9jaygmc2JpLT5mc19sb2NrW25leHRfbG9ja10p CmNhbiBhdm9pZCB1bmJhbGFuY2UgaXNzdWUgbW9zdGx5LgpJTU8sIHRoZSBjYXNlIHR3byBvciBt b3JlIHRocmVhZHMgaW5jcmVhc2Ugc2JpLT5uZXh0X2xvY2tfbnVtIGluIHRoZSBzYW1lIHRpbWUg aXMKcmVhbGx5IHZlcnkgdmVyeSBsaXR0bGUuIElmIHlvdSB0aGluayBpdCBpcyBub3Qgcmlnb3Jv dXMsIGNoYW5nZSBuZXh0X2xvY2tfbnVtIHRvCmF0b21pYyBvbmUgY2FuIGZpeCBpdC4KV2hhdCdz IHlvdXIgb3Bpbmlvbj8KClJlZ2FyZHMsCkd1Cgo+IAo+IFNvLCBob3cgYWJvdXQgcmVtb3Zpbmcg dGhlIHNwaW5fbG9jaz8KPiBBbmQgaG93IGFib3V0IHVzaW5nIGEgcmFuZG9tIG51bWJlcj8KCj4g VGhhbmtzLAo+IAo+IDIwMTMtMDktMDYgKOq4iCksIDA5OjQ4ICswMDAwLCBDaGFvIFl1Ogo+PiBI aSBLaW06Cj4+Cj4+ICAgICAgSSB0aGluayB0aGVyZSBpcyBhIHBlcmZvcm1hbmNlIHByb2JsZW06 IHdoZW4gYWxsIHNiaS0+ZnNfbG9jayBpcwo+PiBob2xkZWQsIAo+Pgo+PiB0aGVuIGFsbCBvdGhl ciB0aHJlYWRzIG1heSBnZXQgdGhlIHNhbWUgbmV4dF9sb2NrIHZhbHVlIGZyb20KPj4gc2JpLT5u ZXh0X2xvY2tfbnVtIGluIGZ1bmN0aW9uIG11dGV4X2xvY2tfb3AsIAo+Pgo+PiBhbmQgd2FpdCB0 byBnZXQgdGhlIHNhbWUgbG9jayBhdCBwb3NpdGlvbiBmc19sb2NrW25leHRfbG9ja10sIGl0Cj4+ IHVuYmFsYW5jZSB0aGUgZnNfbG9jayB1c2FnZS4gCj4+Cj4+IEl0IG1heSBsb3N0IHBlcmZvcm1h bmNlIHdoZW4gd2UgZG8gdGhlIG11bHRpdGhyZWFkIHRlc3QuCj4+Cj4+ICAKPj4KPj4gSGVyZSBp cyB0aGUgcGF0Y2ggdG8gZml4IHRoaXMgcHJvYmxlbToKPj4KPj4gIAo+Pgo+PiBTaWduZWQtb2Zm LWJ5OiBZdSBDaGFvIDxjaGFvMi55dUBzYW1zdW5nLmNvbT4KPj4KPj4gZGlmZiAtLWdpdCBhL2Zz L2YyZnMvZjJmcy5oIGIvZnMvZjJmcy9mMmZzLmgKPj4KPj4gb2xkIG1vZGUgMTAwNjQ0Cj4+Cj4+ IG5ldyBtb2RlIDEwMDc1NQo+Pgo+PiBpbmRleCA0NjdkNDJkLi45ODNiYjQ1Cj4+Cj4+IC0tLSBh L2ZzL2YyZnMvZjJmcy5oCj4+Cj4+ICsrKyBiL2ZzL2YyZnMvZjJmcy5oCj4+Cj4+IEBAIC0zNzEs NiArMzcxLDcgQEAgc3RydWN0IGYyZnNfc2JfaW5mbyB7Cj4+Cj4+ICAgICAgICAgc3RydWN0IG11 dGV4IGZzX2xvY2tbTlJfR0xPQkFMX0xPQ0tTXTsgIC8qIGJsb2NraW5nIEZTCj4+IG9wZXJhdGlv bnMgKi8KPj4KPj4gICAgICAgICBzdHJ1Y3QgbXV0ZXggbm9kZV93cml0ZTsgICAgICAgICAgICAg ICAgLyogbG9ja2luZyBub2RlIHdyaXRlcwo+PiAqLwo+Pgo+PiAgICAgICAgIHN0cnVjdCBtdXRl eCB3cml0ZXBhZ2VzOyAgICAgICAgICAgICAgICAvKiBtdXRleCBmb3IKPj4gd3JpdGVwYWdlcygp ICovCj4+Cj4+ICsgICAgICAgc3BpbmxvY2tfdCBzcGluX2xvY2s7ICAgICAgICAgICAgICAgICAg IC8qIGxvY2sgZm9yCj4+IG5leHRfbG9ja19udW0gKi8KPj4KPj4gICAgICAgICB1bnNpZ25lZCBj aGFyIG5leHRfbG9ja19udW07ICAgICAgICAgICAgLyogcm91bmQtcm9iaW4gZ2xvYmFsCj4+IGxv Y2tzICovCj4+Cj4+ICAgICAgICAgaW50IHBvcl9kb2luZzsgICAgICAgICAgICAgICAgICAgICAg ICAgIC8qIHJlY292ZXJ5IGlzIGRvaW5nCj4+IG9yIG5vdCAqLwo+Pgo+PiAgICAgICAgIGludCBv bl9idWlsZF9mcmVlX25pZHM7ICAgICAgICAgICAgICAgICAvKiBidWlsZF9mcmVlX25pZHMgaXMK Pj4gZG9pbmcgKi8KPj4KPj4gQEAgLTUzMywxNSArNTM0LDE5IEBAIHN0YXRpYyBpbmxpbmUgdm9p ZCBtdXRleF91bmxvY2tfYWxsKHN0cnVjdAo+PiBmMmZzX3NiX2luZm8gKnNiaSkKPj4KPj4gIAo+ Pgo+PiAgc3RhdGljIGlubGluZSBpbnQgbXV0ZXhfbG9ja19vcChzdHJ1Y3QgZjJmc19zYl9pbmZv ICpzYmkpCj4+Cj4+ICB7Cj4+Cj4+IC0gICAgICAgdW5zaWduZWQgY2hhciBuZXh0X2xvY2sgPSBz YmktPm5leHRfbG9ja19udW0gJQo+PiBOUl9HTE9CQUxfTE9DS1M7Cj4+Cj4+ICsgICAgICAgdW5z aWduZWQgY2hhciBuZXh0X2xvY2s7Cj4+Cj4+ICAgICAgICAgaW50IGkgPSAwOwo+Pgo+PiAgCj4+ Cj4+ICAgICAgICAgZm9yICg7IGkgPCBOUl9HTE9CQUxfTE9DS1M7IGkrKykKPj4KPj4gICAgICAg ICAgICAgICAgIGlmIChtdXRleF90cnlsb2NrKCZzYmktPmZzX2xvY2tbaV0pKQo+Pgo+PiAgICAg ICAgICAgICAgICAgICAgICAgICByZXR1cm4gaTsKPj4KPj4gIAo+Pgo+PiAtICAgICAgIG11dGV4 X2xvY2soJnNiaS0+ZnNfbG9ja1tuZXh0X2xvY2tdKTsKPj4KPj4gKyAgICAgICBzcGluX2xvY2so JnNiaS0+c3Bpbl9sb2NrKTsKPj4KPj4gKyAgICAgICBuZXh0X2xvY2sgPSBzYmktPm5leHRfbG9j a19udW0gJSBOUl9HTE9CQUxfTE9DS1M7Cj4+Cj4+ICAgICAgICAgc2JpLT5uZXh0X2xvY2tfbnVt Kys7Cj4+Cj4+ICsgICAgICAgc3Bpbl91bmxvY2soJnNiaS0+c3Bpbl9sb2NrKTsKPj4KPj4gKwo+ Pgo+PiArICAgICAgIG11dGV4X2xvY2soJnNiaS0+ZnNfbG9ja1tuZXh0X2xvY2tdKTsKPj4KPj4g ICAgICAgICByZXR1cm4gbmV4dF9sb2NrOwo+Pgo+PiAgfQo+Pgo+PiAgCj4+Cj4+IGRpZmYgLS1n aXQgYS9mcy9mMmZzL3N1cGVyLmMgYi9mcy9mMmZzL3N1cGVyLmMKPj4KPj4gb2xkIG1vZGUgMTAw NjQ0Cj4+Cj4+IG5ldyBtb2RlIDEwMDc1NQo+Pgo+PiBpbmRleCA3NWM3ZGMzLi40ZjI3NTk2Cj4+ Cj4+IC0tLSBhL2ZzL2YyZnMvc3VwZXIuYwo+Pgo+PiArKysgYi9mcy9mMmZzL3N1cGVyLmMKPj4K Pj4gQEAgLTY1Nyw2ICs2NTcsNyBAQCBzdGF0aWMgaW50IGYyZnNfZmlsbF9zdXBlcihzdHJ1Y3Qg c3VwZXJfYmxvY2sgKnNiLAo+PiB2b2lkICpkYXRhLCBpbnQgc2lsZW50KQo+Pgo+PiAgICAgICAg IG11dGV4X2luaXQoJnNiaS0+Y3BfbXV0ZXgpOwo+Pgo+PiAgICAgICAgIGZvciAoaSA9IDA7IGkg PCBOUl9HTE9CQUxfTE9DS1M7IGkrKykKPj4KPj4gICAgICAgICAgICAgICAgIG11dGV4X2luaXQo JnNiaS0+ZnNfbG9ja1tpXSk7Cj4+Cj4+ICsgICAgICAgc3Bpbl9sb2NrX2luaXQoJnNiaS0+c3Bp bl9sb2NrKTsKPj4KPj4gICAgICAgICBtdXRleF9pbml0KCZzYmktPm5vZGVfd3JpdGUpOwo+Pgo+ PiAgICAgICAgIHNiaS0+cG9yX2RvaW5nID0gMDsKPj4KPj4gICAgICAgICBzcGluX2xvY2tfaW5p dCgmc2JpLT5zdGF0X2xvY2spOwo+Pgo+PiAoRU5EKQo+Pgo+PiAgCj4+Cj4+Cj4+Cj4+Cj4gCgoK Ci0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLQpIb3cgU2VydmljZU5vdyBoZWxwcyBJVCBwZW9wbGUgdHJh bnNmb3JtIElUIGRlcGFydG1lbnRzOgoxLiBDb25zb2xpZGF0ZSBsZWdhY3kgSVQgc3lzdGVtcyB0 byBhIHNpbmdsZSBzeXN0ZW0gb2YgcmVjb3JkIGZvciBJVAoyLiBTdGFuZGFyZGl6ZSBhbmQgZ2xv YmFsaXplIHNlcnZpY2UgcHJvY2Vzc2VzIGFjcm9zcyBJVAozLiBJbXBsZW1lbnQgemVyby10b3Vj aCBhdXRvbWF0aW9uIHRvIHJlcGxhY2UgbWFudWFsLCByZWR1bmRhbnQgdGFza3MKaHR0cDovL3B1 YmFkcy5nLmRvdWJsZWNsaWNrLm5ldC9nYW1wYWQvY2xrP2lkPTUxMjcxMTExJml1PS80MTQwL29z dGcuY2xrdHJrCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f CkxpbnV4LWYyZnMtZGV2ZWwgbWFpbGluZyBsaXN0CkxpbnV4LWYyZnMtZGV2ZWxAbGlzdHMuc291 cmNlZm9yZ2UubmV0Cmh0dHBzOi8vbGlzdHMuc291cmNlZm9yZ2UubmV0L2xpc3RzL2xpc3RpbmZv L2xpbnV4LWYyZnMtZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757494Ab3IKFmx (ORCPT ); Wed, 11 Sep 2013 01:42:53 -0400 Received: from cn.fujitsu.com ([222.73.24.84]:33066 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1754847Ab3IKFmt convert rfc822-to-8bit (ORCPT ); Wed, 11 Sep 2013 01:42:49 -0400 X-IronPort-AV: E=Sophos;i="4.90,882,1371052800"; d="scan'208";a="8485439" Message-ID: <523001B6.3080506@cn.fujitsu.com> Date: Wed, 11 Sep 2013 13:37:58 +0800 From: Gu Zheng User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20110930 Thunderbird/7.0.1 MIME-Version: 1.0 To: jaegeuk.kim@samsung.com CC: chao2.yu@samsung.com, shu.tan@samsung.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev][PATCH] f2fs: optimize fs_lock for better performance References: <88.C4.11914.9D4A9225@epcpsbge6.samsung.com> <1378774324.2354.103.camel@kjgkr> In-Reply-To: <1378774324.2354.103.camel@kjgkr> X-MIMETrack: Itemize by SMTP Server on mailserver/fnst(Release 8.5.3|September 15, 2011) at 2013/09/11 13:40:25, Serialize by Router on mailserver/fnst(Release 8.5.3|September 15, 2011) at 2013/09/11 13:40:26 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jaegeuk, Chao, On 09/10/2013 08:52 AM, Jaegeuk Kim wrote: > Hi, > > At first, thank you for the report and please follow the email writing > rules. :) > > Anyway, I agree to the below issue. > One thing that I can think of is that we don't need to use the > spin_lock, since we don't care about the exact lock number, but just > need to get any not-collided number. IMHO, just moving sbi->next_lock_num++ before mutex_lock(&sbi->fs_lock[next_lock]) can avoid unbalance issue mostly. IMO, the case two or more threads increase sbi->next_lock_num in the same time is really very very little. If you think it is not rigorous, change next_lock_num to atomic one can fix it. What's your opinion? Regards, Gu > > So, how about removing the spin_lock? > And how about using a random number? > Thanks, > > 2013-09-06 (금), 09:48 +0000, Chao Yu: >> Hi Kim: >> >> I think there is a performance problem: when all sbi->fs_lock is >> holded, >> >> then all other threads may get the same next_lock value from >> sbi->next_lock_num in function mutex_lock_op, >> >> and wait to get the same lock at position fs_lock[next_lock], it >> unbalance the fs_lock usage. >> >> It may lost performance when we do the multithread test. >> >> >> >> Here is the patch to fix this problem: >> >> >> >> Signed-off-by: Yu Chao >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> >> old mode 100644 >> >> new mode 100755 >> >> index 467d42d..983bb45 >> >> --- a/fs/f2fs/f2fs.h >> >> +++ b/fs/f2fs/f2fs.h >> >> @@ -371,6 +371,7 @@ struct f2fs_sb_info { >> >> struct mutex fs_lock[NR_GLOBAL_LOCKS]; /* blocking FS >> operations */ >> >> struct mutex node_write; /* locking node writes >> */ >> >> struct mutex writepages; /* mutex for >> writepages() */ >> >> + spinlock_t spin_lock; /* lock for >> next_lock_num */ >> >> unsigned char next_lock_num; /* round-robin global >> locks */ >> >> int por_doing; /* recovery is doing >> or not */ >> >> int on_build_free_nids; /* build_free_nids is >> doing */ >> >> @@ -533,15 +534,19 @@ static inline void mutex_unlock_all(struct >> f2fs_sb_info *sbi) >> >> >> >> static inline int mutex_lock_op(struct f2fs_sb_info *sbi) >> >> { >> >> - unsigned char next_lock = sbi->next_lock_num % >> NR_GLOBAL_LOCKS; >> >> + unsigned char next_lock; >> >> int i = 0; >> >> >> >> for (; i < NR_GLOBAL_LOCKS; i++) >> >> if (mutex_trylock(&sbi->fs_lock[i])) >> >> return i; >> >> >> >> - mutex_lock(&sbi->fs_lock[next_lock]); >> >> + spin_lock(&sbi->spin_lock); >> >> + next_lock = sbi->next_lock_num % NR_GLOBAL_LOCKS; >> >> sbi->next_lock_num++; >> >> + spin_unlock(&sbi->spin_lock); >> >> + >> >> + mutex_lock(&sbi->fs_lock[next_lock]); >> >> return next_lock; >> >> } >> >> >> >> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >> >> old mode 100644 >> >> new mode 100755 >> >> index 75c7dc3..4f27596 >> >> --- a/fs/f2fs/super.c >> >> +++ b/fs/f2fs/super.c >> >> @@ -657,6 +657,7 @@ static int f2fs_fill_super(struct super_block *sb, >> void *data, int silent) >> >> mutex_init(&sbi->cp_mutex); >> >> for (i = 0; i < NR_GLOBAL_LOCKS; i++) >> >> mutex_init(&sbi->fs_lock[i]); >> >> + spin_lock_init(&sbi->spin_lock); >> >> mutex_init(&sbi->node_write); >> >> sbi->por_doing = 0; >> >> spin_lock_init(&sbi->stat_lock); >> >> (END) >> >> >> >> >> >> >