Re: [PATCH v3 3/8] fs: aio: Use acquire/release for ring->tail publication

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message claims to replace `WRITE_ONCE()` and `READ_ONCE()` which do not exist in the baseline code.
--

commit 8b8ddb17380f4323933c3731f5ced979ef864fa3
Author: Jinjie Ruan <ruanjinjie@huawei.com>

fs: aio: Use acquire/release for ring->tail publication

This commit replaces the explicit write and read memory barriers for
ring->tail with smp_store_release() and smp_load_acquire() in the aio
completion and read event paths. It aims to clarify the publish/subscribe
pattern and optimize for architectures with native acquire/release
instructions.

> Replace the smp_wmb() + WRITE_ONCE(ring->tail) and READ_ONCE(ring->tail)
> + smp_rmb() barrier pair with smp_store_release()/smp_load_acquire()
> on `ring->tail`.

[Severity: Medium]
Does this commit message accurately reflect the previous state of the code?

The message explicitly claims to replace WRITE_ONCE(ring->tail) and
READ_ONCE(ring->tail). However, the baseline code only used plain assignments
as seen in the diff below.

> diff --git a/fs/aio.c b/fs/aio.c
> index ebdb0e5b95fd9..a6ec7c51ac456 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1206,13 +1206,12 @@ static void aio_complete(struct aio_kiocb *iocb)
>  	/* after flagging the request as done, we
>  	 * must never even look at it again
>  	 */
> -	smp_wmb();	/* make event visible before updating tail */
> -
>  	ctx->tail = tail;
>  
>  	ring = folio_address(ctx->ring_folios[0]);
>  	head = ring->head;
> -	ring->tail = tail;
> +	/* Make event visible before updating tail */
> +	smp_store_release(&ring->tail, tail);
>  	flush_dcache_folio(ctx->ring_folios[0]);

[Severity: Medium]
Could the commit message be updated to clarify that this patch also fixes the
missing WRITE_ONCE() annotation?

The baseline code here in fs/aio.c:aio_complete() performed a plain write
with "ring->tail = tail;" rather than using WRITE_ONCE() as claimed.

[ ... ]

> @@ -1288,13 +1287,12 @@ static long aio_read_events_ring(struct kioctx *ctx,
>  	/* Access to ->ring_folios here is protected by ctx->ring_lock. */
>  	ring = folio_address(ctx->ring_folios[0]);
>  	head = ring->head;
> -	tail = ring->tail;
> -
>  	/*
>  	 * Ensure that once we've read the current tail pointer, that
>  	 * we also see the events that were stored up to the tail.
> +	 * Pairs with smp_store_release() in aio_complete().
>  	 */
> -	smp_rmb();
> +	tail = smp_load_acquire(&ring->tail);

[Severity: Medium]
Similarly, could the commit message note that this fixes a missing
READ_ONCE() annotation?

The baseline code here in fs/aio.c:aio_read_events_ring() used a plain read
with "tail = ring->tail;" instead of the claimed READ_ONCE().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902074805.398540-1-ruanjinjie@huawei.com?part=3