btrfs: fiemap: make the assert more explicit after handling the error cases

[PATCH] btrfs: fiemap: make the assert more explicit after handling the error cases

Posted by Daniel Vacek 8 months ago

Let's not assert the errors and clearly state the expected result only
after eventual error handling. It makes a bit more sense this way.

Signed-off-by: Daniel Vacek <neelx@suse.com>
---
 fs/btrfs/fiemap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/fiemap.c b/fs/btrfs/fiemap.c
index b80c07ad8c5e7..034f832e10c1a 100644
--- a/fs/btrfs/fiemap.c
+++ b/fs/btrfs/fiemap.c
@@ -568,10 +568,10 @@ static int fiemap_find_last_extent_offset(struct btrfs_inode *inode,
 	 * there might be preallocation past i_size.
 	 */
 	ret = btrfs_lookup_file_extent(NULL, root, path, ino, (u64)-1, 0);
-	/* There can't be a file extent item at offset (u64)-1 */
-	ASSERT(ret != 0);
 	if (ret < 0)
 		return ret;
+	/* There can't be a file extent item at offset (u64)-1 */
+	ASSERT(ret == 1);
 
 	/*
 	 * For a non-existing key, btrfs_search_slot() always leaves us at a
-- 
2.47.2

Re: [PATCH] btrfs: fiemap: make the assert more explicit after handling the error cases

Posted by Filipe Manana 8 months ago

On Wed, Apr 23, 2025 at 9:10 AM Daniel Vacek <neelx@suse.com> wrote:
>
> Let's not assert the errors and clearly state the expected result only
> after eventual error handling. It makes a bit more sense this way.

It doesn't make more sense to me...
I prefer to assert expected results right after the function call.

Thanks.

>
> Signed-off-by: Daniel Vacek <neelx@suse.com>
> ---
>  fs/btrfs/fiemap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/fiemap.c b/fs/btrfs/fiemap.c
> index b80c07ad8c5e7..034f832e10c1a 100644
> --- a/fs/btrfs/fiemap.c
> +++ b/fs/btrfs/fiemap.c
> @@ -568,10 +568,10 @@ static int fiemap_find_last_extent_offset(struct btrfs_inode *inode,
>          * there might be preallocation past i_size.
>          */
>         ret = btrfs_lookup_file_extent(NULL, root, path, ino, (u64)-1, 0);
> -       /* There can't be a file extent item at offset (u64)-1 */
> -       ASSERT(ret != 0);
>         if (ret < 0)
>                 return ret;
> +       /* There can't be a file extent item at offset (u64)-1 */
> +       ASSERT(ret == 1);
>
>         /*
>          * For a non-existing key, btrfs_search_slot() always leaves us at a
> --
> 2.47.2
>
>

Re: [PATCH] btrfs: fiemap: make the assert more explicit after handling the error cases

Posted by Daniel Vacek 8 months ago

On Wed, 23 Apr 2025 at 11:04, Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Wed, Apr 23, 2025 at 9:10 AM Daniel Vacek <neelx@suse.com> wrote:
> >
> > Let's not assert the errors and clearly state the expected result only
> > after eventual error handling. It makes a bit more sense this way.
>
> It doesn't make more sense to me...
> I prefer to assert expected results right after the function call.

Oh well, if an error is expected then I get it. Is an error likely
here? I understood the comment says there can't be a file extent item
at offset (u64)-1 which implies a strict return value of 1 and not an
error or something >1. So that's why. And it's still quite after the
function call.

But I'm happy to scratch it if you don't like it.

> Thanks.
>
> >
> > Signed-off-by: Daniel Vacek <neelx@suse.com>
> > ---
> >  fs/btrfs/fiemap.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/fiemap.c b/fs/btrfs/fiemap.c
> > index b80c07ad8c5e7..034f832e10c1a 100644
> > --- a/fs/btrfs/fiemap.c
> > +++ b/fs/btrfs/fiemap.c
> > @@ -568,10 +568,10 @@ static int fiemap_find_last_extent_offset(struct btrfs_inode *inode,
> >          * there might be preallocation past i_size.
> >          */
> >         ret = btrfs_lookup_file_extent(NULL, root, path, ino, (u64)-1, 0);
> > -       /* There can't be a file extent item at offset (u64)-1 */
> > -       ASSERT(ret != 0);
> >         if (ret < 0)
> >                 return ret;
> > +       /* There can't be a file extent item at offset (u64)-1 */
> > +       ASSERT(ret == 1);
> >
> >         /*
> >          * For a non-existing key, btrfs_search_slot() always leaves us at a
> > --
> > 2.47.2
> >
> >

Re: [PATCH] btrfs: fiemap: make the assert more explicit after handling the error cases

Posted by Filipe Manana 8 months ago

On Wed, Apr 23, 2025 at 10:48 AM Daniel Vacek <neelx@suse.com> wrote:
>
> On Wed, 23 Apr 2025 at 11:04, Filipe Manana <fdmanana@kernel.org> wrote:
> >
> > On Wed, Apr 23, 2025 at 9:10 AM Daniel Vacek <neelx@suse.com> wrote:
> > >
> > > Let's not assert the errors and clearly state the expected result only
> > > after eventual error handling. It makes a bit more sense this way.
> >
> > It doesn't make more sense to me...
> > I prefer to assert expected results right after the function call.
>
> Oh well, if an error is expected then I get it. Is an error likely
> here?

The assertion serves to state what is never expected, and not what is
likely or unlikely.
It's about stating that an exact match shouldn't happen, i.e. ret == 0.

We do this sort of asserts in many places, and I find it more clear this way.

> I understood the comment says there can't be a file extent item
> at offset (u64)-1 which implies a strict return value of 1 and not an
> error or something >1. So that's why. And it's still quite after the
> function call.
>
> But I'm happy to scratch it if you don't like it.
>
> > Thanks.
> >
> > >
> > > Signed-off-by: Daniel Vacek <neelx@suse.com>
> > > ---
> > >  fs/btrfs/fiemap.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/btrfs/fiemap.c b/fs/btrfs/fiemap.c
> > > index b80c07ad8c5e7..034f832e10c1a 100644
> > > --- a/fs/btrfs/fiemap.c
> > > +++ b/fs/btrfs/fiemap.c
> > > @@ -568,10 +568,10 @@ static int fiemap_find_last_extent_offset(struct btrfs_inode *inode,
> > >          * there might be preallocation past i_size.
> > >          */
> > >         ret = btrfs_lookup_file_extent(NULL, root, path, ino, (u64)-1, 0);
> > > -       /* There can't be a file extent item at offset (u64)-1 */
> > > -       ASSERT(ret != 0);
> > >         if (ret < 0)
> > >                 return ret;
> > > +       /* There can't be a file extent item at offset (u64)-1 */
> > > +       ASSERT(ret == 1);
> > >
> > >         /*
> > >          * For a non-existing key, btrfs_search_slot() always leaves us at a
> > > --
> > > 2.47.2
> > >
> > >

Re: [PATCH] btrfs: fiemap: make the assert more explicit after handling the error cases

Posted by Daniel Vacek 8 months ago

On Wed, 23 Apr 2025 at 11:55, Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Wed, Apr 23, 2025 at 10:48 AM Daniel Vacek <neelx@suse.com> wrote:
> >
> > On Wed, 23 Apr 2025 at 11:04, Filipe Manana <fdmanana@kernel.org> wrote:
> > >
> > > On Wed, Apr 23, 2025 at 9:10 AM Daniel Vacek <neelx@suse.com> wrote:
> > > >
> > > > Let's not assert the errors and clearly state the expected result only
> > > > after eventual error handling. It makes a bit more sense this way.
> > >
> > > It doesn't make more sense to me...
> > > I prefer to assert expected results right after the function call.
> >
> > Oh well, if an error is expected then I get it. Is an error likely
> > here?
>
> The assertion serves to state what is never expected, and not what is
> likely or unlikely.
> It's about stating that an exact match shouldn't happen, i.e. ret == 0.
>
> We do this sort of asserts in many places, and I find it more clear this way.

I see. Forget it then.

Thanks.

> > I understood the comment says there can't be a file extent item
> > at offset (u64)-1 which implies a strict return value of 1 and not an
> > error or something >1. So that's why. And it's still quite after the
> > function call.
> >
> > But I'm happy to scratch it if you don't like it.
> >
> > > Thanks.
> > >
> > > >
> > > > Signed-off-by: Daniel Vacek <neelx@suse.com>
> > > > ---
> > > >  fs/btrfs/fiemap.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/btrfs/fiemap.c b/fs/btrfs/fiemap.c
> > > > index b80c07ad8c5e7..034f832e10c1a 100644
> > > > --- a/fs/btrfs/fiemap.c
> > > > +++ b/fs/btrfs/fiemap.c
> > > > @@ -568,10 +568,10 @@ static int fiemap_find_last_extent_offset(struct btrfs_inode *inode,
> > > >          * there might be preallocation past i_size.
> > > >          */
> > > >         ret = btrfs_lookup_file_extent(NULL, root, path, ino, (u64)-1, 0);
> > > > -       /* There can't be a file extent item at offset (u64)-1 */
> > > > -       ASSERT(ret != 0);
> > > >         if (ret < 0)
> > > >                 return ret;
> > > > +       /* There can't be a file extent item at offset (u64)-1 */
> > > > +       ASSERT(ret == 1);
> > > >
> > > >         /*
> > > >          * For a non-existing key, btrfs_search_slot() always leaves us at a
> > > > --
> > > > 2.47.2
> > > >
> > > >