Re: [PATCH 2/3] f2fs: support finding extents after isize

[Chao Yu <chao2.yu@samsung.com>]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Tuesday, January 05, 2016 11:30 AM
> To: Chao Yu
> Cc: 'Fan Li'; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 2/3] f2fs: support finding extents after isize
> 
> Hi,
> 
> ...
> 
> > > > > Nope, what I'm talking about is *correctness* of our ->fiemap interface, but you're
> trying
> > > to avoid it by saying "support more
> > > > cases,
> > > > > it's an improvement". That doesn't make any sense to me, since correctness issue still
> not
> > > be fixed.
> > > >
> > > > I'm not sure what you mean by avoiding, I think the comment and reply I written has already
> > > stated the issue and
> > > > limitation of this patch.
> > > > Now there are two suggestions:
> > > > 1. support one more scenario, and all old scenarios are dealt like before, but it still
> can't
> > > support discontinuous extent after
> > > > isize.
> > > > 2. support all scenarios, but sacrifice performance for lots of common scenarios by checking
> > > about 10^9 blocks.
> > >
> > > IMO, we can think about #2 whether there is an efficient way.
> > >
> > > How many cases does this incur?
> > > One is fallocate with keeping i_size, ana other?
> >
> > AFAIK, no more.
> >
> > >
> > > How about adding FADVISE_OVER_ISIZE to represent inode has blocks beyond i_size?
> > > Then, we can set this flag in fallocate and reset it in f2fs_truncate.
> >
> > Append write in such inode could destroy this convention, right?
> 
> Right, but we have another chance to reset the flag, when fiemap checks the end
> of allocated space.

Agreed, :)

Thanks,

> 
> Thanks,
> 
> >
> > Thanks,
> >
> > >
> > > Thanks,
> > >
> > > >
> > > > I think we can all agree both ideas have their flaws.
> > > > The only divergence is that I vote the first, and you the second. I think the most important
> > > thing is that it works fluently in most
> > > > scenarios, and you think is that it works in every scenarios even it's very slow.
> > > >
> > > > I think my method is more pratical, but balance between performance and utility seems
> to be
> > > an Eternal problem.
> > > >
> > > > >
> > > > > >
> > > > > > use max blocks as boundary would get us the precise result, but it
> > > > > > also means after we reach the EOF, we still need to look up every
> > > > > > block between the EOF and  sb-> s_maxbytes to make sure the EOF is
> > > > > > true, that's about 4TB or 10^9 blocks.
> > > > > > And it affects all scenarios where the search range covers the last
> > > > > > extent in the file, not just extents beyond isize. I think this price
> > > > > > is too high to pay.
> > > > >
> > > > > That's another performance issue.
> > > > >
> > > > > >
> > > > > > I was hoping that I can make f2fs_map_block return an EOF to solve
> > > > > > this problem some time later, or anyone have a better idea?
> > > > >
> > > > > At least we can seek valid dnode block like the way llseek use.
> > > > >
> > > > > In addition, for most cases, few of i_nid[5] in f2fs_inode will be NULL, we could skip
> searching
> > > all dnode block in such
> > > > non-allocated
> > > > > indirect node, instead of searching dnode block f2fs_map_block one by one.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Fan li <fanofcode.li@samsung.com>
> > > > > > > > ---
> > > > > > > >  fs/f2fs/data.c |    7 +------
> > > > > > > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
> > > > > > > > a9a4d89..f89cf07
> > > > > > > > 100644
> > > > > > > > --- a/fs/f2fs/data.c
> > > > > > > > +++ b/fs/f2fs/data.c
> > > > > > > > @@ -798,12 +798,6 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info
> > > *fieinfo,
> > > > > > > >  	isize = i_size_read(inode);
> > > > > > > >
> > > > > > > >  	mutex_lock(&inode->i_mutex);
> > > > > > > > -	if (start >= isize)
> > > > > > > > -		goto out;
> > > > > > > > -
> > > > > > > > -	if (start + len > isize)
> > > > > > > > -		len = isize - start;
> > > > > > > > -
> > > > > > > >  	if (logical_to_blk(inode, len) == 0)
> > > > > > > >  		len = blk_to_logical(inode, 1);
> > > > > > > >
> > > > > > > > @@ -829,6 +823,7 @@ next:
> > > > > > > >  		 * punch holes beyond isize and keep size unchanged.
> > > > > > > >  		 */
> > > > > > > >  		flags |= FIEMAP_EXTENT_LAST;
> > > > > > > > +		last_blk = start_blk - 1;
> > > > > > > >  	}
> > > > > > > >
> > > > > > > >  	if (size)
> > > > > > > >
> > > > > >
> > > > > >
> > > > > > ----------------------------------------------------------------------
> > > > > > -------- _______________________________________________
> > > > > > Linux-f2fs-devel mailing list
> > > > > > Linux-f2fs-devel@lists.sourceforge.net
> > > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


------------------------------------------------------------------------------