fs: rewrite __generic_block_fiemap()
Subsystems (2)
PATCH 3/3 fs: rewrite __generic_block_fiemap()
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.
--- fs/ioctl.c | 93 +++++++++++------------------------------------------------- 1 file changed, 17 insertions(+), 76 deletions(-)
fs/ioctl.c +17 -76
@@ -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 */
Signature · 1 line
1.7.9.5
Re: [PATCH 3/3] rewrite __generic_block_fiemap()
Quoted 94 lines · 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_LASTto the last extent.
Simplify the codes in this patch.Signed-off-by Fan Li <[email protected]>---
fs/ioctl.c| 93 +++++++++++-------------------------------------------------1 file changed, 17 insertions(+), 76 deletions(-)fs/ioctl.c +4
-48@@ -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,
fs/ioctl.c int __generic_block_fiemap(struct inode *inode, · line 358
Quoted 10 lines
fs/ioctl.c +2
-7- 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.
fs/ioctl.c int __generic_block_fiemap(struct inode *inode, · line 312
Quoted 10 lines
fs/ioctl.c +3
-7-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.
fs/ioctl.c int __generic_block_fiemap(struct inode *inode, · line 372
Quoted 8 lines
fs/ioctl.c +2
-5- 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 */
Quoted 15 lines
fs/ioctl.c +5
-8+ 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);
fs/ioctl.c int __generic_block_fiemap(struct inode *inode, · line 331
Quoted 10 lines
fs/ioctl.c +1
-1+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
RE: [PATCH 3/3] rewrite __generic_block_fiemap()
Quoted 108 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Andreas Dilger
Sent: Tuesday, January 12, 2016 6:42 AM
To: Fan Li
Cc: [email protected]
Subject: Re: [PATCH 3/3] rewrite__generic_block_fiemap()Quoted 98 lines · 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_LASTto the last extent. Simplify the codes in this patch.Signed-off-by Fan Li <[email protected]>---
fs/ioctl.c| 93 +++++++++++-------------------------------------------------1 file changed, 17 insertions(+), 76 deletions(-)fs/ioctl.c +1
-1@@ -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;fs/ioctl.c +1
-2-if (start + len > isize) { - whole_file = true; +if (start + len > isize)len = isize - start;fs/ioctl.c +0
-1- }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++;fs/ioctl.c +3
-11-/* -* 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 */fs/ioctl.c +2
-36-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 ; + }
Quoted 6 lines
fs/ioctl.c int __generic_block_fiemap(struct inode *inode, · line 358
Quoted 4 lines
fs/ioctl.c +0
-4-/* -* 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.
Quoted 13 lines
fs/ioctl.c int __generic_block_fiemap(struct inode *inode, · line 312
Quoted 11 lines
fs/ioctl.c +3
-7-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.
Quoted 11 lines
fs/ioctl.c int __generic_block_fiemap(struct inode *inode, · line 372
Quoted 9 lines
fs/ioctl.c +2
-5- 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.
Quoted 26 lines
/* reset values for the next extent */Quoted 24 lines
fs/ioctl.c +6
-8+ 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);
Quoted 3 lines
Quoted 1 line
/* 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
Quoted 1 line
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.
Quoted 6 lines
Cheers, Andreas