Re: [PATCH v2 2/4] buffer: Add lock_buffer_timeout()

[Huang, Ying]

Douglas Anderson <dianders@xxxxxxxxxxxx> writes:

> Add a variant of lock_buffer() that can timeout. This is useful to
> avoid unbounded waits for the page lock in kcompactd.
>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
>
> Changes in v2:
> - "Add lock_buffer_timeout()" new for v2.
>
>  fs/buffer.c                 |  7 +++++++
>  include/linux/buffer_head.h | 10 ++++++++++
>  include/linux/wait_bit.h    | 24 ++++++++++++++++++++++++
>  kernel/sched/wait_bit.c     | 14 ++++++++++++++
>  4 files changed, 55 insertions(+)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 9e1e2add541e..fcd19c270024 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -71,6 +71,13 @@ void __lock_buffer(struct buffer_head *bh)
>  }
>  EXPORT_SYMBOL(__lock_buffer);
>  
> +int __lock_buffer_timeout(struct buffer_head *bh, unsigned long timeout)
> +{
> +	return wait_on_bit_lock_io_timeout(&bh->b_state, BH_Lock,
> +					   TASK_UNINTERRUPTIBLE, timeout);
> +}
> +EXPORT_SYMBOL(__lock_buffer_timeout);
> +
>  void unlock_buffer(struct buffer_head *bh)
>  {
>  	clear_bit_unlock(BH_Lock, &bh->b_state);
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index 8f14dca5fed7..2bae464f89d5 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -237,6 +237,7 @@ struct buffer_head *alloc_buffer_head(gfp_t gfp_flags);
>  void free_buffer_head(struct buffer_head * bh);
>  void unlock_buffer(struct buffer_head *bh);
>  void __lock_buffer(struct buffer_head *bh);
> +int __lock_buffer_timeout(struct buffer_head *bh, unsigned long timeout);
>  int sync_dirty_buffer(struct buffer_head *bh);
>  int __sync_dirty_buffer(struct buffer_head *bh, blk_opf_t op_flags);
>  void write_dirty_buffer(struct buffer_head *bh, blk_opf_t op_flags);
> @@ -400,6 +401,15 @@ static inline void lock_buffer(struct buffer_head *bh)
>  		__lock_buffer(bh);
>  }
>  
> +static inline int lock_buffer_timeout(struct buffer_head *bh,
> +				      unsigned long timeout)
> +{
> +	might_sleep();
> +	if (!trylock_buffer(bh))
> +		return __lock_buffer_timeout(bh, timeout);
> +	return 0;
> +}
> +

Add document about return value of lock_buffer_timeout()?

Otherwise looks good to me.

Best Regards,
Huang, Ying

>  static inline struct buffer_head *getblk_unmovable(struct block_device *bdev,
>  						   sector_t block,
>  						   unsigned size)
> diff --git a/include/linux/wait_bit.h b/include/linux/wait_bit.h
> index 7725b7579b78..33f0f60b1c8c 100644
> --- a/include/linux/wait_bit.h
> +++ b/include/linux/wait_bit.h
> @@ -30,6 +30,7 @@ void wake_up_bit(void *word, int bit);
>  int out_of_line_wait_on_bit(void *word, int, wait_bit_action_f *action, unsigned int mode);
>  int out_of_line_wait_on_bit_timeout(void *word, int, wait_bit_action_f *action, unsigned int mode, unsigned long timeout);
>  int out_of_line_wait_on_bit_lock(void *word, int, wait_bit_action_f *action, unsigned int mode);
> +int out_of_line_wait_on_bit_lock_timeout(void *word, int, wait_bit_action_f *action, unsigned int mode, unsigned long timeout);
>  struct wait_queue_head *bit_waitqueue(void *word, int bit);
>  extern void __init wait_bit_init(void);
>  
> @@ -208,6 +209,29 @@ wait_on_bit_lock_io(unsigned long *word, int bit, unsigned mode)
>  	return out_of_line_wait_on_bit_lock(word, bit, bit_wait_io, mode);
>  }
>  
> +/**
> + * wait_on_bit_lock_io_timeout - wait_on_bit_lock_io() with a timeout
> + * @word: the word being waited on, a kernel virtual address
> + * @bit: the bit of the word being waited on
> + * @mode: the task state to sleep in
> + * @timeout: the timeout in jiffies; %MAX_SCHEDULE_TIMEOUT means wait forever
> + *
> + * Returns zero if the bit was (eventually) found to be clear and was
> + * set.  Returns non-zero if a timeout happened or a signal was delivered to
> + * the process and the @mode allows that signal to wake the process.
> + */
> +static inline int
> +wait_on_bit_lock_io_timeout(unsigned long *word, int bit, unsigned mode,
> +			    unsigned long timeout)
> +{
> +	might_sleep();
> +	if (!test_and_set_bit(bit, word))
> +		return 0;
> +	return out_of_line_wait_on_bit_lock_timeout(word, bit,
> +						    bit_wait_io_timeout,
> +						    mode, timeout);
> +}
> +
>  /**
>   * wait_on_bit_lock_action - wait for a bit to be cleared, when wanting to set it
>   * @word: the word being waited on, a kernel virtual address
> diff --git a/kernel/sched/wait_bit.c b/kernel/sched/wait_bit.c
> index 0b1cd985dc27..629acd1c6c79 100644
> --- a/kernel/sched/wait_bit.c
> +++ b/kernel/sched/wait_bit.c
> @@ -118,6 +118,20 @@ int __sched out_of_line_wait_on_bit_lock(void *word, int bit,
>  }
>  EXPORT_SYMBOL(out_of_line_wait_on_bit_lock);
>  
> +int __sched out_of_line_wait_on_bit_lock_timeout(void *word, int bit,
> +						 wait_bit_action_f *action,
> +						 unsigned mode,
> +						 unsigned long timeout)
> +{
> +	struct wait_queue_head *wq_head = bit_waitqueue(word, bit);
> +	DEFINE_WAIT_BIT(wq_entry, word, bit);
> +
> +	wq_entry.key.timeout = jiffies + timeout;
> +
> +	return __wait_on_bit_lock(wq_head, &wq_entry, action, mode);
> +}
> +EXPORT_SYMBOL(out_of_line_wait_on_bit_lock_timeout);
> +
>  void __wake_up_bit(struct wait_queue_head *wq_head, void *word, int bit)
>  {
>  	struct wait_bit_key key = __WAIT_BIT_KEY_INITIALIZER(word, bit);