Re: [PATCH 1/1] f2fs: fix fallocate failed under pinned block situation

[Wu Bo]

On 2023/11/7 22:39, Chao Yu wrote:
> On 2023/10/30 17:40, Wu Bo wrote:
> > If GC victim has pinned block, it can't be recycled.
> > And if GC is foreground running, after many failure try, the pinned file
> > is expected to be clear pin flag. To enable the section be recycled.
> >
> > But when fallocate trigger FG_GC, GC can never recycle the pinned
> > section. Because GC will go to stop before the failure try meet the 
> > threshold:
> >     if (has_enough_free_secs(sbi, sec_freed, 0)) {
> >         if (!gc_control->no_bg_gc &&
> >             total_sec_freed < gc_control->nr_free_secs)
> >             goto go_gc_more;
> >         goto stop;
> >     }
> >
> > So when fallocate trigger FG_GC, at least recycle one.
>
> Hmm... it may break pinfile's semantics at least on one pinned file?
> In this case, I prefer to fail fallocate() rather than unpinning file,
> in order to avoid leaving invalid LBA references of unpinned file held
> by userspace.

As f2fs designed now, FG_GC is able to unpin the pinned file.

fallocate() triggered FG_GC, but can't recycle space.  It breaks the 
design logic of FG_GC.

This issue is happened in Android OTA scenario.  fallocate() always 
return failure cause OTA fail.

 And this commit changed previous behavior of fallocate():

Commit 2e42b7f817ac ("f2fs: stop allocating pinned sections if EAGAIN 
happens")

Before this commit, if fallocate() meet this situation, it will trigger 
FG_GC to recycle pinned space finally.

FG_GC is expected to recycle pinned space when there is no more free 
space.  And this is the right time to do it when fallocate() need free 
space.

It is weird when f2fs shows enough spare space but can't fallocate(). So 
I think it should be fixed.

> Thoughts?
>
> Thanks,
>
> > This issue can be reproduced by filling f2fs space as following layout.
> > Every segment has one block is pinned:
> > +-+-+-+-+-+-+-----+-+
> > | | |p| | | | ... | | seg_n
> > +-+-+-+-+-+-+-----+-+
> > +-+-+-+-+-+-+-----+-+
> > | | |p| | | | ... | | seg_n+1
> > +-+-+-+-+-+-+-----+-+
> > ...
> > +-+-+-+-+-+-+-----+-+
> > | | |p| | | | ... | | seg_n+k
> > +-+-+-+-+-+-+-----+-+
> >
> > And following are steps to reproduce this issue:
> > dd if=/dev/zero of=./f2fs_pin.img bs=2M count=1024
> > mkfs.f2fs f2fs_pin.img
> > mkdir f2fs
> > mount f2fs_pin.img ./f2fs
> > cd f2fs
> > dd if=/dev/zero of=./large_padding bs=1M count=1760
> > ./pin_filling.sh
> > rm padding*
> > sync
> > touch fallocate_40m
> > f2fs_io pinfile set fallocate_40m
> > fallocate -l 41943040 fallocate_40m
> >
> > fallocate always fail with EAGAIN even there has enough free space.
> >
> > 'pin_filling.sh' is:
> > count=1
> > while :
> > do
> >      # filling the seg space
> >      for i in {1..511}:
> >      do
> >          name=padding_$count-$i
> >          echo write $name
> >          dd if=/dev/zero of=./$name bs=4K count=1 > /dev/null 2>&1
> >          if [ $? -ne 0 ]; then
> >                  exit 0
> >          fi
> >      done
> >      sync
> >
> >      # pin one block in a segment
> >      name=pin_file$count
> >      dd if=/dev/zero of=./$name bs=4K count=1 > /dev/null 2>&1
> >      sync
> >      f2fs_io pinfile set $name
> >      count=$(($count + 1))
> > done
> >
> > Signed-off-by: Wu Bo <bo.wu@xxxxxxxx>
> > ---
> >   fs/f2fs/file.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index ca5904129b16..e8a13616543f 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -1690,7 +1690,7 @@ static int f2fs_expand_inode_data(struct inode 
> > *inode, loff_t offset,
> >               .init_gc_type = FG_GC,
> >               .should_migrate_blocks = false,
> >               .err_gc_skipped = true,
> > -            .nr_free_secs = 0 };
> > +            .nr_free_secs = 1 };
> >       pgoff_t pg_start, pg_end;
> >       loff_t new_size;
> >       loff_t off_end;