Re: [PATCH 16/32] fs: Fold fsync_buffers_list() into sync_mapping_buffers()

[Christian Brauner <brauner@kernel.org>]

On Tue, Mar 03, 2026 at 11:34:05AM +0100, Jan Kara wrote:
> There's only single caller of fsync_buffers_list() so untangle the code
> a bit by folding fsync_buffers_list() into sync_mapping_buffers(). Also
> merge the comments and update them to reflect current state of code.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/buffer.c | 180 +++++++++++++++++++++++-----------------------------
>  1 file changed, 80 insertions(+), 100 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 1c0e7c81a38b..18012afb8289 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -54,7 +54,6 @@
>  
>  #include "internal.h"
>  
> -static int fsync_buffers_list(spinlock_t *lock, struct list_head *list);
>  static void submit_bh_wbc(blk_opf_t opf, struct buffer_head *bh,
>  			  enum rw_hint hint, struct writeback_control *wbc);
>  
> @@ -531,22 +530,96 @@ EXPORT_SYMBOL_GPL(inode_has_buffers);
>   * @mapping: the mapping which wants those buffers written
>   *
>   * Starts I/O against the buffers at mapping->i_private_list, and waits upon
> - * that I/O.
> + * that I/O. Basically, this is a convenience function for fsync().  @mapping
> + * is a file or directory which needs those buffers to be written for a
> + * successful fsync().
>   *
> - * Basically, this is a convenience function for fsync().
> - * @mapping is a file or directory which needs those buffers to be written for
> - * a successful fsync().
> + * We have conflicting pressures: we want to make sure that all
> + * initially dirty buffers get waited on, but that any subsequently
> + * dirtied buffers don't.  After all, we don't want fsync to last
> + * forever if somebody is actively writing to the file.
> + *
> + * Do this in two main stages: first we copy dirty buffers to a
> + * temporary inode list, queueing the writes as we go. Then we clean
> + * up, waiting for those writes to complete. mark_buffer_dirty_inode()
> + * doesn't touch b_assoc_buffers list if b_assoc_map is not NULL so we
> + * are sure the buffer stays on our list until IO completes (at which point
> + * it can be reaped).
>   */
>  int sync_mapping_buffers(struct address_space *mapping)
>  {
>  	struct address_space *buffer_mapping =
>  				mapping->host->i_sb->s_bdev->bd_mapping;
> +	struct buffer_head *bh;
> +	int err = 0;
> +	struct blk_plug plug;
> +	LIST_HEAD(tmp);
>  
>  	if (list_empty(&mapping->i_private_list))
>  		return 0;
>  
> -	return fsync_buffers_list(&buffer_mapping->i_private_lock,
> -					&mapping->i_private_list);
> +	blk_start_plug(&plug);
> +
> +	spin_lock(&buffer_mapping->i_private_lock);
> +	while (!list_empty(&mapping->i_private_list)) {
> +		bh = BH_ENTRY(list->next);


Stray "list" reference? Shouldn't this be
BH_ENTRY(mapping->i_private_list.next)?