Re: [PATCH 31/41] fs: Provide functions for handling mapping_metadata_bhs directly

[Christoph Hellwig <hch@infradead.org>]

On Fri, Mar 20, 2026 at 02:41:26PM +0100, Jan Kara wrote:
> As part of transition toward moving mapping_metadata_bhs to fs-private
> part of the inode, provide functions for operations on this list
> directly instead of going through the inode / mapping.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/buffer.c                 | 93 +++++++++++++++++--------------------
>  include/linux/buffer_head.h | 45 ++++++++++++++----
>  2 files changed, 80 insertions(+), 58 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index c70f8027bdd1..43aca5b7969f 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -467,31 +467,25 @@ EXPORT_SYMBOL(mark_buffer_async_write);
>   * a successful fsync().  For example, ext2 indirect blocks need to be
>   * written back and waited upon before fsync() returns.
>   *
> - * The functions mark_buffer_dirty_inode(), fsync_inode_buffers(),
> - * mmb_has_buffers() and invalidate_inode_buffers() are provided for the
> - * management of a list of dependent buffers in mapping_metadata_bhs struct.
> + * The functions mmb_mark_buffer_dirty(), mmb_sync_buffers(), mmb_has_buffers()
> + * and mmb_invalidate_buffers() are provided for the management of a list of
> + * dependent buffers in mapping_metadata_bhs struct.
>   *
>   * The locking is a little subtle: The list of buffer heads is protected by
>   * the lock in mapping_metadata_bhs so functions coming from bdev mapping
>   * (such as try_to_free_buffers()) need to safely get to mapping_metadata_bhs
>   * using RCU, grab the lock, verify we didn't race with somebody detaching the
>   * bh / moving it to different inode and only then proceeding.
> - *
> - * FIXME: mark_buffer_dirty_inode() is a data-plane operation.  It should
> - * take an address_space, not an inode.  And it should be called
> - * mark_buffer_dirty_fsync() to clearly define why those buffers are being
> - * queued up.
> - *
> - * FIXME: mark_buffer_dirty_inode() doesn't need to add the buffer to the
> - * list if it is already on a list.  Because if the buffer is on a list,
> - * it *must* already be on the right one.  If not, the filesystem is being
> - * silly.  This will save a ton of locking.  But first we have to ensure
> - * that buffers are taken *off* the old inode's list when they are freed
> - * (presumably in truncate).  That requires careful auditing of all
> - * filesystems (do it inside bforget()).  It could also be done by bringing
> - * b_inode back.
>   */
>  
> +void mmb_init(struct mapping_metadata_bhs *mmb, struct address_space *mapping)
> +{
> +	spin_lock_init(&mmb->lock);
> +	INIT_LIST_HEAD(&mmb->list);
> +	mmb->mapping = mapping;
> +}
> +EXPORT_SYMBOL(mmb_init);
> +
>  static void __remove_assoc_queue(struct mapping_metadata_bhs *mmb,
>  			         struct buffer_head *bh)
>  {
> @@ -533,12 +527,12 @@ bool mmb_has_buffers(struct mapping_metadata_bhs *mmb)
>  EXPORT_SYMBOL_GPL(mmb_has_buffers);
>  
>  /**
> - * sync_mapping_buffers - write out & wait upon a mapping's "associated" buffers
> - * @mapping: the mapping which wants those buffers written
> + * mmb_sync_buffers - write out & wait upon all buffers in a list
> + * @mmb: the list of buffers to write
>   *
> - * Starts I/O against the buffers at mapping->i_metadata_bhs and waits upon
> - * 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
> + * Starts I/O against the buffers in the given list and waits upon
> + * that I/O. Basically, this is a convenience function for fsync().  @mmb is
> + * for 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
> @@ -553,9 +547,8 @@ EXPORT_SYMBOL_GPL(mmb_has_buffers);
>   * buffer stays on our list until IO completes (at which point it can be
>   * reaped).
>   */
> -int sync_mapping_buffers(struct address_space *mapping)
> +int mmb_sync_buffers(struct mapping_metadata_bhs *mmb)

mmb and buffers in the same name feels a bit redundant.

mmc_sync_all?  mapping_sync_buffers?

> +int generic_mmb_fsync_noflush(struct file *file,
> +			      struct mapping_metadata_bhs *mmb,
> +			      loff_t start, loff_t end, bool datasync)

mmb_fsync?  mapping_buffers_fsync?

> +int generic_mmb_fsync(struct file *file, struct mapping_metadata_bhs *mmb,
> +		      loff_t start, loff_t end, bool datasync)
>  {
>  	struct inode *inode = file->f_mapping->host;
>  	int ret;
>  
> -	ret = generic_buffers_fsync_noflush(file, start, end, datasync);
> +	ret = generic_mmb_fsync_noflush(file, mmb, start, end, datasync);
>  	if (!ret)
>  		ret = blkdev_issue_flush(inode->i_sb->s_bdev);
>  	return ret;
>  }
> -EXPORT_SYMBOL(generic_buffers_fsync);
> +EXPORT_SYMBOL(generic_mmb_fsync);

Same naming, but do we even need this function?  One the
mapping_metadata_bhs has to be passed in, the file system needs a
wrapper anyway, at which point open coding the flush is not really
much of a burden.