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

[Jan Kara]

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