linux-kernel - Re: [PATCH v2 2/4] buffer: Add lock_buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87bkjfkmrl.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date:   Sun, 23 Apr 2023 16:47:58 +0800
From:   "Huang, Ying" <ying.huang@...el.com>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Vlastimil Babka <vbabka@...e.cz>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Yu Zhao <yuzhao@...gle.com>, linux-fsdevel@...r.kernel.org,
        Matthew Wilcox <willy@...radead.org>,
        Bart Van Assche <bvanassche@....org>,
        Ben Segall <bsegall@...gle.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Ingo Molnar <mingo@...hat.com>, Jan Kara <jack@...e.cz>,
        Juri Lelli <juri.lelli@...hat.com>,
        Mel Gorman <mgorman@...e.de>,
        Mikulas Patocka <mpatocka@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "Ritesh Harjani (IBM)" <ritesh.list@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Valentin Schneider <vschneid@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Will Deacon <will@...nel.org>, Zhang Yi <yi.zhang@...wei.com>
Subject: Re: [PATCH v2 2/4] buffer: Add lock_buffer_timeout()

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