Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 20 May 2022 09:54:05 +0530
From:   Ritesh Harjani <ritesh.list@gmail.com>
To:     Zhang Yi <yi.zhang@huawei.com>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, jack@suse.cz, yukuai3@huawei.com
Subject: Re: [PATCH v2] ext4: fix warning when submitting superblock in
 ext4_commit_super()
Message-ID: <20220520042405.xncu55ceekgrzhyu@riteshh-domain>
References: <20220520023216.3065073-1-yi.zhang@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220520023216.3065073-1-yi.zhang@huawei.com>
Precedence: bulk

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
>