mbox series

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

Message ID 164692883658.2099075.5745824552116419504.stgit@warthog.procyon.org.uk (mailing list archive)
Headers show
Series netfs: Prep for write helpers | expand

Message

David Howells March 10, 2022, 4:13 p.m. UTC
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@canb.auug.org.au/ [1]
Link: https://lore.kernel.org/r/20220303163826.1120936-1-nathan@kernel.org/ [2]
Link: https://lore.kernel.org/r/20220303235647.1297171-1-colin.i.king@gmail.com/ [3]
Link: https://lore.kernel.org/r/527234d849b0de18b326d6db0d59070b70d19b7e.camel@kernel.org/ [4]
Link: https://lore.kernel.org/r/8af0d47f17d89c06bbf602496dd845f2b0bf25b3.camel@kernel.org/ [5]
Link: https://lore.kernel.org/r/beaf4f6a6c2575ed489adb14b257253c868f9a5c.camel@kernel.org/ [6]
Link: https://lore.kernel.org/r/164622970143.3564931.3656393397237724303.stgit@warthog.procyon.org.uk/ # v1
Link: https://lore.kernel.org/r/164678185692.1200972.597611902374126174.stgit@warthog.procyon.org.uk/ # 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

Comments

Jeff Layton March 11, 2022, 2:23 p.m. UTC | #1
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@canb.auug.org.au/ [1]
> Link: https://lore.kernel.org/r/20220303163826.1120936-1-nathan@kernel.org/ [2]
> Link: https://lore.kernel.org/r/20220303235647.1297171-1-colin.i.king@gmail.com/ [3]
> Link: https://lore.kernel.org/r/527234d849b0de18b326d6db0d59070b70d19b7e.camel@kernel.org/ [4]
> Link: https://lore.kernel.org/r/8af0d47f17d89c06bbf602496dd845f2b0bf25b3.camel@kernel.org/ [5]
> Link: https://lore.kernel.org/r/beaf4f6a6c2575ed489adb14b257253c868f9a5c.camel@kernel.org/ [6]
> Link: https://lore.kernel.org/r/164622970143.3564931.3656393397237724303.stgit@warthog.procyon.org.uk/ # v1
> Link: https://lore.kernel.org/r/164678185692.1200972.597611902374126174.stgit@warthog.procyon.org.uk/ # 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@kernel.org>
Dominique Martinet March 12, 2022, 8:13 a.m. UTC | #2
David Howells wrote on Thu, Mar 10, 2022 at 04:13:56PM +0000:
> The patches can be found on this branch:
> 
> 	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-next

Looks good to me from the 9p side:
Tested-by: Dominique Martinet <asmadeus@codewreck.org> # 9p

writes being done by 4k chunk is really slow so will be glad to see this
finished, keep it up! :)