| Message ID | 20241213135013.2964079-8-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | netfs, ceph, nfs, cachefiles: Miscellaneous fixes/changes | expand |
Hi David, David Howells wrote: > Use clear_and_wake_up_bit() rather than something like: > > clear_bit_unlock(NETFS_RREQ_IN_PROGRESS, &rreq->flags); > wake_up_bit(&rreq->flags, NETFS_RREQ_IN_PROGRESS); > > as there needs to be a barrier inserted between which is present in > clear_and_wake_up_bit(). If I am reading the kernel-doc comment of clear_bit_unlock() [1, 2]: This operation is atomic and provides release barrier semantics. correctly, there already seems to be a barrier which should be good enough. [1]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.clear_bit_unlock [2]: include/asm-generic/bitops/instrumented-lock.h > > Fixes: 288ace2f57c9 ("netfs: New writeback implementation") > Fixes: ee4cdf7ba857 ("netfs: Speed up buffered reading") So I'm not sure this fixes anything. What am I missing? Thanks, Akira > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Zilin Guan <zilin@seu.edu.cn> > cc: Akira Yokosawa <akiyks@gmail.com> > cc: Jeff Layton <jlayton@kernel.org> > cc: netfs@lists.linux.dev > cc: linux-fsdevel@vger.kernel.org > --- > fs/netfs/read_collect.c | 3 +-- > fs/netfs/write_collect.c | 9 +++------ > 2 files changed, 4 insertions(+), 8 deletions(-) > [...]
[Adding Paul McKenney as he's the expert.] Akira Yokosawa <akiyks@gmail.com> wrote: > David Howells wrote: > > Use clear_and_wake_up_bit() rather than something like: > > > > clear_bit_unlock(NETFS_RREQ_IN_PROGRESS, &rreq->flags); > > wake_up_bit(&rreq->flags, NETFS_RREQ_IN_PROGRESS); > > > > as there needs to be a barrier inserted between which is present in > > clear_and_wake_up_bit(). > > If I am reading the kernel-doc comment of clear_bit_unlock() [1, 2]: > > This operation is atomic and provides release barrier semantics. > > correctly, there already seems to be a barrier which should be > good enough. > > [1]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.clear_bit_unlock > [2]: include/asm-generic/bitops/instrumented-lock.h > > > > > Fixes: 288ace2f57c9 ("netfs: New writeback implementation") > > Fixes: ee4cdf7ba857 ("netfs: Speed up buffered reading") > > So I'm not sure this fixes anything. > > What am I missing? We may need two barriers. You have three things to synchronise: (1) The stuff you did before unlocking. (2) The lock bit. (3) The task state. clear_bit_unlock() interposes a release barrier between (1) and (2). Neither clear_bit_unlock() nor wake_up_bit(), however, necessarily interpose a barrier between (2) and (3). I'm not sure it entirely matters, but it seems that since we have a function that combines the two, we should probably use it - though, granted, it might not actually be a fix. David
David Howells wrote: > [Adding Paul McKenney as he's the expert.] > > Akira Yokosawa <akiyks@gmail.com> wrote: > >> David Howells wrote: >>> Use clear_and_wake_up_bit() rather than something like: >>> >>> clear_bit_unlock(NETFS_RREQ_IN_PROGRESS, &rreq->flags); >>> wake_up_bit(&rreq->flags, NETFS_RREQ_IN_PROGRESS); >>> >>> as there needs to be a barrier inserted between which is present in >>> clear_and_wake_up_bit(). >> >> If I am reading the kernel-doc comment of clear_bit_unlock() [1, 2]: >> >> This operation is atomic and provides release barrier semantics. >> >> correctly, there already seems to be a barrier which should be >> good enough. >> >> [1]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.clear_bit_unlock >> [2]: include/asm-generic/bitops/instrumented-lock.h >> >>> >>> Fixes: 288ace2f57c9 ("netfs: New writeback implementation") >>> Fixes: ee4cdf7ba857 ("netfs: Speed up buffered reading") >> >> So I'm not sure this fixes anything. >> >> What am I missing? > > We may need two barriers. You have three things to synchronise: > > (1) The stuff you did before unlocking. > > (2) The lock bit. > > (3) The task state. > > clear_bit_unlock() interposes a release barrier between (1) and (2). > > Neither clear_bit_unlock() nor wake_up_bit(), however, necessarily interpose a > barrier between (2) and (3). Got it! I was confused because I compared kernel-doc comments of clear_bit_unlock() and clear_and_wake_up_bit() only, without looking at latter's code. clear_and_wake_up_bit() has this description in its kernel-doc: * The designated bit is cleared and any tasks waiting in wait_on_bit() * or similar will be woken. This call has RELEASE semantics so that * any changes to memory made before this call are guaranteed to be visible * after the corresponding wait_on_bit() completes. , without any mention of additional full barrier at your (3) above. It might be worth mentioning it there. Thoughts? FWIW, Reviewed-by: Akira Yokosawa <akiyks@gmail.com> > I'm not sure it entirely matters, but it seems > that since we have a function that combines the two, we should probably use > it - though, granted, it might not actually be a fix. Looks like it should matter where smp_mb__after_atomic() is stronger than a plain barrier(). Akira
diff --git a/fs/netfs/read_collect.c b/fs/netfs/read_collect.c index b415e3972336..46ce3b7adf07 100644 --- a/fs/netfs/read_collect.c +++ b/fs/netfs/read_collect.c @@ -379,8 +379,7 @@ static void netfs_rreq_assess(struct netfs_io_request *rreq) task_io_account_read(rreq->transferred); trace_netfs_rreq(rreq, netfs_rreq_trace_wake_ip); - clear_bit_unlock(NETFS_RREQ_IN_PROGRESS, &rreq->flags); - wake_up_bit(&rreq->flags, NETFS_RREQ_IN_PROGRESS); + clear_and_wake_up_bit(NETFS_RREQ_IN_PROGRESS, &rreq->flags); trace_netfs_rreq(rreq, netfs_rreq_trace_done); netfs_clear_subrequests(rreq, false); diff --git a/fs/netfs/write_collect.c b/fs/netfs/write_collect.c index 1d438be2e1b4..82290c92ba7a 100644 --- a/fs/netfs/write_collect.c +++ b/fs/netfs/write_collect.c @@ -501,8 +501,7 @@ static void netfs_collect_write_results(struct netfs_io_request *wreq) goto need_retry; if ((notes & MADE_PROGRESS) && test_bit(NETFS_RREQ_PAUSE, &wreq->flags)) { trace_netfs_rreq(wreq, netfs_rreq_trace_unpause); - clear_bit_unlock(NETFS_RREQ_PAUSE, &wreq->flags); - wake_up_bit(&wreq->flags, NETFS_RREQ_PAUSE); + clear_and_wake_up_bit(NETFS_RREQ_PAUSE, &wreq->flags); } if (notes & NEED_REASSESS) { @@ -605,8 +604,7 @@ void netfs_write_collection_worker(struct work_struct *work) _debug("finished"); trace_netfs_rreq(wreq, netfs_rreq_trace_wake_ip); - clear_bit_unlock(NETFS_RREQ_IN_PROGRESS, &wreq->flags); - wake_up_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS); + clear_and_wake_up_bit(NETFS_RREQ_IN_PROGRESS, &wreq->flags); if (wreq->iocb) { size_t written = min(wreq->transferred, wreq->len); @@ -714,8 +712,7 @@ void netfs_write_subrequest_terminated(void *_op, ssize_t transferred_or_error, trace_netfs_sreq(subreq, netfs_sreq_trace_terminated); - clear_bit_unlock(NETFS_SREQ_IN_PROGRESS, &subreq->flags); - wake_up_bit(&subreq->flags, NETFS_SREQ_IN_PROGRESS); + clear_and_wake_up_bit(NETFS_SREQ_IN_PROGRESS, &subreq->flags); /* If we are at the head of the queue, wake up the collector, * transferring a ref to it if we were the ones to do so.
Use clear_and_wake_up_bit() rather than something like: clear_bit_unlock(NETFS_RREQ_IN_PROGRESS, &rreq->flags); wake_up_bit(&rreq->flags, NETFS_RREQ_IN_PROGRESS); as there needs to be a barrier inserted between which is present in clear_and_wake_up_bit(). Fixes: 288ace2f57c9 ("netfs: New writeback implementation") Fixes: ee4cdf7ba857 ("netfs: Speed up buffered reading") Signed-off-by: David Howells <dhowells@redhat.com> cc: Zilin Guan <zilin@seu.edu.cn> cc: Akira Yokosawa <akiyks@gmail.com> cc: Jeff Layton <jlayton@kernel.org> cc: netfs@lists.linux.dev cc: linux-fsdevel@vger.kernel.org --- fs/netfs/read_collect.c | 3 +-- fs/netfs/write_collect.c | 9 +++------ 2 files changed, 4 insertions(+), 8 deletions(-)