> -----Original Message----- > From: Chao Yu [mailto:chao2...@samsung.com] > Sent: Monday, January 04, 2016 6:00 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: 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.
>From viewpoint of users, this patch makes the functionality of fiemap a >little closer to the manual, it supports one more scenario that manual says, why isn't it an improvement? Besides there are a lot of examples in kernel, that is written in comment and different from the manual, the old version of this function happens to be one of them, are you saying that if we don't solve this, we shouldn't support this function at all? Of course there are solutions at the end, as I said in reply, I have an idea about further improvement, but what's wrong with improving it one step at a time? > > > 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
