Re: [PATCH v3 00/20] netfs: Prep for write helpers

[Jeff Layton]

On Thu, 2022-03-10 at 16:13 +0000, David Howells wrote:
> Having had a go at implementing write helpers and content encryption
> support in netfslib, it seems that the netfs_read_{,sub}request structs and
> the equivalent write request structs were almost the same and so should be
> merged, thereby requiring only one set of alloc/get/put functions and a
> common set of tracepoints.
> 
> Merging the structs also has the advantage that if a bounce buffer is added
> to the request struct, a read operation can be performed to fill the bounce
> buffer, the contents of the buffer can be modified and then a write
> operation can be performed on it to send the data wherever it needs to go
> using the same request structure all the way through.  The I/O handlers
> would then transparently perform any required crypto.  This should make it
> easy to perform RMW cycles if needed.
> 
> The potentially common functions and structs, however, by their names all
> proclaim themselves to be associated with the read side of things.  The
> bulk of these changes alter this in the following ways:
> 
>  (1) Rename struct netfs_read_{,sub}request to netfs_io_{,sub}request.
> 
>  (2) Rename some enums, members and flags to make them more appropriate.
> 
>  (3) Adjust some comments to match.
> 
>  (4) Drop "read"/"rreq" from the names of common functions.  For instance,
>      netfs_get_read_request() becomes netfs_get_request().
> 
>  (5) The ->init_rreq() and ->issue_op() methods become ->init_request() and
>      ->issue_read().  I've kept the latter as a read-specific function and
>      in another branch added an ->issue_write() method.
> 
> The driver source is then reorganised into a number of files:
> 
> 	fs/netfs/buffered_read.c	Create read reqs to the pagecache
> 	fs/netfs/io.c			Dispatchers for read and write reqs
> 	fs/netfs/main.c			Some general miscellaneous bits
> 	fs/netfs/objects.c		Alloc, get and put functions
> 	fs/netfs/stats.c		Optional procfs statistics.
> 
> and future development can be fitted into this scheme, e.g.:
> 
> 	fs/netfs/buffered_write.c	Modify the pagecache
> 	fs/netfs/buffered_flush.c	Writeback from the pagecache
> 	fs/netfs/direct_read.c		DIO read support
> 	fs/netfs/direct_write.c		DIO write support
> 	fs/netfs/unbuffered_write.c	Write modifications directly back
> 
> Beyond the above changes, there are also some changes that affect how
> things work:
> 
>  (1) Make fscache_end_operation() generally available.
> 
>  (2) In the netfs tracing header, generate enums from the symbol -> string
>      mapping tables rather than manually coding them.
> 
>  (3) Add a struct for filesystems that uses netfslib to put into their
>      inode wrapper structs to hold extra state that netfslib is interested
>      in, such as the fscache cookie.  This allows netfslib functions to be
>      set in filesystem operation tables and jumped to directly without
>      having to have a filesystem wrapper.
> 
>  (4) Add a member to the struct added in (3) to track the remote inode
>      length as that may differ if local modifications are buffered.  We may
>      need to supply an appropriate EOF pointer when storing data (in AFS
>      for example).
> 
>  (5) Pass extra information to netfs_alloc_request() so that the
>      ->init_request() hook can access it and retain information to indicate
>      the origin of the operation.
> 
>  (6) Make the ->init_request() hook return an error, thereby allowing a
>      filesystem that isn't allowed to cache an inode (ceph or cifs, for
>      example) to skip readahead.
> 
>  (7) Switch to using refcount_t for subrequests and add tracepoints to log
>      refcount changes for the request and subrequest structs.
> 
>  (8) Add a function to consolidate dispatching a read request.  Similar
>      code is used in three places and another couple are likely to be added
>      in the future.
> 
> 
> The patches can be found on this branch:
> 
> 	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-next
> 
> This is based on top of ceph's master branch as some of the patches
> conflict.
> 
> David
> ---
> 
> Changes
> =======
> ver #3)
>  - Rebased one patch back on the ceph tree as the top patch got removed[4].
>  - Split out the bit to move ceph cap-getting on readahead out from the
>    patch adding an inode context[5].
>  - Made ceph_init_request() store the caps got in rreq->netfs_priv for
>    later freeing.
>  - Comment the need to keep the netfs inode context contiguous with the VFS
>    inode struct[6].
>  - Altered the traces to use 'R=' consistently to denote a request debug ID.
>  
> ver #2)
>  - Changed kdoc references to renamed files[1].
>  - Switched the begin-read-function patch and the prepare-to-split patch as
>    fewer functions then need unstatic'ing.
>  - Fixed an uninitialised var in netfs_begin_read()[2][3].
>  - Fixed a refleak caused by an unremoved line when netfs_begin_read() was
>    introduced.
>  - Used "#if IS_ENABLED()" in netfs_i_cookie(), not "#ifdef".
>  - Implemented missing bit of ceph readahead through netfs_readahead().
>  - Rearranged the patch order to make the ceph readahead possible.
> 
> Link: https://lore.kernel.org/r/20220303202811.6a1d53a1@xxxxxxxxxxxxxxxx/ [1]
> Link: https://lore.kernel.org/r/20220303163826.1120936-1-nathan@xxxxxxxxxx/ [2]
> Link: https://lore.kernel.org/r/20220303235647.1297171-1-colin.i.king@xxxxxxxxx/ [3]
> Link: https://lore.kernel.org/r/527234d849b0de18b326d6db0d59070b70d19b7e.camel@xxxxxxxxxx/ [4]
> Link: https://lore.kernel.org/r/8af0d47f17d89c06bbf602496dd845f2b0bf25b3.camel@xxxxxxxxxx/ [5]
> Link: https://lore.kernel.org/r/beaf4f6a6c2575ed489adb14b257253c868f9a5c.camel@xxxxxxxxxx/ [6]
> Link: https://lore.kernel.org/r/164622970143.3564931.3656393397237724303.stgit@xxxxxxxxxxxxxxxxxxxxxx/ # v1
> Link: https://lore.kernel.org/r/164678185692.1200972.597611902374126174.stgit@xxxxxxxxxxxxxxxxxxxxxx/ # v2
> 
> ---
> David Howells (19):
>       netfs: Generate enums from trace symbol mapping lists
>       netfs: Rename netfs_read_*request to netfs_io_*request
>       netfs: Finish off rename of netfs_read_request to netfs_io_request
>       netfs: Split netfs_io_* object handling out
>       netfs: Adjust the netfs_rreq tracepoint slightly
>       netfs: Trace refcounting on the netfs_io_request struct
>       netfs: Trace refcounting on the netfs_io_subrequest struct
>       netfs: Adjust the netfs_failure tracepoint to indicate non-subreq lines
>       netfs: Refactor arguments for netfs_alloc_read_request
>       netfs: Change ->init_request() to return an error code
>       ceph: Make ceph_init_request() check caps on readahead
>       netfs: Add a netfs inode context
>       netfs: Add a function to consolidate beginning a read
>       netfs: Prepare to split read_helper.c
>       netfs: Rename read_helper.c to io.c
>       netfs: Split fs/netfs/read_helper.c
>       netfs: Split some core bits out into their own file
>       netfs: Keep track of the actual remote file size
>       afs: Maintain netfs_i_context::remote_i_size
> 
> Jeffle Xu (1):
>       fscache: export fscache_end_operation()
> 
> 
>  Documentation/filesystems/netfs_library.rst |  140 ++-
>  fs/9p/cache.c                               |   10 +-
>  fs/9p/v9fs.c                                |    4 +-
>  fs/9p/v9fs.h                                |   13 +-
>  fs/9p/vfs_addr.c                            |   62 +-
>  fs/9p/vfs_inode.c                           |   13 +-
>  fs/afs/dynroot.c                            |    1 +
>  fs/afs/file.c                               |   41 +-
>  fs/afs/inode.c                              |   32 +-
>  fs/afs/internal.h                           |   23 +-
>  fs/afs/super.c                              |    4 +-
>  fs/afs/write.c                              |   10 +-
>  fs/cachefiles/io.c                          |   10 +-
>  fs/ceph/addr.c                              |  116 +-
>  fs/ceph/cache.c                             |   28 +-
>  fs/ceph/cache.h                             |   15 +-
>  fs/ceph/inode.c                             |    6 +-
>  fs/ceph/super.h                             |   17 +-
>  fs/cifs/cifsglob.h                          |   10 +-
>  fs/cifs/fscache.c                           |   19 +-
>  fs/cifs/fscache.h                           |    2 +-
>  fs/fscache/internal.h                       |   11 -
>  fs/netfs/Makefile                           |    8 +-
>  fs/netfs/buffered_read.c                    |  428 +++++++
>  fs/netfs/internal.h                         |   49 +-
>  fs/netfs/io.c                               |  657 ++++++++++
>  fs/netfs/main.c                             |   20 +
>  fs/netfs/objects.c                          |  160 +++
>  fs/netfs/read_helper.c                      | 1205 -------------------
>  fs/netfs/stats.c                            |    1 -
>  fs/nfs/fscache.c                            |    8 -
>  include/linux/fscache.h                     |   14 +
>  include/linux/netfs.h                       |  162 ++-
>  include/trace/events/cachefiles.h           |    6 +-
>  include/trace/events/netfs.h                |  190 ++-
>  35 files changed, 1867 insertions(+), 1628 deletions(-)
>  create mode 100644 fs/netfs/buffered_read.c
>  create mode 100644 fs/netfs/io.c
>  create mode 100644 fs/netfs/main.c
>  create mode 100644 fs/netfs/objects.c
>  delete mode 100644 fs/netfs/read_helper.c
> 
> 

I ran this through xfstests on ceph, with fscache enabled and it seemed
to do fine.

Tested-by: Jeff Layton <jlayton@xxxxxxxxxx>