| Message ID | 164678214287.1200972.16734134007649832160.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | netfs: Prep for write helpers | expand |
On Tue, 2022-03-08 at 23:29 +0000, David Howells wrote: > Add a function to do the steps needed to begin a read request, allowing > this code to be removed from several other functions and consolidated. > > Changes > ======= > ver #2) > - Move before the unstaticking patch so that some functions can be left > static. > - Set uninitialised return code in netfs_begin_read()[1][2]. > - Fixed a refleak caused by non-removal of a get from netfs_write_begin() > when the request submission code got moved to netfs_begin_read(). > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: linux-cachefs@redhat.com > > Link: https://lore.kernel.org/r/20220303163826.1120936-1-nathan@kernel.org/ [1] > Link: https://lore.kernel.org/r/20220303235647.1297171-1-colin.i.king@gmail.com/ [2] > Link: https://lore.kernel.org/r/164623004355.3564931.7275693529042495641.stgit@warthog.procyon.org.uk/ # v1 > --- > > fs/netfs/internal.h | 2 - > fs/netfs/objects.c | 2 - > fs/netfs/read_helper.c | 144 +++++++++++++++++++++--------------------- > include/trace/events/netfs.h | 5 + > 4 files changed, 77 insertions(+), 76 deletions(-) > > diff --git a/fs/netfs/internal.h b/fs/netfs/internal.h > index 5f9719409f21..937c2465943f 100644 > --- a/fs/netfs/internal.h > +++ b/fs/netfs/internal.h > @@ -39,7 +39,7 @@ static inline void netfs_see_request(struct netfs_io_request *rreq, > */ > extern unsigned int netfs_debug; > > -void netfs_rreq_work(struct work_struct *work); > +int netfs_begin_read(struct netfs_io_request *rreq, bool sync); > > /* > * stats.c > diff --git a/fs/netfs/objects.c b/fs/netfs/objects.c > index 657b19e60118..82f4d6c4f515 100644 > --- a/fs/netfs/objects.c > +++ b/fs/netfs/objects.c > @@ -35,7 +35,7 @@ struct netfs_io_request *netfs_alloc_request(struct address_space *mapping, > rreq->i_size = i_size_read(inode); > rreq->debug_id = atomic_inc_return(&debug_ids); > INIT_LIST_HEAD(&rreq->subrequests); > - INIT_WORK(&rreq->work, netfs_rreq_work); > + INIT_WORK(&rreq->work, NULL); > refcount_set(&rreq->ref, 1); > __set_bit(NETFS_RREQ_IN_PROGRESS, &rreq->flags); > if (rreq->netfs_ops->init_request) { > diff --git a/fs/netfs/read_helper.c b/fs/netfs/read_helper.c > index 73be06c409bb..19d4fe0b4987 100644 > --- a/fs/netfs/read_helper.c > +++ b/fs/netfs/read_helper.c > @@ -443,7 +443,7 @@ static void netfs_rreq_assess(struct netfs_io_request *rreq, bool was_async) > netfs_rreq_completed(rreq, was_async); > } > > -void netfs_rreq_work(struct work_struct *work) > +static void netfs_rreq_work(struct work_struct *work) > { > struct netfs_io_request *rreq = > container_of(work, struct netfs_io_request, work); > @@ -688,6 +688,69 @@ static bool netfs_rreq_submit_slice(struct netfs_io_request *rreq, > return false; > } > > +/* > + * Begin the process of reading in a chunk of data, where that data may be > + * stitched together from multiple sources, including multiple servers and the > + * local cache. > + */ > +int netfs_begin_read(struct netfs_io_request *rreq, bool sync) > +{ > + unsigned int debug_index = 0; > + int ret; > + > + _enter("R=%x %llx-%llx", > + rreq->debug_id, rreq->start, rreq->start + rreq->len - 1); > + > + if (rreq->len == 0) { > + pr_err("Zero-sized read [R=%x]\n", rreq->debug_id); > + netfs_put_request(rreq, false, netfs_rreq_trace_put_zero_len); > + return -EIO; > + } > + > + rreq->work.func = netfs_rreq_work; > + ^^^ This seems like it should be an INIT_WORK call. I assume you're moving this here this because you intend to use netfs_alloc_request for writes too? > + if (sync) > + netfs_get_request(rreq, netfs_rreq_trace_get_hold); > + > + /* Chop the read into slices according to what the cache and the netfs > + * want and submit each one. > + */ > + atomic_set(&rreq->nr_outstanding, 1); > + do { > + if (!netfs_rreq_submit_slice(rreq, &debug_index)) > + break; > + > + } while (rreq->submitted < rreq->len); > + > + if (sync) { > + /* Keep nr_outstanding incremented so that the ref always belongs to > + * us, and the service code isn't punted off to a random thread pool to > + * process. > + */ > + for (;;) { > + wait_var_event(&rreq->nr_outstanding, > + atomic_read(&rreq->nr_outstanding) == 1); > + netfs_rreq_assess(rreq, false); > + if (!test_bit(NETFS_RREQ_IN_PROGRESS, &rreq->flags)) > + break; > + cond_resched(); > + } > + > + ret = rreq->error; > + if (ret == 0 && rreq->submitted < rreq->len) { > + trace_netfs_failure(rreq, NULL, ret, netfs_fail_short_read); > + ret = -EIO; > + } > + netfs_put_request(rreq, false, netfs_rreq_trace_put_hold); > + } else { > + /* If we decrement nr_outstanding to 0, the ref belongs to us. */ > + if (atomic_dec_and_test(&rreq->nr_outstanding)) > + netfs_rreq_assess(rreq, false); > + ret = 0; > + } > + return ret; > +} > + > static void netfs_cache_expand_readahead(struct netfs_io_request *rreq, > loff_t *_start, size_t *_len, loff_t i_size) > { > @@ -750,7 +813,6 @@ void netfs_readahead(struct readahead_control *ractl) > { > struct netfs_io_request *rreq; > struct netfs_i_context *ctx = netfs_i_context(ractl->mapping->host); > - unsigned int debug_index = 0; > int ret; > > _enter("%lx,%x", readahead_index(ractl), readahead_count(ractl)); > @@ -777,22 +839,13 @@ void netfs_readahead(struct readahead_control *ractl) > > netfs_rreq_expand(rreq, ractl); > > - atomic_set(&rreq->nr_outstanding, 1); > - do { > - if (!netfs_rreq_submit_slice(rreq, &debug_index)) > - break; > - > - } while (rreq->submitted < rreq->len); > - > /* Drop the refs on the folios here rather than in the cache or > * filesystem. The locks will be dropped in netfs_rreq_unlock(). > */ > while (readahead_folio(ractl)) > ; > > - /* If we decrement nr_outstanding to 0, the ref belongs to us. */ > - if (atomic_dec_and_test(&rreq->nr_outstanding)) > - netfs_rreq_assess(rreq, false); > + netfs_begin_read(rreq, false); > return; > > cleanup_free: > @@ -821,7 +874,6 @@ int netfs_readpage(struct file *file, struct page *subpage) > struct address_space *mapping = folio->mapping; > struct netfs_io_request *rreq; > struct netfs_i_context *ctx = netfs_i_context(mapping->host); > - unsigned int debug_index = 0; > int ret; > > _enter("%lx", folio_index(folio)); > @@ -836,42 +888,16 @@ int netfs_readpage(struct file *file, struct page *subpage) > > if (ctx->ops->begin_cache_operation) { > ret = ctx->ops->begin_cache_operation(rreq); > - if (ret == -ENOMEM || ret == -EINTR || ret == -ERESTARTSYS) { > - folio_unlock(folio); > - goto out; > - } > + if (ret == -ENOMEM || ret == -EINTR || ret == -ERESTARTSYS) > + goto discard; > } > > netfs_stat(&netfs_n_rh_readpage); > trace_netfs_read(rreq, rreq->start, rreq->len, netfs_read_trace_readpage); > + return netfs_begin_read(rreq, true); > > - netfs_get_request(rreq, netfs_rreq_trace_get_hold); > - > - atomic_set(&rreq->nr_outstanding, 1); > - do { > - if (!netfs_rreq_submit_slice(rreq, &debug_index)) > - break; > - > - } while (rreq->submitted < rreq->len); > - > - /* Keep nr_outstanding incremented so that the ref always belongs to us, and > - * the service code isn't punted off to a random thread pool to > - * process. > - */ > - do { > - wait_var_event(&rreq->nr_outstanding, > - atomic_read(&rreq->nr_outstanding) == 1); > - netfs_rreq_assess(rreq, false); > - } while (test_bit(NETFS_RREQ_IN_PROGRESS, &rreq->flags)); > - > - ret = rreq->error; > - if (ret == 0 && rreq->submitted < rreq->len) { > - trace_netfs_failure(rreq, NULL, ret, netfs_fail_short_readpage); > - ret = -EIO; > - } > -out: > - netfs_put_request(rreq, false, netfs_rreq_trace_put_hold); > - return ret; > +discard: > + netfs_put_request(rreq, false, netfs_rreq_trace_put_discard); > alloc_error: > folio_unlock(folio); > return ret; > @@ -966,7 +992,7 @@ int netfs_write_begin(struct file *file, struct address_space *mapping, > struct netfs_io_request *rreq; > struct netfs_i_context *ctx = netfs_i_context(file_inode(file )); > struct folio *folio; > - unsigned int debug_index = 0, fgp_flags; > + unsigned int fgp_flags; > pgoff_t index = pos >> PAGE_SHIFT; > int ret; > > @@ -1029,39 +1055,13 @@ int netfs_write_begin(struct file *file, struct address_space *mapping, > */ > ractl._nr_pages = folio_nr_pages(folio); > netfs_rreq_expand(rreq, &ractl); > - netfs_get_request(rreq, netfs_rreq_trace_get_hold); > > /* We hold the folio locks, so we can drop the references */ > folio_get(folio); > while (readahead_folio(&ractl)) > ; > > - atomic_set(&rreq->nr_outstanding, 1); > - do { > - if (!netfs_rreq_submit_slice(rreq, &debug_index)) > - break; > - > - } while (rreq->submitted < rreq->len); > - > - /* Keep nr_outstanding incremented so that the ref always belongs to > - * us, and the service code isn't punted off to a random thread pool to > - * process. > - */ > - for (;;) { > - wait_var_event(&rreq->nr_outstanding, > - atomic_read(&rreq->nr_outstanding) == 1); > - netfs_rreq_assess(rreq, false); > - if (!test_bit(NETFS_RREQ_IN_PROGRESS, &rreq->flags)) > - break; > - cond_resched(); > - } > - > - ret = rreq->error; > - if (ret == 0 && rreq->submitted < rreq->len) { > - trace_netfs_failure(rreq, NULL, ret, netfs_fail_short_write_begin); > - ret = -EIO; > - } > - netfs_put_request(rreq, false, netfs_rreq_trace_put_hold); > + ret = netfs_begin_read(rreq, true); > if (ret < 0) > goto error; > > diff --git a/include/trace/events/netfs.h b/include/trace/events/netfs.h > index 685b07573394..55501d044bbc 100644 > --- a/include/trace/events/netfs.h > +++ b/include/trace/events/netfs.h > @@ -56,17 +56,18 @@ > EM(netfs_fail_check_write_begin, "check-write-begin") \ > EM(netfs_fail_copy_to_cache, "copy-to-cache") \ > EM(netfs_fail_read, "read") \ > - EM(netfs_fail_short_readpage, "short-readpage") \ > - EM(netfs_fail_short_write_begin, "short-write-begin") \ > + EM(netfs_fail_short_read, "short-read") \ > E_(netfs_fail_prepare_write, "prep-write") > > #define netfs_rreq_ref_traces \ > EM(netfs_rreq_trace_get_hold, "GET HOLD ") \ > EM(netfs_rreq_trace_get_subreq, "GET SUBREQ ") \ > EM(netfs_rreq_trace_put_complete, "PUT COMPLT ") \ > + EM(netfs_rreq_trace_put_discard, "PUT DISCARD") \ > EM(netfs_rreq_trace_put_failed, "PUT FAILED ") \ > EM(netfs_rreq_trace_put_hold, "PUT HOLD ") \ > EM(netfs_rreq_trace_put_subreq, "PUT SUBREQ ") \ > + EM(netfs_rreq_trace_put_zero_len, "PUT ZEROLEN") \ > E_(netfs_rreq_trace_new, "NEW ") > > #define netfs_sreq_ref_traces \ > > Seems reasonable otherwise.
Jeff Layton <jlayton@redhat.com> wrote: > > + rreq->work.func = netfs_rreq_work; > > + > > ^^^ > This seems like it should be an INIT_WORK call. I assume you're moving > this here this because you intend to use netfs_alloc_request for writes > too? Interesting question. INIT_WORK() was called in netfs_alloc_request(), so the lockdep state has already been initialised and may even have been used already (say, for instance, we do an RMW cycle buffering in the same request struct). David
diff --git a/fs/netfs/internal.h b/fs/netfs/internal.h index 5f9719409f21..937c2465943f 100644 --- a/fs/netfs/internal.h +++ b/fs/netfs/internal.h @@ -39,7 +39,7 @@ static inline void netfs_see_request(struct netfs_io_request *rreq, */ extern unsigned int netfs_debug; -void netfs_rreq_work(struct work_struct *work); +int netfs_begin_read(struct netfs_io_request *rreq, bool sync); /* * stats.c diff --git a/fs/netfs/objects.c b/fs/netfs/objects.c index 657b19e60118..82f4d6c4f515 100644 --- a/fs/netfs/objects.c +++ b/fs/netfs/objects.c @@ -35,7 +35,7 @@ struct netfs_io_request *netfs_alloc_request(struct address_space *mapping, rreq->i_size = i_size_read(inode); rreq->debug_id = atomic_inc_return(&debug_ids); INIT_LIST_HEAD(&rreq->subrequests); - INIT_WORK(&rreq->work, netfs_rreq_work); + INIT_WORK(&rreq->work, NULL); refcount_set(&rreq->ref, 1); __set_bit(NETFS_RREQ_IN_PROGRESS, &rreq->flags); if (rreq->netfs_ops->init_request) { diff --git a/fs/netfs/read_helper.c b/fs/netfs/read_helper.c index 73be06c409bb..19d4fe0b4987 100644 --- a/fs/netfs/read_helper.c +++ b/fs/netfs/read_helper.c @@ -443,7 +443,7 @@ static void netfs_rreq_assess(struct netfs_io_request *rreq, bool was_async) netfs_rreq_completed(rreq, was_async); } -void netfs_rreq_work(struct work_struct *work) +static void netfs_rreq_work(struct work_struct *work) { struct netfs_io_request *rreq = container_of(work, struct netfs_io_request, work); @@ -688,6 +688,69 @@ static bool netfs_rreq_submit_slice(struct netfs_io_request *rreq, return false; } +/* + * Begin the process of reading in a chunk of data, where that data may be + * stitched together from multiple sources, including multiple servers and the + * local cache. + */ +int netfs_begin_read(struct netfs_io_request *rreq, bool sync) +{ + unsigned int debug_index = 0; + int ret; + + _enter("R=%x %llx-%llx", + rreq->debug_id, rreq->start, rreq->start + rreq->len - 1); + + if (rreq->len == 0) { + pr_err("Zero-sized read [R=%x]\n", rreq->debug_id); + netfs_put_request(rreq, false, netfs_rreq_trace_put_zero_len); + return -EIO; + } + + rreq->work.func = netfs_rreq_work; + + if (sync) + netfs_get_request(rreq, netfs_rreq_trace_get_hold); + + /* Chop the read into slices according to what the cache and the netfs + * want and submit each one. + */ + atomic_set(&rreq->nr_outstanding, 1); + do { + if (!netfs_rreq_submit_slice(rreq, &debug_index)) + break; + + } while (rreq->submitted < rreq->len); + + if (sync) { + /* Keep nr_outstanding incremented so that the ref always belongs to + * us, and the service code isn't punted off to a random thread pool to + * process. + */ + for (;;) { + wait_var_event(&rreq->nr_outstanding, + atomic_read(&rreq->nr_outstanding) == 1); + netfs_rreq_assess(rreq, false); + if (!test_bit(NETFS_RREQ_IN_PROGRESS, &rreq->flags)) + break; + cond_resched(); + } + + ret = rreq->error; + if (ret == 0 && rreq->submitted < rreq->len) { + trace_netfs_failure(rreq, NULL, ret, netfs_fail_short_read); + ret = -EIO; + } + netfs_put_request(rreq, false, netfs_rreq_trace_put_hold); + } else { + /* If we decrement nr_outstanding to 0, the ref belongs to us. */ + if (atomic_dec_and_test(&rreq->nr_outstanding)) + netfs_rreq_assess(rreq, false); + ret = 0; + } + return ret; +} + static void netfs_cache_expand_readahead(struct netfs_io_request *rreq, loff_t *_start, size_t *_len, loff_t i_size) { @@ -750,7 +813,6 @@ void netfs_readahead(struct readahead_control *ractl) { struct netfs_io_request *rreq; struct netfs_i_context *ctx = netfs_i_context(ractl->mapping->host); - unsigned int debug_index = 0; int ret; _enter("%lx,%x", readahead_index(ractl), readahead_count(ractl)); @@ -777,22 +839,13 @@ void netfs_readahead(struct readahead_control *ractl) netfs_rreq_expand(rreq, ractl); - atomic_set(&rreq->nr_outstanding, 1); - do { - if (!netfs_rreq_submit_slice(rreq, &debug_index)) - break; - - } while (rreq->submitted < rreq->len); - /* Drop the refs on the folios here rather than in the cache or * filesystem. The locks will be dropped in netfs_rreq_unlock(). */ while (readahead_folio(ractl)) ; - /* If we decrement nr_outstanding to 0, the ref belongs to us. */ - if (atomic_dec_and_test(&rreq->nr_outstanding)) - netfs_rreq_assess(rreq, false); + netfs_begin_read(rreq, false); return; cleanup_free: @@ -821,7 +874,6 @@ int netfs_readpage(struct file *file, struct page *subpage) struct address_space *mapping = folio->mapping; struct netfs_io_request *rreq; struct netfs_i_context *ctx = netfs_i_context(mapping->host); - unsigned int debug_index = 0; int ret; _enter("%lx", folio_index(folio)); @@ -836,42 +888,16 @@ int netfs_readpage(struct file *file, struct page *subpage) if (ctx->ops->begin_cache_operation) { ret = ctx->ops->begin_cache_operation(rreq); - if (ret == -ENOMEM || ret == -EINTR || ret == -ERESTARTSYS) { - folio_unlock(folio); - goto out; - } + if (ret == -ENOMEM || ret == -EINTR || ret == -ERESTARTSYS) + goto discard; } netfs_stat(&netfs_n_rh_readpage); trace_netfs_read(rreq, rreq->start, rreq->len, netfs_read_trace_readpage); + return netfs_begin_read(rreq, true); - netfs_get_request(rreq, netfs_rreq_trace_get_hold); - - atomic_set(&rreq->nr_outstanding, 1); - do { - if (!netfs_rreq_submit_slice(rreq, &debug_index)) - break; - - } while (rreq->submitted < rreq->len); - - /* Keep nr_outstanding incremented so that the ref always belongs to us, and - * the service code isn't punted off to a random thread pool to - * process. - */ - do { - wait_var_event(&rreq->nr_outstanding, - atomic_read(&rreq->nr_outstanding) == 1); - netfs_rreq_assess(rreq, false); - } while (test_bit(NETFS_RREQ_IN_PROGRESS, &rreq->flags)); - - ret = rreq->error; - if (ret == 0 && rreq->submitted < rreq->len) { - trace_netfs_failure(rreq, NULL, ret, netfs_fail_short_readpage); - ret = -EIO; - } -out: - netfs_put_request(rreq, false, netfs_rreq_trace_put_hold); - return ret; +discard: + netfs_put_request(rreq, false, netfs_rreq_trace_put_discard); alloc_error: folio_unlock(folio); return ret; @@ -966,7 +992,7 @@ int netfs_write_begin(struct file *file, struct address_space *mapping, struct netfs_io_request *rreq; struct netfs_i_context *ctx = netfs_i_context(file_inode(file )); struct folio *folio; - unsigned int debug_index = 0, fgp_flags; + unsigned int fgp_flags; pgoff_t index = pos >> PAGE_SHIFT; int ret; @@ -1029,39 +1055,13 @@ int netfs_write_begin(struct file *file, struct address_space *mapping, */ ractl._nr_pages = folio_nr_pages(folio); netfs_rreq_expand(rreq, &ractl); - netfs_get_request(rreq, netfs_rreq_trace_get_hold); /* We hold the folio locks, so we can drop the references */ folio_get(folio); while (readahead_folio(&ractl)) ; - atomic_set(&rreq->nr_outstanding, 1); - do { - if (!netfs_rreq_submit_slice(rreq, &debug_index)) - break; - - } while (rreq->submitted < rreq->len); - - /* Keep nr_outstanding incremented so that the ref always belongs to - * us, and the service code isn't punted off to a random thread pool to - * process. - */ - for (;;) { - wait_var_event(&rreq->nr_outstanding, - atomic_read(&rreq->nr_outstanding) == 1); - netfs_rreq_assess(rreq, false); - if (!test_bit(NETFS_RREQ_IN_PROGRESS, &rreq->flags)) - break; - cond_resched(); - } - - ret = rreq->error; - if (ret == 0 && rreq->submitted < rreq->len) { - trace_netfs_failure(rreq, NULL, ret, netfs_fail_short_write_begin); - ret = -EIO; - } - netfs_put_request(rreq, false, netfs_rreq_trace_put_hold); + ret = netfs_begin_read(rreq, true); if (ret < 0) goto error; diff --git a/include/trace/events/netfs.h b/include/trace/events/netfs.h index 685b07573394..55501d044bbc 100644 --- a/include/trace/events/netfs.h +++ b/include/trace/events/netfs.h @@ -56,17 +56,18 @@ EM(netfs_fail_check_write_begin, "check-write-begin") \ EM(netfs_fail_copy_to_cache, "copy-to-cache") \ EM(netfs_fail_read, "read") \ - EM(netfs_fail_short_readpage, "short-readpage") \ - EM(netfs_fail_short_write_begin, "short-write-begin") \ + EM(netfs_fail_short_read, "short-read") \ E_(netfs_fail_prepare_write, "prep-write") #define netfs_rreq_ref_traces \ EM(netfs_rreq_trace_get_hold, "GET HOLD ") \ EM(netfs_rreq_trace_get_subreq, "GET SUBREQ ") \ EM(netfs_rreq_trace_put_complete, "PUT COMPLT ") \ + EM(netfs_rreq_trace_put_discard, "PUT DISCARD") \ EM(netfs_rreq_trace_put_failed, "PUT FAILED ") \ EM(netfs_rreq_trace_put_hold, "PUT HOLD ") \ EM(netfs_rreq_trace_put_subreq, "PUT SUBREQ ") \ + EM(netfs_rreq_trace_put_zero_len, "PUT ZEROLEN") \ E_(netfs_rreq_trace_new, "NEW ") #define netfs_sreq_ref_traces \
Add a function to do the steps needed to begin a read request, allowing this code to be removed from several other functions and consolidated. Changes ======= ver #2) - Move before the unstaticking patch so that some functions can be left static. - Set uninitialised return code in netfs_begin_read()[1][2]. - Fixed a refleak caused by non-removal of a get from netfs_write_begin() when the request submission code got moved to netfs_begin_read(). Signed-off-by: David Howells <dhowells@redhat.com> cc: linux-cachefs@redhat.com Link: https://lore.kernel.org/r/20220303163826.1120936-1-nathan@kernel.org/ [1] Link: https://lore.kernel.org/r/20220303235647.1297171-1-colin.i.king@gmail.com/ [2] Link: https://lore.kernel.org/r/164623004355.3564931.7275693529042495641.stgit@warthog.procyon.org.uk/ # v1 --- fs/netfs/internal.h | 2 - fs/netfs/objects.c | 2 - fs/netfs/read_helper.c | 144 +++++++++++++++++++++--------------------- include/trace/events/netfs.h | 5 + 4 files changed, 77 insertions(+), 76 deletions(-)