Re: [Linux-cachefs] [PATCH v7 2/2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache

[Jingbo Xu <jefflexu@linux.alibaba.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