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 058DEEB64D7 for ; Thu, 29 Jun 2023 00:40:16 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8F4E78D0005; Wed, 28 Jun 2023 20:40:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 87E538D0001; Wed, 28 Jun 2023 20:40:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6F8028D0005; Wed, 28 Jun 2023 20:40:15 -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 59C8E8D0001 for ; Wed, 28 Jun 2023 20:40:15 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 20BD01401B2 for ; Thu, 29 Jun 2023 00:40:15 +0000 (UTC) X-FDA: 80953928790.06.5C58359 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf18.hostedemail.com (Postfix) with ESMTP id 0549F1C0013 for ; Thu, 29 Jun 2023 00:40:11 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Kfsq1hgW; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf18.hostedemail.com: domain of xiubli@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=xiubli@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1687999212; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=GBoFOtIWRHIKoWf0lrD2wG9OyN2hTf5a/5og68Y1whI=; b=8QWZMfKA/HN//XjL0eRLuqp9IiQ10xY7iHXW58hz5DyxnA89bL2waihmFEycFQbTf7Z+9W K1ipesaxvkBbr2rm4fiFjkncCfI/idbRtYy4wFCYIwF5Z9e9EHUPWFHriH2cK4AUUrZU3j Fk42hrrlaiyYrJS/kRJpZo8uloIzjfM= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Kfsq1hgW; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf18.hostedemail.com: domain of xiubli@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=xiubli@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1687999212; a=rsa-sha256; cv=none; b=oxeh6eA8K3xss8OZl+5FdYwViKqPv+MpyD1dQothy20y4E8IWPpQ3/ktBBXSG6M6T/rd07 BYElIB0zXvRiFqkCaqfoyrJKCmwsmvYf+UmowYa5nqioAIBKKdBsAGFUILQcDcS5XL2saU PH9MtJY82JLx7/UxyWTT7UnZm27IDOc= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1687999211; h=from:from: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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=GBoFOtIWRHIKoWf0lrD2wG9OyN2hTf5a/5og68Y1whI=; b=Kfsq1hgWTWpirEq6xx6bOJnrE1kLY9WrKHFKbN6c8B40fCOjl0lIDjwhdutx96D/vZ687v cYElKSWzYKUWs52T1n+MtxUEiLqGuDQkvpt6+hhQfTsjYPBncmj7Y/f08y5IeGtQwcxTnH TIjaUA94lR0eViCevVBY97p3JprxC7g= Received: from mail-pg1-f200.google.com (mail-pg1-f200.google.com [209.85.215.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-544-wh0JSaZXM2meVy41u8z7Ow-1; Wed, 28 Jun 2023 20:40:08 -0400 X-MC-Unique: wh0JSaZXM2meVy41u8z7Ow-1 Received: by mail-pg1-f200.google.com with SMTP id 41be03b00d2f7-53425d37fefso78906a12.3 for ; Wed, 28 Jun 2023 17:40:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687999207; x=1690591207; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=GBoFOtIWRHIKoWf0lrD2wG9OyN2hTf5a/5og68Y1whI=; b=LhqyLL6hE0nYjaTuU6NyTk0w4uoUad86WhCVz2Ccf5lOaqR/0pvcEVlGuapqVnXHeM 49lbCaIiv5uiE7IY3Kf6KQjYo+S05RBB8c/cat05fyCwOvkV6IcT6CGiz7iIiftiw7hj U3vLdUls7KqEYEBDcAqMhLpCqNEKAJJXRMPZfUiVXOvJftrWWSR5lKL3CT8C3xgvbHNq TScfjGHPAb9XkURTEu9qoAC8nNAf5fSz/PHQ2qKNhOfXuA5LBmzGEVcCEX/KXHRDm/5v o+wlIl+KPqoFNfCFEqz7IMsMxwEGPz6zhbsB3V2nT5YwId2LqlLni17de/TW5E9j2dpV ltxw== X-Gm-Message-State: AC+VfDxap1Qcvn0jVry/XcEdJQYHA8/AXNdI/0NBUW5OwoScE9e4vCji sz5fWLGwQXNcU/2dy57HEDm64AbimgSQPNOros0bRo6C4zo7wuIJB4SaGrmJBKFU58vb5M0BQoe KJ2+i/BVFevM= X-Received: by 2002:a05:6a21:32a2:b0:125:699a:599b with SMTP id yt34-20020a056a2132a200b00125699a599bmr15662844pzb.34.1687999206976; Wed, 28 Jun 2023 17:40:06 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5vLdftDDvS6DadhOeggrwPU/ORSuzVvDK1nPTA87qiG12zTXbeuXwcom3dX8ucd1whR5HDYQ== X-Received: by 2002:a05:6a21:32a2:b0:125:699a:599b with SMTP id yt34-20020a056a2132a200b00125699a599bmr15662825pzb.34.1687999206657; Wed, 28 Jun 2023 17:40:06 -0700 (PDT) Received: from [10.72.13.91] ([209.132.188.80]) by smtp.gmail.com with ESMTPSA id d14-20020aa78e4e000000b0067acbc74977sm4268764pfr.96.2023.06.28.17.40.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 28 Jun 2023 17:40:06 -0700 (PDT) Message-ID: <41e1c831-29de-8494-d925-6e2eb379567f@redhat.com> Date: Thu, 29 Jun 2023 08:39:58 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [Linux-cachefs] [PATCH v7 2/2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache To: David Howells , Andrew Morton Cc: Shyam Prasad N , Christoph Hellwig , linux-nfs@vger.kernel.org, Ilya Dryomov , linux-cifs@vger.kernel.org, Rohith Surabattula , linux-erofs@lists.ozlabs.org, Jeff Layton , Matthew Wilcox , Steve French , linux-cachefs@redhat.com, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, v9fs-developer@lists.sourceforge.net, ceph-devel@vger.kernel.org, linux-ext4@vger.kernel.org, Linus Torvalds , linux-afs@lists.infradead.org, Dominique Martinet References: <20230628104852.3391651-1-dhowells@redhat.com> <20230628104852.3391651-3-dhowells@redhat.com> From: Xiubo Li In-Reply-To: <20230628104852.3391651-3-dhowells@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 0549F1C0013 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: y6iwkki6buyd9p38izy5aa3p9two1zmg X-HE-Tag: 1687999211-952087 X-HE-Meta: U2FsdGVkX1/RmrjxuCWtvFZqX3FeNipTIUe/bdEbsq20VHMBMEbfp7L4adCJSZKt+DPttf1jUYoIExlfSu9VNHLm8m9PvWYIhbTOOJV/WU9fPI3/ixgjFWCEoYFiuRcX2M3K24zJfPCCkqBydrV1eEBoQZrsRE4xbNK6HKC+eDRRLTMei3A5PuzwSYC8Sjsrl+5XDiKfdfX8PKlAm+jTZXayOuxpgYFE1hnghLhK+imvTN9IH0rDyZk6szk1oeVqEnlvepL1SwLzsxmu/jeaHzlrmDScKgpbqDnG22oCvFIJdmsBInLBK6syba49ZSf6iRBTMo1jtlI7oFqvF+8gIdXkiP4teSVgXmQc3M2Rt1q5HnkVVv9FnuN8VIZxBo3dlYVH+CSsXJ5T9KpURVLxAF7XRfyt1jB0DzbfS8x84bRzQBNEuG2LhRSqWtXCKv0+bApy4BD3btY0HQGUa4AQdFVs0woLJzL+0uCX5yUMrCMNQkQnmnY6Plty20z9OpBl20UP3dlloO38E2blRw+aX7NU37pjC3O22lCKB9qPRYfHF+clrQVajTg1EvDPvbfFiRKWG9CkSeiDIUM3GuxoE66D7zGJln20FCEH96X09upHeZYmerEBhLnOsex1gfgTdkrj0AWwCEg2Rjz5gCmJrLJyfVr2PqmWKDt5K3CXETrsqj4YsUw9BIecpIgg52nElUYqxmm4POUJ17fChBn9tyrv+qQX1jP+z3oUlSGrW5iS8LrOyAhPg3zr9u7dWB2HB0q111r6LBMpEvVErrdkmhIejn5ACOSwUMVQdOZXzULONjChcFotoXKFDiq+KGkxK4EUGFAsODRniKBZS9/ipoRqBO5jUCp4ysphkEUbRX57RpeEH8QXAkJjKy4KYh5X0243CaE+wX5us5a2pI4RcnGYn0YYQB+aKHoncgtARn1UrzefCiNlQ6l0sl+bvbzTEdrqyAF2e4gWWab3dEj imoTbbiq Z78cGMMaCECEX8RV6Gp4VqwErkEd6NENkt+OU6eLwb4GuQXjNbMCB4GwxBAMKmZld27NcnCUhPbDKRgWLIS4cRzonFkiyLZRxWCGCzYWpPdW9R1Vpky+ntnGUBTB9E+5er25Jnqu7rSV0YTgxlpCcRB3GJJr0XKdgA82fHtN9sel5/fMOW124Tc0KmyBWp5YsTtMGO1DhoAh3aZJMMfuy4jF82q8QVk5Zra/DdTqKT6xwAFu402c3PicKfw9rFI2qNiqmNlfkr9iJvRyHdTNCI3WxXjxqbYPHoTI7L8QrVbUKAMt16wCLKIW/yLUI965Mw/pl5kfRUVZ/R4rK4QoICRt19jmyBEP+M3uAxgZjI7sZNjpwLS0ZlXovqZom4lWLANMCQmpjLkA7ov0WCWQLudr8YFOKNR6eGd+uvszvj+m2J0xJohT05h5q6f23X/poLtut5SMx+oCw6wMUIWh58lmlw7EZNRpqwlrUIYzz3jzY5Gc+2NaPzyysBfW9V85sp5dksYuwWj4eaVjjY3TKqYmz6Wl9EDsEfGO6MNWJ7qWf89McDLGpqIVdPg4puVFpoRmNJfo7a9LzVc4WW8n7eSbm5FdkCfvTnwvHEjHjQoX8ot2fhRPdgyqASIdPpQREnNQqsJlCe5WQnW1Ial1rMopb8YSihCogUQZEwkdJos6XDLB/l+QVty0j3ygCpKDiu6FYzbcqlqaNTgxpvydTYPR1Wb9+moCkqus3vLXz9rE70UI6Nt/p1YlNcituLUcgioRt 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 6/28/23 18:48, David Howells wrote: > Fscache has an optimisation by which reads from the cache are skipped until > we know that (a) there's data there to be read and (b) that data isn't > entirely covered by pages resident in the netfs pagecache. This is done > with two flags manipulated by fscache_note_page_release(): > > if (... > test_bit(FSCACHE_COOKIE_HAVE_DATA, &cookie->flags) && > test_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags)) > clear_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags); > > where the NO_DATA_TO_READ flag causes cachefiles_prepare_read() to indicate > that netfslib should download from the server or clear the page instead. > > The fscache_note_page_release() function is intended to be called from > ->releasepage() - but that only gets called if PG_private or PG_private_2 > is set - and currently the former is at the discretion of the network > filesystem and the latter is only set whilst a page is being written to the > cache, so sometimes we miss clearing the optimisation. > > Fix this by following Willy's suggestion[1] and adding an address_space > flag, AS_RELEASE_ALWAYS, that causes filemap_release_folio() to always call > ->release_folio() if it's set, even if PG_private or PG_private_2 aren't > set. > > Note that this would require folio_test_private() and page_has_private() to > become more complicated. To avoid that, in the places[*] where these are > used to conditionalise calls to filemap_release_folio() and > try_to_release_page(), the tests are removed the those functions just > jumped to unconditionally and the test is performed there. > > [*] There are some exceptions in vmscan.c where the check guards more than > just a call to the releaser. I've added a function, folio_needs_release() > to wrap all the checks for that. > > AS_RELEASE_ALWAYS should be set if a non-NULL cookie is obtained from > fscache and cleared in ->evict_inode() before truncate_inode_pages_final() > is called. > > Additionally, the FSCACHE_COOKIE_NO_DATA_TO_READ flag needs to be cleared > and the optimisation cancelled if a cachefiles object already contains data > when we open it. > > Fixes: 1f67e6d0b188 ("fscache: Provide a function to note the release of a page") > Fixes: 047487c947e8 ("cachefiles: Implement the I/O routines") > Reported-by: Rohith Surabattula > Suggested-by: Matthew Wilcox > Signed-off-by: David Howells > cc: Matthew Wilcox > cc: Linus Torvalds > cc: Steve French > cc: Shyam Prasad N > cc: Rohith Surabattula > cc: Dave Wysochanski > cc: Dominique Martinet > cc: Ilya Dryomov > cc: linux-cachefs@redhat.com > cc: linux-cifs@vger.kernel.org > cc: linux-afs@lists.infradead.org > cc: v9fs-developer@lists.sourceforge.net > cc: ceph-devel@vger.kernel.org > cc: linux-nfs@vger.kernel.org > cc: linux-fsdevel@vger.kernel.org > cc: linux-mm@kvack.org > --- > > Notes: > ver #7) > - Make NFS set AS_RELEASE_ALWAYS. > > ver #4) > - Split out merging of folio_has_private()/filemap_release_folio() call > pairs into a preceding patch. > - Don't need to clear AS_RELEASE_ALWAYS in ->evict_inode(). > > ver #3) > - Fixed mapping_clear_release_always() to use clear_bit() not set_bit(). > - Moved a '&&' to the correct line. > > ver #2) > - Rewrote entirely according to Willy's suggestion[1]. > > fs/9p/cache.c | 2 ++ > fs/afs/internal.h | 2 ++ > fs/cachefiles/namei.c | 2 ++ > fs/ceph/cache.c | 2 ++ > fs/nfs/fscache.c | 3 +++ > fs/smb/client/fscache.c | 2 ++ > include/linux/pagemap.h | 16 ++++++++++++++++ > mm/internal.h | 5 ++++- > 8 files changed, 33 insertions(+), 1 deletion(-) Just one question. Shouldn't do this in 'fs/erofs/fscache.c' too ? > > diff --git a/fs/9p/cache.c b/fs/9p/cache.c > index cebba4eaa0b5..12c0ae29f185 100644 > --- a/fs/9p/cache.c > +++ b/fs/9p/cache.c > @@ -68,6 +68,8 @@ void v9fs_cache_inode_get_cookie(struct inode *inode) > &path, sizeof(path), > &version, sizeof(version), > i_size_read(&v9inode->netfs.inode)); > + if (v9inode->netfs.cache) > + mapping_set_release_always(inode->i_mapping); > > p9_debug(P9_DEBUG_FSC, "inode %p get cookie %p\n", > inode, v9fs_inode_cookie(v9inode)); > diff --git a/fs/afs/internal.h b/fs/afs/internal.h > index 9d3d64921106..da73b97e19a9 100644 > --- a/fs/afs/internal.h > +++ b/fs/afs/internal.h > @@ -681,6 +681,8 @@ static inline void afs_vnode_set_cache(struct afs_vnode *vnode, > { > #ifdef CONFIG_AFS_FSCACHE > vnode->netfs.cache = cookie; > + if (cookie) > + mapping_set_release_always(vnode->netfs.inode.i_mapping); If all the cookie need to do the same thing, then how about doing this in 'fscache_acquire_cookie()' ? Thanks - Xiubo > #endif > } > > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c > index d9d22d0ec38a..7bf7a5fcc045 100644 > --- a/fs/cachefiles/namei.c > +++ b/fs/cachefiles/namei.c > @@ -585,6 +585,8 @@ static bool cachefiles_open_file(struct cachefiles_object *object, > if (ret < 0) > goto check_failed; > > + clear_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &object->cookie->flags); > + > object->file = file; > > /* Always update the atime on an object we've just looked up (this is > diff --git a/fs/ceph/cache.c b/fs/ceph/cache.c > index 177d8e8d73fe..de1dee46d3df 100644 > --- a/fs/ceph/cache.c > +++ b/fs/ceph/cache.c > @@ -36,6 +36,8 @@ void ceph_fscache_register_inode_cookie(struct inode *inode) > &ci->i_vino, sizeof(ci->i_vino), > &ci->i_version, sizeof(ci->i_version), > i_size_read(inode)); > + if (ci->netfs.cache) > + mapping_set_release_always(inode->i_mapping); > } > > void ceph_fscache_unregister_inode_cookie(struct ceph_inode_info *ci) > diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c > index 8c35d88a84b1..b05717fe0d4e 100644 > --- a/fs/nfs/fscache.c > +++ b/fs/nfs/fscache.c > @@ -180,6 +180,9 @@ void nfs_fscache_init_inode(struct inode *inode) > &auxdata, /* aux_data */ > sizeof(auxdata), > i_size_read(inode)); > + > + if (netfs_inode(inode)->cache) > + mapping_set_release_always(inode->i_mapping); > } > > /* > diff --git a/fs/smb/client/fscache.c b/fs/smb/client/fscache.c > index 8f6909d633da..3677525ee993 100644 > --- a/fs/smb/client/fscache.c > +++ b/fs/smb/client/fscache.c > @@ -108,6 +108,8 @@ void cifs_fscache_get_inode_cookie(struct inode *inode) > &cifsi->uniqueid, sizeof(cifsi->uniqueid), > &cd, sizeof(cd), > i_size_read(&cifsi->netfs.inode)); > + if (cifsi->netfs.cache) > + mapping_set_release_always(inode->i_mapping); > } > > void cifs_fscache_unuse_inode_cookie(struct inode *inode, bool update) > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h > index a56308a9d1a4..a1176ceb4a0c 100644 > --- a/include/linux/pagemap.h > +++ b/include/linux/pagemap.h > @@ -199,6 +199,7 @@ enum mapping_flags { > /* writeback related tags are not used */ > AS_NO_WRITEBACK_TAGS = 5, > AS_LARGE_FOLIO_SUPPORT = 6, > + AS_RELEASE_ALWAYS, /* Call ->release_folio(), even if no private data */ > }; > > /** > @@ -269,6 +270,21 @@ static inline int mapping_use_writeback_tags(struct address_space *mapping) > return !test_bit(AS_NO_WRITEBACK_TAGS, &mapping->flags); > } > > +static inline bool mapping_release_always(const struct address_space *mapping) > +{ > + return test_bit(AS_RELEASE_ALWAYS, &mapping->flags); > +} > + > +static inline void mapping_set_release_always(struct address_space *mapping) > +{ > + set_bit(AS_RELEASE_ALWAYS, &mapping->flags); > +} > + > +static inline void mapping_clear_release_always(struct address_space *mapping) > +{ > + clear_bit(AS_RELEASE_ALWAYS, &mapping->flags); > +} > + > static inline gfp_t mapping_gfp_mask(struct address_space * mapping) > { > return mapping->gfp_mask; > diff --git a/mm/internal.h b/mm/internal.h > index a76314764d8c..86aef26df905 100644 > --- a/mm/internal.h > +++ b/mm/internal.h > @@ -175,7 +175,10 @@ static inline void set_page_refcounted(struct page *page) > */ > static inline bool folio_needs_release(struct folio *folio) > { > - return folio_has_private(folio); > + struct address_space *mapping = folio->mapping; > + > + return folio_has_private(folio) || > + (mapping && mapping_release_always(mapping)); > } > > extern unsigned long highest_memmap_pfn; > -- > Linux-cachefs mailing list > Linux-cachefs@redhat.com > https://listman.redhat.com/mailman/listinfo/linux-cachefs >