On Wednesday June 3, dan.j.williams@xxxxxxxxx wrote:
> [ Based on an original patch by Yuri Tikhonov ]
>
> The raid_run_ops routine uses the asynchronous offload api and
> the stripe_operations member of a stripe_head to carry out xor+pq+copy
> operations asynchronously, outside the lock.
>
> The operations performed by RAID-6 are the same as in the RAID-5 case
> except for no support of STRIPE_OP_PREXOR operations. All the others
> are supported:
> STRIPE_OP_BIOFILL
> - copy data into request buffers to satisfy a read request
> STRIPE_OP_COMPUTE_BLK
> - generate missing blocks (1 or 2) in the cache from the other blocks
> STRIPE_OP_BIODRAIN
> - copy data out of request buffers to satisfy a write request
> STRIPE_OP_RECONSTRUCT
> - recalculate parity for new data that has entered the cache
> STRIPE_OP_CHECK
> - verify that the parity is correct
>
> The flow is the same as in the RAID-5 case, and reuses some routines, namely:
> 1/ ops_complete_postxor (renamed to ops_complete_reconstruct)
> 2/ ops_complete_compute (updated to set up to 2 targets uptodate)
> 3/ ops_run_check (renamed to ops_run_check_p for xor parity checks)
>
...
> +static struct dma_async_tx_descriptor *
> +ops_run_compute6_1(struct stripe_head *sh)
> +{
> + int disks = sh->disks;
> + struct page **blocks = sh->scribble;
> + int target;
> + int qd_idx = sh->qd_idx;
> + struct dma_async_tx_descriptor *tx;
> + struct async_submit_ctl submit;
> + struct r5dev *tgt;
> + struct page *dest;
> + int i;
> + int count;
> +
> + if (sh->ops.target < 0)
> + target = sh->ops.target2;
> + else if (sh->ops.target2 < 0)
> + target = sh->ops.target;
> + else
> + /* we should only have one valid target */
> + BUG();
> + BUG_ON(target < 0);
This looks wrong.
It seems to suggest that target2 can be >=0 while target < 0
and I don't think it can. So while the code won't actually
malfunction, it is misleading.
Can we just have
target = sh->ops.target;
BUG_ON(target < 0);
BUG_ON(sh->ops.target2 >= 0);
??
> @@ -926,9 +1166,16 @@ static void raid5_run_ops(struct stripe_head *sh, unsigned long ops_request)
> }
>
> if (test_bit(STRIPE_OP_COMPUTE_BLK, &ops_request)) {
> - tx = ops_run_compute5(sh);
> - /* terminate the chain if postxor is not set to be run */
> - if (tx && !test_bit(STRIPE_OP_POSTXOR, &ops_request))
> + if (level < 6)
> + tx = ops_run_compute5(sh);
> + else {
> + if (sh->ops.target2 < 0 || sh->ops.target < 0)
> + tx = ops_run_compute6_1(sh);
> + else
> + tx = ops_run_compute6_2(sh);
> + }
Similarly here.
if (sh->ops.target2 >= 0)
tx = ops_run_compute6_2(sh);
else
tx = ops_run_compute6_1(sh);
And if you add the "count == 1" branch to ops_run_compute6_1,
I think you can use it in place of ops_run_compute5. Then you get
ride of a function, and the above nested ifs becomes a simple:
if (sh->ops.target2 >= 0)
tx = ops_run_compute6_2(sh);
else
tx = ops_run_compute_1(sh);
no need to check the level at all.
Would that be an improvement?
??
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
