Date:   Fri, 30 Jun 2023 11:20:59 +0800
From:   Jingbo Xu <jefflexu@...ux.alibaba.com>
To:     Xiubo Li <xiubli@...hat.com>, David Howells <dhowells@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Shyam Prasad N <nspmangalore@...il.com>,
        linux-cifs@...r.kernel.org, linux-nfs@...r.kernel.org,
        linux-mm@...ck.org, Rohith Surabattula <rohiths.msft@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Dominique Martinet <asmadeus@...ewreck.org>,
        Jeff Layton <jlayton@...nel.org>,
        Matthew Wilcox <willy@...radead.org>,
        linux-afs@...ts.infradead.org,
        Christoph Hellwig <hch@...radead.org>,
        Steve French <sfrench@...ba.org>, linux-cachefs@...hat.com,
        linux-fsdevel@...r.kernel.org,
        v9fs-developer@...ts.sourceforge.net,
        Ilya Dryomov <idryomov@...il.com>, linux-ext4@...r.kernel.org,
        linux-erofs@...ts.ozlabs.org, ceph-devel@...r.kernel.org
Subject: Re: [Linux-cachefs] [PATCH v7 2/2] mm, netfs, fscache: Stop read
 optimisation when folio removed from pagecache



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@...il.com>
>> Suggested-by: Matthew Wilcox <willy@...radead.org>
>> Signed-off-by: David Howells <dhowells@...hat.com>
>> cc: Matthew Wilcox <willy@...radead.org>
>> cc: Linus Torvalds <torvalds@...ux-foundation.org>
>> cc: Steve French <sfrench@...ba.org>
>> cc: Shyam Prasad N <nspmangalore@...il.com>
>> cc: Rohith Surabattula <rohiths.msft@...il.com>
>> cc: Dave Wysochanski <dwysocha@...hat.com>
>> cc: Dominique Martinet <asmadeus@...ewreck.org>
>> cc: Ilya Dryomov <idryomov@...il.com>
>> cc: linux-cachefs@...hat.com
>> cc: linux-cifs@...r.kernel.org
>> cc: linux-afs@...ts.infradead.org
>> cc: v9fs-developer@...ts.sourceforge.net
>> cc: ceph-devel@...r.kernel.org
>> cc: linux-nfs@...r.kernel.org
>> cc: linux-fsdevel@...r.kernel.org
>> cc: linux-mm@...ck.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

Powered by blists - more mailing lists