Re: [PATCH 1/2] f2fs: enable to gc page whose inode already atomic commit

[Yunlong Song]

Is it necessary to add a lock here? What's the problem of this patch (no 
lock at all)? Anyway, the problem is expected to be fixed asap, since 
attackers can easily write an app with only atomic start and no atomic 
commit, which will cause f2fs run into loop gc if the disk layout is 
much fragmented, since f2fs_gc will select the same target victim all 
the time (e.g. one block of target victim belongs to the opened atomic 
file, and it will not be moved and do_garbage_collect will finally 
return 0, and that victim is selected again next time) and goto gc_more 
time and time again, which will block all the fs ops (all the fs ops 
will hang up in f2fs_balance_fs).

On 2018/2/4 22:56, Chao Yu wrote:
> On 2018/2/3 10:47, Yunlong Song wrote:
> > If inode has already started to atomic commit, then set_page_dirty will
> > not mix the gc pages with the inmem atomic pages, so the page can be
> > gced safely.
>
> Let's avoid Ccing fs mailing list if the patch didn't change vfs common
> codes.
>
> As you know, the problem here is mixed dnode block flushing w/o writebacking
> gced data block, result in making transaction unintegrated.
>
> So how about just using dio_rwsem[WRITE] during atomic committing to exclude
> GCing data block of atomic opened file?
>
> Thanks,
>
> > Signed-off-by: Yunlong Song <yunlong.song@xxxxxxxxxx>
> > ---
> >   fs/f2fs/data.c | 5 ++---
> >   fs/f2fs/gc.c   | 6 ++++--
> >   2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 7435830..edafcb6 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1580,14 +1580,13 @@ bool should_update_outplace(struct inode *inode, struct f2fs_io_info *fio)
> >   		return true;
> >   	if (S_ISDIR(inode->i_mode))
> >   		return true;
> > -	if (f2fs_is_atomic_file(inode))
> > -		return true;
> >   	if (fio) {
> >   		if (is_cold_data(fio->page))
> >   			return true;
> >   		if (IS_ATOMIC_WRITTEN_PAGE(fio->page))
> >   			return true;
> > -	}
> > +	} else if (f2fs_is_atomic_file(inode))
> > +		return true;
> >   	return false;
> >   }
> >   
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index b9d93fd..84ab3ff 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -622,7 +622,8 @@ static void move_data_block(struct inode *inode, block_t bidx,
> >   	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
> >   		goto out;
> >   
> > -	if (f2fs_is_atomic_file(inode))
> > +	if (f2fs_is_atomic_file(inode) &&
> > +		!f2fs_is_commit_atomic_write(inode))
> >   		goto out;
> >   
> >   	if (f2fs_is_pinned_file(inode)) {
> > @@ -729,7 +730,8 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
> >   	if (!check_valid_map(F2FS_I_SB(inode), segno, off))
> >   		goto out;
> >   
> > -	if (f2fs_is_atomic_file(inode))
> > +	if (f2fs_is_atomic_file(inode) &&
> > +		!f2fs_is_commit_atomic_write(inode))
> >   		goto out;
> >   	if (f2fs_is_pinned_file(inode)) {
> >   		if (gc_type == FG_GC)
>
> .

--
Thanks,
Yunlong Song