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

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

[Fan Li]

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. 


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)
-- 
1.7.9.5


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

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

[Chao Yu]

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.

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)
> 

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

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

[Fan Li]

> -----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.

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. 

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?

> 
> 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)
> >


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

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

[Chao Yu]

> -----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.

> 
> 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


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

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

[Fan Li]

> -----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.

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


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

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

[Jaegeuk Kim]

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.

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

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

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

[Chao Yu]

> -----Original Message-----
> From: Fan Li [mailto:fanofcode.li@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.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.

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.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


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

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

[Fan Li]

> -----Original Message-----
> From: Chao Yu [mailto:chao2.yu@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.li@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.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.
> 
> 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.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


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

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

[Fan Li]

> -----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.

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.

> 
> 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


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

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

[Jaegeuk Kim]

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?

> 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.

Thanks,

> 
> > 
> > Thanks,
> > 

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

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

[Fan Li]

> -----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,
> > >


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

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

[Jaegeuk Kim]

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.

My real concern is that there no space for i_disksize.

Thanks,

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

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

[Chao Yu]

Hi all,

> -----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?

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?

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


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

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

[Jaegeuk Kim]

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.

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

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

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

[Chao Yu]

> -----Original Message-----
> From: Fan Li [mailto:fanofcode.li@samsung.com]
> Sent: Monday, January 04, 2016 6:55 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.yu@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.li@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.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.
> >
> > 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?

In brief, as we discussed in private, we have reached an agreement that
we will continue to improve on this patch to solve related issue as Jaegeuk
suggested.

Thanks,

> 
> >
> > > 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.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


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

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

[Fan Li]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Tuesday, January 05, 2016 11:08 AM
> To: Fan Li; "^["@jaegeuk-2.local
> Cc: 'Chao Yu'; 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.
> 
> My real concern is that there no space for i_disksize.

I see. How about combine both ideas, use flag as you propose, and if fiemap ever needs to 
look up the last extent beyond isize, we calculate i_disksize first and cache it in f2fs_inode_info, 
use it to determine the last extent.

This way we don't have to look up all blocks up to s_maxbytes, and need no extra space in disk.

> 
> Thanks,


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

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

[Chao Yu]

> -----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


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

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

[Jaegeuk Kim]

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?
> > > > > >
> > > > > > 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.
> > 
> > My real concern is that there no space for i_disksize.
> 
> I see. How about combine both ideas, use flag as you propose, and if fiemap ever needs to 
> look up the last extent beyond isize, we calculate i_disksize first and cache it in f2fs_inode_info, 
> use it to determine the last extent.
> 
> This way we don't have to look up all blocks up to s_maxbytes, and need no extra space in disk.

Looks good to me.

Thanks,

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