| Message ID | 20230720134123.13148-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: propagate error from function unpin_extent_cache() | expand |
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). 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. > + > if (ret < 0) { > btrfs_abort_transaction(trans, ret); > goto out;
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,
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"); + if (ret < 0) { btrfs_abort_transaction(trans, ret); goto out;
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(-)