On 2020/6/17 21:44, zhangyi (F) wrote: > On 2020/6/17 20:41, Jan Kara wrote: >> On Wed 17-06-20 19:59:45, zhangyi (F) wrote: >>> Although we have already introduce s_bdev_wb_err_work to detect and >>> handle async write metadata buffer error as soon as possible, there is >>> still a potential race that could lead to filesystem inconsistency, >>> which is the buffer may reading and re-writing out to journal before >>> s_bdev_wb_err_work run. So this patch detect bdev mapping->wb_err when >>> getting journal's write access and also mark the filesystem error if >>> something bad happened. >>> >>> Signed-off-by: zhangyi (F) <yi.zhang@xxxxxxxxxx> >> >> So instead of all this, cannot we just do: >> >> if (work_pending(sbi->s_bdev_wb_err_work)) >> flush_work(sbi->s_bdev_wb_err_work); >> >> ? And so we are sure the filesystem is aborted if the abort was pending? >> > > Thanks for this suggestion. Yeah, we could do this, it depends on the second > patch, if we check and flush the pending work here, we could not use the > end_buffer_async_write() in ext4_end_buffer_async_write(), we need to open > coding ext4_end_buffer_async_write() and queue the error work before the > buffer is unlocked, or else the race is still there. Do you agree ? > Add one point, add work_pending check here may not safe. We need to make sure the filesystem is aborted, so we need to wait the error handle work is finished, but the work's pending bit is cleared before it start running. I think may better to just invoke flush_work() here. BTW, I also notice another race condition that may lead to inconsistency. In bdev_try_to_free_page(), if we free a write error buffer before the worker is finished, the jbd2 checkpoint procedure will miss this error and wrongly think it has already been written to disk successfully, and finally it will destroy the log and lead to inconsistency (the same to no-journal mode). So I think the ninth patch in my v1 patch set is still needed. What do you think? Thanks, Yi.
