Messages in this thread | ![/](/images/icornerl.gif) | | From | Luís Henriques <> | Subject | Re: [PATCH v2] btrfs: propagate error from function unpin_extent_cache() | Date | Fri, 21 Jul 2023 10:02:24 +0100 |
| |
Filipe Manana <fdmanana@kernel.org> writes:
> On Thu, Jul 20, 2023 at 5:05 PM Luís Henriques <lhenriques@suse.de> wrote: >> >> Function unpin_extent_cache() doesn't propagate an error if the call to >> lookup_extent_mapping() fails. This patch adds an error return (EINVAL) >> and simply logs it in the only caller. >> >> Signed-off-by: Luís Henriques <lhenriques@suse.de> >> --- >> Hi! >> >> As per David and Johannes reviews, I'm now proposing a different approach. >> Note that I kept the WARN_ON() instead of replacing it by an ASSERT(). In >> fact, I considered removing the WARN_ON() completely and simply return the >> error if em->start != start. But I guess it may useful for debug. >> >> Changes since v1: >> Instead of changing unpin_extent_cache() into a void function, make it >> propage an error code instead. >> >> fs/btrfs/extent_map.c | 4 +++- >> fs/btrfs/inode.c | 8 ++++++-- >> 2 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c >> index 0cdb3e86f29b..f4e7956edc05 100644 >> --- a/fs/btrfs/extent_map.c >> +++ b/fs/btrfs/extent_map.c >> @@ -304,8 +304,10 @@ int unpin_extent_cache(struct extent_map_tree *tree, u64 start, u64 len, >> >> WARN_ON(!em || em->start != start); >> >> - if (!em) >> + if (!em) { >> + ret = -EINVAL; >> goto out; >> + } >> >> em->generation = gen; >> clear_bit(EXTENT_FLAG_PINNED, &em->flags); >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index dbbb67293e34..21eb66fcc0df 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -3273,8 +3273,12 @@ int btrfs_finish_one_ordered(struct btrfs_ordered_extent *ordered_extent) >> ordered_extent->disk_num_bytes); >> } >> } >> - unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset, >> - ordered_extent->num_bytes, trans->transid); >> + >> + /* Proceed even if we fail to unpin extent from cache */ >> + if (unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset, >> + ordered_extent->num_bytes, trans->transid) < 0) >> + btrfs_warn(fs_info, "failed to unpin extent from cache"); > > Well, this is not very useful. It doesn't provide any more useful > information than what we get from the WARN_ON() at > unpin_extent_cache(), making the patch not useful. > > This warning has actually happened a few times when running fstests > that exercise relocation (not sure if it's gone and accidently fixed > by something recently).
Oh! In that case replacing the WARN_ON() by an ASSERT() would have definitely be a bad idea.
> But to make this more useful, I would place the message at > unpin_extent_cache() with useful information such as: > > - inode number > - id of the root the inode belongs to > - the file offset (the start argument) and extent length (or end offset) > - why the warning triggered: we didn't find the extent map or we found > one with a different start offset > - if we found an unexpected extent map, dump its flags (so we can see > if it happens only with compressed or prealloc extents for e.g.) and > other details (length/end offset for e.g.)
Thanks, Filipe. This all makes sense. And in this case I guess I should go back to the initial approach of changing the function to a void function, but adding all this useful information.
Cheers, -- Luís
| ![\](/images/icornerr.gif) |