> On Jan 10, 2016, at 11:30 PM, Fan Li <fanofcode.li@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> 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:
signature.asc
Description: Message signed with OpenPGP using GPGMail
