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@xxxxxxxxxxxxx
