[PATCH 0/4] btrfs: some misc cleanups and a fix around page reading

[PATCH 0/4] btrfs: some misc cleanups and a fix around page reading

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

A small set of changes, mostly cleanups, a fix for an error that is not
being returned, and adding a couple lockdep assertions.

Filipe Manana (4):
  btrfs: stop checking for NULL return from btrfs_get_extent()
  btrfs: fix lost error return value when reading a data page
  btrfs: remove no longer used counter when reading data page
  btrfs: assert we have a write lock when removing and replacing extent
    maps

 fs/btrfs/extent_io.c              | 12 +++++-------
 fs/btrfs/extent_map.c             |  4 ++++
 fs/btrfs/inode.c                  |  9 +++++++--
 fs/btrfs/tests/extent-map-tests.c |  2 ++
 4 files changed, 18 insertions(+), 9 deletions(-)

-- 
2.33.0

[PATCH 1/4] btrfs: stop checking for NULL return from btrfs_get_extent()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At extent_io.c, in the read page and write page code paths, we are testing
if the return value from btrfs_get_extent() can be NULL. However that is
not possible, as btrfs_get_extent() always returns either an error pointer
or a (non-NULL) pointer to an extent map structure.

Eveywhere else outside extent_io.c we never check for NULL, we always
treat any returned value as a non-NULL pointer if it does not encode an
error.

So check only for the IS_ERR() case at extent_io.c.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/extent_io.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 409bad3928db..8c09038e3f0f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3534,7 +3534,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
 	}
 
 	em = btrfs_get_extent(BTRFS_I(inode), page, pg_offset, start, len);
-	if (em_cached && !IS_ERR_OR_NULL(em)) {
+	if (em_cached && !IS_ERR(em)) {
 		BUG_ON(*em_cached);
 		refcount_inc(&em->refs);
 		*em_cached = em;
@@ -3608,7 +3608,7 @@ int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
 		}
 		em = __get_extent_map(inode, page, pg_offset, cur,
 				      end - cur + 1, em_cached);
-		if (IS_ERR_OR_NULL(em)) {
+		if (IS_ERR(em)) {
 			unlock_extent(tree, cur, end);
 			end_page_read(page, false, cur, end + 1 - cur);
 			break;
@@ -3951,7 +3951,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
 		}
 
 		em = btrfs_get_extent(inode, NULL, 0, cur, end - cur + 1);
-		if (IS_ERR_OR_NULL(em)) {
+		if (IS_ERR(em)) {
 			btrfs_page_set_error(fs_info, page, cur, end - cur + 1);
 			ret = PTR_ERR_OR_ZERO(em);
 			break;
-- 
2.33.0

Re: [PATCH 1/4] btrfs: stop checking for NULL return from btrfs_get_extent()

[Johannes Thumshirn]

On 03/02/2022 16:37, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> At extent_io.c, in the read page and write page code paths, we are testing
> if the return value from btrfs_get_extent() can be NULL. However that is
> not possible, as btrfs_get_extent() always returns either an error pointer
> or a (non-NULL) pointer to an extent map structure.
> 
> Eveywhere else outside extent_io.c we never check for NULL, we always
> treat any returned value as a non-NULL pointer if it does not encode an
> error.
> 
> So check only for the IS_ERR() case at extent_io.c.
> 

Isn't the same true for btrfs_get_extent_fiemap()? In get_extent_skip_holes() 
we're also checking for IS_ERR_OR_NULL() but AFAICS btrfs_get_extent_fiemap()
will never return NULL only a valid em or an error pointer.

Anyways for this patch:
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 1/4] btrfs: stop checking for NULL return from btrfs_get_extent()

[Filipe Manana]

On Fri, Feb 04, 2022 at 08:19:08AM +0000, Johannes Thumshirn wrote:
> On 03/02/2022 16:37, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> > 
> > At extent_io.c, in the read page and write page code paths, we are testing
> > if the return value from btrfs_get_extent() can be NULL. However that is
> > not possible, as btrfs_get_extent() always returns either an error pointer
> > or a (non-NULL) pointer to an extent map structure.
> > 
> > Eveywhere else outside extent_io.c we never check for NULL, we always
> > treat any returned value as a non-NULL pointer if it does not encode an
> > error.
> > 
> > So check only for the IS_ERR() case at extent_io.c.
> > 
> 
> Isn't the same true for btrfs_get_extent_fiemap()? In get_extent_skip_holes() 
> we're also checking for IS_ERR_OR_NULL() but AFAICS btrfs_get_extent_fiemap()
> will never return NULL only a valid em or an error pointer.

Yep.
I was focused on the read and write page paths, due to the next change, so I
missed that one.

I can send another patch to fix that one, or if you want to do it yourself,
please go ahead.

Thanks.

> 
> Anyways for this patch:
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 2/4] btrfs: fix lost error return value when reading a data page

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At btrfs_do_readpage(), if we get an error when trying to lookup for an
extent map, we end up marking the page with the error bit, clearing
the uptodate bit on it, and doing everything else that should be done.
However we return success (0) to the caller, when we should return the
error encoded in the extent map pointer. So fix that by returning the
error encoded in the pointer.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/extent_io.c | 1 +
 fs/btrfs/inode.c     | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 8c09038e3f0f..e713355c0e15 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3611,6 +3611,7 @@ int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
 		if (IS_ERR(em)) {
 			unlock_extent(tree, cur, end);
 			end_page_read(page, false, cur, end + 1 - cur);
+			ret = PTR_ERR(em);
 			break;
 		}
 		extent_offset = cur - em->start;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0e04624e557d..9f2c9e93eafe 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8095,8 +8095,13 @@ int btrfs_readpage(struct file *file, struct page *page)
 	btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
 
 	ret = btrfs_do_readpage(page, NULL, &bio_ctrl, 0, NULL);
-	if (bio_ctrl.bio)
-		ret = submit_one_bio(bio_ctrl.bio, 0, bio_ctrl.bio_flags);
+	if (bio_ctrl.bio) {
+		int ret2;
+
+		ret2 = submit_one_bio(bio_ctrl.bio, 0, bio_ctrl.bio_flags);
+		if (ret == 0)
+			ret = ret2;
+	}
 	return ret;
 }
 
-- 
2.33.0

[PATCH 3/4] btrfs: remove no longer used counter when reading data page

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

After commit 92082d40976ed0 ("btrfs: integrate page status update for
data read path into begin/end_page_read"), the 'nr' counter at
btrfs_do_readpage() is no longer used, we increment it but we never
read from it. So just remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/extent_io.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index e713355c0e15..5f7f902076f3 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3563,7 +3563,6 @@ int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
 	u64 cur_end;
 	struct extent_map *em;
 	int ret = 0;
-	int nr = 0;
 	size_t pg_offset = 0;
 	size_t iosize;
 	size_t blocksize = inode->i_sb->s_blocksize;
@@ -3722,9 +3721,7 @@ int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
 					 end_bio_extent_readpage, 0,
 					 this_bio_flag,
 					 force_bio_submit);
-		if (!ret) {
-			nr++;
-		} else {
+		if (ret) {
 			unlock_extent(tree, cur, cur + iosize - 1);
 			end_page_read(page, false, cur, iosize);
 			goto out;
-- 
2.33.0

[PATCH 4/4] btrfs: assert we have a write lock when removing and replacing extent maps

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Removing or replacing an extent map requires holding a write lock on the
extent map's tree. We currently do that everywhere, except in one of the
self tests, where it's harmless since there's no concurrency.

In order to catch possible races in the future, assert that we are holding
a write lock on the extent map tree before removing or replacing an extent
map in the tree, and update the self test to obtain a write lock before
removing extent maps.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/extent_map.c             | 4 ++++
 fs/btrfs/tests/extent-map-tests.c | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 5a36add21305..ba43303cb081 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -490,6 +490,8 @@ struct extent_map *search_extent_mapping(struct extent_map_tree *tree,
  */
 void remove_extent_mapping(struct extent_map_tree *tree, struct extent_map *em)
 {
+	lockdep_assert_held_write(&tree->lock);
+
 	WARN_ON(test_bit(EXTENT_FLAG_PINNED, &em->flags));
 	rb_erase_cached(&em->rb_node, &tree->map);
 	if (!test_bit(EXTENT_FLAG_LOGGING, &em->flags))
@@ -504,6 +506,8 @@ void replace_extent_mapping(struct extent_map_tree *tree,
 			    struct extent_map *new,
 			    int modified)
 {
+	lockdep_assert_held_write(&tree->lock);
+
 	WARN_ON(test_bit(EXTENT_FLAG_PINNED, &cur->flags));
 	ASSERT(extent_map_in_tree(cur));
 	if (!test_bit(EXTENT_FLAG_LOGGING, &cur->flags))
diff --git a/fs/btrfs/tests/extent-map-tests.c b/fs/btrfs/tests/extent-map-tests.c
index 319fed82d741..c5b3a631bf4f 100644
--- a/fs/btrfs/tests/extent-map-tests.c
+++ b/fs/btrfs/tests/extent-map-tests.c
@@ -15,6 +15,7 @@ static void free_extent_map_tree(struct extent_map_tree *em_tree)
 	struct extent_map *em;
 	struct rb_node *node;
 
+	write_lock(&em_tree->lock);
 	while (!RB_EMPTY_ROOT(&em_tree->map.rb_root)) {
 		node = rb_first_cached(&em_tree->map);
 		em = rb_entry(node, struct extent_map, rb_node);
@@ -32,6 +33,7 @@ static void free_extent_map_tree(struct extent_map_tree *em_tree)
 #endif
 		free_extent_map(em);
 	}
+	write_unlock(&em_tree->lock);
 }
 
 /*
-- 
2.33.0

Re: [PATCH 0/4] btrfs: some misc cleanups and a fix around page reading

[Johannes Thumshirn]

On 03/02/2022 16:36, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> A small set of changes, mostly cleanups, a fix for an error that is not
> being returned, and adding a couple lockdep assertions.
> 
> Filipe Manana (4):
>   btrfs: stop checking for NULL return from btrfs_get_extent()
>   btrfs: fix lost error return value when reading a data page
>   btrfs: remove no longer used counter when reading data page
>   btrfs: assert we have a write lock when removing and replacing extent
>     maps
> 
>  fs/btrfs/extent_io.c              | 12 +++++-------
>  fs/btrfs/extent_map.c             |  4 ++++
>  fs/btrfs/inode.c                  |  9 +++++++--
>  fs/btrfs/tests/extent-map-tests.c |  2 ++
>  4 files changed, 18 insertions(+), 9 deletions(-)
> 

For the whole series
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 0/4] btrfs: some misc cleanups and a fix around page reading

[David Sterba]

On Thu, Feb 03, 2022 at 03:36:41PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> A small set of changes, mostly cleanups, a fix for an error that is not
> being returned, and adding a couple lockdep assertions.
> 
> Filipe Manana (4):
>   btrfs: stop checking for NULL return from btrfs_get_extent()
>   btrfs: fix lost error return value when reading a data page
>   btrfs: remove no longer used counter when reading data page
>   btrfs: assert we have a write lock when removing and replacing extent
>     maps

Added as topic branch to for-next, thanks.

Re: [PATCH 0/4] btrfs: some misc cleanups and a fix around page reading

[David Sterba]

On Fri, Feb 04, 2022 at 05:35:27PM +0100, David Sterba wrote:
> On Thu, Feb 03, 2022 at 03:36:41PM +0000, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> > 
> > A small set of changes, mostly cleanups, a fix for an error that is not
> > being returned, and adding a couple lockdep assertions.
> > 
> > Filipe Manana (4):
> >   btrfs: stop checking for NULL return from btrfs_get_extent()
> >   btrfs: fix lost error return value when reading a data page
> >   btrfs: remove no longer used counter when reading data page
> >   btrfs: assert we have a write lock when removing and replacing extent
> >     maps
> 
> Added as topic branch to for-next, thanks.

Moved to misc-next.