| Message ID | 20220520023216.3065073-1-yi.zhang@huawei.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [v2] ext4: fix warning when submitting superblock in ext4_commit_super() | expand |
On 22/05/20 10:32AM, Zhang Yi wrote: > We have already check the io_error and uptodate flag before submitting > the superblock buffer, and re-set the uptodate flag if it has been > failed to write out. But it was lockless and could be raced by another > ext4_commit_super(), and finally trigger '!uptodate' WARNING when > marking buffer dirty. Fix it by submit buffer directly. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > --- > v2->v1: > - Add clear_buffer_dirty() before submit_bh(). Thanks for the patch. Reviewed lock_buffer()/unlock_buffer(), get_bh/put_bh() and other possible error paths, looks good to me. Feel free to add - Reviewed-by: Ritesh Harjani <ritesh.list@gmail.com> -ritesh > > fs/ext4/super.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 1466fbdbc8e3..9f6892665be4 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -6002,7 +6002,6 @@ static void ext4_update_super(struct super_block *sb) > static int ext4_commit_super(struct super_block *sb) > { > struct buffer_head *sbh = EXT4_SB(sb)->s_sbh; > - int error = 0; > > if (!sbh) > return -EINVAL; > @@ -6011,6 +6010,13 @@ static int ext4_commit_super(struct super_block *sb) > > ext4_update_super(sb); > > + lock_buffer(sbh); > + /* Buffer got discarded which means block device got invalidated */ > + if (!buffer_mapped(sbh)) { > + unlock_buffer(sbh); > + return -EIO; > + } > + > if (buffer_write_io_error(sbh) || !buffer_uptodate(sbh)) { > /* > * Oh, dear. A previous attempt to write the > @@ -6025,17 +6031,21 @@ static int ext4_commit_super(struct super_block *sb) > clear_buffer_write_io_error(sbh); > set_buffer_uptodate(sbh); > } > - BUFFER_TRACE(sbh, "marking dirty"); > - mark_buffer_dirty(sbh); > - error = __sync_dirty_buffer(sbh, > - REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0)); > + get_bh(sbh); > + /* Clear potential dirty bit if it was journalled update */ > + clear_buffer_dirty(sbh); > + sbh->b_end_io = end_buffer_write_sync; > + submit_bh(REQ_OP_WRITE, > + REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0), sbh); > + wait_on_buffer(sbh); > if (buffer_write_io_error(sbh)) { > ext4_msg(sb, KERN_ERR, "I/O error while writing " > "superblock"); > clear_buffer_write_io_error(sbh); > set_buffer_uptodate(sbh); > + return -EIO; > } > - return error; > + return 0; > } > > /* > -- > 2.31.1 >
On Fri 20-05-22 10:32:16, Zhang Yi wrote: > We have already check the io_error and uptodate flag before submitting > the superblock buffer, and re-set the uptodate flag if it has been > failed to write out. But it was lockless and could be raced by another > ext4_commit_super(), and finally trigger '!uptodate' WARNING when > marking buffer dirty. Fix it by submit buffer directly. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > v2->v1: > - Add clear_buffer_dirty() before submit_bh(). > > fs/ext4/super.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 1466fbdbc8e3..9f6892665be4 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -6002,7 +6002,6 @@ static void ext4_update_super(struct super_block *sb) > static int ext4_commit_super(struct super_block *sb) > { > struct buffer_head *sbh = EXT4_SB(sb)->s_sbh; > - int error = 0; > > if (!sbh) > return -EINVAL; > @@ -6011,6 +6010,13 @@ static int ext4_commit_super(struct super_block *sb) > > ext4_update_super(sb); > > + lock_buffer(sbh); > + /* Buffer got discarded which means block device got invalidated */ > + if (!buffer_mapped(sbh)) { > + unlock_buffer(sbh); > + return -EIO; > + } > + > if (buffer_write_io_error(sbh) || !buffer_uptodate(sbh)) { > /* > * Oh, dear. A previous attempt to write the > @@ -6025,17 +6031,21 @@ static int ext4_commit_super(struct super_block *sb) > clear_buffer_write_io_error(sbh); > set_buffer_uptodate(sbh); > } > - BUFFER_TRACE(sbh, "marking dirty"); > - mark_buffer_dirty(sbh); > - error = __sync_dirty_buffer(sbh, > - REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0)); > + get_bh(sbh); > + /* Clear potential dirty bit if it was journalled update */ > + clear_buffer_dirty(sbh); > + sbh->b_end_io = end_buffer_write_sync; > + submit_bh(REQ_OP_WRITE, > + REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0), sbh); > + wait_on_buffer(sbh); > if (buffer_write_io_error(sbh)) { > ext4_msg(sb, KERN_ERR, "I/O error while writing " > "superblock"); > clear_buffer_write_io_error(sbh); > set_buffer_uptodate(sbh); > + return -EIO; > } > - return error; > + return 0; > } > > /* > -- > 2.31.1 >
On Fri, 20 May 2022 10:32:16 +0800, Zhang Yi wrote: > We have already check the io_error and uptodate flag before submitting > the superblock buffer, and re-set the uptodate flag if it has been > failed to write out. But it was lockless and could be raced by another > ext4_commit_super(), and finally trigger '!uptodate' WARNING when > marking buffer dirty. Fix it by submit buffer directly. > > > [...] Applied, thanks! [1/1] ext4: fix warning when submitting superblock in ext4_commit_super() commit: 15baa7dcadf1c4f0b4f752dc054191855ff2d78e Best regards,
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 1466fbdbc8e3..9f6892665be4 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -6002,7 +6002,6 @@ static void ext4_update_super(struct super_block *sb) static int ext4_commit_super(struct super_block *sb) { struct buffer_head *sbh = EXT4_SB(sb)->s_sbh; - int error = 0; if (!sbh) return -EINVAL; @@ -6011,6 +6010,13 @@ static int ext4_commit_super(struct super_block *sb) ext4_update_super(sb); + lock_buffer(sbh); + /* Buffer got discarded which means block device got invalidated */ + if (!buffer_mapped(sbh)) { + unlock_buffer(sbh); + return -EIO; + } + if (buffer_write_io_error(sbh) || !buffer_uptodate(sbh)) { /* * Oh, dear. A previous attempt to write the @@ -6025,17 +6031,21 @@ static int ext4_commit_super(struct super_block *sb) clear_buffer_write_io_error(sbh); set_buffer_uptodate(sbh); } - BUFFER_TRACE(sbh, "marking dirty"); - mark_buffer_dirty(sbh); - error = __sync_dirty_buffer(sbh, - REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0)); + get_bh(sbh); + /* Clear potential dirty bit if it was journalled update */ + clear_buffer_dirty(sbh); + sbh->b_end_io = end_buffer_write_sync; + submit_bh(REQ_OP_WRITE, + REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0), sbh); + wait_on_buffer(sbh); if (buffer_write_io_error(sbh)) { ext4_msg(sb, KERN_ERR, "I/O error while writing " "superblock"); clear_buffer_write_io_error(sbh); set_buffer_uptodate(sbh); + return -EIO; } - return error; + return 0; } /*
We have already check the io_error and uptodate flag before submitting the superblock buffer, and re-set the uptodate flag if it has been failed to write out. But it was lockless and could be raced by another ext4_commit_super(), and finally trigger '!uptodate' WARNING when marking buffer dirty. Fix it by submit buffer directly. Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> --- v2->v1: - Add clear_buffer_dirty() before submit_bh(). fs/ext4/super.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)