[PATCH 3/3] fs: rewrite __generic_block_fiemap()

[PATCH 3/3] fs: rewrite __generic_block_fiemap()

[Fan Li]

There are a few redundant branches in this function and
if there are more than two blocks of holes after the last
extent of file, it would fail to add FIEMAP_EXTENT_LAST
to the last extent.
Simplify the codes in this patch.


Signed-off-by: Fan Li <fanofcode.li@samsung.com>
---
 fs/ioctl.c |   93 +++++++++++-------------------------------------------------
 1 file changed, 17 insertions(+), 76 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 5d54377..8e2b426 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -256,7 +256,6 @@ int __generic_block_fiemap(struct inode *inode,
 	loff_t isize = i_size_read(inode);
 	u64 logical = 0, phys = 0, size = 0;
 	u32 flags = FIEMAP_EXTENT_MERGED;
-	bool past_eof = false, whole_file = false;
 	int ret = 0;
 
 	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
@@ -271,10 +270,8 @@ int __generic_block_fiemap(struct inode *inode,
 	if (start >= isize)
 		return 0;
 
-	if (start + len > isize) {
-		whole_file = true;
+	if (start + len > isize)
 		len = isize - start;
-	}
 
 	start_blk = logical_to_blk(inode, start);
 	last_blk = logical_to_blk(inode, start + len - 1);
@@ -300,91 +297,35 @@ int __generic_block_fiemap(struct inode *inode,
 		if (!buffer_mapped(&map_bh)) {
 			start_blk++;
 
-			/*
-			 * We want to handle the case where there is an
-			 * allocated block at the front of the file, and then
-			 * nothing but holes up to the end of the file properly,
-			 * to make sure that extent at the front gets properly
-			 * marked with FIEMAP_EXTENT_LAST
-			 */
-			if (!past_eof &&
-			    blk_to_logical(inode, start_blk) >= isize)
-				past_eof = 1;
-
+			/* Skip holes unless it indicates the EOF */
+			if (blk_to_logical(inode, start_blk) < isize)
+				goto next;
 			/*
 			 * First hole after going past the EOF, this is our
 			 * last extent
 			 */
-			if (past_eof && size) {
-				flags = FIEMAP_EXTENT_MERGED|FIEMAP_EXTENT_LAST;
-				ret = fiemap_fill_next_extent(fieinfo, logical,
-							      phys, size,
-							      flags);
-			} else if (size) {
-				ret = fiemap_fill_next_extent(fieinfo, logical,
-							      phys, size, flags);
-				size = 0;
-			}
-
-			/* if we have holes up to/past EOF then we're done */
-			if (start_blk > last_blk || past_eof || ret)
-				break;
-		} else {
-			/*
-			 * We have gone over the length of what we wanted to
-			 * map, and it wasn't the entire file, so add the extent
-			 * we got last time and exit.
-			 *
-			 * This is for the case where say we want to map all the
-			 * way up to the second to the last block in a file, but
-			 * the last block is a hole, making the second to last
-			 * block FIEMAP_EXTENT_LAST.  In this case we want to
-			 * see if there is a hole after the second to last block
-			 * so we can mark it properly.  If we found data after
-			 * we exceeded the length we were requesting, then we
-			 * are good to go, just add the extent to the fieinfo
-			 * and break
-			 */
-			if (start_blk > last_blk && !whole_file) {
-				ret = fiemap_fill_next_extent(fieinfo, logical,
-							      phys, size,
-							      flags);
-				break;
-			}
+			flags |= FIEMAP_EXTENT_LAST;
+		}
 
-			/*
-			 * if size != 0 then we know we already have an extent
-			 * to add, so add it.
-			 */
-			if (size) {
-				ret = fiemap_fill_next_extent(fieinfo, logical,
-							      phys, size,
-							      flags);
-				if (ret)
-					break;
-			}
+		if (size)
+			ret = fiemap_fill_next_extent(fieinfo, logical,
+					phys, size, flags);
 
-			logical = blk_to_logical(inode, start_blk);
-			phys = blk_to_logical(inode, map_bh.b_blocknr);
-			size = map_bh.b_size;
-			flags = FIEMAP_EXTENT_MERGED;
+		if (start_blk > last_blk || ret)
+			break;
 
-			start_blk += logical_to_blk(inode, size);
+		logical = blk_to_logical(inode, start_blk);
+		phys = blk_to_logical(inode, map_bh.b_blocknr);
+		size = map_bh.b_size;
+		flags = FIEMAP_EXTENT_MERGED;
 
-			/*
-			 * If we are past the EOF, then we need to make sure as
-			 * soon as we find a hole that the last extent we found
-			 * is marked with FIEMAP_EXTENT_LAST
-			 */
-			if (!past_eof && logical + size >= isize)
-				past_eof = true;
-		}
+		start_blk += logical_to_blk(inode, size);
+next:
 		cond_resched();
 		if (fatal_signal_pending(current)) {
 			ret = -EINTR;
 			break;
 		}
-
 	} while (1);
 
 	/* If ret is 1 then we just hit the end of the extent array */
-- 
1.7.9.5

Re: [PATCH 3/3] rewrite __generic_block_fiemap()

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 5304 bytes --]


> On Jan 10, 2016, at 11:30 PM, Fan Li <fanofcode.li@samsung.com> wrote:
> 
> There are a few redundant branches in this function and
> if there are more than two blocks of holes after the last
> extent of file, it would fail to add FIEMAP_EXTENT_LAST
> to the last extent.
> Simplify the codes in this patch.
> 
> 
> Signed-off-by: Fan Li <fanofcode.li@samsung.com>
> ---
> fs/ioctl.c |   93 +++++++++++-------------------------------------------------
> 1 file changed, 17 insertions(+), 76 deletions(-)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 5d54377..8e2b426 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -256,7 +256,6 @@ int __generic_block_fiemap(struct inode *inode,
> 	loff_t isize = i_size_read(inode);
> 	u64 logical = 0, phys = 0, size = 0;
> 	u32 flags = FIEMAP_EXTENT_MERGED;
> -	bool past_eof = false, whole_file = false;
> 	int ret = 0;
> 
> 	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
> @@ -271,10 +270,8 @@ int __generic_block_fiemap(struct inode *inode,
> 	if (start >= isize)
> 		return 0;
> 
> -	if (start + len > isize) {
> -		whole_file = true;
> +	if (start + len > isize)
> 		len = isize - start;
> -	}
> 
> 	start_blk = logical_to_blk(inode, start);
> 	last_blk = logical_to_blk(inode, start + len - 1);
> @@ -300,91 +297,35 @@ int __generic_block_fiemap(struct inode *inode,
> 		if (!buffer_mapped(&map_bh)) {
> 			start_blk++;
> 
> -			/*
> -			 * We want to handle the case where there is an
> -			 * allocated block at the front of the file, and then
> -			 * nothing but holes up to the end of the file properly,
> -			 * to make sure that extent at the front gets properly
> -			 * marked with FIEMAP_EXTENT_LAST
> -			 */
> -			if (!past_eof &&
> -			    blk_to_logical(inode, start_blk) >= isize)
> -				past_eof = 1;
> -
> +			/* Skip holes unless it indicates the EOF */
> +			if (blk_to_logical(inode, start_blk) < isize)
> +				goto next;
> 			/*
> 			 * First hole after going past the EOF, this is our
> 			 * last extent
> 			 */
> -			if (past_eof && size) {
> -				flags = FIEMAP_EXTENT_MERGED|FIEMAP_EXTENT_LAST;
> -				ret = fiemap_fill_next_extent(fieinfo, logical,
> -							      phys, size,
> -							      flags);
> -			} else if (size) {
> -				ret = fiemap_fill_next_extent(fieinfo, logical,
> -							      phys, size, flags);
> -				size = 0;
> -			}
> -
> -			/* if we have holes up to/past EOF then we're done */
> -			if (start_blk > last_blk || past_eof || ret)
> -				break;
> -		} else {
> -			/*
> -			 * We have gone over the length of what we wanted to
> -			 * map, and it wasn't the entire file, so add the extent
> -			 * we got last time and exit.
> -			 *
> -			 * This is for the case where say we want to map all the
> -			 * way up to the second to the last block in a file, but
> -			 * the last block is a hole, making the second to last
> -			 * block FIEMAP_EXTENT_LAST.  In this case we want to
> -			 * see if there is a hole after the second to last block
> -			 * so we can mark it properly.  If we found data after
> -			 * we exceeded the length we were requesting, then we
> -			 * are good to go, just add the extent to the fieinfo
> -			 * and break
> -			 */
> -			if (start_blk > last_blk && !whole_file) {
> -				ret = fiemap_fill_next_extent(fieinfo, logical,
> -							      phys, size,
> -							      flags);
> -				break;
> -			}
> +			flags |= FIEMAP_EXTENT_LAST;
> +		}
> 
> -			/*
> -			 * if size != 0 then we know we already have an extent
> -			 * to add, so add it.
> -			 */

Why remove this comment?  This is still true.

> -			if (size) {
> -				ret = fiemap_fill_next_extent(fieinfo, logical,
> -							      phys, size,
> -							      flags);
> -				if (ret)
> -					break;
> -			}
> +		if (size)
> +			ret = fiemap_fill_next_extent(fieinfo, logical,
> +					phys, size, flags);

This should align after '(' from the previous line.

> -			logical = blk_to_logical(inode, start_blk);
> -			phys = blk_to_logical(inode, map_bh.b_blocknr);
> -			size = map_bh.b_size;
> -			flags = FIEMAP_EXTENT_MERGED;
> +		if (start_blk > last_blk || ret)
> +			break;
> 
> -			start_blk += logical_to_blk(inode, size);

It would be good to add a comment to this next chunk of code like:

		/* reset values for the next extent */
> +		logical = blk_to_logical(inode, start_blk);
> +		phys = blk_to_logical(inode, map_bh.b_blocknr);
> +		size = map_bh.b_size;
> +		flags = FIEMAP_EXTENT_MERGED;

> 
> -			/*
> -			 * If we are past the EOF, then we need to make sure as
> -			 * soon as we find a hole that the last extent we found
> -			 * is marked with FIEMAP_EXTENT_LAST
> -			 */
> -			if (!past_eof && logical + size >= isize)
> -				past_eof = true;
> -		}
> +		start_blk += logical_to_blk(inode, size);
> +next:
> 		cond_resched();
> 		if (fatal_signal_pending(current)) {
> 			ret = -EINTR;
> 			break;
> 		}
> -
> 	} while (1);
> 
> 	/* If ret is 1 then we just hit the end of the extent array */

What kind of testing have you done with this patch?  I believe there are
some FIEMAP tests in xfstests, though it might make sense to add another
one to cover the case that this patch is fixing.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 3/3] rewrite __generic_block_fiemap()

[Fan Li]

> -----Original Message-----
> From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On Behalf Of Andreas Dilger
> Sent: Tuesday, January 12, 2016 6:42 AM
> To: Fan Li
> Cc: linux-fsdevel@vger.kernel.org
> Subject: Re: [PATCH 3/3] rewrite __generic_block_fiemap()
> 
> 
> > On Jan 10, 2016, at 11:30 PM, Fan Li <fanofcode.li@samsung.com> wrote:
> >
> > There are a few redundant branches in this function and if there are
> > more than two blocks of holes after the last extent of file, it would
> > fail to add FIEMAP_EXTENT_LAST to the last extent.
> > Simplify the codes in this patch.
> >
> >
> > Signed-off-by: Fan Li <fanofcode.li@samsung.com>
> > ---
> > fs/ioctl.c |   93 +++++++++++-------------------------------------------------
> > 1 file changed, 17 insertions(+), 76 deletions(-)
> >
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 5d54377..8e2b426 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -256,7 +256,6 @@ int __generic_block_fiemap(struct inode *inode,
> > 	loff_t isize = i_size_read(inode);
> > 	u64 logical = 0, phys = 0, size = 0;
> > 	u32 flags = FIEMAP_EXTENT_MERGED;
> > -	bool past_eof = false, whole_file = false;
> > 	int ret = 0;
> >
> > 	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC); @@ -271,10
> > +270,8 @@ int __generic_block_fiemap(struct inode *inode,
> > 	if (start >= isize)
> > 		return 0;
> >
> > -	if (start + len > isize) {
> > -		whole_file = true;
> > +	if (start + len > isize)
> > 		len = isize - start;
> > -	}
> >
> > 	start_blk = logical_to_blk(inode, start);
> > 	last_blk = logical_to_blk(inode, start + len - 1); @@ -300,91 +297,35
> > @@ int __generic_block_fiemap(struct inode *inode,
> > 		if (!buffer_mapped(&map_bh)) {
> > 			start_blk++;
> >
> > -			/*
> > -			 * We want to handle the case where there is an
> > -			 * allocated block at the front of the file, and then
> > -			 * nothing but holes up to the end of the file properly,
> > -			 * to make sure that extent at the front gets properly
> > -			 * marked with FIEMAP_EXTENT_LAST
> > -			 */
> > -			if (!past_eof &&
> > -			    blk_to_logical(inode, start_blk) >= isize)
> > -				past_eof = 1;
> > -
> > +			/* Skip holes unless it indicates the EOF */
> > +			if (blk_to_logical(inode, start_blk) < isize)
> > +				goto next;
> > 			/*
> > 			 * First hole after going past the EOF, this is our
> > 			 * last extent
> > 			 */
> > -			if (past_eof && size) {
> > -				flags = FIEMAP_EXTENT_MERGED|FIEMAP_EXTENT_LAST;
> > -				ret = fiemap_fill_next_extent(fieinfo, logical,
> > -							      phys, size,
> > -							      flags);
> > -			} else if (size) {
> > -				ret = fiemap_fill_next_extent(fieinfo, logical,
> > -							      phys, size, flags);
> > -				size = 0;
> > -			}
> > -
> > -			/* if we have holes up to/past EOF then we're done */
> > -			if (start_blk > last_blk || past_eof || ret)
> > -				break;
> > -		} else {
> > -			/*
> > -			 * We have gone over the length of what we wanted to
> > -			 * map, and it wasn't the entire file, so add the extent
> > -			 * we got last time and exit.
> > -			 *
> > -			 * This is for the case where say we want to map all the
> > -			 * way up to the second to the last block in a file, but
> > -			 * the last block is a hole, making the second to last
> > -			 * block FIEMAP_EXTENT_LAST.  In this case we want to
> > -			 * see if there is a hole after the second to last block
> > -			 * so we can mark it properly.  If we found data after
> > -			 * we exceeded the length we were requesting, then we
> > -			 * are good to go, just add the extent to the fieinfo
> > -			 * and break
> > -			 */
> > -			if (start_blk > last_blk && !whole_file) {
> > -				ret = fiemap_fill_next_extent(fieinfo, logical,
> > -							      phys, size,
> > -							      flags);
> > -				break;
> > -			}
> > +			flags |= FIEMAP_EXTENT_LAST;
> > +		}
> >
> > -			/*
> > -			 * if size != 0 then we know we already have an extent
> > -			 * to add, so add it.
> > -			 */
> 
> Why remove this comment?  This is still true.

I just didn't notice it while I removed the branch, I will add it.

> 
> > -			if (size) {
> > -				ret = fiemap_fill_next_extent(fieinfo, logical,
> > -							      phys, size,
> > -							      flags);
> > -				if (ret)
> > -					break;
> > -			}
> > +		if (size)
> > +			ret = fiemap_fill_next_extent(fieinfo, logical,
> > +					phys, size, flags);
> 
> This should align after '(' from the previous line.

OK, I thought checkpatch.pl would catch such problem.

> 
> > -			logical = blk_to_logical(inode, start_blk);
> > -			phys = blk_to_logical(inode, map_bh.b_blocknr);
> > -			size = map_bh.b_size;
> > -			flags = FIEMAP_EXTENT_MERGED;
> > +		if (start_blk > last_blk || ret)
> > +			break;
> >
> > -			start_blk += logical_to_blk(inode, size);
> 
> It would be good to add a comment to this next chunk of code like:

sure.
> 
> 		/* reset values for the next extent */
> > +		logical = blk_to_logical(inode, start_blk);
> > +		phys = blk_to_logical(inode, map_bh.b_blocknr);
> > +		size = map_bh.b_size;
> > +		flags = FIEMAP_EXTENT_MERGED;
> 
> >
> > -			/*
> > -			 * If we are past the EOF, then we need to make sure as
> > -			 * soon as we find a hole that the last extent we found
> > -			 * is marked with FIEMAP_EXTENT_LAST
> > -			 */
> > -			if (!past_eof && logical + size >= isize)
> > -				past_eof = true;
> > -		}
> > +		start_blk += logical_to_blk(inode, size);
> > +next:
> > 		cond_resched();
> > 		if (fatal_signal_pending(current)) {
> > 			ret = -EINTR;
> > 			break;
> > 		}
> > -
> > 	} while (1);
> >
> > 	/* If ret is 1 then we just hit the end of the extent array */
> 
> What kind of testing have you done with this patch?  I believe there are some FIEMAP tests in xfstests, though it might make sense
to
> add another one to cover the case that this patch is fixing.

I added it to f2fs where I first caught the bugs, and run it in xfstests along 
with about ten cases of my own. 

There is only one difference between this patch and tested codes, in f2fs
FIEMAP_EXTENT_UNWRITTEN will be set if status of map_bh is unwritten.
Two xfstests cases will fail if you don't set it. I'm still trying to figure out 
why __generic_block_fiemap() doesn't set it, but even if it's a bug, it 
should be fixed in another patch, right?

And one case failed because it expects an extent preallocated beyond
isize, which is not supported by __generic_block_fiemap().
The result of rest related test cases are correct.
> 
> Cheers, Andreas
> 
> 
> 
>