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

Huang, Ying
2023-04-23T08:49:14Z

Douglas Anderson <dianders@chromium.org> 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@chromium.org>
> ---
>
> 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);