Re: [f2fs-dev] [PATCH] f2fs: set prefree as free segments after clear prefree segments

[Chao Yu]

Hi Yangtao,

On 2021/4/23 10:40, 李扬韬 wrote:
> HI Chao,
>   
> > > For now, when do_checkpoint fails, the prefree bitmap is not cleared,
> > > but these segments are already in the free state. If these segments
> > > are used, the segments in use will be reset to the free state when
> > > f2fs_clear_prefree_segments is called next time.
> > >
> > > So move set_prefree_as_free_segments after clear_prefree_segments.
> >
> > That's not correct.
> >
> > /*
> >   * Should call f2fs_clear_prefree_segments after checkpoint is done.
> >   */
> > static void set_prefree_as_free_segments(struct f2fs_sb_info *sbi)
> >
> > Comments above set_prefree_as_free_segments() should have told you
> > the rule, otherwise if checkpoint failed, valid data in last valid
> > checkpoint could be corrupted after segment reuse.

Oh, it seems I misunderstood what the patch did, please ignore above
comments.

> For do_checkpoint sucess:
>
> f2fs_write_checkpoint
> ->f2fs_flush_sit_entries
>      ->set_prefree_as_free_segments
> ->do_checkpoint
> ->f2fs_clear_prefree_segments
>
>
> Calling set_prefree_as_free_segments when do_checkpoint fails,
> seems to be incorrect. I think clear free bitmap should be after
> clear prefree bitmap.
>
> For do_checkpoint fail:
>
> f2fs_write_checkpoint
> ->f2fs_flush_sit_entries
>      ->set_prefree_as_free_segments
> ->do_checkpoint
> ->f2fs_release_discard_addrs
>
> The prefree bitmap is not cleared, but free bitmap is cleared，which means
> we can use these segments that are marked as free. When the free segments
> is used, the next f2fs_clear_prefree_segments will mark prefree as free again,
> causing some problem.

Okay, I can understand that.

But the problem here is, after applying this patch, successful checkpoint
may record wrong free_segment value:

- f2fs_write_checkpoint
 - f2fs_flush_sit_entries
 - do_checkpoint
  - ckpt->free_segment_count = cpu_to_le32(free_segments(sbi));
 - f2fs_clear_prefree_segments
  - __set_test_and_free
   - free_i->free_segments++;

I guess for the case of do_checkpoint() fails, maybe we can reset
free segment to prefree status.

Thoughts?

Thanks,

> With this patch, for do_checkpoint fail:
>
> f2fs_write_checkpoint
> ->f2fs_flush_sit_entries
> ->do_checkpoint
> ->f2fs_release_discard_addrs
>
> At this time, we did not mark prefree as free segments, so these segments will not be used.
>
> Thx