Re: [f2fs-dev] [PATCH v4 2/2] fsck.f2fs: Check write pointer consistency with current segments

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Sep 29, 2019 / 10:09, Chao Yu wrote:
> On 2019/9/25 16:05, Shinichiro Kawasaki wrote:
> > On Sep 21, 2019 / 17:42, Chao Yu wrote:
> >> On 2019/9/18 11:07, Shinichiro Kawasaki wrote:
> >>> Thanks for the comments. I read f2fs code further, and think still the
> >>> SIT vs write pointer check can be implemented and meaningful.
> >>>
> >>> F2fs ensures consistency of SIT using two CP areas, two SIT areas and
> >>> sit_bitmap in CP. These metadata are in the conventional zone that not
> >>> affected by write pointer control logic. My current scope is to ensure
> >>> write pointer control logic correctness for zoned block device. From this
> >>> scope and the f2fs SIT consistency feature, I would like to assume that
> >>> SIT entries built in kernel after f2fs mount is correct for the write
> >>> pointer position check.
> >>
> >> SIT may be broken due to software bug or hardware flaw, we'd better not consider
> >> it as a consistent metadata.
> > 
> > I see. I found that kernel checks SIT valid blocks with check_block_count()
> > function, and if mismatch happens it requests fsck. I guess SIT consistency is
> > not ensured during kernel run, but after fsck run. Is this correct? If so, I
> > think the write pointer consistency check with SIT valid block bitmaps should
> > not be done by kernel, but by fsck after SIT rewrite.
> 
> IMO, consistency check can be done in both kernel and fsck.
> 
> If SIT is corrupted, but kernel doesn't aware of it (check_block_count() doesn't
> report any inconsistency), write_pointer consistency check may give some clues
> for filesystem corruption.

OK, then let's add the check in kernel also.

> 
> > 
> > As for non-open zones (curseg do not point to), the other check is needed: if
> > the zone does not have valid blocks and fsync data, need to reset its write
> > pointer. My understanding is that the valid blocks count comes from SIT. Then
> > this fix also should be done not by kernel but by fsck after SIT rewrite. I
> > think kernel just should do this check at mount time to avoid unaligned write
> > error, and if inconsistency found it should request fsck to recheck and fix.
> 
> Yeah, I agree with this.
> 
> > 
> > I assume kernel can check and fix write pointer inconsistency with cursegs in
> > CP. CP (and SB) have checksum guard and their consistency looks ensured during
> > kernel run.
> > 
> >>
> >>>
> >>> Fsck does additional SIT consistency check in fsck_chk_meta(). It would be
> >>> good to do the write pointer position check at the end of fsck_chk_meta().
> >>
> >> SIT can be changed later? e.g. SIT bitmap says one block address is valid,
> >> however fsck found there is no entry can link to it, then it needs to be
> >> deleted? it may affect write_pointer repair, right?
> >>
> >> So we'd better look into all SIT update cases in fsck.
> > 
> > Ah, yes, when fsck updates SIT, write pointer consistency check should be done
> > after that. I found SIT is rewritten at the end of fsck_verify() by
> > rewrite_sit_area_bitmap() and fix_curseg_info() calls. I guess write pointer
> > consistency check with SIT (and write pointer reset if required) should be
> > done after those function calls to reflect all SIT changes by fsck. I'll try
> > to create a patch with this approach.
> 
> Quota file repair may grab and write the block which can change SIT status,
> please notice that case as well.

Thanks for this comment. This made me think of the write pointer consistency
fix steps in fsck. Now I think fsck needs to check and fix write pointer
consistency twice. First fix is at beginning of fsck, and second at the end.
When fsck writes data in main segments such as quota file repair, inconsistent
write pointer will cause unaligned error. To avoid this, write pointer positions
should be fixed at the beginning of. After fsck updates for SIT or quota files
repair, recheck for write pointer consistency is required again to ensure write
pointer is consistent with those updates.

Now I'm preparing patches for fsck as well as kernel based on the valuable
and detailed e-mail discussion between you and me. Here I summarize my
take-aways through the discussion.

---- F2FS write pointer consistency check and fix by fsck and kernel ----

A) Check write pointer consistency for open zones and non-open zones

A-1) For open zones (cursegs point to), check consistency between the cursegs
     in CP and the write pointers. If the curseg is inconsistent with the write
     pointer in the zone that curseg points to, keep the write pointer as is
     and set new zone(=section) to the curseg.

A-2) For non-open zones (cursegs do not point to), check consistency between
     SIT valid blocks bitmap and the write pointers.
     A-2-i) if the zone does not have valid blocks and fsync data, reset the
            write pointer.
     A-2-ii) if the last valid block recorded in SIT valid blocks bit map is
             beyond the write pointer, just report the inconsistency to notice
	     kernel bug. No fix is available for this error.

B-1) Implement A-1) check & fix feature in the kernel. Fsync data beyond
     curseg's next_blkoff should be kept until fsync data recovery completion.
     Fix write pointer consistency just before the check point trigger for fsync
     data recovery.

B-2) Implement A-2) check feature in the kernel to avoid unaligned write error
     to the zone. Do not implement fix feature in the kernel to avoid the risk
     of SIT break. Just check and if check fails, ask users to run fsck.

C-1) Implement A-1) check & fix feature in fsck. Do check and fix twice, at the
     beginning of the fsck to avoid write error during fsck and at the end of
     fsck to ensure consistency with updates by fsck.
     In case fsync data is left after the curseg position, do not fix the
     inconsistency by fsck. Leave it so that kernel can recover it later.

C-2) Add check fix feature A-2) to fsck. Do check and fix twice in same manner
     as C-1).

--------------------------------------------------------------------------

Once the patches get ready, your review will be appreciated. Thanks!

--
Best Regards,
Shin'ichiro Kawasaki

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel