Re: [PATCH v2] xfs: up(ic_sema) if flushing data device fails

[Leah Rumancik <leah.rumancik@gmail.com>]

On Thu, Oct 26, 2023 at 11:41:03AM +1100, Dave Chinner wrote:
> Hmmmm. So we'll leave an unreferenced, unlocked iclog in a SYNCING
> state where something else can access it, then hope that the
> shutdown gets to it first and that it cleans it all up? That doesn't
> seem completely safe to me.
> 
> At entry to this function, if the log is already shut down, it runs
> xlog_state_done_syncing() to force the iclog and it's attached state
> to be cleaned up before dropping the lock and returning.
> 
> If I look at xlog_ioend_work(), if it gets an error it does the
> shutdown, then calls xlog_state_done_syncing() to clean up the iclog
> and attached state, then drops the lock.
> 
> Hence it appears to me that the error handling for a fatal errors in
> IO submission should match the io completion error handling. i.e the
> error stack for this function should look like this:
> 
> ....
> 	if (xlog_is_shutdown(log))
> 		goto out_done_sync;
> ....
> 	if (error)
> 		goto out_do_shutdown;
> ....
> out_do_shutdown:
> 	xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
> out_done_sync:
> 	xlog_state_done_syncing(iclog);
> 	up(&iclog->ic_sema);
> }
> 
> Thoughts?
Sure, that makes sense to me and will make things more consistent. I'll
send out a new version. Thanks for the review!

- Leah

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com