On Fri, Aug 16, 2024 at 05:27:05PM +0100, David Howells wrote:
> Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> > Afaict, we can just rely on inode->i_dio_count for waiting instead of
> > this awkward indirection through __I_DIO_WAKEUP. This survives LTP dio
> > and xfstests dio tests.
> >
> > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> > ---
> > ---
> > fs/inode.c | 23 +++++++++++------------
> > fs/netfs/locking.c | 18 +++---------------
> > include/linux/fs.h | 9 ++++-----
> > 3 files changed, 18 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 7a4e27606fca..46bf05d826db 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -2465,18 +2465,12 @@ EXPORT_SYMBOL(inode_owner_or_capable);
> > /*
> > * Direct i/o helper functions
> > */
> > -static void __inode_dio_wait(struct inode *inode)
> > +bool inode_dio_finished(const struct inode *inode)
> > {
> > - wait_queue_head_t *wq = bit_waitqueue(&inode->i_state, __I_DIO_WAKEUP);
> > - DEFINE_WAIT_BIT(q, &inode->i_state, __I_DIO_WAKEUP);
> > -
> > - do {
> > - prepare_to_wait(wq, &q.wq_entry, TASK_UNINTERRUPTIBLE);
> > - if (atomic_read(&inode->i_dio_count))
> > - schedule();
> > - } while (atomic_read(&inode->i_dio_count));
> > - finish_wait(wq, &q.wq_entry);
> > + smp_mb__before_atomic();
> > + return atomic_read(&inode->i_dio_count) == 0;
>
> Can atomic_read_aquire() be used here?
>
I think the key point is that smp_mb__before_atomic is not usable with
atomic_read and atomic_set -- these merely expand to regular movs, not
providing a full fence (in contrast to lock-prefixed instructions on
amd64).
As for what ordering is needed here (and in particular would acquire do
the job), I don't know yet.
> > static inline void inode_dio_end(struct inode *inode)
> > {
> > + smp_mb__before_atomic();
> > if (atomic_dec_and_test(&inode->i_dio_count))
>
> Doesn't atomic_dec_and_test() imply full barriering? See atomic_t.txt,
> "ORDERING":
>
> - RMW operations that have a return value are fully ordered;
>
smp_mb__before_atomic is indeed redundant here.
atomic_dec_and_test and its variants are used for refcounting relying on
the implicitly provided ordering (e.g., see fput)
