On Wed 17-06-20 19:59:44, zhangyi (F) wrote:
> There is a risk of filesystem inconsistency if we failed to async write
> back metadata buffer in the background. Because of current buffer's end
> io procedure is handled by end_buffer_async_write() in the block layer,
> and it only clear the buffer's uptodate flag and mark the write_io_error
> flag, so ext4 cannot detect such failure immediately. In most cases of
> getting metadata buffer (e.g. ext4_read_inode_bitmap()), although the
> buffer's data is actually uptodate, it may still read data from disk
> because the buffer's uptodate flag has been cleared. Finally, it may
> lead to on-disk filesystem inconsistency if reading old data from the
> disk successfully and write them out again.
>
> This patch propagate ext4 end buffer callback to the block layer which
> could detect metadata buffer's async error and invoke ext4 error handler
> immediately.
>
> Signed-off-by: zhangyi (F) <yi.zhang@xxxxxxxxxx>
Thanks for the patch. It looks good, just some language fixes below...
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 15b062efcff1..2f22476f41d2 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1515,6 +1515,10 @@ struct ext4_sb_info {
> /* workqueue for reserved extent conversions (buffered io) */
> struct workqueue_struct *rsv_conversion_wq;
>
> + /* workqueue for handle metadata buffer async writeback error */
^^ handling
> + struct workqueue_struct *s_bdev_wb_err_wq;
> + struct work_struct s_bdev_wb_err_work;
> +
> /* timer for periodic error stats printing */
> struct timer_list s_err_report;
>
...
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index de6fe969f773..50aa8e26e38c 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -560,3 +560,50 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
> end_page_writeback(page);
> return ret;
> }
> +
> +/*
> + * Handle error of async writeback metadata buffer, just mark the filesystem
> + * error to prevent potential further inconsistency.
> + */
This comment is probably unnecessary. The comment inside the function is
enough. So I'd just delete this one.
> +void ext4_end_buffer_async_write_error(struct work_struct *work)
> +{
> + struct ext4_sb_info *sbi = container_of(work,
> + struct ext4_sb_info, s_bdev_wb_err_work);
> +
> + /*
> + * If we failed to async write back metadata buffer, there is a risk
> + * of filesystem inconsistency since we may read old metadata from the
> + * disk successfully and write them out again.
> + */
> + ext4_error_err(sbi->s_sb, -EIO, "Error while async write back metadata buffer");
> +}
> +
> +static void ext4_end_buffer_async_write(struct buffer_head *bh, int uptodate)
> +{
> + struct super_block *sb = bh->b_bdev->bd_super;
> +
> + end_buffer_async_write(bh, uptodate);
> +
> + if (!uptodate && sb && !sb_rdonly(sb)) {
> + struct ext4_sb_info *sbi = EXT4_SB(sb);
> +
> + /* Handle error of async writeback metadata buffer */
Instead of this comment, which isn't very useful, I'd add a comment
explaining why we do it like this. So something like:
/*
* This function is called from softirq handler and complete abort of a
* filesystem requires taking sleeping locks and submitting IO. So postpone
* the real work to a workqueue.
*/
> + queue_work(sbi->s_bdev_wb_err_wq, &sbi->s_bdev_wb_err_work);
> + }
> +}
> +
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
