| Message ID | 162687511125.276387.15493860267582539643.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,01/12] afs: Sort out symlink reading | expand |
On Wed, 2021-07-21 at 14:45 +0100, David Howells wrote: > Remove netfs_read_subrequest::transferred as it's redundant as the count on > the iterator added to the subrequest can be used instead. > > Signed-off-by: David Howells <dhowells@redhat.com> > --- > > fs/afs/file.c | 4 ++-- > fs/netfs/read_helper.c | 26 ++++---------------------- > include/linux/netfs.h | 1 - > include/trace/events/netfs.h | 12 ++++++------ > 4 files changed, 12 insertions(+), 31 deletions(-) > > diff --git a/fs/afs/file.c b/fs/afs/file.c > index ca529f23515a..82e945dbe379 100644 > --- a/fs/afs/file.c > +++ b/fs/afs/file.c > @@ -315,8 +315,8 @@ static void afs_req_issue_op(struct netfs_read_subrequest *subreq) > return netfs_subreq_terminated(subreq, -ENOMEM, false); > > fsreq->subreq = subreq; > - fsreq->pos = subreq->start + subreq->transferred; > - fsreq->len = subreq->len - subreq->transferred; > + fsreq->pos = subreq->start + subreq->len - iov_iter_count(&subreq->iter); > + fsreq->len = iov_iter_count(&subreq->iter); > fsreq->key = subreq->rreq->netfs_priv; > fsreq->vnode = vnode; > fsreq->iter = &subreq->iter; > diff --git a/fs/netfs/read_helper.c b/fs/netfs/read_helper.c > index 715f3e9c380d..5e1a9be48130 100644 > --- a/fs/netfs/read_helper.c > +++ b/fs/netfs/read_helper.c > @@ -148,12 +148,7 @@ static void __netfs_put_subrequest(struct netfs_read_subrequest *subreq, > */ > static void netfs_clear_unread(struct netfs_read_subrequest *subreq) > { > - struct iov_iter iter; > - > - iov_iter_xarray(&iter, READ, &subreq->rreq->mapping->i_pages, > - subreq->start + subreq->transferred, > - subreq->len - subreq->transferred); > - iov_iter_zero(iov_iter_count(&iter), &iter); > + iov_iter_zero(iov_iter_count(&subreq->iter), &subreq->iter); > } > > static void netfs_cache_read_terminated(void *priv, ssize_t transferred_or_error, > @@ -173,14 +168,9 @@ static void netfs_read_from_cache(struct netfs_read_request *rreq, > bool seek_data) > { > struct netfs_cache_resources *cres = &rreq->cache_resources; > - struct iov_iter iter; > > netfs_stat(&netfs_n_rh_read); > - iov_iter_xarray(&iter, READ, &rreq->mapping->i_pages, > - subreq->start + subreq->transferred, > - subreq->len - subreq->transferred); > - > - cres->ops->read(cres, subreq->start, &iter, seek_data, > + cres->ops->read(cres, subreq->start, &subreq->iter, seek_data, > netfs_cache_read_terminated, subreq); > } > The above two deltas seem like they should have been in patch #2. > @@ -419,7 +409,7 @@ static void netfs_rreq_unlock(struct netfs_read_request *rreq) > if (pgend < iopos + subreq->len) > break; > > - account += subreq->transferred; > + account += subreq->len - iov_iter_count(&subreq->iter); > iopos += subreq->len; > if (!list_is_last(&subreq->rreq_link, &rreq->subrequests)) { > subreq = list_next_entry(subreq, rreq_link); > @@ -635,15 +625,8 @@ void netfs_subreq_terminated(struct netfs_read_subrequest *subreq, > goto failed; > } > > - if (WARN(transferred_or_error > subreq->len - subreq->transferred, > - "Subreq overread: R%x[%x] %zd > %zu - %zu", > - rreq->debug_id, subreq->debug_index, > - transferred_or_error, subreq->len, subreq->transferred)) > - transferred_or_error = subreq->len - subreq->transferred; > - > subreq->error = 0; > - subreq->transferred += transferred_or_error; > - if (subreq->transferred < subreq->len) > + if (iov_iter_count(&subreq->iter)) > goto incomplete; > I must be missing it, but where does subreq->iter get advanced to the end of the current read? If you're getting rid of subreq->transferred then I think that has to happen above, no? > complete: > @@ -667,7 +650,6 @@ void netfs_subreq_terminated(struct netfs_read_subrequest *subreq, > incomplete: > if (test_bit(NETFS_SREQ_CLEAR_TAIL, &subreq->flags)) { > netfs_clear_unread(subreq); > - subreq->transferred = subreq->len; > goto complete; > } > > diff --git a/include/linux/netfs.h b/include/linux/netfs.h > index 5e4fafcc9480..45d40c622205 100644 > --- a/include/linux/netfs.h > +++ b/include/linux/netfs.h > @@ -116,7 +116,6 @@ struct netfs_read_subrequest { > struct iov_iter iter; /* Iterator for this subrequest */ > loff_t start; /* Where to start the I/O */ > size_t len; /* Size of the I/O */ > - size_t transferred; /* Amount of data transferred */ > refcount_t usage; > short error; /* 0 or error that occurred */ > unsigned short debug_index; /* Index in list (for debugging output) */ > diff --git a/include/trace/events/netfs.h b/include/trace/events/netfs.h > index 4d470bffd9f1..04ac29fc700f 100644 > --- a/include/trace/events/netfs.h > +++ b/include/trace/events/netfs.h > @@ -190,7 +190,7 @@ TRACE_EVENT(netfs_sreq, > __field(enum netfs_read_source, source ) > __field(enum netfs_sreq_trace, what ) > __field(size_t, len ) > - __field(size_t, transferred ) > + __field(size_t, remain ) > __field(loff_t, start ) > ), > > @@ -202,7 +202,7 @@ TRACE_EVENT(netfs_sreq, > __entry->source = sreq->source; > __entry->what = what; > __entry->len = sreq->len; > - __entry->transferred = sreq->transferred; > + __entry->remain = iov_iter_count(&sreq->iter); > __entry->start = sreq->start; > ), > > @@ -211,7 +211,7 @@ TRACE_EVENT(netfs_sreq, > __print_symbolic(__entry->what, netfs_sreq_traces), > __print_symbolic(__entry->source, netfs_sreq_sources), > __entry->flags, > - __entry->start, __entry->transferred, __entry->len, > + __entry->start, __entry->len - __entry->remain, __entry->len, > __entry->error) > ); > > @@ -230,7 +230,7 @@ TRACE_EVENT(netfs_failure, > __field(enum netfs_read_source, source ) > __field(enum netfs_failure, what ) > __field(size_t, len ) > - __field(size_t, transferred ) > + __field(size_t, remain ) > __field(loff_t, start ) > ), > > @@ -242,7 +242,7 @@ TRACE_EVENT(netfs_failure, > __entry->source = sreq ? sreq->source : NETFS_INVALID_READ; > __entry->what = what; > __entry->len = sreq ? sreq->len : 0; > - __entry->transferred = sreq ? sreq->transferred : 0; > + __entry->remain = sreq ? iov_iter_count(&sreq->iter) : 0; > __entry->start = sreq ? sreq->start : 0; > ), > > @@ -250,7 +250,7 @@ TRACE_EVENT(netfs_failure, > __entry->rreq, __entry->index, > __print_symbolic(__entry->source, netfs_sreq_sources), > __entry->flags, > - __entry->start, __entry->transferred, __entry->len, > + __entry->start, __entry->len - __entry->remain, __entry->len, > __print_symbolic(__entry->what, netfs_failures), > __entry->error) > ); > >
Jeff Layton <jlayton@redhat.com> wrote: > The above two deltas seem like they should have been in patch #2. Yeah. Looks like at least partially so. > > @@ -635,15 +625,8 @@ void netfs_subreq_terminated(struct netfs_read_subrequest *subreq, > > goto failed; > > } > > > > - if (WARN(transferred_or_error > subreq->len - subreq->transferred, > > - "Subreq overread: R%x[%x] %zd > %zu - %zu", > > - rreq->debug_id, subreq->debug_index, > > - transferred_or_error, subreq->len, subreq->transferred)) > > - transferred_or_error = subreq->len - subreq->transferred; > > - > > subreq->error = 0; > > - subreq->transferred += transferred_or_error; > > - if (subreq->transferred < subreq->len) > > + if (iov_iter_count(&subreq->iter)) > > goto incomplete; > > > > I must be missing it, but where does subreq->iter get advanced to the > end of the current read? If you're getting rid of subreq->transferred > then I think that has to happen above, no? For afs, afs_req_issue_op() points fsreq->iter at the subrequest iterator and calls afs_fetch_data(). Thereafter, we wend our way to afs_deliver_fs_fetch_data() or yfs_deliver_fs_fetch_data() which set call->iter to point to that iterator and then call afs_extract_data() which passes it to rxrpc_kernel_recv_data(), which eventually passes it to skb_copy_datagram_iter(), which advances the iterator. For the cache, the subrequest iterator is passed to the cache backend by netfs_read_from_cache(). This would be cachefiles_read() which calls vfs_iocb_iter_read() which I thought advances the iterator (leastways, filemap_read() keeps going until iov_iter_count() reaches 0 or some other stop condition occurs and doesn't thereafter call iov_iter_revert()). David
On Wed, 2021-07-21 at 19:54 +0100, David Howells wrote: > Jeff Layton <jlayton@redhat.com> wrote: > > > The above two deltas seem like they should have been in patch #2. > > Yeah. Looks like at least partially so. > > > > @@ -635,15 +625,8 @@ void netfs_subreq_terminated(struct netfs_read_subrequest *subreq, > > > goto failed; > > > } > > > > > > - if (WARN(transferred_or_error > subreq->len - subreq->transferred, > > > - "Subreq overread: R%x[%x] %zd > %zu - %zu", > > > - rreq->debug_id, subreq->debug_index, > > > - transferred_or_error, subreq->len, subreq->transferred)) > > > - transferred_or_error = subreq->len - subreq->transferred; > > > - > > > subreq->error = 0; > > > - subreq->transferred += transferred_or_error; > > > - if (subreq->transferred < subreq->len) > > > + if (iov_iter_count(&subreq->iter)) > > > goto incomplete; > > > > > > > I must be missing it, but where does subreq->iter get advanced to the > > end of the current read? If you're getting rid of subreq->transferred > > then I think that has to happen above, no? > > For afs, afs_req_issue_op() points fsreq->iter at the subrequest iterator and > calls afs_fetch_data(). Thereafter, we wend our way to > afs_deliver_fs_fetch_data() or yfs_deliver_fs_fetch_data() which set > call->iter to point to that iterator and then call afs_extract_data() which > passes it to rxrpc_kernel_recv_data(), which eventually passes it to > skb_copy_datagram_iter(), which advances the iterator. > > For the cache, the subrequest iterator is passed to the cache backend by > netfs_read_from_cache(). This would be cachefiles_read() which calls > vfs_iocb_iter_read() which I thought advances the iterator (leastways, > filemap_read() keeps going until iov_iter_count() reaches 0 or some other stop > condition occurs and doesn't thereafter call iov_iter_revert()). > Ok, this will probably regress ceph then. We don't really have anything to do with the subreq->iter at this point and this patch doesn't change that. If you're going to make this change, it'd be cleaner to also fix up ceph_netfs_issue_op to advance the iter at the same time.
diff --git a/fs/afs/file.c b/fs/afs/file.c index ca529f23515a..82e945dbe379 100644 --- a/fs/afs/file.c +++ b/fs/afs/file.c @@ -315,8 +315,8 @@ static void afs_req_issue_op(struct netfs_read_subrequest *subreq) return netfs_subreq_terminated(subreq, -ENOMEM, false); fsreq->subreq = subreq; - fsreq->pos = subreq->start + subreq->transferred; - fsreq->len = subreq->len - subreq->transferred; + fsreq->pos = subreq->start + subreq->len - iov_iter_count(&subreq->iter); + fsreq->len = iov_iter_count(&subreq->iter); fsreq->key = subreq->rreq->netfs_priv; fsreq->vnode = vnode; fsreq->iter = &subreq->iter; diff --git a/fs/netfs/read_helper.c b/fs/netfs/read_helper.c index 715f3e9c380d..5e1a9be48130 100644 --- a/fs/netfs/read_helper.c +++ b/fs/netfs/read_helper.c @@ -148,12 +148,7 @@ static void __netfs_put_subrequest(struct netfs_read_subrequest *subreq, */ static void netfs_clear_unread(struct netfs_read_subrequest *subreq) { - struct iov_iter iter; - - iov_iter_xarray(&iter, READ, &subreq->rreq->mapping->i_pages, - subreq->start + subreq->transferred, - subreq->len - subreq->transferred); - iov_iter_zero(iov_iter_count(&iter), &iter); + iov_iter_zero(iov_iter_count(&subreq->iter), &subreq->iter); } static void netfs_cache_read_terminated(void *priv, ssize_t transferred_or_error, @@ -173,14 +168,9 @@ static void netfs_read_from_cache(struct netfs_read_request *rreq, bool seek_data) { struct netfs_cache_resources *cres = &rreq->cache_resources; - struct iov_iter iter; netfs_stat(&netfs_n_rh_read); - iov_iter_xarray(&iter, READ, &rreq->mapping->i_pages, - subreq->start + subreq->transferred, - subreq->len - subreq->transferred); - - cres->ops->read(cres, subreq->start, &iter, seek_data, + cres->ops->read(cres, subreq->start, &subreq->iter, seek_data, netfs_cache_read_terminated, subreq); } @@ -419,7 +409,7 @@ static void netfs_rreq_unlock(struct netfs_read_request *rreq) if (pgend < iopos + subreq->len) break; - account += subreq->transferred; + account += subreq->len - iov_iter_count(&subreq->iter); iopos += subreq->len; if (!list_is_last(&subreq->rreq_link, &rreq->subrequests)) { subreq = list_next_entry(subreq, rreq_link); @@ -635,15 +625,8 @@ void netfs_subreq_terminated(struct netfs_read_subrequest *subreq, goto failed; } - if (WARN(transferred_or_error > subreq->len - subreq->transferred, - "Subreq overread: R%x[%x] %zd > %zu - %zu", - rreq->debug_id, subreq->debug_index, - transferred_or_error, subreq->len, subreq->transferred)) - transferred_or_error = subreq->len - subreq->transferred; - subreq->error = 0; - subreq->transferred += transferred_or_error; - if (subreq->transferred < subreq->len) + if (iov_iter_count(&subreq->iter)) goto incomplete; complete: @@ -667,7 +650,6 @@ void netfs_subreq_terminated(struct netfs_read_subrequest *subreq, incomplete: if (test_bit(NETFS_SREQ_CLEAR_TAIL, &subreq->flags)) { netfs_clear_unread(subreq); - subreq->transferred = subreq->len; goto complete; } diff --git a/include/linux/netfs.h b/include/linux/netfs.h index 5e4fafcc9480..45d40c622205 100644 --- a/include/linux/netfs.h +++ b/include/linux/netfs.h @@ -116,7 +116,6 @@ struct netfs_read_subrequest { struct iov_iter iter; /* Iterator for this subrequest */ loff_t start; /* Where to start the I/O */ size_t len; /* Size of the I/O */ - size_t transferred; /* Amount of data transferred */ refcount_t usage; short error; /* 0 or error that occurred */ unsigned short debug_index; /* Index in list (for debugging output) */ diff --git a/include/trace/events/netfs.h b/include/trace/events/netfs.h index 4d470bffd9f1..04ac29fc700f 100644 --- a/include/trace/events/netfs.h +++ b/include/trace/events/netfs.h @@ -190,7 +190,7 @@ TRACE_EVENT(netfs_sreq, __field(enum netfs_read_source, source ) __field(enum netfs_sreq_trace, what ) __field(size_t, len ) - __field(size_t, transferred ) + __field(size_t, remain ) __field(loff_t, start ) ), @@ -202,7 +202,7 @@ TRACE_EVENT(netfs_sreq, __entry->source = sreq->source; __entry->what = what; __entry->len = sreq->len; - __entry->transferred = sreq->transferred; + __entry->remain = iov_iter_count(&sreq->iter); __entry->start = sreq->start; ), @@ -211,7 +211,7 @@ TRACE_EVENT(netfs_sreq, __print_symbolic(__entry->what, netfs_sreq_traces), __print_symbolic(__entry->source, netfs_sreq_sources), __entry->flags, - __entry->start, __entry->transferred, __entry->len, + __entry->start, __entry->len - __entry->remain, __entry->len, __entry->error) ); @@ -230,7 +230,7 @@ TRACE_EVENT(netfs_failure, __field(enum netfs_read_source, source ) __field(enum netfs_failure, what ) __field(size_t, len ) - __field(size_t, transferred ) + __field(size_t, remain ) __field(loff_t, start ) ), @@ -242,7 +242,7 @@ TRACE_EVENT(netfs_failure, __entry->source = sreq ? sreq->source : NETFS_INVALID_READ; __entry->what = what; __entry->len = sreq ? sreq->len : 0; - __entry->transferred = sreq ? sreq->transferred : 0; + __entry->remain = sreq ? iov_iter_count(&sreq->iter) : 0; __entry->start = sreq ? sreq->start : 0; ), @@ -250,7 +250,7 @@ TRACE_EVENT(netfs_failure, __entry->rreq, __entry->index, __print_symbolic(__entry->source, netfs_sreq_sources), __entry->flags, - __entry->start, __entry->transferred, __entry->len, + __entry->start, __entry->len - __entry->remain, __entry->len, __print_symbolic(__entry->what, netfs_failures), __entry->error) );
Remove netfs_read_subrequest::transferred as it's redundant as the count on the iterator added to the subrequest can be used instead. Signed-off-by: David Howells <dhowells@redhat.com> --- fs/afs/file.c | 4 ++-- fs/netfs/read_helper.c | 26 ++++---------------------- include/linux/netfs.h | 1 - include/trace/events/netfs.h | 12 ++++++------ 4 files changed, 12 insertions(+), 31 deletions(-)