btrfs: Evaluate io_tree in find_lock_delalloc_range()
diff mbox series

Message ID 20190619003524.32377-1-rgoldwyn@suse.de
State New
Headers show
Series
  • btrfs: Evaluate io_tree in find_lock_delalloc_range()
Related show

Commit Message

Goldwyn Rodrigues June 19, 2019, 12:35 a.m. UTC
Simplification.
No point passing the tree variable when it can be evaluated
from inode.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/extent_io.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Nikolay Borisov June 19, 2019, 6:05 a.m. UTC | #1
On 19.06.19 г. 3:35 ч., Goldwyn Rodrigues wrote:
> Simplification.
> No point passing the tree variable when it can be evaluated
> from inode.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

The patch is good, however, there are several calls to find_
lock_delalloc_range in btrfs tests so you'd need to fix those 
invocations, otherwise compilation of the in-kernel test suite will fails. 

fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,


> ---
>  fs/btrfs/extent_io.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index db337e53aab3..e9475d7e11bf 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1719,10 +1719,10 @@ static noinline int lock_delalloc_pages(struct inode *inode,
>   */
>  EXPORT_FOR_TESTS
>  noinline_for_stack bool find_lock_delalloc_range(struct inode *inode,
> -				    struct extent_io_tree *tree,
>  				    struct page *locked_page, u64 *start,
>  				    u64 *end)
>  {
> +	struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree;
>  	u64 max_bytes = BTRFS_MAX_EXTENT_SIZE;
>  	u64 delalloc_start;
>  	u64 delalloc_end;
> @@ -3290,7 +3290,6 @@ static noinline_for_stack int writepage_delalloc(struct inode *inode,
>  		struct page *page, struct writeback_control *wbc,
>  		u64 delalloc_start, unsigned long *nr_written)
>  {
> -	struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree;
>  	u64 page_end = delalloc_start + PAGE_SIZE - 1;
>  	bool found;
>  	u64 delalloc_to_write = 0;
> @@ -3300,8 +3299,7 @@ static noinline_for_stack int writepage_delalloc(struct inode *inode,
>  
>  
>  	while (delalloc_end < page_end) {
> -		found = find_lock_delalloc_range(inode, tree,
> -					       page,
> +		found = find_lock_delalloc_range(inode, page,
>  					       &delalloc_start,
>  					       &delalloc_end);
>  		if (!found) {
>
Goldwyn Rodrigues June 19, 2019, 11:36 a.m. UTC | #2
On  9:05 19/06, Nikolay Borisov wrote:
> 
> 
> On 19.06.19 г. 3:35 ч., Goldwyn Rodrigues wrote:
> > Simplification.
> > No point passing the tree variable when it can be evaluated
> > from inode.
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> The patch is good, however, there are several calls to find_
> lock_delalloc_range in btrfs tests so you'd need to fix those 
> invocations, otherwise compilation of the in-kernel test suite will fails. 
> 
> fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> 
> 

Oh, I missed the test, even if it was listed as EXPORT_FOR_TESTS.
Looking at the tests, it seems it needs to work on a different io_tree than
in the inode. So, I suppose this patch should NOT be included for the sake
of tests. Perhaps we should use wrapper functions for simplifications.
David Sterba June 19, 2019, 1:09 p.m. UTC | #3
On Wed, Jun 19, 2019 at 06:36:08AM -0500, Goldwyn Rodrigues wrote:
> On  9:05 19/06, Nikolay Borisov wrote:
> > 
> > 
> > On 19.06.19 г. 3:35 ч., Goldwyn Rodrigues wrote:
> > > Simplification.
> > > No point passing the tree variable when it can be evaluated
> > > from inode.
> > > 
> > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > The patch is good, however, there are several calls to find_
> > lock_delalloc_range in btrfs tests so you'd need to fix those 
> > invocations, otherwise compilation of the in-kernel test suite will fails. 
> > 
> > fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> > fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> > fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> > fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> > fs/btrfs/tests/extent-io-tests.c:       found = find_lock_delalloc_range(inode, &tmp, locked_page, &start,
> > 
> > 
> 
> Oh, I missed the test, even if it was listed as EXPORT_FOR_TESTS.
> Looking at the tests, it seems it needs to work on a different io_tree than
> in the inode.

If it's possible/correct to set the test inode tree pointer, then I
guess the change still applies. The testing code works on dummy
or otherwise crafted structures, so this kind of tweak does not sound
off.

Patch
diff mbox series

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index db337e53aab3..e9475d7e11bf 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1719,10 +1719,10 @@  static noinline int lock_delalloc_pages(struct inode *inode,
  */
 EXPORT_FOR_TESTS
 noinline_for_stack bool find_lock_delalloc_range(struct inode *inode,
-				    struct extent_io_tree *tree,
 				    struct page *locked_page, u64 *start,
 				    u64 *end)
 {
+	struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree;
 	u64 max_bytes = BTRFS_MAX_EXTENT_SIZE;
 	u64 delalloc_start;
 	u64 delalloc_end;
@@ -3290,7 +3290,6 @@  static noinline_for_stack int writepage_delalloc(struct inode *inode,
 		struct page *page, struct writeback_control *wbc,
 		u64 delalloc_start, unsigned long *nr_written)
 {
-	struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree;
 	u64 page_end = delalloc_start + PAGE_SIZE - 1;
 	bool found;
 	u64 delalloc_to_write = 0;
@@ -3300,8 +3299,7 @@  static noinline_for_stack int writepage_delalloc(struct inode *inode,
 
 
 	while (delalloc_end < page_end) {
-		found = find_lock_delalloc_range(inode, tree,
-					       page,
+		found = find_lock_delalloc_range(inode, page,
 					       &delalloc_start,
 					       &delalloc_end);
 		if (!found) {