Re: [PATCH] md/raid5: write an empty meta-block when creating logsuper-block

Re: [PATCH] md/raid5: write an empty meta-block when creating logsuper-block

[Zhengyuan Liu]

After discussion with my colleague, I think there is still a problem that
may happen very unlikely.The superblock should point to the last meta
block we have written after log reclaim or point to the emtpy meta block
after log recovery, just consider we write some meta block behind the
superblock position and suppose crash happens. If the first meta block we
have written neighboring the superblock position is invalid,  ctx.seq would 
also equal to last_cp_seq+1 after we did a recovery . So the safest way is 
we always write an empty meta block at ctx.pos no matter how much
ctx.req is more than last_cp_seq after we did a recovery. 
How do you think, Shaohua? If it is necessary, I'd revert this patch and
resend one.

------------------ Original ------------------
From:  "Shaohua Li"<shli@kernel.org>;
Date:  Tue, Oct 25, 2016 05:23 AM
To:  "Zhengyuan Liu"<liuzhengyuan@kylinos.cn>;
Cc:  "shli"<shli@fb.com>; "Song Liu"<songliubraving@fb.com>; "linux-raid"<linux-raid@vger.kernel.org>; "liuzhengyuang521"<liuzhengyuang521@gmail.com>;
Subject:  Re: [PATCH] md/raid5: write an empty meta-block when creating logsuper-block
 
On Mon, Oct 24, 2016 at 04:15:59PM +0800, Zhengyuan Liu wrote:
> If superblock points to an invalid meta block, r5l_load_log will set
> create_super with true and create an new superblock, this runtime path
> would always happen if we do no writing I/O to this array since it was
> created. Writing an empty meta block could avoid this unnecessary
> action at the first time we created log superblock.
> 
> Another reason is for the corretness of log recovery. Currently we have
> bellow code to guarantee log revocery to be correct.
> 
>         if (ctx.seq > log->last_cp_seq + 1) {
>                 int ret;
> 
>                 ret = r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq + 10);
>                 if (ret)
>                         return ret;
>                 log->seq = ctx.seq + 11;
>                 log->log_start = r5l_ring_add(log, ctx.pos, BLOCK_SECTORS);
>                 r5l_write_super(log, ctx.pos);
>         } else {
>                 log->log_start = ctx.pos;
>                 log->seq = ctx.seq;
>         }
> 
> If we just created a array with a journal device, log->log_start and
> log->last_checkpoint should all be 0, then we write three meta block
> which are valid except mid one and supposed crash happened. The ctx.seq
> would equal to log->last_cp_seq + 1 and log->log_start would be set to
> position of mid invalid meta block after we did a recovery, this will
> lead to problems which could be avoided with this patch.

This would be very unlikely, but better to fix. Applied, thanks!

Re: [PATCH] md/raid5: write an empty meta-block when creating logsuper-block

[Shaohua Li]

On Tue, Oct 25, 2016 at 08:43:50PM +0800, Zhengyuan Liu wrote:
> After discussion with my colleague, I think there is still a problem that
> may happen very unlikely.The superblock should point to the last meta
> block we have written after log reclaim or point to the emtpy meta block
> after log recovery, just consider we write some meta block behind the
> superblock position and suppose crash happens. If the first meta block we
> have written neighboring the superblock position is invalid,  ctx.seq would 
> also equal to last_cp_seq+1 after we did a recovery . So the safest way is 
> we always write an empty meta block at ctx.pos no matter how much
> ctx.req is more than last_cp_seq after we did a recovery. 
> How do you think, Shaohua? If it is necessary, I'd revert this patch and
> resend one.

I didn't get the point. Could you please elaborate it again?

Thanks,
Shaohua

> 
> ------------------ Original ------------------
> From:  "Shaohua Li"<shli@kernel.org>;
> Date:  Tue, Oct 25, 2016 05:23 AM
> To:  "Zhengyuan Liu"<liuzhengyuan@kylinos.cn>;
> Cc:  "shli"<shli@fb.com>; "Song Liu"<songliubraving@fb.com>; "linux-raid"<linux-raid@vger.kernel.org>; "liuzhengyuang521"<liuzhengyuang521@gmail.com>;
> Subject:  Re: [PATCH] md/raid5: write an empty meta-block when creating logsuper-block
>  
> On Mon, Oct 24, 2016 at 04:15:59PM +0800, Zhengyuan Liu wrote:
> > If superblock points to an invalid meta block, r5l_load_log will set
> > create_super with true and create an new superblock, this runtime path
> > would always happen if we do no writing I/O to this array since it was
> > created. Writing an empty meta block could avoid this unnecessary
> > action at the first time we created log superblock.
> > 
> > Another reason is for the corretness of log recovery. Currently we have
> > bellow code to guarantee log revocery to be correct.
> > 
> >         if (ctx.seq > log->last_cp_seq + 1) {
> >                 int ret;
> > 
> >                 ret = r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq + 10);
> >                 if (ret)
> >                         return ret;
> >                 log->seq = ctx.seq + 11;
> >                 log->log_start = r5l_ring_add(log, ctx.pos, BLOCK_SECTORS);
> >                 r5l_write_super(log, ctx.pos);
> >         } else {
> >                 log->log_start = ctx.pos;
> >                 log->seq = ctx.seq;
> >         }
> > 
> > If we just created a array with a journal device, log->log_start and
> > log->last_checkpoint should all be 0, then we write three meta block
> > which are valid except mid one and supposed crash happened. The ctx.seq
> > would equal to log->last_cp_seq + 1 and log->log_start would be set to
> > position of mid invalid meta block after we did a recovery, this will
> > lead to problems which could be avoided with this patch.
> 
> This would be very unlikely, but better to fix. Applied, thanks!