Re: [PATCH v2] fs/buffer: serialize set_buffer_uptodate against concurrent clears

[Chris S]

Thank you! I'll update asap.

Best,
Chao

On Tue, Jul 28, 2026 at 3:45 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Sun 19-07-26 02:28:54, Chao S wrote:
> > On Thu 30-04-26 09:28:13, Jan Kara wrote:
> > > But it would be actually very welcome if you took the work, went through
> > > all the places using end_buffer_write_sync() and converted them in
> > > filesystem-by-filesystem to check for IO error by checking
> > > buffer_write_io_error() instead of buffer_uptodate() and then we can drop
> > > clear_buffer_uptodate() call from end_buffer_write_sync().
> >
> > Sorry for the long delay, I've been working on some patches and a paper
> > deadline.  Also, you were right about v2, and I have dropped it.  I
> > would like to
> > take the conversion work.
> >
> > Before writing it, I found your "fs: Fix missed inode write during fsync"
> > series, v4 of 16 July.  It removes four of the sites I had on my list, so
> > I will base this on top of it once it lands.  Say if you would rather
> > sequence it differently.
>
> Yes, please base your changes on top of vfs tree
> (https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all). My
> patches are already in there now.
>
> > (end_buffer_write_sync() is now bh_end_write(), per b20f15420f78.  Current
> > names below; line numbers against v7.2-rc1.)
> >
> > The sites, one patch each:
> >
> >   core     fs/buffer.c:621 (mmb_sync), :2803 (__sync_dirty_buffer)
> >   adfs     fs/adfs/dir.c:194
> >   ext2     fs/ext2/xattr.c:772
> >   ext4     fs/ext4/ext4_jbd2.c:399, fs/ext4/mmp.c:52
> >   omfs     fs/omfs/inode.c:148, :162
> >   exfat    fs/exfat/misc.c:190
> >   fat      fs/fat/misc.c:358
> >   ocfs2    fs/ocfs2/buffer_head_io.c:69, :449, fs/ocfs2/journal.c:675, :691
> >   gfs2     fs/gfs2/log.c:110, :324
> >   jbd2     fs/jbd2/commit.c:880
> >   removal  fs/buffer.c:210 and :444
> >
> > Conversions first, removal last.  The list is short because
> > __sync_dirty_buffer() and mmb_sync() test internally and return -EIO, so
> > the ~40 callers that use the return value need no edit of their own.
>
> Sounds good.
>
> > I would like your suggestions on the following.
> >
> > 1. BH_Write_EIO is sticky.  It is cleared only on rewrite
> > (fs/buffer.c:1196), and only if BH_Req was already set, which
> > BUFFER_FLAGS_DISCARD clears on its own.  So a converted test can see a
> > stale error.  I would fix this first, either by making the clear at :1196
> > unconditional for REQ_OP_WRITE, or by adding BH_Write_EIO to
> > BUFFER_FLAGS_DISCARD.  Which do you prefer?
>
> I think we should add it to BUFFER_FLAGS_DISCARD. It doesn't make sense to
> keep it on a discarded buffer.
>
> > 2. jbd2: log_bufs is a mixed list.  Commit descriptors come from
> > journal_end_buffer_io_sync(), which never sets BH_Write_EIO; revoke
> > descriptors from write_dirty_buffer(), so bh_end_write().  I would convert
> > fs/jbd2/commit.c:880 to the disjunct !buffer_uptodate(bh) ||
> > buffer_write_io_error(bh).  The alternative is to add
> > mark_buffer_write_io_error() to journal_end_buffer_io_sync() and convert
> > all three jbd2 sites.  Which do you prefer?
>
> Please convert journal_end_buffer_io_sync() to use
> mark_buffer_write_io_error() so that we completely get rid of this
> antipattern.
>
> > 3. bh_end_async_write() has the same clear at fs/buffer.c:444.  I would
> > remove both, which needs the gfs2 patch first since gfs2 reads that bit at
> > fs/gfs2/log.c:110 and :324.  I can also leave the async half out.
>
> Please change gfs2 the same way as you are fixing jbd2. Keeping the mix of
> uptodate & write_io_error checks only makes things more fragile...
>
> > 4. ocfs2: fs/ocfs2/journal.c:691 sits inside an outer
> > if (!buffer_uptodate(bh)) at :675, so dropping the clear stops ocfs2 going
> > read-only after a failed metadata checkpoint.  I would convert both sites
> > and Cc ocfs2-devel.
>
> Yes, please.
>
>                                                                 Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR