Re: [PATCH] inode: remove __I_DIO_WAKEUP

[Jeff Layton <jlayton@kernel.org>]

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@kernel.org>
> ---
> ---
>  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@kernel.org>