From: Jingbo Xu <jefflexu@linux.alibaba.com>
To: Xiubo Li <xiubli@redhat.com>, David Howells <dhowells@redhat.com>,
Andrew Morton <akpm@linux-foundation.org>
Cc: Shyam Prasad N <nspmangalore@gmail.com>,
linux-cifs@vger.kernel.org, linux-nfs@vger.kernel.org,
linux-mm@kvack.org, Rohith Surabattula <rohiths.msft@gmail.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Dominique Martinet <asmadeus@codewreck.org>,
Jeff Layton <jlayton@kernel.org>,
Matthew Wilcox <willy@infradead.org>,
linux-afs@lists.infradead.org,
Christoph Hellwig <hch@infradead.org>,
Steve French <sfrench@samba.org>,
linux-cachefs@redhat.com, linux-fsdevel@vger.kernel.org,
v9fs-developer@lists.sourceforge.net,
Ilya Dryomov <idryomov@gmail.com>,
linux-ext4@vger.kernel.org, linux-erofs@lists.ozlabs.org,
ceph-devel@vger.kernel.org
Subject: Re: [Linux-cachefs] [PATCH v7 2/2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache
Date: Fri, 30 Jun 2023 11:20:59 +0800 [thread overview]
Message-ID: <bc37b040-701d-3b5a-5cf2-370c320affbb@linux.alibaba.com> (raw)
In-Reply-To: <41e1c831-29de-8494-d925-6e2eb379567f@redhat.com>
On 6/29/23 8:39 AM, Xiubo Li wrote:
>
> 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 <rohiths.msft@gmail.com>
>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> cc: Matthew Wilcox <willy@infradead.org>
>> cc: Linus Torvalds <torvalds@linux-foundation.org>
>> cc: Steve French <sfrench@samba.org>
>> cc: Shyam Prasad N <nspmangalore@gmail.com>
>> cc: Rohith Surabattula <rohiths.msft@gmail.com>
>> cc: Dave Wysochanski <dwysocha@redhat.com>
>> cc: Dominique Martinet <asmadeus@codewreck.org>
>> cc: Ilya Dryomov <idryomov@gmail.com>
>> 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 ?
>
Currently the read optimization is not used in fscache ondemand mode
(used by erofs), though it may not be intended...
cachefiles_ondemand_copen
if (size)
clear_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags);
The read optimization is disabled as long as the backing file size is
not 0 (which is the most case). And thus currently erofs doesn't need
to clear FSCACHE_COOKIE_NO_DATA_TO_READ in .release_folio().
--
Thanks,
Jingbo
WARNING: multiple messages have this Message-ID (diff)
From: Jingbo Xu <jefflexu@linux.alibaba.com>
To: Xiubo Li <xiubli@redhat.com>, David Howells <dhowells@redhat.com>,
Andrew Morton <akpm@linux-foundation.org>
Cc: Shyam Prasad N <nspmangalore@gmail.com>,
linux-cifs@vger.kernel.org, linux-nfs@vger.kernel.org,
Rohith Surabattula <rohiths.msft@gmail.com>,
linux-erofs@lists.ozlabs.org,
Dominique Martinet <asmadeus@codewreck.org>,
Jeff Layton <jlayton@kernel.org>,
Matthew Wilcox <willy@infradead.org>,
Christoph Hellwig <hch@infradead.org>,
linux-mm@kvack.org, ceph-devel@vger.kernel.org,
linux-cachefs@redhat.com, v9fs-developer@lists.sourceforge.net,
Steve French <sfrench@samba.org>,
linux-fsdevel@vger.kernel.org, Ilya Dryomov <idryomov@gmail.com>,
linux-ext4@vger.kernel.org,
Linus Torvalds <torvalds@linux-foundation.org>,
linux-afs@lists.infradead.org
Subject: Re: [Linux-cachefs] [PATCH v7 2/2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache
Date: Fri, 30 Jun 2023 11:20:59 +0800 [thread overview]
Message-ID: <bc37b040-701d-3b5a-5cf2-370c320affbb@linux.alibaba.com> (raw)
In-Reply-To: <41e1c831-29de-8494-d925-6e2eb379567f@redhat.com>
On 6/29/23 8:39 AM, Xiubo Li wrote:
>
> 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 <rohiths.msft@gmail.com>
>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> cc: Matthew Wilcox <willy@infradead.org>
>> cc: Linus Torvalds <torvalds@linux-foundation.org>
>> cc: Steve French <sfrench@samba.org>
>> cc: Shyam Prasad N <nspmangalore@gmail.com>
>> cc: Rohith Surabattula <rohiths.msft@gmail.com>
>> cc: Dave Wysochanski <dwysocha@redhat.com>
>> cc: Dominique Martinet <asmadeus@codewreck.org>
>> cc: Ilya Dryomov <idryomov@gmail.com>
>> 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 ?
>
Currently the read optimization is not used in fscache ondemand mode
(used by erofs), though it may not be intended...
cachefiles_ondemand_copen
if (size)
clear_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags);
The read optimization is disabled as long as the backing file size is
not 0 (which is the most case). And thus currently erofs doesn't need
to clear FSCACHE_COOKIE_NO_DATA_TO_READ in .release_folio().
--
Thanks,
Jingbo
next prev parent reply other threads:[~2023-06-30 3:21 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-28 10:48 [PATCH v7 0/2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache David Howells
2023-06-28 10:48 ` David Howells
2023-06-28 10:48 ` [PATCH v7 1/2] mm: Merge folio_has_private()/filemap_release_folio() call pairs David Howells
2023-06-28 10:48 ` David Howells
2023-06-28 10:48 ` [PATCH v7 2/2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache David Howells
2023-06-28 10:48 ` David Howells
2023-06-29 0:39 ` [Linux-cachefs] " Xiubo Li
2023-06-29 0:39 ` Xiubo Li
2023-06-30 3:20 ` Jingbo Xu [this message]
2023-06-30 3:20 ` Jingbo Xu
2023-07-07 16:38 ` [BUG mm-unstable] BUG: KASAN: use-after-free in shrink_folio_list+0x9f4/0x1ae0 Hyeonggon Yoo
2023-07-07 16:38 ` Hyeonggon Yoo
2023-07-07 16:46 ` Hyeonggon Yoo
2023-07-07 16:46 ` Hyeonggon Yoo
2023-07-07 18:12 ` David Wysochanski
2023-07-07 18:12 ` David Wysochanski
2023-07-07 18:27 ` Hyeonggon Yoo
2023-07-07 18:27 ` Hyeonggon Yoo
2023-07-07 18:40 ` Matthew Wilcox
2023-07-07 18:40 ` Matthew Wilcox
2023-07-07 18:33 ` Matthew Wilcox
2023-07-07 18:33 ` Matthew Wilcox
2023-07-07 19:23 ` SeongJae Park
2023-07-07 19:23 ` SeongJae Park
2023-07-17 7:34 ` [PATCH v7 2/2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache kernel test robot
2023-07-17 7:34 ` kernel test robot
2023-07-17 12:43 ` David Wysochanski
2023-07-17 12:43 ` David Wysochanski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bc37b040-701d-3b5a-5cf2-370c320affbb@linux.alibaba.com \
--to=jefflexu@linux.alibaba.com \
--cc=akpm@linux-foundation.org \
--cc=asmadeus@codewreck.org \
--cc=ceph-devel@vger.kernel.org \
--cc=dhowells@redhat.com \
--cc=hch@infradead.org \
--cc=idryomov@gmail.com \
--cc=jlayton@kernel.org \
--cc=linux-afs@lists.infradead.org \
--cc=linux-cachefs@redhat.com \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-erofs@lists.ozlabs.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-nfs@vger.kernel.org \
--cc=nspmangalore@gmail.com \
--cc=rohiths.msft@gmail.com \
--cc=sfrench@samba.org \
--cc=torvalds@linux-foundation.org \
--cc=v9fs-developer@lists.sourceforge.net \
--cc=willy@infradead.org \
--cc=xiubli@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.