On 3/31/23 17:06, James Bottomley wrote: > On Fri, 2023-03-31 at 16:11 +0200, Tomas Henzl wrote: >> On 3/31/23 13:48, James Bottomley wrote: >>> On Thu, 2023-03-30 at 12:12 -0500, Mike Christie wrote: > [...] >>>> Are we going to do: >>>> 1. __scsi_remove_device sets the state to SDEV_CANCEL at the >>>> beginning of the function >>> >>> It will also interfere with target and host device removal. They >>> traverse their own lists and assume that anything in DEL is already >>> being removed, which won't be the case here. So basically, after >>> this happens it's impossible to clean the device trees. It also >>> means any I/O to the root device wouldn't be allowed. >> >> How will it interfere? After a return from sd_remove or via >> device_unregister->__scsi_remove_device the device state is SDEV_DEL >> regardless whether this patch has been added or not. Or is >> sd_shutdown called directly? > > I thought it was called directly from the restart logic via the > device_shutdown() call (see kernel/reboot.c:kernel_restart_prepare()) > and was completely independent of any other state transitions within > the device model ... however, I'd really like *you* to confirm this. > > I think by the time it's called, we're already in the system death > throes, so if there were going to be an orderly shut down it's already > happened, but if not, once you set a devices state to DEL, it will > defeat any later attempt to do orderly remove of the host or target. After the first device's shutdown method in device_shutdown has been called there is now way back (we don't have an opposite to shutdown like for suspend resume is) so it can't be undone and also it doesn't matter if some structures remain in memory. To me it seems to be the only logical consequence but I can't prove it. >>> I assume the contention is that if we get here, we're either going >>> for immediate shutdown or all the root device remounting to read >>> only has already been done? If so, could you say that? >> >> I can't say that, quite the opposite (see body of the mail). When the >> system goes shutdown the individual device's .shutdown is called. >> Just moments after sd shutdown the LLD shutdown is entered and the >> driver stops any I/O immediately anyway. With this patch the I/O is >> stopped before reaching LLD with a reasonable message and without >> error correction mechanism in place. >> >> I also assume that no I/O after sd_shutdown was projected when it was >> written as there is a cache sync followed by a device power down. > > It seems reasonable, but can you validate that? Shutdown is called > both from reboot and kexec and if we stop IO to a quiescing root device > before it completes, so that all filesystems come back dirty, we'll > have a lot of unhappy users ... Validate, I don't know how or what you mean. Making sure that everything is in a state when the kernel shutdown procedure may be entered is for example on my system done by userspace systemd-shutdown service like so :"systemd-shutdown[1]: Syncing filesystems and block devices ..." When the kernel shutdown procedure is entered the I/O will be cut off abruptly by a LLD.shutdown. That means it depends on scripts like systemd and that works well on all systems I checked for reboot but not for kexec on everywhere. The patch may stop the I/O few msec sooner that is true. But the clearer messages may bring a script writer to fix it. The patch doesn't fix a real bug so it isn't urgent nor important, seeing the congestion it creates please just drop it. Tomas > > James >
