Re: [PATCH] fs/netfs: Remove redundant use of smp_rmb()

[Akira Yokosawa <akiyks@xxxxxxxxx>]

[+CC: Mateusz, who responded ZiLin's original question at:
  https://lore.kernel.org/ulg54pf2qnlzqfj247fypypzun2yvwepqrcwaqzlr6sn3ukuab@rov7btfppktc/ ]

On Sat,  7 Dec 2024 02:19:52 +0000, Zilin Guan wrote:
> The function netfs_unbuffered_write_iter_locked() in
> fs/netfs/direct_write.c contains an unnecessary smp_rmb() call after
> wait_on_bit(). Since wait_on_bit() already incorporates a memory barrier
> that ensures the flag update is visible before the function returns, the
> smp_rmb() provides no additional benefit and incurs unnecessary overhead.
> 
> This patch removes the redundant barrier to simplify and optimize the code.
> 
> Signed-off-by: Zilin Guan <zilin@xxxxxxxxxx>
> ---
>  fs/netfs/direct_write.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/netfs/direct_write.c b/fs/netfs/direct_write.c
> index 88f2adfab75e..173e8b5e6a93 100644
> --- a/fs/netfs/direct_write.c
> +++ b/fs/netfs/direct_write.c
> @@ -104,7 +104,6 @@ ssize_t netfs_unbuffered_write_iter_locked(struct kiocb *iocb, struct iov_iter *
>  		trace_netfs_rreq(wreq, netfs_rreq_trace_wait_ip);
>  		wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS,
>  			    TASK_UNINTERRUPTIBLE);
> -		smp_rmb(); /* Read error/transferred after RIP flag */
>  		ret = wreq->error;
>  		if (ret == 0) {
>  			ret = wreq->transferred;

You are removing a barrier which is deemed to be required by LKMM.
See section "SLEEP AND WAKE-UP FUNCTIONS" in Documentation/memory-barriers.txt.

Quoting relevant note below:

-----------------------------------------------------------------------------

[!] Note that the memory barriers implied by the sleeper and the waker do *not*
order multiple stores before the wake-up with respect to loads of those stored
values after the sleeper has called set_current_state().  For instance, if the
sleeper does:

	set_current_state(TASK_INTERRUPTIBLE);
	if (event_indicated)
		break;
	__set_current_state(TASK_RUNNING);
	do_something(my_data);

and the waker does:

	my_data = value;
	event_indicated = 1;
	wake_up(&event_wait_queue);

there's no guarantee that the change to event_indicated will be perceived by
the sleeper as coming after the change to my_data.  In such a circumstance, the
code on both sides must interpolate its own memory barriers between the
separate data accesses.  Thus the above sleeper ought to do:

	set_current_state(TASK_INTERRUPTIBLE);
	if (event_indicated) {
		smp_rmb();
		do_something(my_data);
	}

and the waker should do:

	my_data = value;
	smp_wmb();
	event_indicated = 1;
	wake_up(&event_wait_queue);

-----------------------------------------------------------------------------

Are you sure removing the smp_rmb() is realy the right thing to do?

        Thanks, Akira

> -- 
> 2.34.1