Re: [f2fs-dev] [PATCH] f2fs: merge meta writes as many possible

[Jaegeuk Kim]

Hi Chao,

[snip]

> > > >  	struct writeback_control wbc = {
> > > > @@ -277,6 +277,11 @@ long sync_meta_pages(struct f2fs_sb_info *sbi, enum page_type type,
> > > >  		for (i = 0; i < nr_pages; i++) {
> > > >  			struct page *page = pvec.pages[i];
> > > >
> > > > +			if (prev == LONG_MAX)
> > > > +				prev = page->index - 1;
> > > > +			if (nr_to_write != LONG_MAX && page->index != prev + 1)
> 
> If we reach this condition, should we break the 'while' loop outside?
> Because it's not possible to meet the page with 'prev + 1' index in any
> page vec found afterward.

Alright, I fixed this too.

> 
> > >
> > > Does this mean we only writeback consecutive meta page of SSA region? If these meta
> > > pages are updated randomly (in collapse range or insert range case), we will writeback
> > > very few meta pages in one round of flush, it may cause low performance since FTL will
> > > do the force GC on our meta page update region if we writeback meta pages in one segment
> > > repeatly.
> > 
> > I don't think this would degrade the performance pretty much. Rather than that,
> > as the above traces, I could see more possitive impact on IO patterns when I
> > gave some delay on flushing meta pages. (I got the traces when copying kernel
> > source tree.) Moreover, the amount of the meta page writes is relatively lower
> > than other data types.
> 
> Previously I only track collapse/insert case, so I'm just worry about those corner
> cases. Today I track the normal case, the trace info looks good to me! :)
> 
> > 
> > > IMO, when the distribution of dirty SSA pages are random, at least we should writeback
> > > all dirty meta pages in SSA region which align to our segment size under block plug.
> > >
> > > One more thing is that I found in xfstest tests/generic/019 always cause inconsistent
> > > between ssa info and meta info of inode (i_blocks != total_blk_cnt). The reason of the
> > > problem is in ->falloc::collapse, we will update ssa meta page and node page info
> > > together, however, unfortunately ssa meta page is writeback by kworker, node page will
> > > no longer be persisted since fail_make_request made f2fs flagged as CP_ERROR_FLAG.
> > >
> > > So writeback SSA pages leads the potential risk of producing consistent issue. I think
> > > there are some ways can fix this:
> > > 1) don't writeback SSA pages in kworker, but side-effect is more memory will be cost
> > > since cp is executed, of course, periodical cp can fix the memory cost issue.
> > > 2) add some tag in somewhere, we can recover the ssa info with dnode page, but this is
> > > really a big change.
> > >
> > > How do you think? Any better idea?
> > 
> > Good catch!
> > I think we can allocate a new block address for this case like the below patch.
> > 
> > From 4979a1cbd7c718cff946cb421fbc0763a3dbd0df Mon Sep 17 00:00:00 2001
> > From: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> > Date: Tue, 6 Oct 2015 15:41:58 -0700
> > Subject: [PATCH] f2fs: avoid SSA corruption when replacing block addresses
> > 
> > If block addresses are pointed by the previous checkpoint, we should not update
> > the SSA when replacing block addresses by f2fs_collapse_range.
> > 
> > This patch allocates a new block address if there is an exising block address.
> 
> This patch doesn't fix that issue, because SSA block related to new_blkaddr will
> be overwritten, not the one related to old_blkaddr.
> So still I can reproduce the issue after applying this patch.

Urg, right.

I wrote a different patch to resolve this issue.
Actually, I think there is a hole that new_address can be allocated to other
thread after initial truncation.
I've been running xfstests and fsstress for a while.
Could you also check this patch?