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

[Fan Li <fanofcode.li@samsung.com>]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Tuesday, January 05, 2016 7:14 AM
> To: Fan Li
> Cc: 'Chao Yu'; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 2/3] f2fs: support finding extents after isize
> 
> Hi,
> 
> On Mon, Jan 04, 2016 at 07:13:43PM +0800, Fan Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > Sent: Monday, January 04, 2016 2:56 PM
> > > To: Fan Li
> > > Cc: 'Chao Yu'; linux-f2fs-devel@lists.sourceforge.net
> > > Subject: Re: [f2fs-dev] [PATCH 2/3] f2fs: support finding extents
> > > after isize
> > >
> > > Hello,
> > >
> > > On Mon, Jan 04, 2016 at 01:57:27PM +0800, Fan Li wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Chao Yu [mailto:chao2.yu@samsung.com]
> > > > > Sent: Thursday, December 31, 2015 2:34 PM
> > > > > To: 'Fan Li'; 'Jaegeuk Kim'
> > > > > Cc: linux-f2fs-devel@lists.sourceforge.net
> > > > > Subject: RE: [f2fs-dev] [PATCH 2/3] f2fs: support finding
> > > > > extents after isize
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Fan Li [mailto:fanofcode.li@samsung.com]
> > > > > > Sent: Thursday, December 31, 2015 11:37 AM
> > > > > > To: 'Chao Yu'; 'Jaegeuk Kim'
> > > > > > Cc: linux-f2fs-devel@lists.sourceforge.net
> > > > > > Subject: Re: [f2fs-dev] [PATCH 2/3] f2fs: support finding
> > > > > > extents after isize
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Chao Yu [mailto:chao@kernel.org]
> > > > > > > Sent: Wednesday, December 30, 2015 9:28 PM
> > > > > > > To: Fan Li; 'Jaegeuk Kim'
> > > > > > > Cc: linux-f2fs-devel@lists.sourceforge.net
> > > > > > > Subject: Re: [f2fs-dev] [PATCH 2/3] f2fs: support finding
> > > > > > > extents after isize
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > On 12/30/15 5:17 PM, Fan Li wrote:
> > > > > > > > f2fs allows preallocation beyond isize, but f2fs_fiemap
> > > > > > > > only look up extents within isize. Therefore add this support.
> > > > > > > >
> > > > > > > > Note: It's possible that there are holes after isize, for
> > > > > > > > example, fallocate  multiple discontinuous extents after
> > > > > > > > isize with FALLOC_FL_KEEP_SIZE set. Since I can tell no
> > > > > > > > differences between EOF and holes from return of
> > > > > > > > get_data_block, I'm afaid this patch can't support such scenarios.
> > > > > > >
> > > > > > > As you mentioned, preallocated block beyond isize can be
> > > > > > > allocated in f2fs, and we are trying
> > > > > > to support mapping extents across
> > > > > > > whole data space of inode, so why we treat theses extents
> > > > > > > inside i_size and outside i_size
> > > > > > separately? IMO, instead using i_size, we
> > > > > > > should use max blocks as boundary.
> > > > > > >
> > > > > > > Most important, this interface still can't support finding
> > > > > > > all extents after i_size, which
> > > > > > looks buggy for our user.
> > > > > >
> > > > > > Notice that this issue exists before my patch, by adding this
> > > > > > patch, at least now it can support more scenarios such as
> > > > > > fallocate a range right after isize. I'd say it's an improvement.
> > > > >
> > > > > 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?
> > >
> > > 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.
> >
> > I have a similar idea that add an actual size which marks the end of
> > last extent, so we can know if the current extent is the last one, even without searching for extents behind.
> 
> Where do you want to store that size in disk?

I'm still not very confident about this idea, so I didn't really think that through yet, 
inode may be a proper place.
Or we just write a special function to find it, and call it only when fiemap is called and disk size isn't 
initiated yet. After all this isn't frequently-used.

> 
> > But there is a problem I still can't figure out,  after truncate an
> > extent at the end of file beyond isize , how do I know where the new
> > last extent ends or if there are still extents beyond isize? after all, the extents beyond isize could be discontinuous.
> 
> So, that's why I proposed a flag instead of a kind of i_disksize.
> We can just set the flag, only if a file *may* have a extent beyond i_size in fallocate, and unset it through f2fs_truncate.
> Moreover, I don't expect that this happens so frequently.

if flag could indicate "may", will i_disksize do a better job in the same way?
let i_disksize be the end of the last extent that *may* exist beyond i_size, set it also in fallocate,when truncate, if 
the length is still beyond isize, we set the new length as i_disksize, so in worse scenario we won't have to 
search up to s_maxbytes.

> 
> Thanks,
> 
> >
> > >
> > > Thanks,
> > >


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