Re: [PATCH v3 07/10] cifs: (untested) Move to using the alternate fallback fscache I/O API

From: David Howells <dhowells@redhat.com>
To: unlisted-recipients:; (no To-header on input)
Cc: dhowells@redhat.com, Trond Myklebust <trondmy@hammerspace.com>,
	Anna Schumaker <anna.schumaker@netapp.com>,
	Steve French <sfrench@samba.org>,
	Dominique Martinet <asmadeus@codewreck.org>,
	Shyam Prasad N <nspmangalore@gmail.com>,
	linux-cifs@vger.kernel.org, linux-cachefs@redhat.com,
	Jeff Layton <jlayton@redhat.com>,
	Matthew Wilcox <willy@infradead.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-afs@lists.infradead.org, linux-nfs@vger.kernel.org,
	ceph-devel@vger.kernel.org, v9fs-developer@lists.sourceforge.net,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 07/10] cifs: (untested) Move to using the alternate fallback fscache I/O API
Date: Fri, 29 Oct 2021 15:02:36 +0100	[thread overview]
Message-ID: <1876665.1635516156@warthog.procyon.org.uk> (raw)
In-Reply-To: <163363944839.1980952.3311507543724895463.stgit@warthog.procyon.org.uk>

David Howells <dhowells@redhat.com> wrote:

> Move cifs/smb to using the alternate fallback fscache I/O API instead of
> the old upstream I/O API as that is about to be deleted.  The alternate API
> will also be deleted at some point in the future as it's dangerous (as is
> the old API) and can lead to data corruption if the backing filesystem can
> insert/remove bridging blocks of zeros into its extent list[1].
> 
> The alternate API reads and writes pages synchronously, with the intention
> of allowing removal of the operation management framework and thence the
> object management framework from fscache.
> 
> The preferred change would be to use the netfs lib, but the new I/O API can
> be used directly.  It's just that as the cache now needs to track data for
> itself, caching blocks may exceed page size...
> 
> Changes
> =======
> ver #2:
>   - Changed "deprecated" to "fallback" in the new function names[2].

I've managed to test this now.  There was a bug in it, fixed by the following
incremental change:

--- a/fs/cifs/fscache.h
+++ b/fs/cifs/fscache.h
@@ -75,7 +75,7 @@ static inline int cifs_readpage_from_fscache(struct inode *inode,
 static inline void cifs_readpage_to_fscache(struct inode *inode,
 					    struct page *page)
 {
-	if (PageFsCache(page))
+	if (CIFS_I(inode)->fscache)
 		__cifs_readpage_to_fscache(inode, page);
 }
 

It shouldn't be using PageFsCache() here.  That's only used to indicate that
an async DIO is in progress on the page, but since we're using the synchronous
fallback API, that should not happen.  Also, it's no longer used to indicate
that a page is being cached and trigger writeback that way.

David

