* [PATCH] md: make the code more readable in the for-loop
@ 2016-05-08 12:56 Tiezhu Yang
2016-05-08 23:03 ` Shaohua Li
2016-05-08 23:33 ` Eyal Lebedinsky
0 siblings, 2 replies; 7+ messages in thread
From: Tiezhu Yang @ 2016-05-08 12:56 UTC (permalink / raw)
To: shli; +Cc: linux-raid, linux-kernel
This patch modifies raid1.c, raid10.c and raid5.c
to make the code more readable in the for-loop
and also fixes the scripts/checkpatch.pl error:
ERROR: trailing statements should be on next line.
Signed-off-by: Tiezhu Yang <kernelpatch@126.com>
---
drivers/md/raid1.c | 6 +++---
drivers/md/raid10.c | 2 +-
drivers/md/raid5.c | 59 +++++++++++++++++++++++++++--------------------------
3 files changed, 34 insertions(+), 33 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index a7f2b9c..760de56 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -109,7 +109,7 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
/*
* Allocate bios : 1 for reading, n-1 for writing
*/
- for (j = pi->raid_disks ; j-- ; ) {
+ for (j = pi->raid_disks - 1; j >= 0; j--) {
bio = bio_kmalloc(gfp_flags, RESYNC_PAGES);
if (!bio)
goto out_free_bio;
@@ -166,7 +166,7 @@ static void r1buf_pool_free(void *__r1_bio, void *data)
struct r1bio *r1bio = __r1_bio;
for (i = 0; i < RESYNC_PAGES; i++)
- for (j = pi->raid_disks; j-- ;) {
+ for (j = pi->raid_disks - 1; j >= 0; j--) {
if (j == 0 ||
r1bio->bios[j]->bi_io_vec[i].bv_page !=
r1bio->bios[0]->bi_io_vec[i].bv_page)
@@ -1976,7 +1976,7 @@ static void process_checks(struct r1bio *r1_bio)
sbio->bi_error = 0;
if (!error) {
- for (j = vcnt; j-- ; ) {
+ for (j = vcnt - 1; j >= 0; j--) {
struct page *p, *s;
p = pbio->bi_io_vec[j].bv_page;
s = sbio->bi_io_vec[j].bv_page;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 84e24e6..52f1e07 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -157,7 +157,7 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
/*
* Allocate bios.
*/
- for (j = nalloc ; j-- ; ) {
+ for (j = nalloc - 1; j >= 0; j--) {
bio = bio_kmalloc(gfp_flags, RESYNC_PAGES);
if (!bio)
goto out_free_bio;
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 4d31b23..db978ab 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -536,7 +536,7 @@ retry:
stripe_set_idx(sector, conf, previous, sh);
sh->state = 0;
- for (i = sh->disks; i--; ) {
+ for (i = sh->disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if (dev->toread || dev->read || dev->towrite || dev->written ||
@@ -890,7 +890,7 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
if (r5l_write_stripe(conf->log, sh) == 0)
return;
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
int rw;
int replace_only = 0;
struct bio *bi, *rbi;
@@ -1175,7 +1175,7 @@ static void ops_complete_biofill(void *stripe_head_ref)
(unsigned long long)sh->sector);
/* clear completed biofills */
- for (i = sh->disks; i--; ) {
+ for (i = sh->disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
/* acknowledge completion of a biofill operation */
@@ -1216,7 +1216,7 @@ static void ops_run_biofill(struct stripe_head *sh)
pr_debug("%s: stripe %llu\n", __func__,
(unsigned long long)sh->sector);
- for (i = sh->disks; i--; ) {
+ for (i = sh->disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if (test_bit(R5_Wantfill, &dev->flags)) {
struct bio *rbi;
@@ -1307,7 +1307,7 @@ ops_run_compute5(struct stripe_head *sh, struct raid5_percpu *percpu)
__func__, (unsigned long long)sh->sector, target);
BUG_ON(!test_bit(R5_Wantcompute, &tgt->flags));
- for (i = disks; i--; )
+ for (i = disks - 1; i >= 0; i--)
if (i != target)
xor_srcs[count++] = sh->dev[i].page;
@@ -1407,7 +1407,7 @@ ops_run_compute6_1(struct stripe_head *sh, struct raid5_percpu *percpu)
} else {
/* Compute any data- or p-drive using XOR */
count = 0;
- for (i = disks; i-- ; ) {
+ for (i = disks - 1; i >= 0; i--) {
if (i == target || i == qd_idx)
continue;
blocks[count++] = sh->dev[i].page;
@@ -1492,7 +1492,7 @@ ops_run_compute6_2(struct stripe_head *sh, struct raid5_percpu *percpu)
data_target = target;
count = 0;
- for (i = disks; i-- ; ) {
+ for (i = disks - 1; i >= 0; i--) {
if (i == data_target || i == qd_idx)
continue;
blocks[count++] = sh->dev[i].page;
@@ -1554,7 +1554,7 @@ ops_run_prexor5(struct stripe_head *sh, struct raid5_percpu *percpu,
pr_debug("%s: stripe %llu\n", __func__,
(unsigned long long)sh->sector);
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
/* Only process blocks that are known to be uptodate */
if (test_bit(R5_Wantdrain, &dev->flags))
@@ -1598,7 +1598,7 @@ ops_run_biodrain(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
pr_debug("%s: stripe %llu\n", __func__,
(unsigned long long)sh->sector);
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev;
struct bio *chosen;
@@ -1663,13 +1663,13 @@ static void ops_complete_reconstruct(void *stripe_head_ref)
pr_debug("%s: stripe %llu\n", __func__,
(unsigned long long)sh->sector);
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
fua |= test_bit(R5_WantFUA, &sh->dev[i].flags);
sync |= test_bit(R5_SyncIO, &sh->dev[i].flags);
discard |= test_bit(R5_Discard, &sh->dev[i].flags);
}
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if (dev->written || i == pd_idx || i == qd_idx) {
@@ -1734,14 +1734,14 @@ again:
if (head_sh->reconstruct_state == reconstruct_state_prexor_drain_run) {
prexor = 1;
xor_dest = xor_srcs[count++] = sh->dev[pd_idx].page;
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if (head_sh->dev[i].written)
xor_srcs[count++] = dev->page;
}
} else {
xor_dest = sh->dev[pd_idx].page;
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if (i != pd_idx)
xor_srcs[count++] = dev->page;
@@ -1872,7 +1872,7 @@ static void ops_run_check_p(struct stripe_head *sh, struct raid5_percpu *percpu)
count = 0;
xor_dest = sh->dev[pd_idx].page;
xor_srcs[count++] = xor_dest;
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
if (i == pd_idx || i == qd_idx)
continue;
xor_srcs[count++] = sh->dev[i].page;
@@ -1970,7 +1970,7 @@ static void raid_run_ops(struct stripe_head *sh, unsigned long ops_request)
}
if (overlap_clear && !sh->batch_head)
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if (test_and_clear_bit(R5_Overlap, &dev->flags))
wake_up(&sh->raid_conf->wait_for_overlap);
@@ -2861,7 +2861,7 @@ schedule_reconstruction(struct stripe_head *sh, struct stripe_head_state *s,
if (rcw) {
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if (dev->towrite) {
@@ -2897,7 +2897,7 @@ schedule_reconstruction(struct stripe_head *sh, struct stripe_head_state *s,
(!(test_bit(R5_UPTODATE, &sh->dev[qd_idx].flags) ||
test_bit(R5_Wantcompute, &sh->dev[qd_idx].flags))));
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if (i == pd_idx || i == qd_idx)
continue;
@@ -3072,7 +3072,7 @@ handle_failed_stripe(struct r5conf *conf, struct stripe_head *sh,
{
int i;
BUG_ON(sh->batch_head);
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct bio *bi;
int bitmap_end = 0;
@@ -3386,7 +3386,7 @@ static int fetch_block(struct stripe_head *sh, struct stripe_head_state *s,
* do it if failed >= 2
*/
int other;
- for (other = disks; other--; ) {
+ for (other = disks - 1; other >= 0; other--) {
if (other == disk_idx)
continue;
if (!test_bit(R5_UPTODATE,
@@ -3433,7 +3433,7 @@ static void handle_stripe_fill(struct stripe_head *sh,
*/
if (!test_bit(STRIPE_COMPUTE_RUN, &sh->state) && !sh->check_state &&
!sh->reconstruct_state)
- for (i = disks; i--; )
+ for (i = disks - 1; i >= 0; i--)
if (fetch_block(sh, s, i, disks))
break;
set_bit(STRIPE_HANDLE, &sh->state);
@@ -3455,7 +3455,7 @@ static void handle_stripe_clean_event(struct r5conf *conf,
struct stripe_head *head_sh = sh;
bool do_endio = false;
- for (i = disks; i--; )
+ for (i = disks - 1; i >= 0; i--)
if (sh->dev[i].written) {
dev = &sh->dev[i];
if (!test_bit(R5_LOCKED, &dev->flags) &&
@@ -3573,7 +3573,8 @@ static void handle_stripe_dirtying(struct r5conf *conf,
pr_debug("force RCW rmw_level=%u, recovery_cp=%llu sh->sector=%llu\n",
conf->rmw_level, (unsigned long long)recovery_cp,
(unsigned long long)sh->sector);
- } else for (i = disks; i--; ) {
+ } else
+ for (i = disks - 1; i >= 0; i--) {
/* would I have to read this buffer for read_modify_write */
struct r5dev *dev = &sh->dev[i];
if ((dev->towrite || i == sh->pd_idx || i == sh->qd_idx) &&
@@ -3606,7 +3607,7 @@ static void handle_stripe_dirtying(struct r5conf *conf,
blk_add_trace_msg(conf->mddev->queue,
"raid5 rmw %llu %d",
(unsigned long long)sh->sector, rmw);
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if ((dev->towrite || i == sh->pd_idx || i == sh->qd_idx) &&
!test_bit(R5_LOCKED, &dev->flags) &&
@@ -3631,7 +3632,7 @@ static void handle_stripe_dirtying(struct r5conf *conf,
/* want reconstruct write, but need to get some data */
int qread =0;
rcw = 0;
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if (!test_bit(R5_OVERWRITE, &dev->flags) &&
i != sh->pd_idx && i != sh->qd_idx &&
@@ -4021,7 +4022,7 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
/* Now to look around and see what can be done */
rcu_read_lock();
- for (i=disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct md_rdev *rdev;
sector_t first_bad;
int bad_sectors;
@@ -4391,7 +4392,7 @@ static void handle_stripe(struct stripe_head *sh)
BUG_ON(sh->qd_idx >= 0 &&
!test_bit(R5_UPTODATE, &sh->dev[sh->qd_idx].flags) &&
!test_bit(R5_Discard, &sh->dev[sh->qd_idx].flags));
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct r5dev *dev = &sh->dev[i];
if (test_bit(R5_LOCKED, &dev->flags) &&
(i == sh->pd_idx || i == sh->qd_idx ||
@@ -4539,7 +4540,7 @@ static void handle_stripe(struct stripe_head *sh)
sh->reconstruct_state = reconstruct_state_idle;
clear_bit(STRIPE_EXPANDING, &sh->state);
- for (i = conf->raid_disks; i--; ) {
+ for (i = conf->raid_disks - 1; i >= 0; i--) {
set_bit(R5_Wantwrite, &sh->dev[i].flags);
set_bit(R5_LOCKED, &sh->dev[i].flags);
s.locked++;
@@ -4579,7 +4580,7 @@ finish:
}
if (s.handle_bad_blocks)
- for (i = disks; i--; ) {
+ for (i = disks - 1; i >= 0; i--) {
struct md_rdev *rdev;
struct r5dev *dev = &sh->dev[i];
if (test_and_clear_bit(R5_WriteError, &dev->flags)) {
@@ -5486,7 +5487,7 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr, int *sk
/* If any of this stripe is beyond the end of the old
* array, then we need to zero those blocks
*/
- for (j=sh->disks; j--;) {
+ for (j = sh->disks - 1; j >= 0; j--) {
sector_t s;
if (j == sh->pd_idx)
continue;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] md: make the code more readable in the for-loop
2016-05-08 12:56 [PATCH] md: make the code more readable in the for-loop Tiezhu Yang
@ 2016-05-08 23:03 ` Shaohua Li
2016-05-08 23:33 ` Eyal Lebedinsky
1 sibling, 0 replies; 7+ messages in thread
From: Shaohua Li @ 2016-05-08 23:03 UTC (permalink / raw)
To: Tiezhu Yang; +Cc: linux-raid, linux-kernel
On Sun, May 08, 2016 at 08:56:55PM +0800, Tiezhu Yang wrote:
> This patch modifies raid1.c, raid10.c and raid5.c
> to make the code more readable in the for-loop
> and also fixes the scripts/checkpatch.pl error:
> ERROR: trailing statements should be on next line.
>
> Signed-off-by: Tiezhu Yang <kernelpatch@126.com>
> @@ -3573,7 +3573,8 @@ static void handle_stripe_dirtying(struct r5conf *conf,
> pr_debug("force RCW rmw_level=%u, recovery_cp=%llu sh->sector=%llu\n",
> conf->rmw_level, (unsigned long long)recovery_cp,
> (unsigned long long)sh->sector);
> - } else for (i = disks; i--; ) {
> + } else
> + for (i = disks - 1; i >= 0; i--) {
> /* would I have to read this buffer for read_modify_write */
> struct r5dev *dev = &sh->dev[i];
> if ((dev->towrite || i == sh->pd_idx || i == sh->qd_idx) &&
Applied. I move the for statement to be in a {} of else statement.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] md: make the code more readable in the for-loop
2016-05-08 12:56 [PATCH] md: make the code more readable in the for-loop Tiezhu Yang
2016-05-08 23:03 ` Shaohua Li
@ 2016-05-08 23:33 ` Eyal Lebedinsky
2016-05-09 3:53 ` Shaohua Li
1 sibling, 1 reply; 7+ messages in thread
From: Eyal Lebedinsky @ 2016-05-08 23:33 UTC (permalink / raw)
To: linux-raid
I do not see how this change makes it clearer. The original form is actually a very common and clear
scan an array in reverse order
Eyal
On 08/05/16 22:56, Tiezhu Yang wrote:
> This patch modifies raid1.c, raid10.c and raid5.c
> to make the code more readable in the for-loop
> and also fixes the scripts/checkpatch.pl error:
> ERROR: trailing statements should be on next line.
>
> Signed-off-by: Tiezhu Yang <kernelpatch@126.com>
> ---
> drivers/md/raid1.c | 6 +++---
> drivers/md/raid10.c | 2 +-
> drivers/md/raid5.c | 59 +++++++++++++++++++++++++++--------------------------
> 3 files changed, 34 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index a7f2b9c..760de56 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -109,7 +109,7 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
> /*
> * Allocate bios : 1 for reading, n-1 for writing
> */
> - for (j = pi->raid_disks ; j-- ; ) {
> + for (j = pi->raid_disks - 1; j >= 0; j--) {
> bio = bio_kmalloc(gfp_flags, RESYNC_PAGES);
> if (!bio)
> goto out_free_bio;
> @@ -166,7 +166,7 @@ static void r1buf_pool_free(void *__r1_bio, void *data)
> struct r1bio *r1bio = __r1_bio;
>
> for (i = 0; i < RESYNC_PAGES; i++)
> - for (j = pi->raid_disks; j-- ;) {
> + for (j = pi->raid_disks - 1; j >= 0; j--) {
> if (j == 0 ||
> r1bio->bios[j]->bi_io_vec[i].bv_page !=
> r1bio->bios[0]->bi_io_vec[i].bv_page)
> @@ -1976,7 +1976,7 @@ static void process_checks(struct r1bio *r1_bio)
> sbio->bi_error = 0;
>
> if (!error) {
> - for (j = vcnt; j-- ; ) {
> + for (j = vcnt - 1; j >= 0; j--) {
> struct page *p, *s;
> p = pbio->bi_io_vec[j].bv_page;
> s = sbio->bi_io_vec[j].bv_page;
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 84e24e6..52f1e07 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -157,7 +157,7 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
> /*
> * Allocate bios.
> */
> - for (j = nalloc ; j-- ; ) {
> + for (j = nalloc - 1; j >= 0; j--) {
> bio = bio_kmalloc(gfp_flags, RESYNC_PAGES);
> if (!bio)
> goto out_free_bio;
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 4d31b23..db978ab 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -536,7 +536,7 @@ retry:
> stripe_set_idx(sector, conf, previous, sh);
> sh->state = 0;
>
> - for (i = sh->disks; i--; ) {
> + for (i = sh->disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
>
> if (dev->toread || dev->read || dev->towrite || dev->written ||
> @@ -890,7 +890,7 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
>
> if (r5l_write_stripe(conf->log, sh) == 0)
> return;
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> int rw;
> int replace_only = 0;
> struct bio *bi, *rbi;
> @@ -1175,7 +1175,7 @@ static void ops_complete_biofill(void *stripe_head_ref)
> (unsigned long long)sh->sector);
>
> /* clear completed biofills */
> - for (i = sh->disks; i--; ) {
> + for (i = sh->disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
>
> /* acknowledge completion of a biofill operation */
> @@ -1216,7 +1216,7 @@ static void ops_run_biofill(struct stripe_head *sh)
> pr_debug("%s: stripe %llu\n", __func__,
> (unsigned long long)sh->sector);
>
> - for (i = sh->disks; i--; ) {
> + for (i = sh->disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
> if (test_bit(R5_Wantfill, &dev->flags)) {
> struct bio *rbi;
> @@ -1307,7 +1307,7 @@ ops_run_compute5(struct stripe_head *sh, struct raid5_percpu *percpu)
> __func__, (unsigned long long)sh->sector, target);
> BUG_ON(!test_bit(R5_Wantcompute, &tgt->flags));
>
> - for (i = disks; i--; )
> + for (i = disks - 1; i >= 0; i--)
> if (i != target)
> xor_srcs[count++] = sh->dev[i].page;
>
> @@ -1407,7 +1407,7 @@ ops_run_compute6_1(struct stripe_head *sh, struct raid5_percpu *percpu)
> } else {
> /* Compute any data- or p-drive using XOR */
> count = 0;
> - for (i = disks; i-- ; ) {
> + for (i = disks - 1; i >= 0; i--) {
> if (i == target || i == qd_idx)
> continue;
> blocks[count++] = sh->dev[i].page;
> @@ -1492,7 +1492,7 @@ ops_run_compute6_2(struct stripe_head *sh, struct raid5_percpu *percpu)
> data_target = target;
>
> count = 0;
> - for (i = disks; i-- ; ) {
> + for (i = disks - 1; i >= 0; i--) {
> if (i == data_target || i == qd_idx)
> continue;
> blocks[count++] = sh->dev[i].page;
> @@ -1554,7 +1554,7 @@ ops_run_prexor5(struct stripe_head *sh, struct raid5_percpu *percpu,
> pr_debug("%s: stripe %llu\n", __func__,
> (unsigned long long)sh->sector);
>
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
> /* Only process blocks that are known to be uptodate */
> if (test_bit(R5_Wantdrain, &dev->flags))
> @@ -1598,7 +1598,7 @@ ops_run_biodrain(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
> pr_debug("%s: stripe %llu\n", __func__,
> (unsigned long long)sh->sector);
>
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev;
> struct bio *chosen;
>
> @@ -1663,13 +1663,13 @@ static void ops_complete_reconstruct(void *stripe_head_ref)
> pr_debug("%s: stripe %llu\n", __func__,
> (unsigned long long)sh->sector);
>
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> fua |= test_bit(R5_WantFUA, &sh->dev[i].flags);
> sync |= test_bit(R5_SyncIO, &sh->dev[i].flags);
> discard |= test_bit(R5_Discard, &sh->dev[i].flags);
> }
>
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
>
> if (dev->written || i == pd_idx || i == qd_idx) {
> @@ -1734,14 +1734,14 @@ again:
> if (head_sh->reconstruct_state == reconstruct_state_prexor_drain_run) {
> prexor = 1;
> xor_dest = xor_srcs[count++] = sh->dev[pd_idx].page;
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
> if (head_sh->dev[i].written)
> xor_srcs[count++] = dev->page;
> }
> } else {
> xor_dest = sh->dev[pd_idx].page;
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
> if (i != pd_idx)
> xor_srcs[count++] = dev->page;
> @@ -1872,7 +1872,7 @@ static void ops_run_check_p(struct stripe_head *sh, struct raid5_percpu *percpu)
> count = 0;
> xor_dest = sh->dev[pd_idx].page;
> xor_srcs[count++] = xor_dest;
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> if (i == pd_idx || i == qd_idx)
> continue;
> xor_srcs[count++] = sh->dev[i].page;
> @@ -1970,7 +1970,7 @@ static void raid_run_ops(struct stripe_head *sh, unsigned long ops_request)
> }
>
> if (overlap_clear && !sh->batch_head)
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
> if (test_and_clear_bit(R5_Overlap, &dev->flags))
> wake_up(&sh->raid_conf->wait_for_overlap);
> @@ -2861,7 +2861,7 @@ schedule_reconstruction(struct stripe_head *sh, struct stripe_head_state *s,
>
> if (rcw) {
>
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
>
> if (dev->towrite) {
> @@ -2897,7 +2897,7 @@ schedule_reconstruction(struct stripe_head *sh, struct stripe_head_state *s,
> (!(test_bit(R5_UPTODATE, &sh->dev[qd_idx].flags) ||
> test_bit(R5_Wantcompute, &sh->dev[qd_idx].flags))));
>
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
> if (i == pd_idx || i == qd_idx)
> continue;
> @@ -3072,7 +3072,7 @@ handle_failed_stripe(struct r5conf *conf, struct stripe_head *sh,
> {
> int i;
> BUG_ON(sh->batch_head);
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct bio *bi;
> int bitmap_end = 0;
>
> @@ -3386,7 +3386,7 @@ static int fetch_block(struct stripe_head *sh, struct stripe_head_state *s,
> * do it if failed >= 2
> */
> int other;
> - for (other = disks; other--; ) {
> + for (other = disks - 1; other >= 0; other--) {
> if (other == disk_idx)
> continue;
> if (!test_bit(R5_UPTODATE,
> @@ -3433,7 +3433,7 @@ static void handle_stripe_fill(struct stripe_head *sh,
> */
> if (!test_bit(STRIPE_COMPUTE_RUN, &sh->state) && !sh->check_state &&
> !sh->reconstruct_state)
> - for (i = disks; i--; )
> + for (i = disks - 1; i >= 0; i--)
> if (fetch_block(sh, s, i, disks))
> break;
> set_bit(STRIPE_HANDLE, &sh->state);
> @@ -3455,7 +3455,7 @@ static void handle_stripe_clean_event(struct r5conf *conf,
> struct stripe_head *head_sh = sh;
> bool do_endio = false;
>
> - for (i = disks; i--; )
> + for (i = disks - 1; i >= 0; i--)
> if (sh->dev[i].written) {
> dev = &sh->dev[i];
> if (!test_bit(R5_LOCKED, &dev->flags) &&
> @@ -3573,7 +3573,8 @@ static void handle_stripe_dirtying(struct r5conf *conf,
> pr_debug("force RCW rmw_level=%u, recovery_cp=%llu sh->sector=%llu\n",
> conf->rmw_level, (unsigned long long)recovery_cp,
> (unsigned long long)sh->sector);
> - } else for (i = disks; i--; ) {
> + } else
> + for (i = disks - 1; i >= 0; i--) {
> /* would I have to read this buffer for read_modify_write */
> struct r5dev *dev = &sh->dev[i];
> if ((dev->towrite || i == sh->pd_idx || i == sh->qd_idx) &&
> @@ -3606,7 +3607,7 @@ static void handle_stripe_dirtying(struct r5conf *conf,
> blk_add_trace_msg(conf->mddev->queue,
> "raid5 rmw %llu %d",
> (unsigned long long)sh->sector, rmw);
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
> if ((dev->towrite || i == sh->pd_idx || i == sh->qd_idx) &&
> !test_bit(R5_LOCKED, &dev->flags) &&
> @@ -3631,7 +3632,7 @@ static void handle_stripe_dirtying(struct r5conf *conf,
> /* want reconstruct write, but need to get some data */
> int qread =0;
> rcw = 0;
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
> if (!test_bit(R5_OVERWRITE, &dev->flags) &&
> i != sh->pd_idx && i != sh->qd_idx &&
> @@ -4021,7 +4022,7 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
>
> /* Now to look around and see what can be done */
> rcu_read_lock();
> - for (i=disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct md_rdev *rdev;
> sector_t first_bad;
> int bad_sectors;
> @@ -4391,7 +4392,7 @@ static void handle_stripe(struct stripe_head *sh)
> BUG_ON(sh->qd_idx >= 0 &&
> !test_bit(R5_UPTODATE, &sh->dev[sh->qd_idx].flags) &&
> !test_bit(R5_Discard, &sh->dev[sh->qd_idx].flags));
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct r5dev *dev = &sh->dev[i];
> if (test_bit(R5_LOCKED, &dev->flags) &&
> (i == sh->pd_idx || i == sh->qd_idx ||
> @@ -4539,7 +4540,7 @@ static void handle_stripe(struct stripe_head *sh)
>
> sh->reconstruct_state = reconstruct_state_idle;
> clear_bit(STRIPE_EXPANDING, &sh->state);
> - for (i = conf->raid_disks; i--; ) {
> + for (i = conf->raid_disks - 1; i >= 0; i--) {
> set_bit(R5_Wantwrite, &sh->dev[i].flags);
> set_bit(R5_LOCKED, &sh->dev[i].flags);
> s.locked++;
> @@ -4579,7 +4580,7 @@ finish:
> }
>
> if (s.handle_bad_blocks)
> - for (i = disks; i--; ) {
> + for (i = disks - 1; i >= 0; i--) {
> struct md_rdev *rdev;
> struct r5dev *dev = &sh->dev[i];
> if (test_and_clear_bit(R5_WriteError, &dev->flags)) {
> @@ -5486,7 +5487,7 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr, int *sk
> /* If any of this stripe is beyond the end of the old
> * array, then we need to zero those blocks
> */
> - for (j=sh->disks; j--;) {
> + for (j = sh->disks - 1; j >= 0; j--) {
> sector_t s;
> if (j == sh->pd_idx)
> continue;
>
--
Eyal Lebedinsky (eyal@eyal.emu.id.au)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] md: make the code more readable in the for-loop
2016-05-08 23:33 ` Eyal Lebedinsky
@ 2016-05-09 3:53 ` Shaohua Li
2016-05-09 6:31 ` NeilBrown
0 siblings, 1 reply; 7+ messages in thread
From: Shaohua Li @ 2016-05-09 3:53 UTC (permalink / raw)
To: Eyal Lebedinsky; +Cc: linux-raid
On Mon, May 09, 2016 at 09:33:39AM +1000, Eyal Lebedinsky wrote:
> I do not see how this change makes it clearer. The original form is actually a very common and clear
> scan an array in reverse order
People always have different opinions for this stuff. When I read '--j' or
'j--', I always think extra time what the value of j is. So for me the change
actually makes the code more readable :)
Thanks,
Shaohua
> On 08/05/16 22:56, Tiezhu Yang wrote:
> >This patch modifies raid1.c, raid10.c and raid5.c
> >to make the code more readable in the for-loop
> >and also fixes the scripts/checkpatch.pl error:
> >ERROR: trailing statements should be on next line.
> >
> >Signed-off-by: Tiezhu Yang <kernelpatch@126.com>
> >---
> > drivers/md/raid1.c | 6 +++---
> > drivers/md/raid10.c | 2 +-
> > drivers/md/raid5.c | 59 +++++++++++++++++++++++++++--------------------------
> > 3 files changed, 34 insertions(+), 33 deletions(-)
> >
> >diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> >index a7f2b9c..760de56 100644
> >--- a/drivers/md/raid1.c
> >+++ b/drivers/md/raid1.c
> >@@ -109,7 +109,7 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
> > /*
> > * Allocate bios : 1 for reading, n-1 for writing
> > */
> >- for (j = pi->raid_disks ; j-- ; ) {
> >+ for (j = pi->raid_disks - 1; j >= 0; j--) {
> > bio = bio_kmalloc(gfp_flags, RESYNC_PAGES);
> > if (!bio)
> > goto out_free_bio;
> >@@ -166,7 +166,7 @@ static void r1buf_pool_free(void *__r1_bio, void *data)
> > struct r1bio *r1bio = __r1_bio;
> >
> > for (i = 0; i < RESYNC_PAGES; i++)
> >- for (j = pi->raid_disks; j-- ;) {
> >+ for (j = pi->raid_disks - 1; j >= 0; j--) {
> > if (j == 0 ||
> > r1bio->bios[j]->bi_io_vec[i].bv_page !=
> > r1bio->bios[0]->bi_io_vec[i].bv_page)
> >@@ -1976,7 +1976,7 @@ static void process_checks(struct r1bio *r1_bio)
> > sbio->bi_error = 0;
> >
> > if (!error) {
> >- for (j = vcnt; j-- ; ) {
> >+ for (j = vcnt - 1; j >= 0; j--) {
> > struct page *p, *s;
> > p = pbio->bi_io_vec[j].bv_page;
> > s = sbio->bi_io_vec[j].bv_page;
> >diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> >index 84e24e6..52f1e07 100644
> >--- a/drivers/md/raid10.c
> >+++ b/drivers/md/raid10.c
> >@@ -157,7 +157,7 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
> > /*
> > * Allocate bios.
> > */
> >- for (j = nalloc ; j-- ; ) {
> >+ for (j = nalloc - 1; j >= 0; j--) {
> > bio = bio_kmalloc(gfp_flags, RESYNC_PAGES);
> > if (!bio)
> > goto out_free_bio;
> >diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> >index 4d31b23..db978ab 100644
> >--- a/drivers/md/raid5.c
> >+++ b/drivers/md/raid5.c
> >@@ -536,7 +536,7 @@ retry:
> > stripe_set_idx(sector, conf, previous, sh);
> > sh->state = 0;
> >
> >- for (i = sh->disks; i--; ) {
> >+ for (i = sh->disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> >
> > if (dev->toread || dev->read || dev->towrite || dev->written ||
> >@@ -890,7 +890,7 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
> >
> > if (r5l_write_stripe(conf->log, sh) == 0)
> > return;
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > int rw;
> > int replace_only = 0;
> > struct bio *bi, *rbi;
> >@@ -1175,7 +1175,7 @@ static void ops_complete_biofill(void *stripe_head_ref)
> > (unsigned long long)sh->sector);
> >
> > /* clear completed biofills */
> >- for (i = sh->disks; i--; ) {
> >+ for (i = sh->disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> >
> > /* acknowledge completion of a biofill operation */
> >@@ -1216,7 +1216,7 @@ static void ops_run_biofill(struct stripe_head *sh)
> > pr_debug("%s: stripe %llu\n", __func__,
> > (unsigned long long)sh->sector);
> >
> >- for (i = sh->disks; i--; ) {
> >+ for (i = sh->disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> > if (test_bit(R5_Wantfill, &dev->flags)) {
> > struct bio *rbi;
> >@@ -1307,7 +1307,7 @@ ops_run_compute5(struct stripe_head *sh, struct raid5_percpu *percpu)
> > __func__, (unsigned long long)sh->sector, target);
> > BUG_ON(!test_bit(R5_Wantcompute, &tgt->flags));
> >
> >- for (i = disks; i--; )
> >+ for (i = disks - 1; i >= 0; i--)
> > if (i != target)
> > xor_srcs[count++] = sh->dev[i].page;
> >
> >@@ -1407,7 +1407,7 @@ ops_run_compute6_1(struct stripe_head *sh, struct raid5_percpu *percpu)
> > } else {
> > /* Compute any data- or p-drive using XOR */
> > count = 0;
> >- for (i = disks; i-- ; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > if (i == target || i == qd_idx)
> > continue;
> > blocks[count++] = sh->dev[i].page;
> >@@ -1492,7 +1492,7 @@ ops_run_compute6_2(struct stripe_head *sh, struct raid5_percpu *percpu)
> > data_target = target;
> >
> > count = 0;
> >- for (i = disks; i-- ; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > if (i == data_target || i == qd_idx)
> > continue;
> > blocks[count++] = sh->dev[i].page;
> >@@ -1554,7 +1554,7 @@ ops_run_prexor5(struct stripe_head *sh, struct raid5_percpu *percpu,
> > pr_debug("%s: stripe %llu\n", __func__,
> > (unsigned long long)sh->sector);
> >
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> > /* Only process blocks that are known to be uptodate */
> > if (test_bit(R5_Wantdrain, &dev->flags))
> >@@ -1598,7 +1598,7 @@ ops_run_biodrain(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
> > pr_debug("%s: stripe %llu\n", __func__,
> > (unsigned long long)sh->sector);
> >
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev;
> > struct bio *chosen;
> >
> >@@ -1663,13 +1663,13 @@ static void ops_complete_reconstruct(void *stripe_head_ref)
> > pr_debug("%s: stripe %llu\n", __func__,
> > (unsigned long long)sh->sector);
> >
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > fua |= test_bit(R5_WantFUA, &sh->dev[i].flags);
> > sync |= test_bit(R5_SyncIO, &sh->dev[i].flags);
> > discard |= test_bit(R5_Discard, &sh->dev[i].flags);
> > }
> >
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> >
> > if (dev->written || i == pd_idx || i == qd_idx) {
> >@@ -1734,14 +1734,14 @@ again:
> > if (head_sh->reconstruct_state == reconstruct_state_prexor_drain_run) {
> > prexor = 1;
> > xor_dest = xor_srcs[count++] = sh->dev[pd_idx].page;
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> > if (head_sh->dev[i].written)
> > xor_srcs[count++] = dev->page;
> > }
> > } else {
> > xor_dest = sh->dev[pd_idx].page;
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> > if (i != pd_idx)
> > xor_srcs[count++] = dev->page;
> >@@ -1872,7 +1872,7 @@ static void ops_run_check_p(struct stripe_head *sh, struct raid5_percpu *percpu)
> > count = 0;
> > xor_dest = sh->dev[pd_idx].page;
> > xor_srcs[count++] = xor_dest;
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > if (i == pd_idx || i == qd_idx)
> > continue;
> > xor_srcs[count++] = sh->dev[i].page;
> >@@ -1970,7 +1970,7 @@ static void raid_run_ops(struct stripe_head *sh, unsigned long ops_request)
> > }
> >
> > if (overlap_clear && !sh->batch_head)
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> > if (test_and_clear_bit(R5_Overlap, &dev->flags))
> > wake_up(&sh->raid_conf->wait_for_overlap);
> >@@ -2861,7 +2861,7 @@ schedule_reconstruction(struct stripe_head *sh, struct stripe_head_state *s,
> >
> > if (rcw) {
> >
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> >
> > if (dev->towrite) {
> >@@ -2897,7 +2897,7 @@ schedule_reconstruction(struct stripe_head *sh, struct stripe_head_state *s,
> > (!(test_bit(R5_UPTODATE, &sh->dev[qd_idx].flags) ||
> > test_bit(R5_Wantcompute, &sh->dev[qd_idx].flags))));
> >
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> > if (i == pd_idx || i == qd_idx)
> > continue;
> >@@ -3072,7 +3072,7 @@ handle_failed_stripe(struct r5conf *conf, struct stripe_head *sh,
> > {
> > int i;
> > BUG_ON(sh->batch_head);
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct bio *bi;
> > int bitmap_end = 0;
> >
> >@@ -3386,7 +3386,7 @@ static int fetch_block(struct stripe_head *sh, struct stripe_head_state *s,
> > * do it if failed >= 2
> > */
> > int other;
> >- for (other = disks; other--; ) {
> >+ for (other = disks - 1; other >= 0; other--) {
> > if (other == disk_idx)
> > continue;
> > if (!test_bit(R5_UPTODATE,
> >@@ -3433,7 +3433,7 @@ static void handle_stripe_fill(struct stripe_head *sh,
> > */
> > if (!test_bit(STRIPE_COMPUTE_RUN, &sh->state) && !sh->check_state &&
> > !sh->reconstruct_state)
> >- for (i = disks; i--; )
> >+ for (i = disks - 1; i >= 0; i--)
> > if (fetch_block(sh, s, i, disks))
> > break;
> > set_bit(STRIPE_HANDLE, &sh->state);
> >@@ -3455,7 +3455,7 @@ static void handle_stripe_clean_event(struct r5conf *conf,
> > struct stripe_head *head_sh = sh;
> > bool do_endio = false;
> >
> >- for (i = disks; i--; )
> >+ for (i = disks - 1; i >= 0; i--)
> > if (sh->dev[i].written) {
> > dev = &sh->dev[i];
> > if (!test_bit(R5_LOCKED, &dev->flags) &&
> >@@ -3573,7 +3573,8 @@ static void handle_stripe_dirtying(struct r5conf *conf,
> > pr_debug("force RCW rmw_level=%u, recovery_cp=%llu sh->sector=%llu\n",
> > conf->rmw_level, (unsigned long long)recovery_cp,
> > (unsigned long long)sh->sector);
> >- } else for (i = disks; i--; ) {
> >+ } else
> >+ for (i = disks - 1; i >= 0; i--) {
> > /* would I have to read this buffer for read_modify_write */
> > struct r5dev *dev = &sh->dev[i];
> > if ((dev->towrite || i == sh->pd_idx || i == sh->qd_idx) &&
> >@@ -3606,7 +3607,7 @@ static void handle_stripe_dirtying(struct r5conf *conf,
> > blk_add_trace_msg(conf->mddev->queue,
> > "raid5 rmw %llu %d",
> > (unsigned long long)sh->sector, rmw);
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> > if ((dev->towrite || i == sh->pd_idx || i == sh->qd_idx) &&
> > !test_bit(R5_LOCKED, &dev->flags) &&
> >@@ -3631,7 +3632,7 @@ static void handle_stripe_dirtying(struct r5conf *conf,
> > /* want reconstruct write, but need to get some data */
> > int qread =0;
> > rcw = 0;
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> > if (!test_bit(R5_OVERWRITE, &dev->flags) &&
> > i != sh->pd_idx && i != sh->qd_idx &&
> >@@ -4021,7 +4022,7 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
> >
> > /* Now to look around and see what can be done */
> > rcu_read_lock();
> >- for (i=disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct md_rdev *rdev;
> > sector_t first_bad;
> > int bad_sectors;
> >@@ -4391,7 +4392,7 @@ static void handle_stripe(struct stripe_head *sh)
> > BUG_ON(sh->qd_idx >= 0 &&
> > !test_bit(R5_UPTODATE, &sh->dev[sh->qd_idx].flags) &&
> > !test_bit(R5_Discard, &sh->dev[sh->qd_idx].flags));
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct r5dev *dev = &sh->dev[i];
> > if (test_bit(R5_LOCKED, &dev->flags) &&
> > (i == sh->pd_idx || i == sh->qd_idx ||
> >@@ -4539,7 +4540,7 @@ static void handle_stripe(struct stripe_head *sh)
> >
> > sh->reconstruct_state = reconstruct_state_idle;
> > clear_bit(STRIPE_EXPANDING, &sh->state);
> >- for (i = conf->raid_disks; i--; ) {
> >+ for (i = conf->raid_disks - 1; i >= 0; i--) {
> > set_bit(R5_Wantwrite, &sh->dev[i].flags);
> > set_bit(R5_LOCKED, &sh->dev[i].flags);
> > s.locked++;
> >@@ -4579,7 +4580,7 @@ finish:
> > }
> >
> > if (s.handle_bad_blocks)
> >- for (i = disks; i--; ) {
> >+ for (i = disks - 1; i >= 0; i--) {
> > struct md_rdev *rdev;
> > struct r5dev *dev = &sh->dev[i];
> > if (test_and_clear_bit(R5_WriteError, &dev->flags)) {
> >@@ -5486,7 +5487,7 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr, int *sk
> > /* If any of this stripe is beyond the end of the old
> > * array, then we need to zero those blocks
> > */
> >- for (j=sh->disks; j--;) {
> >+ for (j = sh->disks - 1; j >= 0; j--) {
> > sector_t s;
> > if (j == sh->pd_idx)
> > continue;
> >
>
> --
> Eyal Lebedinsky (eyal@eyal.emu.id.au)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] md: make the code more readable in the for-loop
2016-05-09 3:53 ` Shaohua Li
@ 2016-05-09 6:31 ` NeilBrown
2016-05-09 20:45 ` Jes Sorensen
0 siblings, 1 reply; 7+ messages in thread
From: NeilBrown @ 2016-05-09 6:31 UTC (permalink / raw)
To: Shaohua Li, Eyal Lebedinsky; +Cc: linux-raid
[-- Attachment #1: Type: text/plain, Size: 710 bytes --]
On Mon, May 09 2016, Shaohua Li wrote:
> On Mon, May 09, 2016 at 09:33:39AM +1000, Eyal Lebedinsky wrote:
>> I do not see how this change makes it clearer. The original form is actually a very common and clear
>> scan an array in reverse order
>
> People always have different opinions for this stuff. When I read '--j' or
> 'j--', I always think extra time what the value of j is. So for me the change
> actually makes the code more readable :)
If the goal is to make the code more readable, you may as well make it:
for (j = 0; i < ->raid_disk; j++)
That will be clearer to most people than the current code, which I don't
think is very much clearer than the original (maybe a little bit).
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] md: make the code more readable in the for-loop
2016-05-09 6:31 ` NeilBrown
@ 2016-05-09 20:45 ` Jes Sorensen
2016-05-11 0:29 ` Shaohua Li
0 siblings, 1 reply; 7+ messages in thread
From: Jes Sorensen @ 2016-05-09 20:45 UTC (permalink / raw)
To: NeilBrown; +Cc: Shaohua Li, Eyal Lebedinsky, linux-raid
NeilBrown <nfbrown@novell.com> writes:
> On Mon, May 09 2016, Shaohua Li wrote:
>
>> On Mon, May 09, 2016 at 09:33:39AM +1000, Eyal Lebedinsky wrote:
>>> I do not see how this change makes it clearer. The original form is
>>> actually a very common and clear
>>> scan an array in reverse order
>>
>> People always have different opinions for this stuff. When I read '--j' or
>> 'j--', I always think extra time what the value of j is. So for me the change
>> actually makes the code more readable :)
>
> If the goal is to make the code more readable, you may as well make it:
>
> for (j = 0; i < ->raid_disk; j++)
>
> That will be clearer to most people than the current code, which I don't
> think is very much clearer than the original (maybe a little bit).
I agree - I had to read the updated version multiple times to convince
myself it was doing the same thing as the original.
Cheers,
Jes
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] md: make the code more readable in the for-loop
2016-05-09 20:45 ` Jes Sorensen
@ 2016-05-11 0:29 ` Shaohua Li
0 siblings, 0 replies; 7+ messages in thread
From: Shaohua Li @ 2016-05-11 0:29 UTC (permalink / raw)
To: Jes Sorensen; +Cc: NeilBrown, Eyal Lebedinsky, linux-raid, kernelpatch
On Mon, May 09, 2016 at 04:45:38PM -0400, Jes Sorensen wrote:
> NeilBrown <nfbrown@novell.com> writes:
> > On Mon, May 09 2016, Shaohua Li wrote:
> >
> >> On Mon, May 09, 2016 at 09:33:39AM +1000, Eyal Lebedinsky wrote:
> >>> I do not see how this change makes it clearer. The original form is
> >>> actually a very common and clear
> >>> scan an array in reverse order
> >>
> >> People always have different opinions for this stuff. When I read '--j' or
> >> 'j--', I always think extra time what the value of j is. So for me the change
> >> actually makes the code more readable :)
> >
> > If the goal is to make the code more readable, you may as well make it:
> >
> > for (j = 0; i < ->raid_disk; j++)
> >
> > That will be clearer to most people than the current code, which I don't
> > think is very much clearer than the original (maybe a little bit).
>
> I agree - I had to read the updated version multiple times to convince
> myself it was doing the same thing as the original.
ok, droped the patch. If Tiezhu Yang is willing to post a new one as neil
suggested, I'll still apply. To be honest I hate the 'j--' and check 'j' stuff.
Thanks,
Shaohua
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-05-11 0:29 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-08 12:56 [PATCH] md: make the code more readable in the for-loop Tiezhu Yang
2016-05-08 23:03 ` Shaohua Li
2016-05-08 23:33 ` Eyal Lebedinsky
2016-05-09 3:53 ` Shaohua Li
2016-05-09 6:31 ` NeilBrown
2016-05-09 20:45 ` Jes Sorensen
2016-05-11 0:29 ` Shaohua Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).