From patchwork Mon Dec 11 04:23:15 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Matthew Wilcox (Oracle)" X-Patchwork-Id: 10104125 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 9851B602B3 for ; Mon, 11 Dec 2017 04:23:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E2FBB29287 for ; Mon, 11 Dec 2017 04:23:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D7D532929E; Mon, 11 Dec 2017 04:23:41 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 638F729287 for ; Mon, 11 Dec 2017 04:23:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752258AbdLKEXU (ORCPT ); Sun, 10 Dec 2017 23:23:20 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:39391 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752070AbdLKEXS (ORCPT ); Sun, 10 Dec 2017 23:23:18 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=RLOHBIrA0TOdeUVUjrq7egOn1K5MJ5cp6En3yKjbe+Y=; b=JxBEKfBn/9GAIrJkMSYyb0iMp JtXX+HraeqOT8ksz0/ZOPX6zGOsTI4sN/96xM+Kc3Ncoe89EVYtzunP7qThD1sqZX4OVF5yA0fDLw KxhPc8uL4IIDKkTudRP59t0Vi0AmahSYBZdpW3gRxytngmK9RgIIHmqop8fEYFCgm+DpZK5LAiJkG q0Lnp09hbWbeCTc4m/EtVcwjY5rgXvr2k1EEH50O8VeIWIJ+OdPDd5vKfyYHr8IKc/4eA/pchJFJD 9hDSUJZOL9aJfujZnNWKAK1vIqlrVsDTFbXEuQLE/6r8jCQ5AFbid3umrkm6h8UPZeBrKyYKaKher Uy59/3CiQ==; Received: from willy by bombadil.infradead.org with local (Exim 4.87 #1 (Red Hat Linux)) id 1eOFcZ-0002O0-KY; Mon, 11 Dec 2017 04:23:15 +0000 Date: Sun, 10 Dec 2017 20:23:15 -0800 From: Matthew Wilcox To: Dave Chinner 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: <20171211042315.GA25236@bombadil.infradead.org> References: <20171206004159.3755-73-willy@infradead.org> <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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20171210235745.GR5858@dastard> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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. --- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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; diff --git a/include/linux/xarray.h b/include/linux/xarray.h index 5792b6dbb040..cc7cc5253a67 100644 --- a/include/linux/xarray.h +++ b/include/linux/xarray.h @@ -271,43 +271,30 @@ static inline int xa_err(void *entry) } /** - * __xa_race() - Turn a cmpxchg result into an errno. - * @entry: Result from calling an XArray function. - * @errno: Error number to return if we lost the race. + * xa_store_empty() - Store this entry in the XArray unless another entry is + * already present. + * @xa: XArray. + * @index: Index into array. + * @entry: New entry. + * @gfp: Memory allocation flags. + * @rc: Number to return if another entry was present. * - * Like xa_race(), but returns the error number of your choice. Calling - * __xa_race(entry, 0) has the same result (but is less efficient) as - * calling xa_err(). + * Like xa_store(), but will fail and return the supplied error number if + * the existing entry at @index is not %NULL. * * Return: A negative errno or 0. */ -static inline int __xa_race(void *entry, int errno) +static inline int xa_store_empty(struct xarray *xa, unsigned long index, + void *entry, gfp_t gfp, int errno) { - if (!entry) + void *curr = xa_cmpxchg(xa, index, NULL, entry, gfp); + if (!curr) return 0; - if (xa_is_err(entry)) - return (long)entry >> 2; + if (xa_is_err(curr)) + return xa_err(curr); return errno; } -/** - * xa_race() - Turn a cmpxchg result into an errno. - * @entry: Result from calling an XArray function. - * - * It is common to use xa_cmpxchg() to ensure that only one thread assigns - * a value to an index. Pass the result from xa_cmpxchg() to xa_race() to - * get an errno back. This function also handles any other error which - * may have been returned by xa_cmpxchg() such as ENOMEM. - * - * If you don't care that you lost the race, you can use xa_err() instead. - * - * Return: A negative errno or 0. - */ -static inline int xa_race(void *entry) -{ - return __xa_race(entry, -EEXIST); -} - /* Everything below here is the Advanced API. Proceed with caution. */ #define xa_trylock(xa) spin_trylock(&(xa)->xa_lock) diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 85d1bc963ab6..87ed55af823e 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -614,8 +614,8 @@ static int cgwb_create(struct backing_dev_info *bdi, spin_lock_irqsave(&cgwb_lock, flags); if (test_bit(WB_registered, &bdi->wb.state) && blkcg_cgwb_list->next && memcg_cgwb_list->next) { - ret = xa_race(xa_cmpxchg(&bdi->cgwb_xa, memcg_css->id, NULL, - wb, GFP_ATOMIC)); + ret = xa_store_empty(&bdi->cgwb_xa, memcg_css->id, wb, + GFP_ATOMIC, -EEXIST); if (!ret) { list_add_tail_rcu(&wb->bdi_node, &bdi->wb_list); list_add(&wb->memcg_node, memcg_cgwb_list);