On Fri, 2024-08-16 at 16:35 +0200, Christian Brauner 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;
> }
> +EXPORT_SYMBOL(inode_dio_finished);
>
> /**
> * inode_dio_wait - wait for outstanding DIO requests to finish
> @@ -2490,11 +2484,16 @@ static void __inode_dio_wait(struct inode
> *inode)
> */
> void inode_dio_wait(struct inode *inode)
> {
> - if (atomic_read(&inode->i_dio_count))
> - __inode_dio_wait(inode);
> + wait_var_event(&inode->i_dio_count, inode_dio_finished);
> }
> EXPORT_SYMBOL(inode_dio_wait);
>
> +void inode_dio_wait_interruptible(struct inode *inode)
> +{
> + wait_var_event_interruptible(&inode->i_dio_count,
> inode_dio_finished);
> +}
> +EXPORT_SYMBOL(inode_dio_wait_interruptible);
> +
> /*
> * inode_set_flags - atomically set some inode flags
> *
> diff --git a/fs/netfs/locking.c b/fs/netfs/locking.c
> index 75dc52a49b3a..c2cfdda85230 100644
> --- a/fs/netfs/locking.c
> +++ b/fs/netfs/locking.c
> @@ -21,23 +21,11 @@
> */
> static int inode_dio_wait_interruptible(struct inode *inode)
> {
> - if (!atomic_read(&inode->i_dio_count))
> + if (inode_dio_finished(inode))
> return 0;
>
> - wait_queue_head_t *wq = bit_waitqueue(&inode->i_state,
> __I_DIO_WAKEUP);
> - DEFINE_WAIT_BIT(q, &inode->i_state, __I_DIO_WAKEUP);
> -
> - for (;;) {
> - prepare_to_wait(wq, &q.wq_entry,
> TASK_INTERRUPTIBLE);
> - if (!atomic_read(&inode->i_dio_count))
> - break;
> - if (signal_pending(current))
> - break;
> - schedule();
> - }
> - finish_wait(wq, &q.wq_entry);
> -
> - return atomic_read(&inode->i_dio_count) ? -ERESTARTSYS : 0;
> + inode_dio_wait_interruptible(inode);
> + return !inode_dio_finished(inode) ? -ERESTARTSYS : 0;
> }
>
> /* Call with exclusively locked inode->i_rwsem */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b6f2e2a1e513..f744cd918259 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2380,8 +2380,6 @@ static inline void kiocb_clone(struct kiocb
> *kiocb, struct kiocb *kiocb_src,
> *
> * I_REFERENCED Marks the inode as recently
> references on the LRU list.
> *
> - * I_DIO_WAKEUP Never set. Only used as a key for
> wait_on_bit().
> - *
> * I_WB_SWITCH Cgroup bdi_writeback switching in progress.
> Used to
> * synchronize competing switching instances
> and to tell
> * wb stat updates to grab the i_pages lock.
> See
> @@ -2413,8 +2411,6 @@ static inline void kiocb_clone(struct kiocb
> *kiocb, struct kiocb *kiocb_src,
> #define __I_SYNC 7
> #define I_SYNC (1 << __I_SYNC)
> #define I_REFERENCED (1 << 8)
> -#define __I_DIO_WAKEUP 9
> -#define I_DIO_WAKEUP (1 << __I_DIO_WAKEUP)
> #define I_LINKABLE (1 << 10)
> #define I_DIRTY_TIME (1 << 11)
> #define I_WB_SWITCH (1 << 13)
> @@ -3230,6 +3226,7 @@ static inline ssize_t blockdev_direct_IO(struct
> kiocb *iocb,
> #endif
>
> void inode_dio_wait(struct inode *inode);
> +void inode_dio_wait_interruptible(struct inode *inode);
>
> /**
> * inode_dio_begin - signal start of a direct I/O requests
> @@ -3241,6 +3238,7 @@ void inode_dio_wait(struct inode *inode);
> static inline void inode_dio_begin(struct inode *inode)
> {
> atomic_inc(&inode->i_dio_count);
> + smp_mb__after_atomic();
> }
>
> /**
> @@ -3252,8 +3250,9 @@ static inline void inode_dio_begin(struct inode
> *inode)
> */
> static inline void inode_dio_end(struct inode *inode)
> {
> + smp_mb__before_atomic();
> if (atomic_dec_and_test(&inode->i_dio_count))
> - wake_up_bit(&inode->i_state, __I_DIO_WAKEUP);
> + wake_up_var(&inode->i_dio_count);
> }
>
> extern void inode_set_flags(struct inode *inode, unsigned int flags,
>
> ---
> base-commit: 5570f04d0bb1a34ebcb27caac76c797a7c9e36c9
> change-id: 20240816-vfs-misc-dio-5cb07eaae155
>
Nice cleanup!
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
