| Message ID | 20190619003524.32377-1-rgoldwyn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Evaluate io_tree in find_lock_delalloc_range() | expand |
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) { >
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.
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.
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) {
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(-)