> -----Original Message----- > From: Fan Li [mailto:fanofcode...@samsung.com] > Sent: Monday, January 04, 2016 1:57 PM > 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:chao2...@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...@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:c...@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.
What I mean here is it's better to stand in user's viewpoint, let the interface return the correct result, since user only refer the manual of interface, rather than comments in patch or reply in email. > 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. > > 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 didn't vote the second, IMO, it's better support all cases firstly, at least, we should not let user experience 'sometimes success, sometimes fail' in our ->fiemap. Then, if there are performance issue for common cases, we could try to do some improvement on it as I mentioned below. I think Jaegeuk has better idea for the performance issue, please refer to his comments. Thanks, > 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...@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 ------------------------------------------------------------------------------ _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
