From: David Howells <dhowells@redhat.com> Date: 2023-10-13 15:57:41
Hi Jeff, Steve,
I have been working on my netfslib helpers to the point that I can run
xfstests on AFS to completion (both with write-back buffering and, with a
small patch, write-through buffering in the pagecache). I can also run a
certain amount of xfstests on CIFS, though that requires some more
debugging. However, this seems like a good time to post a preview of the
patches.
The patches remove a little over 800 lines from AFS and over 2000 from
CIFS, albeit with around 3000 lines added to netfs. Hopefully, I will be
able to remove a bunch of lines from 9P and Ceph too.
The main aims of these patches are to get high-level I/O and knowledge of
the pagecache out of the filesystem drivers as much as possible and to get
rid, as much of possible, of the knowledge that pages/folios exist.
Further, I would like to see ->write_begin, ->write_end and ->launder_folio
go away.
Features that are added by these patches to that which is already there in
netfslib:
(1) NFS-style (and Ceph-style) locking around DIO vs buffered I/O calls to
prevent these from happening at the same time. mmap'd I/O can, of
necessity, happen at any time ignoring these locks.
(2) Support for unbuffered I/O. The data is kept in the bounce buffer and
the pagecache is not used. This can be turned on with an inode flag.
(3) Support for direct I/O. This is basically unbuffered I/O with some
extra restrictions and no RMW.
(4) Support for using a bounce buffer in an operation. The bounce buffer
may be bigger than the target data/buffer, allowing for crypto
rounding.
(5) Support for content encryption. This isn't supported yet by AFS/CIFS
but is aimed initially at Ceph.
(6) ->write_begin() and ->write_end() are ignored in favour of merging all
of that into one function, netfs_perform_write(), thereby avoiding the
function pointer traversals.
(7) Support for write-through caching in the pagecache.
netfs_perform_write() adds the pages is modifies to an I/O operation
as it goes and directly marks them writeback rather than dirty. When
writing back from write-through, it limits the range written back.
This should allow CIFS to deal with byte-range mandatory locks
correctly.
(8) O_*SYNC and RWF_*SYNC writes use write-through rather than writing to
the pagecache and then flushing afterwards. An AIO O_*SYNC write will
notify of completion when the sub-writes all complete.
(9) Support for write-streaming where modifed data is held in !uptodate
folios, with a private struct attached indicating the range that is
valid.
(10) Support for write grouping, multiplexing a pointer to a group in the
folio private data with the write-streaming data. The writepages
algorithm only writes stuff back that's in the nominated group. This
is intended for use by Ceph to write is snaps in order.
(11) Skipping reads for which we know the server could only supply zeros or
EOF (for instance if we've done a local write that leaves a hole in
the file and extends the local inode size).
General notes:
(1) netfslib now makes use of folio->private, which means the filesystem
can't use it.
(2) Use of fscache is not yet tested. I'm not sure whether to allow a
cache to be used with a write-through write.
(3) The filesystem provides wrappers to call the write helpers, allowing
it to do pre-validation, oplock/capability fetching and the passing in
of write group info.
(4) I want to try flushing the data when tearing down an inode before
invalidating it to try and render launder_folio unnecessary.
(5) Write-through caching will generate and dispatch write subrequests as
it gathers enough data to hit wsize and has whole pages that at least
span that size. This needs to be a bit more flexible, allowing for a
filesystem such as CIFS to have a variable wsize.
(6) The filesystem driver is just given read and write calls with an
iov_iter describing the data/buffer to use. Ideally, they don't see
pages or folios at all. A function, extract_iter_to_sg(), is already
available to decant part of an iterator into a scatterlist for crypto
purposes.
CIFS notes:
(1) CIFS is made to use unbuffered I/O for unbuffered caching modes and
write-through caching for cache=strict.
(2) cifs_init_request() occasionally throws an error that it can't get a
writable file when trying to do writeback.
(3) Apparent file corruption frequently appears in the target file when
cifs_copy_file_range(), even though it doesn't use any netfslib
helpers and even if it doesn't overlap with any pages in the
pagecache.
(4) I should be able to turn multipage folio support on in CIFS now.
(5) The then-unused CIFS code is removed in three patches, not one, to
avoid the git patch generator from producing confusing patches in
which it thinks code is being moved around rather than just being
removed.
David
David Howells (53):
netfs: Add a procfile to list in-progress requests
netfs: Track the fpos above which the server has no data
netfs: Note nonblockingness in the netfs_io_request struct
netfs: Allow the netfs to make the io (sub)request alloc larger
netfs: Add a ->free_subrequest() op
afs: Don't use folio->private to record partial modification
netfs: Provide invalidate_folio and release_folio calls
netfs: Add rsize to netfs_io_request
netfs: Implement unbuffered/DIO vs buffered I/O locking
netfs: Add iov_iters to (sub)requests to describe various buffers
netfs: Add support for DIO buffering
netfs: Provide tools to create a buffer in an xarray
netfs: Add bounce buffering support
netfs: Add func to calculate pagecount/size-limited span of an
iterator
netfs: Limit subrequest by size or number of segments
netfs: Export netfs_put_subrequest() and some tracepoints
netfs: Extend the netfs_io_*request structs to handle writes
netfs: Add a hook to allow tell the netfs to update its i_size
netfs: Make netfs_put_request() handle a NULL pointer
fscache: Add a function to begin an cache op from a netfslib request
netfs: Make the refcounting of netfs_begin_read() easier to use
netfs: Prep to use folio->private for write grouping and streaming
write
netfs: Dispatch write requests to process a writeback slice
netfs: Provide func to copy data to pagecache for buffered write
netfs: Make netfs_read_folio() handle streaming-write pages
netfs: Allocate multipage folios in the writepath
netfs: Implement support for unbuffered/DIO read
netfs: Implement unbuffered/DIO write support
netfs: Implement buffered write API
netfs: Allow buffered shared-writeable mmap through
netfs_page_mkwrite()
netfs: Provide netfs_file_read_iter()
netfs: Provide a writepages implementation
netfs: Provide minimum blocksize parameter
netfs: Make netfs_skip_folio_read() take account of blocksize
netfs: Perform content encryption
netfs: Decrypt encrypted content
netfs: Support decryption on ubuffered/DIO read
netfs: Support encryption on Unbuffered/DIO write
netfs: Provide a launder_folio implementation
netfs: Implement a write-through caching option
netfs: Rearrange netfs_io_subrequest to put request pointer first
afs: Use the netfs write helpers
cifs: Replace cifs_readdata with a wrapper around netfs_io_subrequest
cifs: Share server EOF pos with netfslib
cifs: Replace cifs_writedata with a wrapper around netfs_io_subrequest
cifs: Use more fields from netfs_io_subrequest
cifs: Make wait_mtu_credits take size_t args
cifs: Implement netfslib hooks
cifs: Move cifs_loose_read_iter() and cifs_file_write_iter() to file.c
cifs: Cut over to using netfslib
cifs: Remove some code that's no longer used, part 1
cifs: Remove some code that's no longer used, part 2
cifs: Remove some code that's no longer used, part 3
fs/9p/vfs_addr.c | 51 +-
fs/afs/file.c | 206 +--
fs/afs/inode.c | 15 +-
fs/afs/internal.h | 66 +-
fs/afs/write.c | 816 +---------
fs/ceph/addr.c | 28 +-
fs/ceph/cache.h | 12 -
fs/fscache/io.c | 42 +
fs/netfs/Makefile | 9 +-
fs/netfs/buffered_read.c | 245 ++-
fs/netfs/buffered_write.c | 1223 ++++++++++++++
fs/netfs/crypto.c | 148 ++
fs/netfs/direct_read.c | 263 +++
fs/netfs/direct_write.c | 359 +++++
fs/netfs/internal.h | 121 ++
fs/netfs/io.c | 325 +++-
fs/netfs/iterator.c | 97 ++
fs/netfs/locking.c | 209 +++
fs/netfs/main.c | 101 ++
fs/netfs/misc.c | 237 +++
fs/netfs/objects.c | 64 +-
fs/netfs/output.c | 485 ++++++
fs/netfs/stats.c | 22 +-
fs/smb/client/Kconfig | 1 +
fs/smb/client/cifsfs.c | 65 +-
fs/smb/client/cifsfs.h | 10 +-
fs/smb/client/cifsglob.h | 59 +-
fs/smb/client/cifsproto.h | 10 +-
fs/smb/client/cifssmb.c | 111 +-
fs/smb/client/file.c | 2905 ++++++----------------------------
fs/smb/client/fscache.c | 109 --
fs/smb/client/fscache.h | 54 -
fs/smb/client/inode.c | 25 +-
fs/smb/client/smb2ops.c | 20 +-
fs/smb/client/smb2pdu.c | 168 +-
fs/smb/client/smb2proto.h | 5 +-
fs/smb/client/trace.h | 144 +-
fs/smb/client/transport.c | 17 +-
include/linux/fscache.h | 6 +
include/linux/netfs.h | 173 +-
include/trace/events/afs.h | 31 -
include/trace/events/netfs.h | 158 +-
42 files changed, 5136 insertions(+), 4079 deletions(-)
create mode 100644 fs/netfs/buffered_write.c
create mode 100644 fs/netfs/crypto.c
create mode 100644 fs/netfs/direct_read.c
create mode 100644 fs/netfs/direct_write.c
create mode 100644 fs/netfs/locking.c
create mode 100644 fs/netfs/misc.c
create mode 100644 fs/netfs/output.c
From: David Howells <dhowells@redhat.com> Date: 2023-10-13 15:57:45
Track the file position above which the server is not expected to have any
data and preemptively assume that we can simply fill blocks with zeroes
locally rather than attempting to download them - even if we've written
data back to the server. Assume that any data that was written back above
that position is held in the local cache. Call this the "zero point".
Make use of this to optimise away some reads from the server. We need to
set the zero point in the following circumstances:
(1) When we see an extant remote inode and have no cache for it, we set
the zero_point to i_size.
(2) On local inode creation, we set zero_point to 0.
(3) On local truncation down, we reduce zero_point to the new i_size if
the new i_size is lower.
(4) On local truncation up, we don't change zero_point.
(5) On local modification, we don't change zero_point.
(6) On remote invalidation, we set zero_point to the new i_size.
(7) If stored data is culled from the local cache, we must set zero_point
above that if the data also got written to the server.
(8) If dirty data is written back to the server, but not the local cache,
we must set zero_point above that.
Assuming the above, any read from the server at or above the zero_point
position will return all zeroes.
The zero_point value can be stored in the cache, provided the above rules
are applied to it by any code that culls part of the local cache.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cachefs@redhat.com
cc: linux-fsdevel@vger.kernel.org
cc: linux-mm@kvack.org
---
fs/afs/inode.c | 13 +++++++------
fs/netfs/buffered_read.c | 40 +++++++++++++++++++++++++---------------
include/linux/netfs.h | 5 +++++
3 files changed, 37 insertions(+), 21 deletions(-)
@@ -147,6 +147,22 @@ static void netfs_rreq_expand(struct netfs_io_request *rreq,}}+/*+*Beginanoperation,andfetchthestoredzeropointvaluefromthecookieif+*available.+*/+staticintnetfs_begin_cache_operation(structnetfs_io_request*rreq,+structnetfs_inode*ctx)+{+intret=-ENOBUFS;++if(ctx->ops->begin_cache_operation){+ret=ctx->ops->begin_cache_operation(rreq);+/* TODO: Get the zero point value from the cache */+}+returnret;+}+/***netfs_readahead-Helpertomanageareadrequest*@ractl:Thedescriptionofthereadaheadrequest
@@ -129,6 +129,8 @@ struct netfs_inode {structfscache_cookie*cache;#endifloff_tremote_i_size;/* Size of the remote file */+loff_tzero_point;/* Size after which we assume there's no data+*ontheserver*/};/*
From: David Howells <dhowells@redhat.com> Date: 2023-10-13 15:57:53
Allow O_NONBLOCK to be noted in the netfs_io_request struct. Also add a
flag, NETFS_RREQ_BLOCKED to record if we did block.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cachefs@redhat.com
cc: linux-fsdevel@vger.kernel.org
cc: linux-mm@kvack.org
---
fs/netfs/objects.c | 2 ++
include/linux/netfs.h | 2 ++
2 files changed, 4 insertions(+)
@@ -175,10 +175,14 @@ enum netfs_io_origin {*operationstoavarietyofdatastoresandthenstitchtheresulttogether.*/structnetfs_io_request{-structwork_structwork;+union{+structwork_structwork;+structrcu_headrcu;+};structinode*inode;/* The file being accessed */structaddress_space*mapping;/* The mapping being accessed */structnetfs_cache_resourcescache_resources;+structlist_headproc_link;/* Link in netfs_iorequests */structlist_headsubrequests;/* Contributory I/O operations */void*netfs_priv;/* Private data for the netfs */unsignedintdebug_id;
From: David Howells <dhowells@redhat.com> Date: 2023-10-13 15:57:54
Allow the network filesystem to specify extra space to be allocated on the
end of the io (sub)request. This allows cifs, for example, to use this
space rather than allocating its own cifs_readdata struct.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cachefs@redhat.com
cc: linux-fsdevel@vger.kernel.org
cc: linux-mm@kvack.org
---
fs/netfs/objects.c | 7 +++++--
include/linux/netfs.h | 2 ++
2 files changed, 7 insertions(+), 2 deletions(-)
From: David Howells <dhowells@redhat.com> Date: 2023-10-13 15:57:58
Add a ->free_subrequest() op so that the netfs can clean up data attached
to a subrequest.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cachefs@redhat.com
cc: linux-fsdevel@vger.kernel.org
cc: linux-mm@kvack.org
---
fs/netfs/objects.c | 2 ++
include/linux/netfs.h | 1 +
2 files changed, 3 insertions(+)
@@ -175,10 +175,14 @@ enum netfs_io_origin {*operationstoavarietyofdatastoresandthenstitchtheresulttogether.*/structnetfs_io_request{-structwork_structwork;+union{+structwork_structwork;+structrcu_headrcu;+};structinode*inode;/* The file being accessed */structaddress_space*mapping;/* The mapping being accessed */structnetfs_cache_resourcescache_resources;+structlist_headproc_link;/* Link in netfs_iorequests */structlist_headsubrequests;/* Contributory I/O operations */void*netfs_priv;/* Private data for the netfs */unsignedintdebug_id;
ACK on the general concept however. This is useful debugging info.
--
Jeff Layton [off-list ref]
From: Jeff Layton <jlayton@kernel.org> Date: 2023-10-16 15:43:08
On Fri, 2023-10-13 at 16:56 +0100, David Howells wrote:
Track the file position above which the server is not expected to have any
data and preemptively assume that we can simply fill blocks with zeroes
locally rather than attempting to download them - even if we've written
data back to the server. Assume that any data that was written back above
that position is held in the local cache. Call this the "zero point".
Make use of this to optimise away some reads from the server. We need to
set the zero point in the following circumstances:
(1) When we see an extant remote inode and have no cache for it, we set
the zero_point to i_size.
(2) On local inode creation, we set zero_point to 0.
(3) On local truncation down, we reduce zero_point to the new i_size if
the new i_size is lower.
(4) On local truncation up, we don't change zero_point.
(5) On local modification, we don't change zero_point.
(6) On remote invalidation, we set zero_point to the new i_size.
(7) If stored data is culled from the local cache, we must set zero_point
above that if the data also got written to the server.
When you say culled here, it sounds like you're just throwing out the
dirty cache without writing the data back. That shouldn't be allowed
though, so I must be misunderstanding what you mean here. Can you
explain?
(8) If dirty data is written back to the server, but not the local cache,
we must set zero_point above that.
How do you write back without writing to the local cache? I'm guessing
this means you're doing a non-buffered write?
quoted hunk
Assuming the above, any read from the server at or above the zero_point
position will return all zeroes.
The zero_point value can be stored in the cache, provided the above rules
are applied to it by any code that culls part of the local cache.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cachefs@redhat.com
cc: linux-fsdevel@vger.kernel.org
cc: linux-mm@kvack.org
---
fs/afs/inode.c | 13 +++++++------
fs/netfs/buffered_read.c | 40 +++++++++++++++++++++++++---------------
include/linux/netfs.h | 5 +++++
3 files changed, 37 insertions(+), 21 deletions(-)
Isn't this an existing bug? AFS is not setting remote_i_size in the
setattr path currently? I think this probably ought to be done in a
preliminary AFS patch.
@@ -147,6 +147,22 @@ static void netfs_rreq_expand(struct netfs_io_request *rreq,}}+/*+*Beginanoperation,andfetchthestoredzeropointvaluefromthecookieif+*available.+*/+staticintnetfs_begin_cache_operation(structnetfs_io_request*rreq,+structnetfs_inode*ctx)+{+intret=-ENOBUFS;++if(ctx->ops->begin_cache_operation){+ret=ctx->ops->begin_cache_operation(rreq);+/* TODO: Get the zero point value from the cache */+}+returnret;+}+/***netfs_readahead-Helpertomanageareadrequest*@ractl:Thedescriptionofthereadaheadrequest
@@ -129,6 +129,8 @@ struct netfs_inode {structfscache_cookie*cache;#endifloff_tremote_i_size;/* Size of the remote file */+loff_tzero_point;/* Size after which we assume there's no data+*ontheserver*/
While I understand the concept, I'm not yet sure I understand how this
new value will be used. It might be better to merge this patch in with
the patch that adds the first user of this data.
From: Jeff Layton <jlayton@kernel.org> Date: 2023-10-16 15:44:57
On Fri, 2023-10-13 at 16:56 +0100, David Howells wrote:
quoted hunk
Allow O_NONBLOCK to be noted in the netfs_io_request struct. Also add a
flag, NETFS_RREQ_BLOCKED to record if we did block.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cachefs@redhat.com
cc: linux-fsdevel@vger.kernel.org
cc: linux-mm@kvack.org
---
fs/netfs/objects.c | 2 ++
include/linux/netfs.h | 2 ++
2 files changed, 4 insertions(+)
@@ -205,6 +205,8 @@ struct netfs_io_request {#define NETFS_RREQ_DONT_UNLOCK_FOLIOS 3 /* Don't unlock the folios on completion */#define NETFS_RREQ_FAILED 4 /* The request failed */#define NETFS_RREQ_IN_PROGRESS 5 /* Unlocked when the request completes */+#define NETFS_RREQ_NONBLOCK 6 /* Don't block if possible (O_NONBLOCK) */+#define NETFS_RREQ_BLOCKED 7 /* We blocked */conststructnetfs_request_ops*netfs_ops;};
I'd prefer to see this patch squashed in with the first patches that
actually check for these flags. I can't look at this patch alone and
tell how it'll be used.
--
Jeff Layton [off-list ref]
From: David Howells <dhowells@redhat.com> Date: 2023-10-16 16:31:26
Jeff Layton [off-list ref] wrote:
quoted
(7) If stored data is culled from the local cache, we must set zero_point
above that if the data also got written to the server.
When you say culled here, it sounds like you're just throwing out the
dirty cache without writing the data back. That shouldn't be allowed
though, so I must be misunderstanding what you mean here. Can you
explain?
I meant fscache specifically. Too many caches - and some of them with the
same names!
quoted
(8) If dirty data is written back to the server, but not the local cache,
we must set zero_point above that.
How do you write back without writing to the local cache? I'm guessing
this means you're doing a non-buffered write?
I meant fscache. fscache can decline to honour a request to store data.
Isn't this an existing bug? AFS is not setting remote_i_size in the
setattr path currently? I think this probably ought to be done in a
preliminary AFS patch.
It is being set. afs_apply_status() sets it. This is called by
afs_vnode_commit_status() which is called from afs_setattr_success(). The
value isn't updated until we get the return status from the server that
includes the new value.
quoted
+ loff_t zero_point; /* Size after which we assume there's no data
+ * on the server */
While I understand the concept, I'm not yet sure I understand how this
new value will be used. It might be better to merge this patch in with
the patch that adds the first user of this data.
I'll consider it. At least it might make sense to move them adjacent to each
other in the series.
David
On Fri, 13 Oct 2023 at 16:58, David Howells [off-list ref] wrote:
(2) Use of fscache is not yet tested. I'm not sure whether to allow a
cache to be used with a write-through write.
Just adding a quick end user "thumbs up" for this potential feature.
We currently use fscache as the backend for "NFS re-export" servers to
extend our onprem storage to remote cloud compute (which works great).
But batch compute hosts (think VFX render farm) often chunk up stages
of work into multiple batch jobs such that they read data, write
results and then read the same data on different clients. Having the
ability to also cache the recent writes closer to the compute clients
(on the re-export server) would open up a lot of new workload
possibilities for us.
(5) Write-through caching will generate and dispatch write subrequests as
it gathers enough data to hit wsize and has whole pages that at least
span that size. This needs to be a bit more flexible, allowing for a
filesystem such as CIFS to have a variable wsize.
If I understand correctly, this is above and beyond the normal write
back cache and is more in tune with the wsize (of NFS, CIFS etc) for
each file? Again, our workloads are over longer latencies than are
normal (NFS over 200ms!) so this sounds like a nice optimisation when
dealing with slow stuttering file writes over high latency.
I can definitely volunteer for some of the fscache + NFS testing.
Cheers,
Daire