From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ipmail03.adl6.internode.on.net ([150.101.137.143]:63822 "EHLO ipmail03.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751759AbdLKVzE (ORCPT ); Mon, 11 Dec 2017 16:55:04 -0500 Date: Tue, 12 Dec 2017 08:55:00 +1100 From: Dave Chinner To: Matthew Wilcox Cc: Matthew Wilcox , Ross Zwisler , Jens Axboe , Rehas Sachdeva , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-nilfs@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-xfs@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray Message-ID: <20171211215500.GU5858@dastard> References: <20171206012901.GZ4094@dastard> <20171206020208.GK26021@bombadil.infradead.org> <20171206031456.GE4094@dastard> <20171206044549.GO26021@bombadil.infradead.org> <20171206084404.GF4094@dastard> <20171206140648.GB32044@bombadil.infradead.org> <20171207003843.GG4094@dastard> <20171208230131.GC32293@bombadil.infradead.org> <20171210235745.GR5858@dastard> <20171211042315.GA25236@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20171211042315.GA25236@bombadil.infradead.org> Sender: linux-btrfs-owner@vger.kernel.org List-ID: On Sun, Dec 10, 2017 at 08:23:15PM -0800, Matthew Wilcox wrote: > On Mon, Dec 11, 2017 at 10:57:45AM +1100, Dave Chinner wrote: > > i.e. the fact the cmpxchg failed may not have anything to do with a > > race condtion - it failed because the slot wasn't empty like we > > expected it to be. There can be any number of reasons the slot isn't > > empty - the API should not "document" that the reason the insert > > failed was a race condition. It should document the case that we > > "couldn't insert because there was an existing entry in the slot". > > Let the surrounding code document the reason why that might have > > happened - it's not for the API to assume reasons for failure. > > > > i.e. this API and potential internal implementation makes much > > more sense: > > > > int > > xa_store_iff_empty(...) > > { > > curr = xa_cmpxchg(&pag->pag_ici_xa, agino, NULL, ip, GFP_NOFS); > > if (!curr) > > return 0; /* success! */ > > if (!IS_ERR(curr)) > > return -EEXIST; /* failed - slot not empty */ > > return PTR_ERR(curr); /* failed - XA internal issue */ > > } > > > > as it replaces the existing preload and insert code in the XFS code > > paths whilst letting us handle and document the "insert failed > > because slot not empty" case however we want. It implies nothing > > about *why* the slot wasn't empty, just that we couldn't do the > > insert because it wasn't empty. > > Yeah, OK. So, over the top of the recent changes I'm looking at this: > > I'm not in love with xa_store_empty() as a name. I almost want > xa_store_weak(), but after my MAP_FIXED_WEAK proposed name got shot > down, I'm leery of it. "empty" is at least a concept we already have > in the API with the comment for xa_init() talking about an empty array > and xa_empty(). I also considered xa_store_null and xa_overwrite_null > and xa_replace_null(). Naming remains hard. > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > index 941f38bb94a4..586b43836905 100644 > --- a/fs/xfs/xfs_icache.c > +++ b/fs/xfs/xfs_icache.c > @@ -451,7 +451,7 @@ xfs_iget_cache_miss( > int flags, > int lock_flags) > { > - struct xfs_inode *ip, *curr; > + struct xfs_inode *ip; > int error; > xfs_agino_t agino = XFS_INO_TO_AGINO(mp, ino); > int iflags; > @@ -498,8 +498,7 @@ xfs_iget_cache_miss( > xfs_iflags_set(ip, iflags); > > /* insert the new inode */ > - curr = xa_cmpxchg(&pag->pag_ici_xa, agino, NULL, ip, GFP_NOFS); > - error = __xa_race(curr, -EAGAIN); > + error = xa_store_empty(&pag->pag_ici_xa, agino, ip, GFP_NOFS, -EAGAIN); > if (error) > goto out_unlock; Can we avoid encoding the error to be returned into the function call? No other generic/library API does this, so this seems like a highly unusual special snowflake approach. I just don't see how creating a whole new error specification convention is justified for the case where it *might* save a line or two of error checking code in a caller? I'd much prefer that the API defines the error on failure, and let the caller handle that specific error however they need to like every other library interface we have... Cheers, Dave. -- Dave Chinner david@fromorbit.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Chinner Subject: Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray Date: Tue, 12 Dec 2017 08:55:00 +1100 Message-ID: <20171211215500.GU5858@dastard> References: <20171206012901.GZ4094@dastard> <20171206020208.GK26021@bombadil.infradead.org> <20171206031456.GE4094@dastard> <20171206044549.GO26021@bombadil.infradead.org> <20171206084404.GF4094@dastard> <20171206140648.GB32044@bombadil.infradead.org> <20171207003843.GG4094@dastard> <20171208230131.GC32293@bombadil.infradead.org> <20171210235745.GR5858@dastard> <20171211042315.GA25236@bombadil.infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20171211042315.GA25236@bombadil.infradead.org> Sender: owner-linux-mm@kvack.org To: Matthew Wilcox Cc: Matthew Wilcox , Ross Zwisler , Jens Axboe , Rehas Sachdeva , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-nilfs@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-xfs@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-f2fs-devel.lists.sourceforge.net On Sun, Dec 10, 2017 at 08:23:15PM -0800, Matthew Wilcox wrote: > On Mon, Dec 11, 2017 at 10:57:45AM +1100, Dave Chinner wrote: > > i.e. the fact the cmpxchg failed may not have anything to do with a > > race condtion - it failed because the slot wasn't empty like we > > expected it to be. There can be any number of reasons the slot isn't > > empty - the API should not "document" that the reason the insert > > failed was a race condition. It should document the case that we > > "couldn't insert because there was an existing entry in the slot". > > Let the surrounding code document the reason why that might have > > happened - it's not for the API to assume reasons for failure. > > > > i.e. this API and potential internal implementation makes much > > more sense: > > > > int > > xa_store_iff_empty(...) > > { > > curr = xa_cmpxchg(&pag->pag_ici_xa, agino, NULL, ip, GFP_NOFS); > > if (!curr) > > return 0; /* success! */ > > if (!IS_ERR(curr)) > > return -EEXIST; /* failed - slot not empty */ > > return PTR_ERR(curr); /* failed - XA internal issue */ > > } > > > > as it replaces the existing preload and insert code in the XFS code > > paths whilst letting us handle and document the "insert failed > > because slot not empty" case however we want. It implies nothing > > about *why* the slot wasn't empty, just that we couldn't do the > > insert because it wasn't empty. > > Yeah, OK. So, over the top of the recent changes I'm looking at this: > > I'm not in love with xa_store_empty() as a name. I almost want > xa_store_weak(), but after my MAP_FIXED_WEAK proposed name got shot > down, I'm leery of it. "empty" is at least a concept we already have > in the API with the comment for xa_init() talking about an empty array > and xa_empty(). I also considered xa_store_null and xa_overwrite_null > and xa_replace_null(). Naming remains hard. > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > index 941f38bb94a4..586b43836905 100644 > --- a/fs/xfs/xfs_icache.c > +++ b/fs/xfs/xfs_icache.c > @@ -451,7 +451,7 @@ xfs_iget_cache_miss( > int flags, > int lock_flags) > { > - struct xfs_inode *ip, *curr; > + struct xfs_inode *ip; > int error; > xfs_agino_t agino = XFS_INO_TO_AGINO(mp, ino); > int iflags; > @@ -498,8 +498,7 @@ xfs_iget_cache_miss( > xfs_iflags_set(ip, iflags); > > /* insert the new inode */ > - curr = xa_cmpxchg(&pag->pag_ici_xa, agino, NULL, ip, GFP_NOFS); > - error = __xa_race(curr, -EAGAIN); > + error = xa_store_empty(&pag->pag_ici_xa, agino, ip, GFP_NOFS, -EAGAIN); > if (error) > goto out_unlock; Can we avoid encoding the error to be returned into the function call? No other generic/library API does this, so this seems like a highly unusual special snowflake approach. I just don't see how creating a whole new error specification convention is justified for the case where it *might* save a line or two of error checking code in a caller? I'd much prefer that the API defines the error on failure, and let the caller handle that specific error however they need to like every other library interface we have... Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v4,72/73] xfs: Convert mru cache to XArray From: Dave Chinner Message-Id: <20171211215500.GU5858@dastard> Date: Tue, 12 Dec 2017 08:55:00 +1100 To: Matthew Wilcox Cc: Matthew Wilcox , Ross Zwisler , Jens Axboe , Rehas Sachdeva , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-nilfs@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-xfs@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org List-ID: T24gU3VuLCBEZWMgMTAsIDIwMTcgYXQgMDg6MjM6MTVQTSAtMDgwMCwgTWF0dGhldyBXaWxjb3gg d3JvdGU6Cj4gT24gTW9uLCBEZWMgMTEsIDIwMTcgYXQgMTA6NTc6NDVBTSArMTEwMCwgRGF2ZSBD aGlubmVyIHdyb3RlOgo+ID4gaS5lLiAgdGhlIGZhY3QgdGhlIGNtcHhjaGcgZmFpbGVkIG1heSBu b3QgaGF2ZSBhbnl0aGluZyB0byBkbyB3aXRoIGEKPiA+IHJhY2UgY29uZHRpb24gLSBpdCBmYWls ZWQgYmVjYXVzZSB0aGUgc2xvdCB3YXNuJ3QgZW1wdHkgbGlrZSB3ZQo+ID4gZXhwZWN0ZWQgaXQg dG8gYmUuIFRoZXJlIGNhbiBiZSBhbnkgbnVtYmVyIG9mIHJlYXNvbnMgdGhlIHNsb3QgaXNuJ3QK PiA+IGVtcHR5IC0gdGhlIEFQSSBzaG91bGQgbm90ICJkb2N1bWVudCIgdGhhdCB0aGUgcmVhc29u IHRoZSBpbnNlcnQKPiA+IGZhaWxlZCB3YXMgYSByYWNlIGNvbmRpdGlvbi4gSXQgc2hvdWxkIGRv Y3VtZW50IHRoZSBjYXNlIHRoYXQgd2UKPiA+ICJjb3VsZG4ndCBpbnNlcnQgYmVjYXVzZSB0aGVy ZSB3YXMgYW4gZXhpc3RpbmcgZW50cnkgaW4gdGhlIHNsb3QiLgo+ID4gTGV0IHRoZSBzdXJyb3Vu ZGluZyBjb2RlIGRvY3VtZW50IHRoZSByZWFzb24gd2h5IHRoYXQgbWlnaHQgaGF2ZQo+ID4gaGFw cGVuZWQgLSBpdCdzIG5vdCBmb3IgdGhlIEFQSSB0byBhc3N1bWUgcmVhc29ucyBmb3IgZmFpbHVy ZS4KPiA+IAo+ID4gaS5lLiB0aGlzIEFQSSBhbmQgcG90ZW50aWFsIGludGVybmFsIGltcGxlbWVu dGF0aW9uIG1ha2VzIG11Y2gKPiA+IG1vcmUgc2Vuc2U6Cj4gPiAKPiA+IGludAo+ID4geGFfc3Rv cmVfaWZmX2VtcHR5KC4uLikKPiA+IHsKPiA+IAljdXJyID0geGFfY21weGNoZygmcGFnLT5wYWdf aWNpX3hhLCBhZ2lubywgTlVMTCwgaXAsIEdGUF9OT0ZTKTsKPiA+IAlpZiAoIWN1cnIpCj4gPiAJ CXJldHVybiAwOwkvKiBzdWNjZXNzISAqLwo+ID4gCWlmICghSVNfRVJSKGN1cnIpKQo+ID4gCQly ZXR1cm4gLUVFWElTVDsJLyogZmFpbGVkIC0gc2xvdCBub3QgZW1wdHkgKi8KPiA+IAlyZXR1cm4g UFRSX0VSUihjdXJyKTsJLyogZmFpbGVkIC0gWEEgaW50ZXJuYWwgaXNzdWUgKi8KPiA+IH0KPiA+ IAo+ID4gYXMgaXQgcmVwbGFjZXMgdGhlIGV4aXN0aW5nIHByZWxvYWQgYW5kIGluc2VydCBjb2Rl IGluIHRoZSBYRlMgY29kZQo+ID4gcGF0aHMgd2hpbHN0IGxldHRpbmcgdXMgaGFuZGxlIGFuZCBk b2N1bWVudCB0aGUgImluc2VydCBmYWlsZWQKPiA+IGJlY2F1c2Ugc2xvdCBub3QgZW1wdHkiIGNh c2UgaG93ZXZlciB3ZSB3YW50LiBJdCBpbXBsaWVzIG5vdGhpbmcKPiA+IGFib3V0ICp3aHkqIHRo ZSBzbG90IHdhc24ndCBlbXB0eSwganVzdCB0aGF0IHdlIGNvdWxkbid0IGRvIHRoZQo+ID4gaW5z ZXJ0IGJlY2F1c2UgaXQgd2Fzbid0IGVtcHR5Lgo+IAo+IFllYWgsIE9LLiAgU28sIG92ZXIgdGhl IHRvcCBvZiB0aGUgcmVjZW50IGNoYW5nZXMgSSdtIGxvb2tpbmcgYXQgdGhpczoKPiAKPiBJJ20g bm90IGluIGxvdmUgd2l0aCB4YV9zdG9yZV9lbXB0eSgpIGFzIGEgbmFtZS4gIEkgYWxtb3N0IHdh bnQKPiB4YV9zdG9yZV93ZWFrKCksIGJ1dCBhZnRlciBteSBNQVBfRklYRURfV0VBSyBwcm9wb3Nl ZCBuYW1lIGdvdCBzaG90Cj4gZG93biwgSSdtIGxlZXJ5IG9mIGl0LiAgImVtcHR5IiBpcyBhdCBs ZWFzdCBhIGNvbmNlcHQgd2UgYWxyZWFkeSBoYXZlCj4gaW4gdGhlIEFQSSB3aXRoIHRoZSBjb21t ZW50IGZvciB4YV9pbml0KCkgdGFsa2luZyBhYm91dCBhbiBlbXB0eSBhcnJheQo+IGFuZCB4YV9l bXB0eSgpLiAgSSBhbHNvIGNvbnNpZGVyZWQgeGFfc3RvcmVfbnVsbCBhbmQgeGFfb3ZlcndyaXRl X251bGwKPiBhbmQgeGFfcmVwbGFjZV9udWxsKCkuICBOYW1pbmcgcmVtYWlucyBoYXJkLgo+IAo+ IGRpZmYgLS1naXQgYS9mcy94ZnMveGZzX2ljYWNoZS5jIGIvZnMveGZzL3hmc19pY2FjaGUuYwo+ IGluZGV4IDk0MWYzOGJiOTRhNC4uNTg2YjQzODM2OTA1IDEwMDY0NAo+IC0tLSBhL2ZzL3hmcy94 ZnNfaWNhY2hlLmMKPiArKysgYi9mcy94ZnMveGZzX2ljYWNoZS5jCj4gQEAgLTQ1MSw3ICs0NTEs NyBAQCB4ZnNfaWdldF9jYWNoZV9taXNzKAo+ICAJaW50CQkJZmxhZ3MsCj4gIAlpbnQJCQlsb2Nr X2ZsYWdzKQo+ICB7Cj4gLQlzdHJ1Y3QgeGZzX2lub2RlCSppcCwgKmN1cnI7Cj4gKwlzdHJ1Y3Qg eGZzX2lub2RlCSppcDsKPiAgCWludAkJCWVycm9yOwo+ICAJeGZzX2FnaW5vX3QJCWFnaW5vID0g WEZTX0lOT19UT19BR0lOTyhtcCwgaW5vKTsKPiAgCWludAkJCWlmbGFnczsKPiBAQCAtNDk4LDgg KzQ5OCw3IEBAIHhmc19pZ2V0X2NhY2hlX21pc3MoCj4gIAl4ZnNfaWZsYWdzX3NldChpcCwgaWZs YWdzKTsKPiAgCj4gIAkvKiBpbnNlcnQgdGhlIG5ldyBpbm9kZSAqLwo+IC0JY3VyciA9IHhhX2Nt cHhjaGcoJnBhZy0+cGFnX2ljaV94YSwgYWdpbm8sIE5VTEwsIGlwLCBHRlBfTk9GUyk7Cj4gLQll cnJvciA9IF9feGFfcmFjZShjdXJyLCAtRUFHQUlOKTsKPiArCWVycm9yID0geGFfc3RvcmVfZW1w dHkoJnBhZy0+cGFnX2ljaV94YSwgYWdpbm8sIGlwLCBHRlBfTk9GUywgLUVBR0FJTik7Cj4gIAlp ZiAoZXJyb3IpCj4gIAkJZ290byBvdXRfdW5sb2NrOwoKQ2FuIHdlIGF2b2lkIGVuY29kaW5nIHRo ZSBlcnJvciB0byBiZSByZXR1cm5lZCBpbnRvIHRoZSBmdW5jdGlvbgpjYWxsPyBObyBvdGhlciBn ZW5lcmljL2xpYnJhcnkgQVBJIGRvZXMgdGhpcywgc28gdGhpcyBzZWVtcyBsaWtlIGEKaGlnaGx5 IHVudXN1YWwgc3BlY2lhbCBzbm93Zmxha2UgYXBwcm9hY2guIEkganVzdCBkb24ndCBzZWUgaG93 CmNyZWF0aW5nIGEgd2hvbGUgbmV3IGVycm9yIHNwZWNpZmljYXRpb24gY29udmVudGlvbiBpcyBq dXN0aWZpZWQKZm9yIHRoZSBjYXNlIHdoZXJlIGl0ICptaWdodCogc2F2ZSBhIGxpbmUgb3IgdHdv IG9mIGVycm9yIGNoZWNraW5nCmNvZGUgaW4gYSBjYWxsZXI/CgpJJ2QgbXVjaCBwcmVmZXIgdGhh dCB0aGUgQVBJIGRlZmluZXMgdGhlIGVycm9yIG9uIGZhaWx1cmUsIGFuZCBsZXQKdGhlIGNhbGxl ciBoYW5kbGUgdGhhdCBzcGVjaWZpYyBlcnJvciBob3dldmVyIHRoZXkgbmVlZCB0byBsaWtlCmV2 ZXJ5IG90aGVyIGxpYnJhcnkgaW50ZXJmYWNlIHdlIGhhdmUuLi4KCkNoZWVycywKCkRhdmUuCg==