[PATCH 01/02] libata: shift host flag constants

[PATCH 01/02] libata: shift host flag constants

[Tejun Heo]

Nudge host flag constants to make a room after ATA_FLAG_EH_PENDING.
New EH flag will be added.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 include/linux/libata.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

7395acb2c840fd4d0cacc91d6fb71440057141ab
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b0ee1c1..3f9c65f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -149,14 +149,14 @@ enum {
 	ATA_FLAG_NO_ATAPI	= (1 << 6), /* No ATAPI support */
 	ATA_FLAG_PIO_DMA	= (1 << 7), /* PIO cmds via DMA */
 	ATA_FLAG_PIO_LBA48	= (1 << 8), /* Host DMA engine is LBA28 only */
-	ATA_FLAG_PIO_POLLING	= (1 << 10), /* use polling PIO if LLD
-					      * doesn't handle PIO interrupts */
-	ATA_FLAG_NCQ		= (1 << 11), /* host supports NCQ */
+	ATA_FLAG_PIO_POLLING	= (1 << 9), /* use polling PIO if LLD
+					     * doesn't handle PIO interrupts */
+	ATA_FLAG_NCQ		= (1 << 10), /* host supports NCQ */
 
-	ATA_FLAG_DEBUGMSG	= (1 << 14),
-	ATA_FLAG_FLUSH_PORT_TASK = (1 << 15), /* flush port task */
+	ATA_FLAG_DEBUGMSG	= (1 << 13),
+	ATA_FLAG_FLUSH_PORT_TASK = (1 << 14), /* flush port task */
 
-	ATA_FLAG_EH_PENDING	= (1 << 16), /* EH pending */
+	ATA_FLAG_EH_PENDING	= (1 << 15), /* EH pending */
 	ATA_FLAG_FROZEN		= (1 << 17), /* port is frozen */
 	ATA_FLAG_RECOVERED	= (1 << 18), /* recovery action performed */
 
-- 
1.3.2

[PATCH 02/02] libata: implement ata_eh_wait()

[Tejun Heo]

Implement ata_eh_wait().  On return from this function, it's
guaranteed that the EH which was pending or in progress when the
function was called is complete - including the tailing part of SCSI
EH.  This will be used by hotplug and others to synchronize with EH.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    1 +
 drivers/scsi/libata-eh.c   |   38 ++++++++++++++++++++++++++++++++++++++
 drivers/scsi/libata.h      |    1 +
 include/linux/libata.h     |    2 ++
 4 files changed, 42 insertions(+), 0 deletions(-)

c6cf9e99d1de5ca6a08fb639bb73031ffe50d802
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 11df827..66df895 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -5189,6 +5189,7 @@ static void ata_host_init(struct ata_por
 
 	INIT_WORK(&ap->port_task, NULL, NULL);
 	INIT_LIST_HEAD(&ap->eh_done_q);
+	init_waitqueue_head(&ap->eh_wait_q);
 
 	/* set cable type */
 	ap->cbl = ATA_CBL_NONE;
diff --git a/drivers/scsi/libata-eh.c b/drivers/scsi/libata-eh.c
index b88f492..9173d8f 100644
--- a/drivers/scsi/libata-eh.c
+++ b/drivers/scsi/libata-eh.c
@@ -237,6 +237,7 @@ void ata_scsi_error(struct Scsi_Host *ho
 		ap->eh_context.i = ap->eh_info;
 		memset(&ap->eh_info, 0, sizeof(ap->eh_info));
 
+		ap->flags |= ATA_FLAG_EH_IN_PROGRESS;
 		ap->flags &= ~ATA_FLAG_EH_PENDING;
 
 		spin_unlock_irqrestore(hs_lock, flags);
@@ -290,12 +291,49 @@ void ata_scsi_error(struct Scsi_Host *ho
 		ata_port_printk(ap, KERN_INFO, "EH complete\n");
 	ap->flags &= ~ATA_FLAG_RECOVERED;
 
+	/* tell wait_eh that we're done */
+	ap->flags &= ~ATA_FLAG_EH_IN_PROGRESS;
+	wake_up_all(&ap->eh_wait_q);
+
 	spin_unlock_irqrestore(hs_lock, flags);
 
 	DPRINTK("EXIT\n");
 }
 
 /**
+ *	ata_port_wait_eh - Wait for the currently pending EH to complete
+ *	@ap: Port to wait EH for
+ *
+ *	Wait until the currently pending EH is complete.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep).
+ */
+void ata_port_wait_eh(struct ata_port *ap)
+{
+	unsigned long flags;
+	DEFINE_WAIT(wait);
+
+ retry:
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+
+	while (ap->flags & (ATA_FLAG_EH_PENDING | ATA_FLAG_EH_IN_PROGRESS)) {
+		prepare_to_wait(&ap->eh_wait_q, &wait, TASK_UNINTERRUPTIBLE);
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
+		schedule();
+		spin_lock_irqsave(&ap->host_set->lock, flags);
+	}
+
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	/* make sure SCSI EH is complete */
+	if (scsi_host_in_recovery(ap->host)) {
+		msleep(10);
+		goto retry;
+	}
+}
+
+/**
  *	ata_qc_timeout - Handle timeout of queued command
  *	@qc: Command that timed out
  *
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index b76ad7d..d56d9e1 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -103,6 +103,7 @@ extern void ata_schedule_scsi_eh(struct 
 /* libata-eh.c */
 extern enum scsi_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd);
 extern void ata_scsi_error(struct Scsi_Host *host);
+extern void ata_port_wait_eh(struct ata_port *ap);
 extern void ata_qc_schedule_eh(struct ata_queued_cmd *qc);
 
 #endif /* __LIBATA_H__ */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3f9c65f..2eb5828 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -157,6 +157,7 @@ enum {
 	ATA_FLAG_FLUSH_PORT_TASK = (1 << 14), /* flush port task */
 
 	ATA_FLAG_EH_PENDING	= (1 << 15), /* EH pending */
+	ATA_FLAG_EH_IN_PROGRESS	= (1 << 16), /* EH in progress */
 	ATA_FLAG_FROZEN		= (1 << 17), /* port is frozen */
 	ATA_FLAG_RECOVERED	= (1 << 18), /* recovery action performed */
 
@@ -490,6 +491,7 @@ struct ata_port {
 
 	u32			msg_enable;
 	struct list_head	eh_done_q;
+	wait_queue_head_t	eh_wait_q;
 
 	void			*private_data;
 
-- 
1.3.2

Re: [PATCH 01/02] libata: shift host flag constants

[Jeff Garzik]

Tejun Heo wrote:
> Nudge host flag constants to make a room after ATA_FLAG_EH_PENDING.
> New EH flag will be added.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK

Re: [PATCH 02/02] libata: implement ata_eh_wait()

[Jeff Garzik]

Tejun Heo wrote:
> +	spin_lock_irqsave(&ap->host_set->lock, flags);
> +
> +	while (ap->flags & (ATA_FLAG_EH_PENDING | ATA_FLAG_EH_IN_PROGRESS)) {
> +		prepare_to_wait(&ap->eh_wait_q, &wait, TASK_UNINTERRUPTIBLE);
> +		spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +		schedule();
> +		spin_lock_irqsave(&ap->host_set->lock, flags);
> +	}


Two comments:

* why not use completions?

* don't use schedule().  If there's nothing to schedule, it IMO chews up 
too much CPU busy-waiting.  schedule_timeout(1) will at least wait for 
the next timer tick.

Re: [PATCH 02/02] libata: implement ata_eh_wait()

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> +    spin_lock_irqsave(&ap->host_set->lock, flags);
>> +
>> +    while (ap->flags & (ATA_FLAG_EH_PENDING | 
>> ATA_FLAG_EH_IN_PROGRESS)) {
>> +        prepare_to_wait(&ap->eh_wait_q, &wait, TASK_UNINTERRUPTIBLE);
>> +        spin_unlock_irqrestore(&ap->host_set->lock, flags);
>> +        schedule();
>> +        spin_lock_irqsave(&ap->host_set->lock, flags);
>> +    }
> 
> 
> Two comments:
> 
> * why not use completions?

Mainly because there can be more than one waiters and also because the 
waiting doesn't really fit completion semantics.  In some places, 
eh_wait is used just to flush EH without actually scheduling EH.  In 
such cases, there's no synchronization point to start waiting for 
completion.

> * don't use schedule().  If there's nothing to schedule, it IMO chews up 
> too much CPU busy-waiting.  schedule_timeout(1) will at least wait for 
> the next timer tick.

I don't really understand your point here.  All that schedule_timeout() 
does is adding a timer to wake it up after the given jiffies pass.  It 
only adds wake up condition.  ie. it only makes sleeps shorter not 
longer.  e.g. both of the following code snippets result in busy 
sleep/wake up loop.

__set_current_state(TASK_RUNNING);
while (some_condition)
	schedule();

-------

__set_current_state(TASK_RUNNING);
while (some_condition)
	schedule_timeout(1);

The only behavior difference between above two code snippets is that the 
latter registers and unregisters timer every iteration which probably 
never expires as the thread would usually get scheduled again before a 
full tick passes.

AFAICS, schedule_timeout() is to be used when the wait should be 'timed 
out' when the thread has other wake up condition it's waiting for but it 
doesn't want to wait forever.  As a special case, when the condition is 
nil, it works as unconditional sleep.

Whether a thread gets rescheduled or not is determined by 
current->task_state.  If it's RUNNING, it will get rescheduled 
immediately.  If it's UNINTERRUPTIBLE or INTERRUPTIBLE, it will wait 
till gets woken up.  schedule() simply means the caller is done with the 
CPU at the moment and giving it away.

In eh_wait(), prepare_to_wait() is called w/ TASK_UNINTERRUPTIBLE before 
calling schedule, which sets current->task_state to UNINTERRUPTIBLE. 
The thread won't be scheduled again until the wait condition triggers 
and restores task_state to RUNNING.

Oops, I forgot calling finish_wait() after exiting the loop.  I'll 
submit a patch as soon as #upstream is updated.  Sorry.

Anyways, please read workqueue.c::flush_cpu_workqueue() and 
sched.c::wait_for_completion().  These two functions use similar waiting 
loops.

Thanks.

-- 
tejun

Re: [PATCH 02/02] libata: implement ata_eh_wait()

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> +    spin_lock_irqsave(&ap->host_set->lock, flags);
>>> +
>>> +    while (ap->flags & (ATA_FLAG_EH_PENDING | 
>>> ATA_FLAG_EH_IN_PROGRESS)) {
>>> +        prepare_to_wait(&ap->eh_wait_q, &wait, TASK_UNINTERRUPTIBLE);
>>> +        spin_unlock_irqrestore(&ap->host_set->lock, flags);
>>> +        schedule();
>>> +        spin_lock_irqsave(&ap->host_set->lock, flags);

DUH.  Ignore my objection, I read the code incorrectly, thinking it was 
just a simple polling delay, when obviously it is not.

ACK.

	Jeff