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@xxxxxxxxx>
>> Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
>> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
>> cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
>> cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
>> cc: Steve French <sfrench@xxxxxxxxx>
>> cc: Shyam Prasad N <nspmangalore@xxxxxxxxx>
>> cc: Rohith Surabattula <rohiths.msft@xxxxxxxxx>
>> cc: Dave Wysochanski <dwysocha@xxxxxxxxxx>
>> cc: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
>> cc: Ilya Dryomov <idryomov@xxxxxxxxx>
>> cc: linux-cachefs@xxxxxxxxxx
>> cc: linux-cifs@xxxxxxxxxxxxxxx
>> cc: linux-afs@xxxxxxxxxxxxxxxxxxx
>> cc: v9fs-developer@xxxxxxxxxxxxxxxxxxxxx
>> cc: ceph-devel@xxxxxxxxxxxxxxx
>> cc: linux-nfs@xxxxxxxxxxxxxxx
>> cc: linux-fsdevel@xxxxxxxxxxxxxxx
>> cc: linux-mm@xxxxxxxxx
>> ---
>>
>> 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
