On Wed, Dec 04, 2024 at 09:27:22AM+0100, Mateusz Guzik wrote:
> On Wed, Dec 04, 2024 at 06:48:18AM +0000, Zilin Guan wrote:
> > Hello,
> >
> > I have a question regarding the use of smp_rmb() to enforce
> > memory ordering in two related functions.
> >
> > In the function netfs_unbuffered_write_iter_locked() from the file
> > fs/netfs/direct_write.c, smp_rmb() is explicitly used after the
> > wait_on_bit() call to ensure that the error and transferred fields are
> > read in the correct order following the NETFS_RREQ_IN_PROGRESS flag:
> >
> > 105 wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS,
> > 106 TASK_UNINTERRUPTIBLE);
> > 107 smp_rmb(); /* Read error/transferred after RIP flag */
> > 108 ret = wreq->error;
> > 109 if (ret == 0) {
> > 110 ret = wreq->transferred;
> > 111 iocb->ki_pos += ret;
> > 112 }
> >
> > However, in the function netfs_end_writethrough() from the file
> > fs/netfs/write_issue.c, there is no such use of smp_rmb() after
> > the corresponding wait_on_bit() call, despite accessing the same filed
> > of wreq->error and relying on the same NETFS_RREQ_IN_PROGRESS flag:
> >
> > 681 wait_on_bit(&wreq->flags, NETFS_RREQ_IN_PROGRESS,
> > TASK_UNINTERRUPTIBLE);
> > 682 ret = wreq->error;
> >
> > My question is why does the first function require a CPU memory barrier
> > smp_rmb() to enforce ordering, whereas the second function does not?
>
> The fence is redundant.
>
> Per the comment in wait_on_bit:
> * Returned value will be zero if the bit was cleared in which case the
> * call has ACQUIRE semantics, or %-EINTR if the process received a
> * signal and the mode permitted wake up on that signal.
>
> Since both sites pass TASK_UNINTERRUPTIBLE this will only ever return
> after the bit is sorted out, already providing the needed fence.
Since the code does not need the fence, should I send a patch to
remove it? Commit 2df8654 introduced this fence during the transition
to a new writeback implementation. However, the author added this fence
as part of the changes and did not intend to address a specific CPU
reordering issue.
