Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid multiple node page writes due to inline_data

[Jaegeuk Kim]

Hi Chao,

On Mon, Jan 25, 2016 at 05:42:40PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@xxxxxxxxxx]
> > Sent: Sunday, January 24, 2016 4:16 AM
> > To: linux-kernel@xxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx;
> > linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 1/2] f2fs: avoid multiple node page writes due to inline_data
> > 
> > The sceanrio is:
> > 1. create fully node blocks
> > 2. flush node blocks
> > 3. write inline_data for all the node blocks again
> > 4. flush node blocks redundantly
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> > ---
> >  fs/f2fs/data.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 8d0d9ec..011456e 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1622,14 +1622,22 @@ static int f2fs_write_end(struct file *file,
> > 
> >  	trace_f2fs_write_end(inode, pos, len, copied);
> > 
> > -	set_page_dirty(page);
> > -
> >  	if (pos + copied > i_size_read(inode)) {
> >  		i_size_write(inode, pos + copied);
> >  		mark_inode_dirty(inode);
> > -		update_inode_page(inode);
> >  	}
> > 
> > +	if (f2fs_has_inline_data(inode) &&
> > +			is_inode_flag_set(F2FS_I(inode), FI_DATA_EXIST)) {
> > +		int err = f2fs_write_inline_data(inode, page);
> 
> Oh, I'm sure this can fix that issue, but IMO:
> a) this implementation has side-effect, it triggers inline data copying
> between data page and node page whenever user write inline datas, so if
> user updates inline data frequently, write-through approach would cause
> memory copy overhead.

Agreed.

> b) inline storm should be a rare case, as we didn't get any report about
> problem for long time until Dave's, and write_end is a hot path, I think
> it's better to be cautious to change our inline data cache policy for
> fixing a rare issue in hot path.
> 
> What about delaying the merge operation? like:
> 1) as I proposed before, merging inline page into inode page when
> detecting free_sections <= (node_secs + 2 * dent_secs + inline_secs).
> 2) merge inline page into inode page before writeback inode page in
> sync_node_pages.

Okay, I'm thinking more general way where we can get rid of every inlien_data
write when we flush node pages.

I've been testing this patch.