| Message ID | 20230906063321.905103-1-linan666@huaweicloud.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] ata: libata-eh: Honor all EH scheduling requests | expand |
On 9/6/23 15:33, linan666@huaweicloud.com wrote: > From: Li Nan <linan122@huawei.com> > > If a disk is removed and quickly inserted when an I/O error is processing, > the disk may not be able to be re-added. The function call timeline is as > follows: > > interrupt scsi_eh > > ahci_error_intr > ata_port_freeze > __ata_port_freeze > =>ahci_freeze (turn IRQ off) > ata_port_abort > ata_do_link_abort > ata_port_schedule_eh > =>ata_std_sched_eh > ata_eh_set_pending > set EH_PENDING > scsi_schedule_eh > shost->host_eh_scheduled++ (=1) > scsi_error_handler > =>ata_scsi_error > ata_scsi_port_error_handler > clear EH_PENDING > =>ahci_error_handler > . sata_pmp_error_handler > . ata_eh_reset > . ata_eh_thaw_port > . . =>ahci_thaw (turn IRQ on) > ahci_error_intr . . > ata_port_freeze . . > __ata_port_freeze . . > =>ahci_freeze (turn IRQ off) . . > ... . . > ata_eh_set_pending . . > set EH_PENDING . . > scsi_schedule_eh . . > shost->host_eh_scheduled++ (=2) . . > . clear EH_PENDING > check EH_PENDING > =>ata_std_end_eh > host->host_eh_scheduled = 0; > > 'host_eh_scheduled' is 0 and scsi eh thread will not be scheduled again. > The ata port remains frozen and will never be enabled. > > To fix this issue, decrease 'host_eh_scheduled' instead of setting it to 0 > so that EH is scheduled again to re-enable the port. Also move the update > of 'nr_active_links' to 0 when 'host_eh_scheduled' is 0 to > ata_scsi_port_error_handler(). > > Reported-by: luojian <luojian5@huawei.com> > Signed-off-by: Li Nan <linan122@huawei.com> Looks good to me, but I think this needs a Fixes tag, and "Cc: stable". Please add that. > --- > Changes in v3: > - change patch title, previously it was: "scsi: ata: Fix a race condition > between scsi error handler and ahci interrupt". > - drop the variable 'host' in ata_std_end_eh(). > - improve commit message. > > drivers/ata/libata-eh.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > index 159ba6ba19eb..2d5ecd68b7e0 100644 > --- a/drivers/ata/libata-eh.c > +++ b/drivers/ata/libata-eh.c > @@ -735,6 +735,12 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap) > */ > ap->ops->end_eh(ap); > > + if (!ap->scsi_host->host_eh_scheduled) { > + /* make sure nr_active_links is zero after EH */ > + WARN_ON(ap->nr_active_links); > + ap->nr_active_links = 0; > + } > + > spin_unlock_irqrestore(ap->lock, flags); > ata_eh_release(ap); > > @@ -946,9 +952,7 @@ EXPORT_SYMBOL_GPL(ata_std_sched_eh); > */ > void ata_std_end_eh(struct ata_port *ap) > { > - struct Scsi_Host *host = ap->scsi_host; > - > - host->host_eh_scheduled = 0; > + ap->scsi_host->host_eh_scheduled--; > } > EXPORT_SYMBOL(ata_std_end_eh); > > @@ -3922,10 +3926,6 @@ void ata_eh_finish(struct ata_port *ap) > } > } > } > - > - /* make sure nr_active_links is zero after EH */ > - WARN_ON(ap->nr_active_links); > - ap->nr_active_links = 0; > } > > /**
On Wed, Sep 06, 2023 at 02:33:21PM +0800, linan666@huaweicloud.com wrote: > From: Li Nan <linan122@huawei.com> > > If a disk is removed and quickly inserted when an I/O error is processing, > the disk may not be able to be re-added. The function call timeline is as > follows: > > interrupt scsi_eh > > ahci_error_intr > ata_port_freeze > __ata_port_freeze > =>ahci_freeze (turn IRQ off) > ata_port_abort > ata_do_link_abort > ata_port_schedule_eh > =>ata_std_sched_eh > ata_eh_set_pending > set EH_PENDING > scsi_schedule_eh > shost->host_eh_scheduled++ (=1) > scsi_error_handler > =>ata_scsi_error > ata_scsi_port_error_handler > clear EH_PENDING > =>ahci_error_handler > . sata_pmp_error_handler > . ata_eh_reset > . ata_eh_thaw_port > . . =>ahci_thaw (turn IRQ on) > ahci_error_intr . . > ata_port_freeze . . > __ata_port_freeze . . > =>ahci_freeze (turn IRQ off) . . > ... . . > ata_eh_set_pending . . > set EH_PENDING . . > scsi_schedule_eh . . > shost->host_eh_scheduled++ (=2) . . > . clear EH_PENDING > check EH_PENDING > =>ata_std_end_eh > host->host_eh_scheduled = 0; > > 'host_eh_scheduled' is 0 and scsi eh thread will not be scheduled again. > The ata port remains frozen and will never be enabled. > > To fix this issue, decrease 'host_eh_scheduled' instead of setting it to 0 > so that EH is scheduled again to re-enable the port. Also move the update > of 'nr_active_links' to 0 when 'host_eh_scheduled' is 0 to > ata_scsi_port_error_handler(). > > Reported-by: luojian <luojian5@huawei.com> > Signed-off-by: Li Nan <linan122@huawei.com> > --- > Changes in v3: > - change patch title, previously it was: "scsi: ata: Fix a race condition > between scsi error handler and ahci interrupt". > - drop the variable 'host' in ata_std_end_eh(). > - improve commit message. > > drivers/ata/libata-eh.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > index 159ba6ba19eb..2d5ecd68b7e0 100644 > --- a/drivers/ata/libata-eh.c > +++ b/drivers/ata/libata-eh.c > @@ -735,6 +735,12 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap) > */ > ap->ops->end_eh(ap); > > + if (!ap->scsi_host->host_eh_scheduled) { > + /* make sure nr_active_links is zero after EH */ > + WARN_ON(ap->nr_active_links); > + ap->nr_active_links = 0; > + } > + > spin_unlock_irqrestore(ap->lock, flags); > ata_eh_release(ap); > > @@ -946,9 +952,7 @@ EXPORT_SYMBOL_GPL(ata_std_sched_eh); > */ > void ata_std_end_eh(struct ata_port *ap) > { > - struct Scsi_Host *host = ap->scsi_host; > - > - host->host_eh_scheduled = 0; > + ap->scsi_host->host_eh_scheduled--; > } > EXPORT_SYMBOL(ata_std_end_eh); > > @@ -3922,10 +3926,6 @@ void ata_eh_finish(struct ata_port *ap) > } > } > } > - > - /* make sure nr_active_links is zero after EH */ > - WARN_ON(ap->nr_active_links); > - ap->nr_active_links = 0; > } > > /** > -- > 2.39.2 > Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 159ba6ba19eb..2d5ecd68b7e0 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -735,6 +735,12 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap) */ ap->ops->end_eh(ap); + if (!ap->scsi_host->host_eh_scheduled) { + /* make sure nr_active_links is zero after EH */ + WARN_ON(ap->nr_active_links); + ap->nr_active_links = 0; + } + spin_unlock_irqrestore(ap->lock, flags); ata_eh_release(ap); @@ -946,9 +952,7 @@ EXPORT_SYMBOL_GPL(ata_std_sched_eh); */ void ata_std_end_eh(struct ata_port *ap) { - struct Scsi_Host *host = ap->scsi_host; - - host->host_eh_scheduled = 0; + ap->scsi_host->host_eh_scheduled--; } EXPORT_SYMBOL(ata_std_end_eh); @@ -3922,10 +3926,6 @@ void ata_eh_finish(struct ata_port *ap) } } } - - /* make sure nr_active_links is zero after EH */ - WARN_ON(ap->nr_active_links); - ap->nr_active_links = 0; } /**