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

[Jan Kara <jack@suse.cz>]

On Wed 04-03-26 14:38:47, Christian Brauner wrote:
> 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)?

Indeed. It is just a temporary breakage in the series but still. Fixed.
Thanks.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR