All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] md/raid5-cache: remove unnecessary function parameters
@ 2016-11-28  8:19 JackieLiu
  2016-11-28  8:19 ` [PATCH 2/4] md/raid5-cache: use ring add to prevent overflow JackieLiu
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: JackieLiu @ 2016-11-28  8:19 UTC (permalink / raw)
  To: songliubraving; +Cc: liuzhengyuan, linux-raid, JackieLiu

The function parameter 'recovery_list' is not used in
body, we can delete it

Signed-off-by: JackieLiu <liuyun01@kylinos.cn>
---
 drivers/md/raid5-cache.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 5f817bd..576c88d 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -67,7 +67,7 @@ static char *r5c_journal_mode_str[] = {"write-through",
 /*
  * raid5 cache state machine
  *
- * With rhe RAID cache, each stripe works in two phases:
+ * With the RAID cache, each stripe works in two phases:
  *	- caching phase
  *	- writing-out phase
  *
@@ -1674,7 +1674,6 @@ r5l_recovery_replay_one_stripe(struct r5conf *conf,
 
 static struct stripe_head *
 r5c_recovery_alloc_stripe(struct r5conf *conf,
-			  struct list_head *recovery_list,
 			  sector_t stripe_sect,
 			  sector_t log_start)
 {
@@ -1855,8 +1854,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
 						stripe_sect);
 
 		if (!sh) {
-			sh = r5c_recovery_alloc_stripe(conf, cached_stripe_list,
-						       stripe_sect, ctx->pos);
+			sh = r5c_recovery_alloc_stripe(conf, stripe_sect, ctx->pos);
 			/*
 			 * cannot get stripe from raid5_get_active_stripe
 			 * try replay some stripes
@@ -1865,8 +1863,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
 				r5c_recovery_replay_stripes(
 					cached_stripe_list, ctx);
 				sh = r5c_recovery_alloc_stripe(
-					conf, cached_stripe_list,
-					stripe_sect, ctx->pos);
+					conf, stripe_sect, ctx->pos);
 			}
 			if (!sh) {
 				pr_debug("md/raid:%s: Increasing stripe cache size to %d to recovery data on journal.\n",
@@ -1875,8 +1872,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
 				raid5_set_cache_size(mddev,
 						     conf->min_nr_stripes * 2);
 				sh = r5c_recovery_alloc_stripe(
-					conf, cached_stripe_list, stripe_sect,
-					ctx->pos);
+					conf, stripe_sect, ctx->pos);
 			}
 			if (!sh) {
 				pr_err("md/raid:%s: Cannot get enough stripes due to memory pressure. Recovery failed.\n",
-- 
2.7.4




^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] md/raid5-cache: use ring add to prevent overflow
  2016-11-28  8:19 [PATCH 1/4] md/raid5-cache: remove unnecessary function parameters JackieLiu
@ 2016-11-28  8:19 ` JackieLiu
  2016-11-29 20:31   ` Song Liu
  2016-11-28  8:19 ` [PATCH 3/4] md/raid5-cache: release the stripe_head at the appropriate location JackieLiu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: JackieLiu @ 2016-11-28  8:19 UTC (permalink / raw)
  To: songliubraving; +Cc: liuzhengyuan, linux-raid, JackieLiu

'write_pos' must be protected with 'r5l_ring_add', or it may overflow

Signed-off-by: JackieLiu <liuyun01@kylinos.cn>
---
 drivers/md/raid5-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 576c88d..6d2b1da 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -2086,7 +2086,7 @@ r5c_recovery_rewrite_data_only_stripes(struct r5l_log *log,
 						     ctx->pos, ctx->seq);
 		mb = page_address(page);
 		offset = le32_to_cpu(mb->meta_size);
-		write_pos = ctx->pos + BLOCK_SECTORS;
+		write_pos = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
 
 		for (i = sh->disks; i--; ) {
 			struct r5dev *dev = &sh->dev[i];
-- 
2.7.4




^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] md/raid5-cache: release the stripe_head at the appropriate location
  2016-11-28  8:19 [PATCH 1/4] md/raid5-cache: remove unnecessary function parameters JackieLiu
  2016-11-28  8:19 ` [PATCH 2/4] md/raid5-cache: use ring add to prevent overflow JackieLiu
@ 2016-11-28  8:19 ` JackieLiu
  2016-11-29 20:33   ` Song Liu
  2016-11-28  8:19 ` [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint JackieLiu
  2016-11-29 20:14 ` [PATCH 1/4] md/raid5-cache: remove unnecessary function parameters Song Liu
  3 siblings, 1 reply; 13+ messages in thread
From: JackieLiu @ 2016-11-28  8:19 UTC (permalink / raw)
  To: songliubraving; +Cc: liuzhengyuan, linux-raid, JackieLiu

If we released the 'stripe_head' in r5c_recovery_flush_log,
ctx->cached_list will both release the data-parity stripes and
data-only stripes, which will become empty.
And we also need to use the data-only stripes in
r5c_recovery_rewrite_data_only_stripes, so we should wait util rewrite
data-only stripes is done before releasing them.

Reviewed-by: Zhengyuan Liu <liuzhengyuan@kylinos.cn>
Signed-off-by: JackieLiu <liuyun01@kylinos.cn>
---
 drivers/md/raid5-cache.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 6d2b1da..9e72180 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -1950,7 +1950,7 @@ static void r5c_recovery_load_one_stripe(struct r5l_log *log,
 static int r5c_recovery_flush_log(struct r5l_log *log,
 				  struct r5l_recovery_ctx *ctx)
 {
-	struct stripe_head *sh, *next;
+	struct stripe_head *sh;
 	int ret = 0;
 
 	/* scan through the log */
@@ -1979,11 +1979,9 @@ static int r5c_recovery_flush_log(struct r5l_log *log,
 	r5c_recovery_replay_stripes(&ctx->cached_list, ctx);
 
 	/* load data-only stripes to stripe cache */
-	list_for_each_entry_safe(sh, next, &ctx->cached_list, lru) {
+	list_for_each_entry(sh, &ctx->cached_list, lru) {
 		WARN_ON(!test_bit(STRIPE_R5C_CACHING, &sh->state));
 		r5c_recovery_load_one_stripe(log, sh);
-		list_del_init(&sh->lru);
-		raid5_release_stripe(sh);
 		ctx->data_only_stripes++;
 	}
 
@@ -2063,7 +2061,7 @@ static int
 r5c_recovery_rewrite_data_only_stripes(struct r5l_log *log,
 				       struct r5l_recovery_ctx *ctx)
 {
-	struct stripe_head *sh;
+	struct stripe_head *sh, *next;
 	struct mddev *mddev = log->rdev->mddev;
 	struct page *page;
 
@@ -2075,7 +2073,7 @@ r5c_recovery_rewrite_data_only_stripes(struct r5l_log *log,
 	}
 
 	ctx->seq += 10;
-	list_for_each_entry(sh, &ctx->cached_list, lru) {
+	list_for_each_entry_safe(sh, next, &ctx->cached_list, lru) {
 		struct r5l_meta_block *mb;
 		int i;
 		int offset;
@@ -2121,6 +2119,9 @@ r5c_recovery_rewrite_data_only_stripes(struct r5l_log *log,
 		sh->log_start = ctx->pos;
 		ctx->pos = write_pos;
 		ctx->seq += 1;
+
+		list_del_init(&sh->lru);
+		raid5_release_stripe(sh);
 	}
 	__free_page(page);
 	return 0;
-- 
2.7.4




^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint
  2016-11-28  8:19 [PATCH 1/4] md/raid5-cache: remove unnecessary function parameters JackieLiu
  2016-11-28  8:19 ` [PATCH 2/4] md/raid5-cache: use ring add to prevent overflow JackieLiu
  2016-11-28  8:19 ` [PATCH 3/4] md/raid5-cache: release the stripe_head at the appropriate location JackieLiu
@ 2016-11-28  8:19 ` JackieLiu
  2016-11-29 20:33   ` Song Liu
  2016-11-29 22:31   ` Shaohua Li
  2016-11-29 20:14 ` [PATCH 1/4] md/raid5-cache: remove unnecessary function parameters Song Liu
  3 siblings, 2 replies; 13+ messages in thread
From: JackieLiu @ 2016-11-28  8:19 UTC (permalink / raw)
  To: songliubraving; +Cc: liuzhengyuan, linux-raid, JackieLiu

When recovery is complete, we write an empty block and record his
position first, then make the data-only stripes rewritten done,
the location of the empty block as the last checkpoint position
to write into the super block. And we should update last_checkpoint
to this empty block position.

------------------------------------------------------------------
|  old log       | empty block | data only stripes | invalid log |
------------------------------------------------------------------
^                ^                                 ^
|                |- log->last_checkpoint           |- log->log_start
|                |- log->last_cp_seq               |- log->next_checkpoint
|- log->seq=n    |- log->seq=10+n

At the same time, if there is no data-only stripes, this scene may appear,
| meta1 | meta2 | meta3 |
meta 1 is valid, meta 2 is invalid. meta 3 could be valid. so we should 
The solution is we create a new meta in meta2 with its seq == meta1's 
seq + 10 and let superblock points to meta2. 

Reviewed-by: Zhengyuan Liu <liuzhengyuan@kylinos.cn>
Signed-off-by: JackieLiu <liuyun01@kylinos.cn>
---
 drivers/md/raid5-cache.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 9e72180..20594f7 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -2072,7 +2072,6 @@ r5c_recovery_rewrite_data_only_stripes(struct r5l_log *log,
 		return -ENOMEM;
 	}
 
-	ctx->seq += 10;
 	list_for_each_entry_safe(sh, next, &ctx->cached_list, lru) {
 		struct r5l_meta_block *mb;
 		int i;
@@ -2132,6 +2131,7 @@ static int r5l_recovery_log(struct r5l_log *log)
 	struct mddev *mddev = log->rdev->mddev;
 	struct r5l_recovery_ctx ctx;
 	int ret;
+	sector_t pos;
 
 	ctx.pos = log->last_checkpoint;
 	ctx.seq = log->last_cp_seq;
@@ -2149,6 +2149,10 @@ static int r5l_recovery_log(struct r5l_log *log)
 	if (ret)
 		return ret;
 
+	pos = ctx.pos;
+	r5l_log_write_empty_meta_block(log, ctx.pos, (ctx.seq += 10));
+	ctx.pos = r5l_ring_add(log, ctx.pos, BLOCK_SECTORS);
+
 	if ((ctx.data_only_stripes == 0) && (ctx.data_parity_stripes == 0))
 		pr_debug("md/raid:%s: starting from clean shutdown\n",
 			 mdname(mddev));
@@ -2167,9 +2171,9 @@ static int r5l_recovery_log(struct r5l_log *log)
 
 	log->log_start = ctx.pos;
 	log->next_checkpoint = ctx.pos;
+	log->last_checkpoint = pos;
 	log->seq = ctx.seq;
-	r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq);
-	r5l_write_super(log, ctx.pos);
+	r5l_write_super(log, pos);
 	return 0;
 }
 
-- 
2.7.4




^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] md/raid5-cache: remove unnecessary function parameters
  2016-11-28  8:19 [PATCH 1/4] md/raid5-cache: remove unnecessary function parameters JackieLiu
                   ` (2 preceding siblings ...)
  2016-11-28  8:19 ` [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint JackieLiu
@ 2016-11-29 20:14 ` Song Liu
  3 siblings, 0 replies; 13+ messages in thread
From: Song Liu @ 2016-11-29 20:14 UTC (permalink / raw)
  To: linux-raid; +Cc: Song Liu

Reviewed-by: Song Liu <songliubraving@fb.com>



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] md/raid5-cache: use ring add to prevent overflow
  2016-11-28  8:19 ` [PATCH 2/4] md/raid5-cache: use ring add to prevent overflow JackieLiu
@ 2016-11-29 20:31   ` Song Liu
  0 siblings, 0 replies; 13+ messages in thread
From: Song Liu @ 2016-11-29 20:31 UTC (permalink / raw)
  To: linux-raid; +Cc: Song Liu

Reviewed-by: Song Liu <songliubraving@fb.com>



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] md/raid5-cache: release the stripe_head at the appropriate location
  2016-11-28  8:19 ` [PATCH 3/4] md/raid5-cache: release the stripe_head at the appropriate location JackieLiu
@ 2016-11-29 20:33   ` Song Liu
  0 siblings, 0 replies; 13+ messages in thread
From: Song Liu @ 2016-11-29 20:33 UTC (permalink / raw)
  To: linux-raid; +Cc: Song Liu

Reviewed-by: Song Liu <songliubraving@fb.com>



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint
  2016-11-28  8:19 ` [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint JackieLiu
@ 2016-11-29 20:33   ` Song Liu
  2016-11-29 22:31   ` Shaohua Li
  1 sibling, 0 replies; 13+ messages in thread
From: Song Liu @ 2016-11-29 20:33 UTC (permalink / raw)
  To: linux-raid; +Cc: Song Liu

Reviewed-by: Song Liu <songliubraving@fb.com>



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint
  2016-11-28  8:19 ` [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint JackieLiu
  2016-11-29 20:33   ` Song Liu
@ 2016-11-29 22:31   ` Shaohua Li
  2016-11-30  4:03     ` JackieLiu
  1 sibling, 1 reply; 13+ messages in thread
From: Shaohua Li @ 2016-11-29 22:31 UTC (permalink / raw)
  To: JackieLiu; +Cc: songliubraving, liuzhengyuan, linux-raid

On Mon, Nov 28, 2016 at 04:19:21PM +0800, JackieLiu wrote:
> When recovery is complete, we write an empty block and record his
> position first, then make the data-only stripes rewritten done,
> the location of the empty block as the last checkpoint position
> to write into the super block. And we should update last_checkpoint
> to this empty block position.
> 
> ------------------------------------------------------------------
> |  old log       | empty block | data only stripes | invalid log |
> ------------------------------------------------------------------
> ^                ^                                 ^
> |                |- log->last_checkpoint           |- log->log_start
> |                |- log->last_cp_seq               |- log->next_checkpoint
> |- log->seq=n    |- log->seq=10+n
> 
> At the same time, if there is no data-only stripes, this scene may appear,
> | meta1 | meta2 | meta3 |
> meta 1 is valid, meta 2 is invalid. meta 3 could be valid. so we should 
> The solution is we create a new meta in meta2 with its seq == meta1's 
> seq + 10 and let superblock points to meta2. 
> 
> Reviewed-by: Zhengyuan Liu <liuzhengyuan@kylinos.cn>
> Signed-off-by: JackieLiu <liuyun01@kylinos.cn>
> ---
>  drivers/md/raid5-cache.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 9e72180..20594f7 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -2072,7 +2072,6 @@ r5c_recovery_rewrite_data_only_stripes(struct r5l_log *log,
>  		return -ENOMEM;
>  	}
>  
> -	ctx->seq += 10;
>  	list_for_each_entry_safe(sh, next, &ctx->cached_list, lru) {
>  		struct r5l_meta_block *mb;
>  		int i;
> @@ -2132,6 +2131,7 @@ static int r5l_recovery_log(struct r5l_log *log)
>  	struct mddev *mddev = log->rdev->mddev;
>  	struct r5l_recovery_ctx ctx;
>  	int ret;
> +	sector_t pos;
>  
>  	ctx.pos = log->last_checkpoint;
>  	ctx.seq = log->last_cp_seq;
> @@ -2149,6 +2149,10 @@ static int r5l_recovery_log(struct r5l_log *log)
>  	if (ret)
>  		return ret;
>  
> +	pos = ctx.pos;
> +	r5l_log_write_empty_meta_block(log, ctx.pos, (ctx.seq += 10));

hmm, move the ctx.seq += 10 out please
> +	ctx.pos = r5l_ring_add(log, ctx.pos, BLOCK_SECTORS);
> +
>  	if ((ctx.data_only_stripes == 0) && (ctx.data_parity_stripes == 0))
>  		pr_debug("md/raid:%s: starting from clean shutdown\n",
>  			 mdname(mddev));
> @@ -2167,9 +2171,9 @@ static int r5l_recovery_log(struct r5l_log *log)
>  
>  	log->log_start = ctx.pos;
>  	log->next_checkpoint = ctx.pos;
> +	log->last_checkpoint = pos;
>  	log->seq = ctx.seq;
> -	r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq);
> -	r5l_write_super(log, ctx.pos);
> +	r5l_write_super(log, pos);
>  	return 0;
>  }

Applied the first 3 patches in the series. This one looks good too, but why we
always create the empty meta block? It's only required when we don't rewrite
the data. Eg, the data_only_stripes == 0.

Thanks,
Shaohua

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint
  2016-11-29 22:31   ` Shaohua Li
@ 2016-11-30  4:03     ` JackieLiu
  2016-12-02 20:10       ` Shaohua Li
  0 siblings, 1 reply; 13+ messages in thread
From: JackieLiu @ 2016-11-30  4:03 UTC (permalink / raw)
  To: Shaohua Li; +Cc: songliubraving, 刘正元, linux-raid


> 在 2016年11月30日,06:31,Shaohua Li <shli@kernel.org> 写道:
> 
> On Mon, Nov 28, 2016 at 04:19:21PM +0800, JackieLiu wrote:
>> When recovery is complete, we write an empty block and record his
>> position first, then make the data-only stripes rewritten done,
>> the location of the empty block as the last checkpoint position
>> to write into the super block. And we should update last_checkpoint
>> to this empty block position.
>> ...
>> +	pos = ctx.pos;
>> +	r5l_log_write_empty_meta_block(log, ctx.pos, (ctx.seq += 10));
> 
> hmm, move the ctx.seq += 10 out please

yep, if this patch is OK,I will resend an appropriate patch.

>> +	ctx.pos = r5l_ring_add(log, ctx.pos, BLOCK_SECTORS);
>> +
>> 	if ((ctx.data_only_stripes == 0) && (ctx.data_parity_stripes == 0))
>> 		pr_debug("md/raid:%s: starting from clean shutdown\n",
>> 			 mdname(mddev));
>> @@ -2167,9 +2171,9 @@ static int r5l_recovery_log(struct r5l_log *log)
>> 
>> 	log->log_start = ctx.pos;
>> 	log->next_checkpoint = ctx.pos;
>> +	log->last_checkpoint = pos;
>> 	log->seq = ctx.seq;
>> -	r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq);
>> -	r5l_write_super(log, ctx.pos);
>> +	r5l_write_super(log, pos);
>> 	return 0;
>> }
> 
> Applied the first 3 patches in the series. This one looks good too, but why we
> always create the empty meta block? It's only required when we don't rewrite
> the data. Eg, the data_only_stripes == 0.
> 
> Thanks,
> Shaohua

As you said, when data_only_stripes != 0, does not need to write an empty 
meta block, but we need to calculate the position of the first list member and
save it.  at the same time, when data_only_stripes == 0, then you need to write
an empty block, and let the super block pointing to him; In any case, Since 
there is a possibility that invalid blocks are connected to valid blocks, we still 
need to add 10 to them.

In my option, if this empty block has been added, we just let the super block 
pointing to him, everything is OK now, the code is more clean, and the logic 
is clear.

Thanks
Jackie





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint
  2016-11-30  4:03     ` JackieLiu
@ 2016-12-02 20:10       ` Shaohua Li
  2016-12-05  3:58         ` JackieLiu
  0 siblings, 1 reply; 13+ messages in thread
From: Shaohua Li @ 2016-12-02 20:10 UTC (permalink / raw)
  To: JackieLiu; +Cc: songliubraving, 刘正元, linux-raid

On Wed, Nov 30, 2016 at 12:03:14PM +0800, JackieLiu wrote:
> 
> > 在 2016年11月30日,06:31,Shaohua Li <shli@kernel.org> 写道:
> > 
> > On Mon, Nov 28, 2016 at 04:19:21PM +0800, JackieLiu wrote:
> >> When recovery is complete, we write an empty block and record his
> >> position first, then make the data-only stripes rewritten done,
> >> the location of the empty block as the last checkpoint position
> >> to write into the super block. And we should update last_checkpoint
> >> to this empty block position.
> >> ...
> >> +	pos = ctx.pos;
> >> +	r5l_log_write_empty_meta_block(log, ctx.pos, (ctx.seq += 10));
> > 
> > hmm, move the ctx.seq += 10 out please
> 
> yep, if this patch is OK,I will resend an appropriate patch.
> 
> >> +	ctx.pos = r5l_ring_add(log, ctx.pos, BLOCK_SECTORS);
> >> +
> >> 	if ((ctx.data_only_stripes == 0) && (ctx.data_parity_stripes == 0))
> >> 		pr_debug("md/raid:%s: starting from clean shutdown\n",
> >> 			 mdname(mddev));
> >> @@ -2167,9 +2171,9 @@ static int r5l_recovery_log(struct r5l_log *log)
> >> 
> >> 	log->log_start = ctx.pos;
> >> 	log->next_checkpoint = ctx.pos;
> >> +	log->last_checkpoint = pos;
> >> 	log->seq = ctx.seq;
> >> -	r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq);
> >> -	r5l_write_super(log, ctx.pos);
> >> +	r5l_write_super(log, pos);
> >> 	return 0;
> >> }
> > 
> > Applied the first 3 patches in the series. This one looks good too, but why we
> > always create the empty meta block? It's only required when we don't rewrite
> > the data. Eg, the data_only_stripes == 0.
> > 
> > Thanks,
> > Shaohua
> 
> As you said, when data_only_stripes != 0, does not need to write an empty 
> meta block, but we need to calculate the position of the first list member and
> save it.  at the same time, when data_only_stripes == 0, then you need to write
> an empty block, and let the super block pointing to him; In any case, Since 
> there is a possibility that invalid blocks are connected to valid blocks, we still 
> need to add 10 to them.
> 
> In my option, if this empty block has been added, we just let the super block 
> pointing to him, everything is OK now, the code is more clean, and the logic 
> is clear.

I'd prefer not writting the empty block unconditionally. It's unnecessary and
confusing reading the code. Don't think a 'if () write_empty_block' makes the
logic complecated.

Thanks,
Shaohua

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint
  2016-12-02 20:10       ` Shaohua Li
@ 2016-12-05  3:58         ` JackieLiu
  2016-12-06  1:00           ` Shaohua Li
  0 siblings, 1 reply; 13+ messages in thread
From: JackieLiu @ 2016-12-05  3:58 UTC (permalink / raw)
  To: Shaohua Li; +Cc: songliubraving, 刘正元, linux-raid


> 在 2016年12月3日,04:10,Shaohua Li <shli@kernel.org> 写道:
> 
> confusing reading the code. Don't think a 'if () write_empty_block' makes the

Hi ShaoHua. 

I rewrote this part of the code, now without data_only_stripes, we write an empty block here,
with data_only_stripes, mark the first cache block as last_checkpoint.

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 9e72180..5697724 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -2072,7 +2072,6 @@ r5c_recovery_rewrite_data_only_stripes(struct r5l_log *log,
                return -ENOMEM;
        }

-       ctx->seq += 10;
        list_for_each_entry_safe(sh, next, &ctx->cached_list, lru) {
                struct r5l_meta_block *mb;
                int i;
@@ -2132,6 +2131,8 @@ static int r5l_recovery_log(struct r5l_log *log)
        struct mddev *mddev = log->rdev->mddev;
        struct r5l_recovery_ctx ctx;
        int ret;
+       sector_t pos;
+       struct stripe_head *sh;

        ctx.pos = log->last_checkpoint;
        ctx.seq = log->last_cp_seq;
@@ -2149,6 +2150,18 @@ static int r5l_recovery_log(struct r5l_log *log)
        if (ret)
                return ret;

+       pos = ctx.pos;
+       ctx.seq += 10;
+
+       if (ctx.data_only_stripes == 0) {
+               log->next_checkpoint = ctx.pos;
+               r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq++);
+               ctx.pos = r5l_ring_add(log, ctx.pos, BLOCK_SECTORS);
+       } else {
+               sh = list_last_entry(&ctx.cached_list, struct stripe_head, lru);
+               log->next_checkpoint = sh->log_start;
+       }
+
        if ((ctx.data_only_stripes == 0) && (ctx.data_parity_stripes == 0))
                pr_debug("md/raid:%s: starting from clean shutdown\n",
                         mdname(mddev));
@@ -2166,10 +2179,9 @@ static int r5l_recovery_log(struct r5l_log *log)
        }

        log->log_start = ctx.pos;
-       log->next_checkpoint = ctx.pos;
        log->seq = ctx.seq;
-       r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq);
-       r5l_write_super(log, ctx.pos);
+       log->last_checkpoint = pos;
+       r5l_write_super(log, pos);
        return 0;
 }


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint
  2016-12-05  3:58         ` JackieLiu
@ 2016-12-06  1:00           ` Shaohua Li
  0 siblings, 0 replies; 13+ messages in thread
From: Shaohua Li @ 2016-12-06  1:00 UTC (permalink / raw)
  To: JackieLiu; +Cc: songliubraving, 刘正元, linux-raid

On Mon, Dec 05, 2016 at 11:58:53AM +0800, JackieLiu wrote:
> 
> > 在 2016年12月3日,04:10,Shaohua Li <shli@kernel.org> 写道:
> > 
> > confusing reading the code. Don't think a 'if () write_empty_block' makes the
> 
> Hi ShaoHua. 
> 
> I rewrote this part of the code, now without data_only_stripes, we write an empty block here,
> with data_only_stripes, mark the first cache block as last_checkpoint.

Next time please send formal patch and fix your mail client (it skews up tabs).
I manually applied this patch this time. Thanks!

Best Regards,
Shaohua

 
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 9e72180..5697724 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -2072,7 +2072,6 @@ r5c_recovery_rewrite_data_only_stripes(struct r5l_log *log,
>                 return -ENOMEM;
>         }
> 
> -       ctx->seq += 10;
>         list_for_each_entry_safe(sh, next, &ctx->cached_list, lru) {
>                 struct r5l_meta_block *mb;
>                 int i;
> @@ -2132,6 +2131,8 @@ static int r5l_recovery_log(struct r5l_log *log)
>         struct mddev *mddev = log->rdev->mddev;
>         struct r5l_recovery_ctx ctx;
>         int ret;
> +       sector_t pos;
> +       struct stripe_head *sh;
> 
>         ctx.pos = log->last_checkpoint;
>         ctx.seq = log->last_cp_seq;
> @@ -2149,6 +2150,18 @@ static int r5l_recovery_log(struct r5l_log *log)
>         if (ret)
>                 return ret;
> 
> +       pos = ctx.pos;
> +       ctx.seq += 10;
> +
> +       if (ctx.data_only_stripes == 0) {
> +               log->next_checkpoint = ctx.pos;
> +               r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq++);
> +               ctx.pos = r5l_ring_add(log, ctx.pos, BLOCK_SECTORS);
> +       } else {
> +               sh = list_last_entry(&ctx.cached_list, struct stripe_head, lru);
> +               log->next_checkpoint = sh->log_start;
> +       }
> +
>         if ((ctx.data_only_stripes == 0) && (ctx.data_parity_stripes == 0))
>                 pr_debug("md/raid:%s: starting from clean shutdown\n",
>                          mdname(mddev));
> @@ -2166,10 +2179,9 @@ static int r5l_recovery_log(struct r5l_log *log)
>         }
> 
>         log->log_start = ctx.pos;
> -       log->next_checkpoint = ctx.pos;
>         log->seq = ctx.seq;
> -       r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq);
> -       r5l_write_super(log, ctx.pos);
> +       log->last_checkpoint = pos;
> +       r5l_write_super(log, pos);
>         return 0;
>  }
> 
> --
> 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] 13+ messages in thread

end of thread, other threads:[~2016-12-06  1:00 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-28  8:19 [PATCH 1/4] md/raid5-cache: remove unnecessary function parameters JackieLiu
2016-11-28  8:19 ` [PATCH 2/4] md/raid5-cache: use ring add to prevent overflow JackieLiu
2016-11-29 20:31   ` Song Liu
2016-11-28  8:19 ` [PATCH 3/4] md/raid5-cache: release the stripe_head at the appropriate location JackieLiu
2016-11-29 20:33   ` Song Liu
2016-11-28  8:19 ` [PATCH 4/4] md/raid5-cache: adjust the write position of the empty block and mark it as a checkpoint JackieLiu
2016-11-29 20:33   ` Song Liu
2016-11-29 22:31   ` Shaohua Li
2016-11-30  4:03     ` JackieLiu
2016-12-02 20:10       ` Shaohua Li
2016-12-05  3:58         ` JackieLiu
2016-12-06  1:00           ` Shaohua Li
2016-11-29 20:14 ` [PATCH 1/4] md/raid5-cache: remove unnecessary function parameters Song Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.