[PATCH 2.6.23-rc7 0/3] async_tx and md-accel fixes for 2.6.23

[PATCH 2.6.23-rc7 0/3] async_tx and md-accel fixes for 2.6.23

[Dan Williams]

Fix a couple bugs and provide documentation for the async_tx api.

Neil, please 'ack' patch #3.

git://lost.foo-projects.org/~dwillia2/git/iop async-tx-fixes-for-linus

Dan Williams (3):
      async_tx: usage documentation and developer notes
      async_tx: fix dma_wait_for_async_tx
      raid5: fix ops_complete_biofill

Documentation/crypto/async-tx-api.txt |  217 +++++++++++++++++++++++++++++++++
crypto/async_tx/async_tx.c            |   12 ++-
drivers/md/raid5.c                    |   90 +++++++-------
3 files changed, 273 insertions(+), 46 deletions(-)

--
Dan

[PATCH 2.6.23-rc7 1/3] async_tx: usage documentation and developer notes

[Dan Williams]

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

 Documentation/crypto/async-tx-api.txt |  217 +++++++++++++++++++++++++++++++++
 1 files changed, 217 insertions(+), 0 deletions(-)

diff --git a/Documentation/crypto/async-tx-api.txt b/Documentation/crypto/async-tx-api.txt
new file mode 100644
index 0000000..48d685a
--- /dev/null
+++ b/Documentation/crypto/async-tx-api.txt
@@ -0,0 +1,217 @@
+		 Asynchronous Transfers/Transforms API
+
+1 INTRODUCTION
+
+2 GENEALOGY
+
+3 USAGE
+3.1 General format of the API
+3.2 Supported operations
+3.2 Descriptor management
+3.3 When does the operation execute?
+3.4 When does the operation complete?
+3.5 Constraints
+3.6 Example
+
+4 DRIVER DEVELOPER NOTES
+4.1 Conformance points
+4.2 "My application needs finer control of hardware channels"
+
+5 SOURCE
+
+---
+
+1 INTRODUCTION
+
+The async_tx api provides methods for describing a chain of asynchronous
+bulk memory transfers/transforms with support for inter-transactional
+dependencies.  It is implemented as a dmaengine client that smooths over
+the details of different hardware offload engine implementations.  Code
+that is written to the api can optimize for asynchronous operation and
+the api will fit the chain of operations to the available offload
+resources.
+
+2 GENEALOGY
+
+The api was initially designed to offload the memory copy and
+xor-parity-calculations of the md-raid5 driver using the offload engines
+present in the Intel(R) Xscale series of I/O processors.  It also built
+on the 'dmaengine' layer developed for offloading memory copies in the
+network stack using Intel(R) I/OAT engines.  The following design
+features surfaced as a result:
+1/ implicit synchronous path: users of the API do not need to know if
+   the platform they are running on has offload capabilities.  The
+   operation will be offloaded when an engine is available and carried out
+   in software otherwise.
+2/ cross channel dependency chains: the API allows a chain of dependent
+   operations to be submitted, like xor->copy->xor in the raid5 case.  The
+   API automatically handles cases where the transition from one operation
+   to another implies a hardware channel switch.
+3/ dmaengine extensions to support multiple clients and operation types
+   beyond 'memcpy'
+
+3 USAGE
+
+3.1 General format of the API:
+struct dma_async_tx_descriptor *
+async_<operation>(<op specific parameters>,
+		  enum async_tx_flags flags,
+        	  struct dma_async_tx_descriptor *dependency,
+        	  dma_async_tx_callback callback_routine,
+		  void *callback_parameter);
+
+3.2 Supported operations:
+memcpy       - memory copy between a source and a destination buffer
+memset       - fill a destination buffer with a byte value
+xor 	     - xor a series of source buffers and write the result to a
+	       destination buffer
+xor_zero_sum - xor a series of source buffers and set a flag if the
+	       result is zero.  The implementation attempts to prevent
+	       writes to memory
+
+3.2 Descriptor management:
+The return value is non-NULL and points to a 'descriptor' when the operation
+has been queued to execute asynchronously.  Descriptors are recycled
+resources, under control of the offload engine driver, to be reused as
+operations complete.  When an application needs to submit a chain of
+operations it must guarantee that the descriptor is not automatically recycled
+before the dependency is submitted.  This requires that all descriptors be
+acknowledged by the application before the offload engine driver is allowed to
+recycle (or free) the descriptor.  A descriptor can be acked by:
+1/ setting the ASYNC_TX_ACK flag if no operations are to be submitted
+2/ setting the ASYNC_TX_DEP_ACK flag to acknowledge the parent
+   descriptor of a new operation.
+3/ calling async_tx_ack() on the descriptor.
+
+3.3 When does the operation execute?:
+Operations do not immediately issue after return from the
+async_<operation> call.  Offload engine drivers batch operations to
+improve performance by reducing the number of mmio cycles needed to
+manage the channel.  Once a driver specific threshold is met the driver
+automatically issues pending operations.  An application can force this
+event by calling async_tx_issue_pending_all().  This operates on all
+channels since the application has no knowledge of channel to operation
+mapping.
+
+3.4 When does the operation complete?:
+There are two methods for an application to learn about the completion
+of an operation.
+1/ Call dma_wait_for_async_tx().  This call causes the cpu to spin while
+   it polls for the completion of the operation.  It handles dependency
+   chains and issuing pending operations.
+2/ Specify a completion callback.  The callback routine runs in tasklet
+   context if the offload engine driver supports interrupts, or it is
+   called in application context if the operation is carried out
+   synchronously in software.  The callback can be set in the call to
+   async_<operation>, or when the application needs to submit a chain of
+   unknown length it can use the async_trigger_callback() routine to set a
+   completion interrupt/callback at the end of the chain.
+
+3.5 Constraints:
+1/ Calls to async_<operation> are not permitted in irq context.  Other
+   contexts are permitted provided constraint #2 is not violated.
+2/ Completion callback routines can not submit new operations.  This
+   results in recursion in the synchronous case and spin_locks being
+   acquired twice in the asynchronous case.
+
+3.6 Example:
+Perform a xor->copy->xor operation where each operation depends on the
+result from the previous operation:
+
+void complete_xor_copy_xor(void *param)
+{
+	printk("complete\n");
+}
+
+int run_xor_copy_xor(struct page **xor_srcs,
+		     int xor_src_cnt,
+		     struct page *xor_dest,
+		     size_t xor_len,
+		     struct page *copy_src,
+		     struct page *copy_dest,
+		     size_t copy_len)
+{
+	struct dma_async_tx_descriptor *tx;
+
+	tx = async_xor(xor_dest, xor_srcs, 0, xor_src_cnt, xor_len,
+		       ASYNC_TX_XOR_DROP_DST, NULL, NULL, NULL);
+	tx = async_memcpy(copy_dest, copy_src, 0, 0, copy_len,
+			  ASYNC_TX_DEP_ACK, tx, NULL, NULL);
+	tx = async_xor(xor_dest, xor_srcs, 0, xor_src_cnt, xor_len,
+		       ASYNC_TX_XOR_DROP_DST | ASYNC_TX_DEP_ACK | ASYNC_TX_ACK,
+		       tx, complete_xor_copy_xor, NULL);
+
+	async_tx_issue_pending_all();
+}
+
+See include/linux/async_tx.h for more information on the flags
+See the ops_run_* and ops_complete_* routines drivers/md/raid5.c for more
+implementation examples.
+
+4 DRIVER DEVELOPMENT NOTES
+4.1 Conformance points:
+There are a few conformance points required in dmaengine drivers to
+accommodate assumptions made by applications using the async_tx api:
+1/ Completion callbacks are expected to happen in tasklet or process
+   context
+2/ dma_async_tx_descriptor fields are never manipulated in irq context
+3/ Use async_tx_run_dependencies() in the descriptor clean up path to
+   handle submission of dependent operations
+
+4.2 "My application needs finer control of hardware channels"
+This requirement seems to arise from cases where a DMA engine driver is
+trying to support device-to-memory DMA.  The dmaengine and async_tx
+implementations were designed for offloading memory-to-memory
+operations; however, there are some capabilities of the dmaengine layer
+that can be used for platform specific channel management.  Platform
+specific constraints can be handled by registering the application as a
+'dma_client' and implementing a 'dma_event_callback' to apply a filter
+to the available channels in the system.  Before showing how to
+implement a custom dma_event callback some background of dmaengine's
+client support is required.
+
+The following routines in dmaengine support multiple clients requesting
+use of a channel:
+- dma_async_client_register(struct dma_client *client)
+- dma_async_client_chan_request(struct dma_client *client)
+
+dma_async_client_register takes a pointer to an initialized dma_client
+structure.  It expects that the 'event_callback' and 'cap_mask' fields
+are already initialized.
+
+dma_async_client_chan_request triggers dmaeninge to notify the client of
+all channels that satisfy the capability mask.  It is up to the client's
+event_callback routine to track how many channels the client needs and
+how many it is currently using.  The dma_event_callback routine returns a
+dma_state_client code to let dmaengine know the status of the
+allocation.
+
+Below is the example of how to extend this functionality for platform
+specific filtering of the available channels beyond the standard
+capability mask:
+
+static enum dma_state_client
+my_dma_client_callback(struct dma_client *client,
+			struct dma_chan *chan, enum dma_state state)
+{
+	struct dma_device *dma_dev;
+	struct my_platform_specific_dma *plat_dma_dev;
+	
+	dma_dev = chan->device;
+	plat_dma_dev = container_of(dma_dev,
+				    struct my_platform_specific_dma,
+				    dma_dev);
+
+	if (!plat_dma_dev->platform_specific_capability)
+		return DMA_DUP;
+
+	. . .
+}
+
+5 SOURCE
+drivers/dma/dmaengine.c: offload engine channel management routines
+drivers/dma/: location for offload engine drivers
+crypto/async_tx/async_tx.c: async_tx interface to dmaengine and common code
+crypto/async_tx/async_memcpy.c: copy offload
+crypto/async_tx/async_memset.c: memory fill offload
+crypto/async_tx/async_xor.c: xor offload

Re: [PATCH 2.6.23-rc7 1/3] async_tx: usage documentation and developer notes

[Randy Dunlap]

On Thu, 20 Sep 2007 18:27:40 -0700 Dan Williams wrote:

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---

Hi Dan,

Looks pretty good and informative.  Thanks.

(nits below :)


>  Documentation/crypto/async-tx-api.txt |  217 +++++++++++++++++++++++++++++++++
>  1 files changed, 217 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/crypto/async-tx-api.txt b/Documentation/crypto/async-tx-api.txt
> new file mode 100644
> index 0000000..48d685a
> --- /dev/null
> +++ b/Documentation/crypto/async-tx-api.txt
> @@ -0,0 +1,217 @@
> +		 Asynchronous Transfers/Transforms API
> +
> +1 INTRODUCTION
> +
> +2 GENEALOGY
> +
> +3 USAGE
> +3.1 General format of the API
> +3.2 Supported operations
> +3.2 Descriptor management

duplicate 3.2

> +3.3 When does the operation execute?
> +3.4 When does the operation complete?
> +3.5 Constraints
> +3.6 Example
> +
> +4 DRIVER DEVELOPER NOTES
> +4.1 Conformance points
> +4.2 "My application needs finer control of hardware channels"
> +
> +5 SOURCE
> +
> +---
> +
> +1 INTRODUCTION
> +
> +The async_tx api provides methods for describing a chain of asynchronous
> +bulk memory transfers/transforms with support for inter-transactional
> +dependencies.  It is implemented as a dmaengine client that smooths over
> +the details of different hardware offload engine implementations.  Code
> +that is written to the api can optimize for asynchronous operation and
> +the api will fit the chain of operations to the available offload
> +resources.
> +

I would s/api/API/g .

> +2 GENEALOGY
> +
[snip]

> +
> +3 USAGE
> +
> +3.1 General format of the API:
> +struct dma_async_tx_descriptor *
> +async_<operation>(<op specific parameters>,
> +		  enum async_tx_flags flags,
> +        	  struct dma_async_tx_descriptor *dependency,
> +        	  dma_async_tx_callback callback_routine,
> +		  void *callback_parameter);
> +
> +3.2 Supported operations:
> +memcpy       - memory copy between a source and a destination buffer
> +memset       - fill a destination buffer with a byte value
> +xor 	     - xor a series of source buffers and write the result to a
> +	       destination buffer
> +xor_zero_sum - xor a series of source buffers and set a flag if the
> +	       result is zero.  The implementation attempts to prevent
> +	       writes to memory
> +
> +3.2 Descriptor management:

duplicate 3.2

> +The return value is non-NULL and points to a 'descriptor' when the operation
> +has been queued to execute asynchronously.  Descriptors are recycled
> +resources, under control of the offload engine driver, to be reused as
> +operations complete.  When an application needs to submit a chain of
> +operations it must guarantee that the descriptor is not automatically recycled
> +before the dependency is submitted.  This requires that all descriptors be
> +acknowledged by the application before the offload engine driver is allowed to
> +recycle (or free) the descriptor.  A descriptor can be acked by:

can be acked by any of:   (?)

> +1/ setting the ASYNC_TX_ACK flag if no operations are to be submitted
> +2/ setting the ASYNC_TX_DEP_ACK flag to acknowledge the parent
> +   descriptor of a new operation.
> +3/ calling async_tx_ack() on the descriptor.
> +
> +3.3 When does the operation execute?:

Drop ':'

> +Operations do not immediately issue after return from the
> +async_<operation> call.  Offload engine drivers batch operations to
> +improve performance by reducing the number of mmio cycles needed to
> +manage the channel.  Once a driver specific threshold is met the driver

                               driver-specific

> +automatically issues pending operations.  An application can force this
> +event by calling async_tx_issue_pending_all().  This operates on all
> +channels since the application has no knowledge of channel to operation
> +mapping.
> +
> +3.4 When does the operation complete?:

drop ':'

> +There are two methods for an application to learn about the completion
> +of an operation.
> +1/ Call dma_wait_for_async_tx().  This call causes the cpu to spin while

s/cpu/CPU/g

> +   it polls for the completion of the operation.  It handles dependency
> +   chains and issuing pending operations.
> +2/ Specify a completion callback.  The callback routine runs in tasklet
> +   context if the offload engine driver supports interrupts, or it is
> +   called in application context if the operation is carried out
> +   synchronously in software.  The callback can be set in the call to
> +   async_<operation>, or when the application needs to submit a chain of
> +   unknown length it can use the async_trigger_callback() routine to set a
> +   completion interrupt/callback at the end of the chain.
> +
> +3.5 Constraints:
> +1/ Calls to async_<operation> are not permitted in irq context.  Other

s/irq/IRQ/g

> +   contexts are permitted provided constraint #2 is not violated.
> +2/ Completion callback routines can not submit new operations.  This

                                   cannot

> +   results in recursion in the synchronous case and spin_locks being
> +   acquired twice in the asynchronous case.
> +
> +3.6 Example:
> +Perform a xor->copy->xor operation where each operation depends on the
> +result from the previous operation:
> +
> +void complete_xor_copy_xor(void *param)
> +{
> +	printk("complete\n");
> +}
> +
> +int run_xor_copy_xor(struct page **xor_srcs,
[snip]

> +}
> +
> +See include/linux/async_tx.h for more information on the flags

end with '.'

> +See the ops_run_* and ops_complete_* routines drivers/md/raid5.c for more

                                                ^in

> +implementation examples.
> +
> +4 DRIVER DEVELOPMENT NOTES
> +4.1 Conformance points:
> +There are a few conformance points required in dmaengine drivers to
> +accommodate assumptions made by applications using the async_tx api:
> +1/ Completion callbacks are expected to happen in tasklet or process
> +   context
> +2/ dma_async_tx_descriptor fields are never manipulated in irq context
> +3/ Use async_tx_run_dependencies() in the descriptor clean up path to
> +   handle submission of dependent operations
> +
> +4.2 "My application needs finer control of hardware channels"
> +This requirement seems to arise from cases where a DMA engine driver is
> +trying to support device-to-memory DMA.  The dmaengine and async_tx
> +implementations were designed for offloading memory-to-memory
> +operations; however, there are some capabilities of the dmaengine layer
> +that can be used for platform specific channel management.  Platform

                        platform-specific                      Platform-

> +specific constraints can be handled by registering the application as a
> +'dma_client' and implementing a 'dma_event_callback' to apply a filter
> +to the available channels in the system.  Before showing how to
> +implement a custom dma_event callback some background of dmaengine's
> +client support is required.
> +
> +The following routines in dmaengine support multiple clients requesting
> +use of a channel:
> +- dma_async_client_register(struct dma_client *client)
> +- dma_async_client_chan_request(struct dma_client *client)
> +
> +dma_async_client_register takes a pointer to an initialized dma_client
> +structure.  It expects that the 'event_callback' and 'cap_mask' fields
> +are already initialized.
> +
> +dma_async_client_chan_request triggers dmaeninge to notify the client of
> +all channels that satisfy the capability mask.  It is up to the client's
> +event_callback routine to track how many channels the client needs and
> +how many it is currently using.  The dma_event_callback routine returns a
> +dma_state_client code to let dmaengine know the status of the
> +allocation.
> +
> +Below is the example of how to extend this functionality for platform

                                                                platform-

> +specific filtering of the available channels beyond the standard
> +capability mask:
> +
> +static enum dma_state_client
> +my_dma_client_callback(struct dma_client *client,
> +			struct dma_chan *chan, enum dma_state state)
> +{
> +	. . .
> +}
> +
> +5 SOURCE
> +drivers/dma/dmaengine.c: offload engine channel management routines
> +drivers/dma/: location for offload engine drivers
> +crypto/async_tx/async_tx.c: async_tx interface to dmaengine and common code
> +crypto/async_tx/async_memcpy.c: copy offload
> +crypto/async_tx/async_memset.c: memory fill offload
> +crypto/async_tx/async_xor.c: xor offload
> -

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 2.6.23-rc7 1/3] async_tx: usage documentation and developer notes

[Bill Davidsen]

Dan Williams wrote:
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>
>  Documentation/crypto/async-tx-api.txt |  217 +++++++++++++++++++++++++++++++++
>  1 files changed, 217 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/crypto/async-tx-api.txt b/Documentation/crypto/async-tx-api.txt
> new file mode 100644
> index 0000000..48d685a
> --- /dev/null
> +++ b/Documentation/crypto/async-tx-api.txt
> @@ -0,0 +1,217 @@
> +		 Asynchronous Transfers/Transforms API
> +
> +1 INTRODUCTION
> +
> +2 GENEALOGY
> +
> +3 USAGE
> +3.1 General format of the API
> +3.2 Supported operations
> +3.2 Descriptor management
> +3.3 When does the operation execute?
> +3.4 When does the operation complete?
> +3.5 Constraints
> +3.6 Example
> +

This is very readable, and appears extensible to any new forthcoming 
technology I'm aware of. Great job!

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979

RE: [PATCH 2.6.23-rc7 1/3] async_tx: usage documentation and developernotes

[Nelson, Shannon]

>From: Williams, Dan J 
>Sent: Thursday, September 20, 2007 6:28 PM
>To: linux-kernel@vger.kernel.org; linux-raid@vger.kernel.org; 
>neilb@suse.de
>Cc: akpm@linux-foundation.org; yur@emcraft.com; 
>saeed.bishara@gmail.com; Nelson, Shannon
>Subject: [PATCH 2.6.23-rc7 1/3] async_tx: usage documentation 
>and developernotes
>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>---
>
> Documentation/crypto/async-tx-api.txt |  217 
>+++++++++++++++++++++++++++++++++
> 1 files changed, 217 insertions(+), 0 deletions(-)

Thanks, Dan.  In addition to Randy's most excellent nit-picking, here
are a couple more minor nits.

[...]
>+4.2 "My application needs finer control of hardware channels"
>+This requirement seems to arise from cases where a DMA engine 
>driver is
[...]
>+dma_async_client_chan_request triggers dmaeninge to notify 
>the client of

s/dmaeninge/dmaengine/

[...]
>+5 SOURCE
>+drivers/dma/dmaengine.c: offload engine channel management routines
>+drivers/dma/: location for offload engine drivers
>+crypto/async_tx/async_tx.c: async_tx interface to dmaengine 
>and common code
>+crypto/async_tx/async_memcpy.c: copy offload
>+crypto/async_tx/async_memset.c: memory fill offload
>+crypto/async_tx/async_xor.c: xor offload

Also include/linux/dmaengine.h


sln
--
======================================================================
Mr. Shannon Nelson                 LAN Access Division, Intel Corp.
Shannon.Nelson@intel.com                I don't speak for Intel
(503) 712-7659                    Parents can't afford to be squeamish.

[PATCH 2.6.23-rc7 2/3] async_tx: fix dma_wait_for_async_tx

[Dan Williams]

Fix dma_wait_for_async_tx to not loop forever in the case where a
dependency chain is longer than two entries.  This condition will not
happen with current in-kernel drivers, but fix it for future drivers.

Found-by: Saeed Bishara <saeed.bishara@gmail.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

 crypto/async_tx/async_tx.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/crypto/async_tx/async_tx.c b/crypto/async_tx/async_tx.c
index 0350071..bc18cbb 100644
--- a/crypto/async_tx/async_tx.c
+++ b/crypto/async_tx/async_tx.c
@@ -80,6 +80,7 @@ dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx)
 {
 	enum dma_status status;
 	struct dma_async_tx_descriptor *iter;
+	struct dma_async_tx_descriptor *parent;
 
 	if (!tx)
 		return DMA_SUCCESS;
@@ -87,8 +88,15 @@ dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx)
 	/* poll through the dependency chain, return when tx is complete */
 	do {
 		iter = tx;
-		while (iter->cookie == -EBUSY)
-			iter = iter->parent;
+
+		/* find the root of the unsubmitted dependency chain */
+		while (iter->cookie == -EBUSY) {
+			parent = iter->parent;
+			if (parent && parent->cookie == -EBUSY)
+				iter = iter->parent;
+			else
+				break;
+		}
 
 		status = dma_sync_wait(iter->chan, iter->cookie);
 	} while (status == DMA_IN_PROGRESS || (iter != tx));

[PATCH 2.6.23-rc7 3/3] raid5: fix ops_complete_biofill

[Dan Williams]

ops_complete_biofill tried to avoid calling handle_stripe since all the
state necessary to return read completions is available.  However the
process of determining whether more read requests are pending requires
locking the stripe (to block add_stripe_bio from updating dev->toead).
ops_complete_biofill can run in tasklet context, so rather than upgrading
all the stripe locks from spin_lock to spin_lock_bh this patch just moves
read completion handling back into handle_stripe.

Found-by: Yuri Tikhonov <yur@emcraft.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

 drivers/md/raid5.c |   90 +++++++++++++++++++++++++++-------------------------
 1 files changed, 46 insertions(+), 44 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 4d63773..38c8893 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -512,54 +512,12 @@ async_copy_data(int frombio, struct bio *bio, struct page *page,
 static void ops_complete_biofill(void *stripe_head_ref)
 {
 	struct stripe_head *sh = stripe_head_ref;
-	struct bio *return_bi = NULL;
-	raid5_conf_t *conf = sh->raid_conf;
-	int i, more_to_read = 0;
 
 	pr_debug("%s: stripe %llu\n", __FUNCTION__,
 		(unsigned long long)sh->sector);
 
-	/* clear completed biofills */
-	for (i = sh->disks; i--; ) {
-		struct r5dev *dev = &sh->dev[i];
-		/* check if this stripe has new incoming reads */
-		if (dev->toread)
-			more_to_read++;
-
-		/* acknowledge completion of a biofill operation */
-		/* and check if we need to reply to a read request
-		*/
-		if (test_bit(R5_Wantfill, &dev->flags) && !dev->toread) {
-			struct bio *rbi, *rbi2;
-			clear_bit(R5_Wantfill, &dev->flags);
-
-			/* The access to dev->read is outside of the
-			 * spin_lock_irq(&conf->device_lock), but is protected
-			 * by the STRIPE_OP_BIOFILL pending bit
-			 */
-			BUG_ON(!dev->read);
-			rbi = dev->read;
-			dev->read = NULL;
-			while (rbi && rbi->bi_sector <
-				dev->sector + STRIPE_SECTORS) {
-				rbi2 = r5_next_bio(rbi, dev->sector);
-				spin_lock_irq(&conf->device_lock);
-				if (--rbi->bi_phys_segments == 0) {
-					rbi->bi_next = return_bi;
-					return_bi = rbi;
-				}
-				spin_unlock_irq(&conf->device_lock);
-				rbi = rbi2;
-			}
-		}
-	}
-	clear_bit(STRIPE_OP_BIOFILL, &sh->ops.ack);
-	clear_bit(STRIPE_OP_BIOFILL, &sh->ops.pending);
-
-	return_io(return_bi);
-
-	if (more_to_read)
-		set_bit(STRIPE_HANDLE, &sh->state);
+	set_bit(STRIPE_OP_BIOFILL, &sh->ops.complete);
+	set_bit(STRIPE_HANDLE, &sh->state);
 	release_stripe(sh);
 }
 
@@ -2112,6 +2070,42 @@ static void handle_issuing_new_read_requests6(struct stripe_head *sh,
 }
 
 
+/* handle_completed_read_requests - return completion for reads and allow
+ * new read operations to be submitted to the stripe.
+ */
+static void handle_completed_read_requests(raid5_conf_t *conf,
+					    struct stripe_head *sh,
+					    struct bio **return_bi)
+{
+	int i;
+
+	pr_debug("%s: stripe %llu\n", __FUNCTION__,
+		(unsigned long long)sh->sector);
+
+	/* check if we need to reply to a read request */
+	for (i = sh->disks; i--; ) {
+		struct r5dev *dev = &sh->dev[i];
+
+		if (test_and_clear_bit(R5_Wantfill, &dev->flags)) {
+			struct bio *rbi, *rbi2;
+
+			rbi = dev->read;
+			dev->read = NULL;
+			while (rbi && rbi->bi_sector <
+				dev->sector + STRIPE_SECTORS) {
+				rbi2 = r5_next_bio(rbi, dev->sector);
+				spin_lock_irq(&conf->device_lock);
+				if (--rbi->bi_phys_segments == 0) {
+					rbi->bi_next = *return_bi;
+					*return_bi = rbi;
+				}
+				spin_unlock_irq(&conf->device_lock);
+				rbi = rbi2;
+			}
+		}
+	}
+}
+
 /* handle_completed_write_requests
  * any written block on an uptodate or failed drive can be returned.
  * Note that if we 'wrote' to a failed drive, it will be UPTODATE, but
@@ -2633,6 +2627,14 @@ static void handle_stripe5(struct stripe_head *sh)
 	s.expanded = test_bit(STRIPE_EXPAND_READY, &sh->state);
 	/* Now to look around and see what can be done */
 
+	if (test_bit(STRIPE_OP_BIOFILL, &sh->ops.complete)) {
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.ack);
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.pending);
+		clear_bit(STRIPE_OP_BIOFILL, &sh->ops.complete);
+
+		handle_completed_read_requests(conf, sh, &return_bi);
+	}
+
 	rcu_read_lock();
 	for (i=disks; i--; ) {
 		mdk_rdev_t *rdev;

Re: [PATCH 2.6.23-rc7 0/3] async_tx and md-accel fixes for 2.6.23

[Andrew Morton]

On Thu, 20 Sep 2007 18:27:35 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Fix a couple bugs and provide documentation for the async_tx api.
> 
> Neil, please 'ack' patch #3.
> 
> git://lost.foo-projects.org/~dwillia2/git/iop async-tx-fixes-for-linus

Well it looks like Neil is on vacation or is hiding from us or something.

In which case our options would be to go ahead and merge your #3 with
our fingers crossed, or wait and do it in 2.6.23.1 or .2.

How serious is this bug which you're fixing?  Will it affect users
of "legacy" MD setups, or only those who have enabled fancy dma-engine
features?

Either way, we need to get #2 (at least) in to Linus.

RE: [PATCH 2.6.23-rc7 0/3] async_tx and md-accel fixes for 2.6.23

[Williams, Dan J]

> From: Andrew Morton [mailto:akpm@linux-foundation.org]
> > Fix a couple bugs and provide documentation for the async_tx api.
> >
> > Neil, please 'ack' patch #3.
> >
> > git://lost.foo-projects.org/~dwillia2/git/iop
async-tx-fixes-for-linus
> 
> Well it looks like Neil is on vacation or is hiding from us or
something.
> 
> In which case our options would be to go ahead and merge your #3 with
> our fingers crossed, or wait and do it in 2.6.23.1 or .2.
> 
> How serious is this bug which you're fixing?  Will it affect users
> of "legacy" MD setups, or only those who have enabled fancy dma-engine
> features?
> 
The symptom is raid array that fails to complete a read request.

It has only been triggered by Yuri when he tried a dma-engine
configuration on his PPC platform with PAGE_SIZE=64K, this opens a large
window for the bug to hit.  The window is much smaller but not 100%
closed in the !dma-engine case, although no one else has hit it. 

2.6.22 returned reads from handle_stripe under the stripe lock so this
patch can be seen as 'reverting a botched micro-optimization' versus
'introducing a new way to handle reads'.

> Either way, we need to get #2 (at least) in to Linus.

#2 is less critical and cannot hit with any of the current drivers in
the kernel, so it is ok if this one misses 2.6.23.

Re: [PATCH 2.6.23-rc7 0/3] async_tx and md-accel fixes for 2.6.23

[Neil Brown]

On Friday September 21, akpm@linux-foundation.org wrote:
> On Thu, 20 Sep 2007 18:27:35 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Fix a couple bugs and provide documentation for the async_tx api.
> > 
> > Neil, please 'ack' patch #3.
> > 
> > git://lost.foo-projects.org/~dwillia2/git/iop async-tx-fixes-for-linus
> 
> Well it looks like Neil is on vacation or is hiding from us or something.

Neil is just not coping well with jet-lag....

Patch #3 looks good and necessary
  Acked-By: NeilBrown <neilb@suse.de>

I know that should probably be a "reviewed-by"....  I was a bit
surprised that the "handle_completed_read_requests" call was so early
in handle_stripe5 - I don't think the code was originally that early.
But it is probably right.   Hopefully my brain will have cleared by
Monday and I'll review it again then.

NeilBrown

RE: [PATCH 2.6.23-rc7 0/3] async_tx and md-accel fixes for 2.6.23

[Williams, Dan J]

> From: Neil Brown [mailto:neilb@suse.de]
> On Friday September 21, akpm@linux-foundation.org wrote:
> > On Thu, 20 Sep 2007 18:27:35 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > > Fix a couple bugs and provide documentation for the async_tx api.
> > >
> > > Neil, please 'ack' patch #3.
> > >
> > > git://lost.foo-projects.org/~dwillia2/git/iop
async-tx-fixes-for-linus
> >
> > Well it looks like Neil is on vacation or is hiding from us or
something.
> 
> Neil is just not coping well with jet-lag....
> 
> Patch #3 looks good and necessary
>   Acked-By: NeilBrown <neilb@suse.de>
> 
> I know that should probably be a "reviewed-by"....  I was a bit
I went ahead and added reviewed-by.

> surprised that the "handle_completed_read_requests" call was so early
> in handle_stripe5 - I don't think the code was originally that early.
It is slightly earlier than 2.6.22 (outside the '/* now count some
things */' loop) to make sure the R5_Wantfill flags from the last
request have been cleared before starting a new one:

		/* maybe we can request a biofill operation
		 *
		 * new wantfill requests are only permitted while
		 * STRIPE_OP_BIOFILL is clear
		 */
		if (test_bit(R5_UPTODATE, &dev->flags) && dev->toread &&
			!test_bit(STRIPE_OP_BIOFILL, &sh->ops.pending))
			set_bit(R5_Wantfill, &dev->flags);

> But it is probably right.   Hopefully my brain will have cleared by
> Monday and I'll review it again then.
>

Ok, the tree is updated with 'Reviewed-by' tags and the proposed
documentation updates from Randy and Shannon.

git://lost.foo-projects.org/~dwillia2/git/iop async-tx-fixes-for-linus

Dan Williams (3):
      async_tx: usage documentation and developer notes (v2)
      async_tx: fix dma_wait_for_async_tx
      raid5: fix ops_complete_biofill

> NeilBrown

--
Dan