From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id D80EBEE49A6 for ; Fri, 25 Aug 2023 21:39:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 13D37680013; Fri, 25 Aug 2023 17:39:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0EE8568000E; Fri, 25 Aug 2023 17:39:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id ED07D680013; Fri, 25 Aug 2023 17:39:19 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id DE89568000E for ; Fri, 25 Aug 2023 17:39:19 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id B2E5B407DA for ; Fri, 25 Aug 2023 21:39:19 +0000 (UTC) X-FDA: 81163943238.06.91169D8 Received: from mail-pf1-f176.google.com (mail-pf1-f176.google.com [209.85.210.176]) by imf27.hostedemail.com (Postfix) with ESMTP id BE19B40003 for ; Fri, 25 Aug 2023 21:39:17 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=fromorbit-com.20221208.gappssmtp.com header.s=20221208 header.b=SlHW1FHh; spf=pass (imf27.hostedemail.com: domain of david@fromorbit.com designates 209.85.210.176 as permitted sender) smtp.mailfrom=david@fromorbit.com; dmarc=pass (policy=quarantine) header.from=fromorbit.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1692999558; a=rsa-sha256; cv=none; b=P+ix0QNBHKXqAF6PtIu+IiuMPgbC4NDpPrbT3I4SB8OWOpRGdHpiwz7D3M8sgdMF1hoGay PGsn6b6TvhW5HMduOCL9g9q9nWZp56Oy8lq1Lq/t/2rwkC/C0O4OdGrj2DiBS97uoTivkw gjwmbMISTjxAsAqmnIZuQUsD0fZWq+I= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=fromorbit-com.20221208.gappssmtp.com header.s=20221208 header.b=SlHW1FHh; spf=pass (imf27.hostedemail.com: domain of david@fromorbit.com designates 209.85.210.176 as permitted sender) smtp.mailfrom=david@fromorbit.com; dmarc=pass (policy=quarantine) header.from=fromorbit.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1692999558; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=o7dRQ9By/T99QWDW5xev9b8rVkM9NZXXwZrmSduoTKY=; b=aBKubVXU1cO7bVAUbz/tjT/xGZSoZdzSwqoT8fflUUMtWNQKyp8ZJ8EcsMTpF3oM6AVJTs VcsKsYfpJKLe335z4SjihC2LMiZhoHJaxQbzq5iEximjUkRZ2Hg0OTH5M/Wqyw9g8wZB3M Ah2U5HwPLkUWO+0jdzXzQm99id2COpE= Received: by mail-pf1-f176.google.com with SMTP id d2e1a72fcca58-68a4bcf8a97so1069029b3a.1 for ; Fri, 25 Aug 2023 14:39:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20221208.gappssmtp.com; s=20221208; t=1692999556; x=1693604356; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=o7dRQ9By/T99QWDW5xev9b8rVkM9NZXXwZrmSduoTKY=; b=SlHW1FHh55Miyt5dC5YbHFzbz8/Ww6dpIRyxo6vWvL1BUT9wpnYc90O8W3sh6jbVDI 8VSN/Xe7i/NpqgfmZUfGif/OWVY4CvO1XNCApsJ0gTKpBFDUVISAdbUZIXX0AL7Ymm4P GTcb7wyrBEoupwm2T77MgFldNvL555iN8Gi3uYCzgcocNv0xnFj+WWlwbcbOTqmgKZI+ MLGEhJoBOR8ij98DujYJDG8/ErJs1WTRaajOZxKdaJWii5ir39jpSlGIz75MH3BFujgn 75aCRb2KJIpxhwlYFJHEvUeGNqpylJdIJf8XyyAJR0UoHIDg5wblyZNVqnwV4VZyvNd5 jfAQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692999556; x=1693604356; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=o7dRQ9By/T99QWDW5xev9b8rVkM9NZXXwZrmSduoTKY=; b=kXicThaHdQhI+IRJymYXBX9BpiNY/DpyH05LUJRgobZTN9idmUsRWHSenH3ykYUy4l 1nu/07Hn8Xj8gpjiakfpoG/e7wATX2aIPkTgTeZoF5lNwQ7TYvGUzOmk/S9Wn4g03ZKr EN/Imbx34E1ma5GiQRx21FcAXau6u4mTZV58Lobq4+elRXpV0MhxLmjw89gAQ80S/inc IBO3w2YJDCWMbrZH376F7Pyo8ElgBpB7afRUsbepmPPON/H/shFImdVndBUqtXdLspcB EXbU3gxpXOeXX39I0K0zsoNLPvOAUOzTRyrVvjYIDASSL5go+MUS/FZtPkd/4ID5e03Z 8oMg== X-Gm-Message-State: AOJu0Yz9OO7Rbll/L/9my6tfLs/8C0r+B9Q+mIZYWi9M6PuQeHEWr3At zzVa8NPJnkHbFmoc2a51ympUOQ== X-Google-Smtp-Source: AGHT+IFGwDu4J6IE5hfbjc7KSF4OfJEndquVBvBoV5vhWuIhyGy4zoBWcpcC8Lokr19GaOsT0RulfQ== X-Received: by 2002:a05:6a20:7fa0:b0:140:324c:124c with SMTP id d32-20020a056a207fa000b00140324c124cmr22387249pzj.62.1692999556447; Fri, 25 Aug 2023 14:39:16 -0700 (PDT) Received: from dread.disaster.area (pa49-195-66-88.pa.nsw.optusnet.com.au. [49.195.66.88]) by smtp.gmail.com with ESMTPSA id a14-20020a62bd0e000000b006875df4773fsm1997221pff.163.2023.08.25.14.39.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Aug 2023 14:39:15 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1qZeWO-006Uvd-0J; Sat, 26 Aug 2023 07:39:12 +1000 Date: Sat, 26 Aug 2023 07:39:12 +1000 From: Dave Chinner To: Hao Xu Cc: io-uring@vger.kernel.org, Jens Axboe , Dominique Martinet , Pavel Begunkov , Christian Brauner , Alexander Viro , Stefan Roesch , Clay Harris , "Darrick J . Wong" , linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-ext4@vger.kernel.org, linux-cachefs@redhat.com, ecryptfs@vger.kernel.org, linux-nfs@vger.kernel.org, linux-unionfs@vger.kernel.org, bpf@vger.kernel.org, netdev@vger.kernel.org, linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, linux-block@vger.kernel.org, linux-btrfs@vger.kernel.org, codalist@coda.cs.cmu.edu, linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com, linux-mm@kvack.org, linux-nilfs@vger.kernel.org, devel@lists.orangefs.org, linux-cifs@vger.kernel.org, samba-technical@lists.samba.org, linux-mtd@lists.infradead.org, Wanpeng Li Subject: Re: [PATCH 02/29] xfs: rename XBF_TRYLOCK to XBF_NOWAIT Message-ID: References: <20230825135431.1317785-1-hao.xu@linux.dev> <20230825135431.1317785-3-hao.xu@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230825135431.1317785-3-hao.xu@linux.dev> X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: BE19B40003 X-Stat-Signature: aohxcuakcj8az9h6rchstoea6cr37ikq X-Rspam-User: X-HE-Tag: 1692999557-512061 X-HE-Meta: U2FsdGVkX19E61YvAZ37H9pgM9eeJaVhZde3IpquP42qJn89mEQ7ikW1zYcx79d0DZC5Iaw9yVf7U9GoIeuQiBBpjIUdQOUsjzofMMV5XPShUYEVFdYPZGdc53oJhJ5WMIRm4hehuHgMp+a8EqJcgtYJnmL0vJ0CYU4dctTwe+OCq1Dz3A67FO6sTdPNGSPbwYhFS1WBOQIZlN3ABBeONw9W6WCu54WFkGBdtDmcF9PkMn6cb0Ed9ERBP+YCk0y328rBfH7APOIQkjWzjpDbX5VwhURsYFA0zHLqwIoJttz8pxzoZj2hWbTSO0wA+QFkK+gAYehtqPhxxT+iBczVX++blCXNrnj5YI+RYTvke3eFtawz020/xKmBohjEiUYopvG02uKk9rkpGuAnteOGBCsHNQIz3MXZ6XPRFuyQPact0AaLqfXy/13BBmXkbGkaCQFpqByuR3s0Wpc8EVR1fmIQGR/8OT6TSI5agcvIV4yYBCIZ9DZA06tEuA0Y+A6/Y4yBrK8QFuNrgPcOlHh2yVhOBV+wm7aaSUOqECiGRay7DtuoYlFZrtTasURb2vO6Mw/M3UQf7HqcFR/DxFFU01BX0u2gXJpqOQqy6cjEIwvqlFhXjzWv4OcCMzGAW7AGJ2+mCyJSpuDSI/0A2X0CLOa8uj1Kf3imJi0beNtsMen2E0G3cWQ6UyUuNw0K0MJgVC7ONAVQGD9OYqXhsfxYWmGxb6nMbl6/VaGzcn1W6mTphXMJi9c9ZnoZNAzLzHQ2NjUdvPNqAM5P6BcPp2CBlajLwOvZSn1Dc79zCDJkqneXLiWp8tHyZSlGJ2vSZHBbORJ6uDlNanTc30PpxQ9gVHU6Kf6oYJ/ccZ6rytYGIaRPTBXJmXBxbTJmWiJKlC7SDEJOsW8JHfPTXwVgW1nkxdd5oSD35K15PBWWeFkFmIy33DWMuT7KkueQNfti1NcqPBYfLQjTrVNbziJ7QP7 tYqOPrXl HOxyfgw/MadElKomzAntkyFP0UjvrcdJKRhiE8qWPRlDTgc/swNeTz/8U+SeCEXn3y/1UBLAyU50nBTbgzjQ7ST1IzEg2GxaXxbEmLLml/sE10HDZVK0B3H4D2sMHmlk0keRg9bXBFUooc9qTWa2zX+n6zX1dlnpHWA0kv/KfezJW2UzhiKupFUucaBQhb3xeSMcTgRs2YwaMbGG1RvrOdnzuKUJ4EQ5Chv9dMvZU88pBvBkSkULF7tI4LFAs+MWbais8SSKc8oEBshzGARQ0Fm6HjRnVVkgG8e8bJzMaVumKxlHso9NY1VLNXFbd7bLY05RYMy6DCPQe9n2fg/5VQtueiQopuPto8MjNpt0YckAfdsMYxMlnCGhxpfgQhxN47PVuP3lORl1RrhIm4rPZihOSVCWLnq0ay8H/ofvz63zPU5ROHC5PAwDzwsR/y6ncIGrNlWcjUaRzy89CbUxSSMwwXCUetXLvxLnerXuvykUsPlTUpN0nv7BfSCV53CtpC5PXecz2wcvMLfRytw7VzsZW4ayPCctaQ75nd6XKamgzfJB3T6VQDvyryR9xUb/ZLe7IVz69XHexpvZVF7XiZrA6CqKXE52Y/f92qeOue9h9HhklU5iV+2F2uA== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Aug 25, 2023 at 09:54:04PM +0800, Hao Xu wrote: > From: Hao Xu > > XBF_TRYLOCK means we need lock but don't block on it, Yes. > we can use it to > stand for not waiting for memory allcation. Rename XBF_TRYLOCK to > XBF_NOWAIT, which is more generic. No. Not only can XBF_TRYLOCK require memory allocation, it can require IO to be issued. We use TRYLOCK for -readahead- and so we *must* be able to allocate memory and issue IO under TRYLOCK caller conditions. [...] > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c > index d440393b40eb..2ccb0867824c 100644 > --- a/fs/xfs/libxfs/xfs_attr_remote.c > +++ b/fs/xfs/libxfs/xfs_attr_remote.c > @@ -661,7 +661,7 @@ xfs_attr_rmtval_invalidate( > return error; > if (XFS_IS_CORRUPT(args->dp->i_mount, nmap != 1)) > return -EFSCORRUPTED; > - error = xfs_attr_rmtval_stale(args->dp, &map, XBF_TRYLOCK); > + error = xfs_attr_rmtval_stale(args->dp, &map, XBF_NOWAIT); > if (error) > return error; XBF_INCORE | XBF_NOWAIT makes no real sense. I mean, XBF_INCORE is exactly "find a cached buffer or fail" - it's not going to do any memory allocation or IO so NOWAIT smeantics don't make any sense here. It's the buffer lock that this lookup is explicitly avoiding, and so TRYLOCK describes exactly the semantics we want from this incore lookup. Indeed, this is a deadlock avoidance mechanism as the transaction may already have the buffer locked and so we don't want the xfs_buf_incore() lookup to try to lock the buffer again. TRYLOCK documents this pretty clearly - NOWAIT loses that context.... > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c > index 6a6503ab0cd7..77c4f1d83475 100644 > --- a/fs/xfs/libxfs/xfs_btree.c > +++ b/fs/xfs/libxfs/xfs_btree.c > @@ -1343,7 +1343,7 @@ xfs_btree_read_buf_block( > int error; > > /* need to sort out how callers deal with failures first */ > - ASSERT(!(flags & XBF_TRYLOCK)); > + ASSERT(!(flags & XBF_NOWAIT)); > > error = xfs_btree_ptr_to_daddr(cur, ptr, &d); > if (error) > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c > index ac6d8803e660..9312cf3b20e2 100644 > --- a/fs/xfs/scrub/repair.c > +++ b/fs/xfs/scrub/repair.c > @@ -460,7 +460,7 @@ xrep_invalidate_block( > > error = xfs_buf_incore(sc->mp->m_ddev_targp, > XFS_FSB_TO_DADDR(sc->mp, fsbno), > - XFS_FSB_TO_BB(sc->mp, 1), XBF_TRYLOCK, &bp); > + XFS_FSB_TO_BB(sc->mp, 1), XBF_NOWAIT, &bp); My point exactly. xfs_buf_incore() is simply a lookup with XBF_INCORE set. (XBF_INCORE | XBF_TRYLOCK) has the exactly semantics of "return the buffer only if it is cached and we can lock it without blocking. It will not instantiate a new buffer (i.e. do memory allocation) or do IO because the if it is under IO the buffer lock will be held. So, essentially, this "NOWAIT" semantic you want is already supplied by (XBF_INCORE | XBF_TRYLOCK) buffer lookups. > if (error) > return 0; > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 15d1e5a7c2d3..9f84bc3b802c 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -228,7 +228,7 @@ _xfs_buf_alloc( > * We don't want certain flags to appear in b_flags unless they are > * specifically set by later operations on the buffer. > */ > - flags &= ~(XBF_UNMAPPED | XBF_TRYLOCK | XBF_ASYNC | XBF_READ_AHEAD); > + flags &= ~(XBF_UNMAPPED | XBF_NOWAIT | XBF_ASYNC | XBF_READ_AHEAD); > > atomic_set(&bp->b_hold, 1); > atomic_set(&bp->b_lru_ref, 1); > @@ -543,7 +543,7 @@ xfs_buf_find_lock( > struct xfs_buf *bp, > xfs_buf_flags_t flags) > { > - if (flags & XBF_TRYLOCK) { > + if (flags & XBF_NOWAIT) { > if (!xfs_buf_trylock(bp)) { > XFS_STATS_INC(bp->b_mount, xb_busy_locked); > return -EAGAIN; > @@ -886,7 +886,7 @@ xfs_buf_readahead_map( > struct xfs_buf *bp; > > xfs_buf_read_map(target, map, nmaps, > - XBF_TRYLOCK | XBF_ASYNC | XBF_READ_AHEAD, &bp, ops, > + XBF_NOWAIT | XBF_ASYNC | XBF_READ_AHEAD, &bp, ops, > __this_address); That will break readahead (which we use extensively in getdents operations) if we can't allocate buffers and issue IO under NOWAIT conditions. > } > > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h > index 549c60942208..8cd307626939 100644 > --- a/fs/xfs/xfs_buf.h > +++ b/fs/xfs/xfs_buf.h > @@ -45,7 +45,7 @@ struct xfs_buf; > > /* flags used only as arguments to access routines */ > #define XBF_INCORE (1u << 29)/* lookup only, return if found in cache */ > -#define XBF_TRYLOCK (1u << 30)/* lock requested, but do not wait */ > +#define XBF_NOWAIT (1u << 30)/* mem/lock requested, but do not wait */ That's now a really poor comment. It doesn't describe the semantics or constraints that NOWAIT might imply. -Dave. -- Dave Chinner david@fromorbit.com