diff mbox

fs: fix bugs for __generic_block_fiemap()

Message ID 001401d14aba$dfe6c370$9fb44a50$@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fan Li Jan. 9, 2016, 8:50 a.m. UTC
Fix 3 bugs:
1. 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.
2. len hasn't been updated correctly, if len > isize and
   start > 0.
3. If len is less than one block, it will be extended to
   one block. If start + len exceeds the boundary of the 
   original block because of the extension, one extra block 
   will be returned.

And simplify the codes of __generic_block_fiemap() as well.

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

Comments

Dave Chinner Jan. 10, 2016, 10:21 p.m. UTC | #1
On Sat, Jan 09, 2016 at 04:50:34PM +0800, Fan Li wrote:
> Fix 3 bugs:
> 1. 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.
> 2. len hasn't been updated correctly, if len > isize and
>    start > 0.
> 3. If len is less than one block, it will be extended to
>    one block. If start + len exceeds the boundary of the 
>    original block because of the extension, one extra block 
>    will be returned.
> 
> And simplify the codes of __generic_block_fiemap() as well.

One bug fix/change per patch, please, so they can be easily
reviewed.

Cheers,

Dave.
Fan Li Jan. 11, 2016, 6:41 a.m. UTC | #2
> -----Original Message-----
> From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On Behalf Of Dave Chinner
> Sent: Monday, January 11, 2016 6:22 AM
> To: Fan Li
> Cc: linux-fsdevel@vger.kernel.org
> Subject: Re: [PATCH] fs: fix bugs for __generic_block_fiemap()
> 
> On Sat, Jan 09, 2016 at 04:50:34PM +0800, Fan Li wrote:
> > Fix 3 bugs:
> > 1. 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.
> > 2. len hasn't been updated correctly, if len > isize and
> >    start > 0.
> > 3. If len is less than one block, it will be extended to
> >    one block. If start + len exceeds the boundary of the
> >    original block because of the extension, one extra block
> >    will be returned.
> >
> > And simplify the codes of __generic_block_fiemap() as well.
> 
> One bug fix/change per patch, please, so they can be easily reviewed.

OK, I have split it into three patches, but I'm afraid the fix for the first bug
involves a lot of modification that can't be easily divided, I hope it's enough.

Thanks.
> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 41c352e..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);
@@ -268,28 +267,27 @@  int __generic_block_fiemap(struct inode *inode,
 	 * since we expect isize to not change at all through the duration of
 	 * this call.
 	 */
-	if (len >= isize) {
-		whole_file = true;
-		len = isize;
-	}
+	if (start >= isize)
+		return 0;
 
-	/*
-	 * Some filesystems can't deal with being asked to map less than
-	 * blocksize, so make sure our len is at least block length.
-	 */
-	if (logical_to_blk(inode, len) == 0)
-		len = blk_to_logical(inode, 1);
+	if (start + len > isize)
+		len = isize - start;
 
 	start_blk = logical_to_blk(inode, start);
 	last_blk = logical_to_blk(inode, start + len - 1);
 
 	do {
+		memset(&map_bh, 0, sizeof(struct buffer_head));
 		/*
-		 * we set b_size to the total size we want so it will map as
-		 * many contiguous blocks as possible at once
+		 * Some filesystems would round down b_size to align
+		 * with block size, if len isn't aligned already, the last
+		 * block may not be returned. Let's round it up first.
 		 */
-		memset(&map_bh, 0, sizeof(struct buffer_head));
-		map_bh.b_size = len;
+		if (last_blk > start_blk)
+			map_bh.b_size = blk_to_logical(inode,
+						last_blk - start_blk + 1);
+		else
+			map_bh.b_size = blk_to_logical(inode, 1);
 
 		ret = get_block(inode, start_blk, &map_bh, 0);
 		if (ret)
@@ -299,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 */