Re: [PATCH] 9p/virtio: add a read barrier in p9_virtio_zc_request

[Christian Schoenebeck]

On Saturday, December 10, 2022 1:10:44 AM CET Dominique Martinet wrote:
> The request receiving thread writes into request then marks the request
> valid in p9_client_cb by setting status after a write barrier.
> 
> p9_virtio_zc_request must like p9_client_rpc issue a read barrier after
> getting notified of the new request status before reading other request
> files.
> 
> (This has been noticed while fixing the usage of READ/WRITE_ONCE macros
> for request status)
> 
> Link: https://lkml.kernel.org/r/167052961.MU3OA6Uzks@silver
> Reported-by: Christian Schoenebeck <linux_oss@xxxxxxxxxxxxx>
> Signed-off-by: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
> ---
>  net/9p/trans_virtio.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 3c27ffb781e3..98425c63b3c3 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -533,6 +533,12 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
>  	p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
>  	err = wait_event_killable(req->wq,
>  			          READ_ONCE(req->status) >= REQ_STATUS_RCVD);
> +
> +	/* Make sure our req is coherent with regard to updates in other
> +	 * threads - echoes to wmb() in the callback like p9_client_rpc
> +	 */
> +	smp_rmb();
> +

Oh, I had p9_client_zc_rpc() for this in mind, but I see why you chose this
place in p9_virtio_zc_request() instead. LGTM

I also made some tests to check whether this barrier would hurt performance,
but I measured no difference. So this should be good to go:

Reviewed-by: Christian Schoenebeck <linux_oss@xxxxxxxxxxxxx>

>  	// RERROR needs reply (== error string) in static data
>  	if (READ_ONCE(req->status) == REQ_STATUS_RCVD &&
>  	    unlikely(req->rc.sdata[4] == P9_RERROR))
>