Message-ID: <2cfdbfd834bb6ff1f7f5cf47e3ea72449fe683b6.camel@redhat.com>
Subject: Re: [Linux-cachefs] [PATCH] netfs: Adjust docs after foliation
From:   Jeff Layton <jlayton@redhat.com>
To:     David Howells <dhowells@redhat.com>, linux-cachefs@redhat.com
Cc:     linux-mm@kvack.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, Matthew Wilcox <willy@infradead.org>
Date:   Mon, 29 Nov 2021 09:31:00 -0500
In-Reply-To: <163706992597.3179783.18360472879717076435.stgit@warthog.procyon.org.uk>
References: <163706992597.3179783.18360472879717076435.stgit@warthog.procyon.org.uk>
Content-Type: text/plain; charset="ISO-8859-15"
User-Agent: Evolution 3.42.1 (3.42.1-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2021-11-16 at 13:38 +0000, David Howells wrote:
> Adjust the netfslib docs in light of the foliation changes.
> 
> Also un-kdoc-mark netfs_skip_folio_read() since it's internal and isn't
> part of the API.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Matthew Wilcox <willy@infradead.org>
> cc: linux-cachefs@redhat.com
> cc: linux-mm@kvack.org
> ---
> 
>  Documentation/filesystems/netfs_library.rst |   95 ++++++++++++++++-----------
>  fs/netfs/read_helper.c                      |    4 +
>  2 files changed, 58 insertions(+), 41 deletions(-)
> 
> diff --git a/Documentation/filesystems/netfs_library.rst b/Documentation/filesystems/netfs_library.rst
> index bb68d39f03b7..375baca7edcd 100644
> --- a/Documentation/filesystems/netfs_library.rst
> +++ b/Documentation/filesystems/netfs_library.rst
> @@ -1,7 +1,7 @@
>  .. SPDX-License-Identifier: GPL-2.0
>  
>  =================================
> -NETWORK FILESYSTEM HELPER LIBRARY
> +Network Filesystem Helper Library
>  =================================
>  
>  .. Contents:
> @@ -37,22 +37,22 @@ into a common call framework.
>  
>  The following services are provided:
>  
> - * Handles transparent huge pages (THPs).
> + * Handle folios that span multiple pages.
>  
> - * Insulates the netfs from VM interface changes.
> + * Insulate the netfs from VM interface changes.
>  
> - * Allows the netfs to arbitrarily split reads up into pieces, even ones that
> -   don't match page sizes or page alignments and that may cross pages.
> + * Allow the netfs to arbitrarily split reads up into pieces, even ones that
> +   don't match folio sizes or folio alignments and that may cross folios.
>  
> - * Allows the netfs to expand a readahead request in both directions to meet
> -   its needs.
> + * Allow the netfs to expand a readahead request in both directions to meet its
> +   needs.
>  
> - * Allows the netfs to partially fulfil a read, which will then be resubmitted.
> + * Allow the netfs to partially fulfil a read, which will then be resubmitted.
>  
> - * Handles local caching, allowing cached data and server-read data to be
> + * Handle local caching, allowing cached data and server-read data to be
>     interleaved for a single request.
>  
> - * Handles clearing of bufferage that aren't on the server.
> + * Handle clearing of bufferage that aren't on the server.
>  
>   * Handle retrying of reads that failed, switching reads from the cache to the
>     server as necessary.
> @@ -70,22 +70,22 @@ Read Helper Functions
>  
>  Three read helpers are provided::
>  
> - * void netfs_readahead(struct readahead_control *ractl,
> -			const struct netfs_read_request_ops *ops,
> -			void *netfs_priv);``
> - * int netfs_readpage(struct file *file,
> -		      struct page *page,
> -		      const struct netfs_read_request_ops *ops,
> -		      void *netfs_priv);
> - * int netfs_write_begin(struct file *file,
> -			 struct address_space *mapping,
> -			 loff_t pos,
> -			 unsigned int len,
> -			 unsigned int flags,
> -			 struct page **_page,
> -			 void **_fsdata,
> -			 const struct netfs_read_request_ops *ops,
> -			 void *netfs_priv);
> +	void netfs_readahead(struct readahead_control *ractl,
> +			     const struct netfs_read_request_ops *ops,
> +			     void *netfs_priv);
> +	int netfs_readpage(struct file *file,
> +			   struct folio *folio,
> +			   const struct netfs_read_request_ops *ops,
> +			   void *netfs_priv);
> +	int netfs_write_begin(struct file *file,
> +			      struct address_space *mapping,
> +			      loff_t pos,
> +			      unsigned int len,
> +			      unsigned int flags,
> +			      struct folio **_folio,
> +			      void **_fsdata,
> +			      const struct netfs_read_request_ops *ops,
> +			      void *netfs_priv);
>  
>  Each corresponds to a VM operation, with the addition of a couple of parameters
>  for the use of the read helpers:
> @@ -103,8 +103,8 @@ Both of these values will be stored into the read request structure.
>  For ->readahead() and ->readpage(), the network filesystem should just jump
>  into the corresponding read helper; whereas for ->write_begin(), it may be a
>  little more complicated as the network filesystem might want to flush
> -conflicting writes or track dirty data and needs to put the acquired page if an
> -error occurs after calling the helper.
> +conflicting writes or track dirty data and needs to put the acquired folio if
> +an error occurs after calling the helper.
>  
>  The helpers manage the read request, calling back into the network filesystem
>  through the suppplied table of operations.  Waits will be performed as
> @@ -253,7 +253,7 @@ through which it can issue requests and negotiate::
>  		void (*issue_op)(struct netfs_read_subrequest *subreq);
>  		bool (*is_still_valid)(struct netfs_read_request *rreq);
>  		int (*check_write_begin)(struct file *file, loff_t pos, unsigned len,
> -					 struct page *page, void **_fsdata);
> +					 struct folio *folio, void **_fsdata);
>  		void (*done)(struct netfs_read_request *rreq);
>  		void (*cleanup)(struct address_space *mapping, void *netfs_priv);
>  	};
> @@ -313,13 +313,14 @@ The operations are as follows:
>  
>     There is no return value; the netfs_subreq_terminated() function should be
>     called to indicate whether or not the operation succeeded and how much data
> -   it transferred.  The filesystem also should not deal with setting pages
> +   it transferred.  The filesystem also should not deal with setting folios
>     uptodate, unlocking them or dropping their refs - the helpers need to deal
>     with this as they have to coordinate with copying to the local cache.
>  
> -   Note that the helpers have the pages locked, but not pinned.  It is possible
> -   to use the ITER_XARRAY iov iterator to refer to the range of the inode that
> -   is being operated upon without the need to allocate large bvec tables.
> +   Note that the helpers have the folios locked, but not pinned.  It is
> +   possible to use the ITER_XARRAY iov iterator to refer to the range of the
> +   inode that is being operated upon without the need to allocate large bvec
> +   tables.
>  
>   * ``is_still_valid()``
>  
> @@ -330,15 +331,15 @@ The operations are as follows:
>   * ``check_write_begin()``
>  
>     [Optional] This is called from the netfs_write_begin() helper once it has
> -   allocated/grabbed the page to be modified to allow the filesystem to flush
> +   allocated/grabbed the folio to be modified to allow the filesystem to flush
>     conflicting state before allowing it to be modified.
>  
> -   It should return 0 if everything is now fine, -EAGAIN if the page should be
> +   It should return 0 if everything is now fine, -EAGAIN if the folio should be
>     regrabbed and any other error code to abort the operation.
>  
>   * ``done``
>  
> -   [Optional] This is called after the pages in the request have all been
> +   [Optional] This is called after the folios in the request have all been
>     unlocked (and marked uptodate if applicable).
>  
>   * ``cleanup``
> @@ -390,7 +391,7 @@ The read helpers work by the following general procedure:
>       * If NETFS_SREQ_CLEAR_TAIL was set, a short read will be cleared to the
>         end of the slice instead of reissuing.
>  
> - * Once the data is read, the pages that have been fully read/cleared:
> + * Once the data is read, the folios that have been fully read/cleared:
>  
>     * Will be marked uptodate.
>  
> @@ -398,11 +399,11 @@ The read helpers work by the following general procedure:
>  
>     * Unlocked
>  
> - * Any pages that need writing to the cache will then have DIO writes issued.
> + * Any folios that need writing to the cache will then have DIO writes issued.
>  
>   * Synchronous operations will wait for reading to be complete.
>  
> - * Writes to the cache will proceed asynchronously and the pages will have the
> + * Writes to the cache will proceed asynchronously and the folios will have the
>     PG_fscache mark removed when that completes.
>  
>   * The request structures will be cleaned up when everything has completed.
> @@ -452,6 +453,9 @@ operation table looks like the following::
>  			    netfs_io_terminated_t term_func,
>  			    void *term_func_priv);
>  
> +		int (*prepare_write)(struct netfs_cache_resources *cres,
> +				     loff_t *_start, size_t *_len, loff_t i_size);
> +
>  		int (*write)(struct netfs_cache_resources *cres,
>  			     loff_t start_pos,
>  			     struct iov_iter *iter,
> @@ -509,6 +513,14 @@ The methods defined in the table are:
>     indicating whether the termination is definitely happening in the caller's
>     context.
>  
> + * ``prepare_write()``
> +
> +   [Required] Called to adjust a write to the cache and check that there is
> +   sufficient space in the cache.  The start and length values indicate the
> +   size of the write that netfslib is proposing, and this can be adjusted by
> +   the cache to respect DIO boundaries.  The file size is passed for
> +   information.
> +
>   * ``write()``
>  
>     [Required] Called to write to the cache.  The start file offset is given
> @@ -525,4 +537,9 @@ not the read request structure as they could be used in other situations where
>  there isn't a read request structure as well, such as writing dirty data to the
>  cache.
>  
> +
> +API Function Reference
> +======================
> +
>  .. kernel-doc:: include/linux/netfs.h
> +.. kernel-doc:: fs/netfs/read_helper.c
> diff --git a/fs/netfs/read_helper.c b/fs/netfs/read_helper.c
> index 9320a42dfaf9..7046f9bdd8dc 100644
> --- a/fs/netfs/read_helper.c
> +++ b/fs/netfs/read_helper.c
> @@ -1008,8 +1008,8 @@ int netfs_readpage(struct file *file,
>  }
>  EXPORT_SYMBOL(netfs_readpage);
>  
> -/**
> - * netfs_skip_folio_read - prep a folio for writing without reading first
> +/*
> + * Prepare a folio for writing without reading first
>   * @folio: The folio being prepared
>   * @pos: starting position for the write
>   * @len: length of write
> 
> 

Not sure why you decided to change the last one not to be a kerneldoc
comment, but OK. The rest of the changes look straightforward.

Reviewed-by: Jeff Layton <jlayton@redhat.com>