[v2] btrfs: Properly handle backref_in_log retval
diff mbox series

Message ID 20190925110303.20466-1-nborisov@suse.com
State New
Headers show
Series
  • [v2] btrfs: Properly handle backref_in_log retval
Related show

Commit Message

Nikolay Borisov Sept. 25, 2019, 11:03 a.m. UTC
This function can return a negative error value if btrfs_search_slot
errors for whatever reason or if btrfs_alloc_path runs out of memory.
This is currently problemattic because backref_in_log is treated by its
callers as if it returns boolean.

Fix this by adding proper error handling in callers. That also enables
the function to return the direct error code from btrfs_search_slot.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

v2: 
 * Return 0 from backref_in_log in case btrfs_search_slot returns 1 (meaning it 
 didn't find appropriate item in the btree). 

 fs/btrfs/tree-log.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

Comments

Filipe Manana Sept. 26, 2019, 9:31 a.m. UTC | #1
On Thu, Sep 26, 2019 at 10:27 AM Nikolay Borisov <nborisov@suse.com> wrote:
>
> This function can return a negative error value if btrfs_search_slot
> errors for whatever reason or if btrfs_alloc_path runs out of memory.
> This is currently problemattic because backref_in_log is treated by its
> callers as if it returns boolean.
>
> Fix this by adding proper error handling in callers. That also enables
> the function to return the direct error code from btrfs_search_slot.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>
> v2:
>  * Return 0 from backref_in_log in case btrfs_search_slot returns 1 (meaning it
>  didn't find appropriate item in the btree).
>
>  fs/btrfs/tree-log.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 7cac09a6f007..7332f7a00790 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -952,7 +952,9 @@ static noinline int backref_in_log(struct btrfs_root *log,
>                 return -ENOMEM;
>
>         ret = btrfs_search_slot(NULL, log, key, path, 0, 0);
> -       if (ret != 0) {
> +       if (ret < 0) {
> +               goto out;
> +       } else if (ret == 1) {
>                 ret = 0;
>                 goto out;
>         }
> @@ -1026,10 +1028,13 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
>                                            (unsigned long)(victim_ref + 1),
>                                            victim_name_len);
>
> -                       if (!backref_in_log(log_root, &search_key,
> -                                           parent_objectid,
> -                                           victim_name,
> -                                           victim_name_len)) {
> +                       ret = backref_in_log(log_root, &search_key,
> +                                            parent_objectid, victim_name,
> +                                            victim_name_len);
> +                       if (ret < 0) {
> +                               kfree(victim_name);
> +                               return ret;
> +                       } else if (!ret) {
>                                 inc_nlink(&inode->vfs_inode);
>                                 btrfs_release_path(path);
>
> @@ -1091,10 +1096,12 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
>                         search_key.offset = btrfs_extref_hash(parent_objectid,
>                                                               victim_name,
>                                                               victim_name_len);
> -                       ret = 0;
> -                       if (!backref_in_log(log_root, &search_key,
> -                                           parent_objectid, victim_name,
> -                                           victim_name_len)) {
> +                       ret = backref_in_log(log_root, &search_key,
> +                                            parent_objectid, victim_name,
> +                                            victim_name_len);
> +                       if (ret < 0) {
> +                               return ret;
> +                       } else if (!ret) {
>                                 ret = -ENOENT;
>                                 victim_parent = read_one_inode(root,
>                                                 parent_objectid);
> @@ -1869,16 +1876,19 @@ static bool name_in_log_ref(struct btrfs_root *log_root,
>                             const u64 dirid, const u64 ino)
>  {
>         struct btrfs_key search_key;
> +       int ret;
>
>         search_key.objectid = ino;
>         search_key.type = BTRFS_INODE_REF_KEY;
>         search_key.offset = dirid;
> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
> +       if (ret == 1)
>                 return true;
>
>         search_key.type = BTRFS_INODE_EXTREF_KEY;
>         search_key.offset = btrfs_extref_hash(dirid, name, name_len);
> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
> +       if (ret == 1)
>                 return true;

This function also needs to be able to return errors and its caller
check for errors.

Thanks.

>
>         return false;
> --
> 2.17.1
>
Nikolay Borisov Sept. 26, 2019, 10:39 a.m. UTC | #2
On 26.09.19 г. 12:31 ч., Filipe Manana wrote:
> On Thu, Sep 26, 2019 at 10:27 AM Nikolay Borisov <nborisov@suse.com> wrote:
>>
>> This function can return a negative error value if btrfs_search_slot
>> errors for whatever reason or if btrfs_alloc_path runs out of memory.
>> This is currently problemattic because backref_in_log is treated by its
>> callers as if it returns boolean.
>>
>> Fix this by adding proper error handling in callers. That also enables
>> the function to return the direct error code from btrfs_search_slot.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>
>> v2:
>>  * Return 0 from backref_in_log in case btrfs_search_slot returns 1 (meaning it
>>  didn't find appropriate item in the btree).
>>
>>  fs/btrfs/tree-log.c | 32 +++++++++++++++++++++-----------
>>  1 file changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>> index 7cac09a6f007..7332f7a00790 100644
>> --- a/fs/btrfs/tree-log.c
>> +++ b/fs/btrfs/tree-log.c
>> @@ -952,7 +952,9 @@ static noinline int backref_in_log(struct btrfs_root *log,
>>                 return -ENOMEM;
>>
>>         ret = btrfs_search_slot(NULL, log, key, path, 0, 0);
>> -       if (ret != 0) {
>> +       if (ret < 0) {
>> +               goto out;
>> +       } else if (ret == 1) {
>>                 ret = 0;
>>                 goto out;
>>         }
>> @@ -1026,10 +1028,13 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
>>                                            (unsigned long)(victim_ref + 1),
>>                                            victim_name_len);
>>
>> -                       if (!backref_in_log(log_root, &search_key,
>> -                                           parent_objectid,
>> -                                           victim_name,
>> -                                           victim_name_len)) {
>> +                       ret = backref_in_log(log_root, &search_key,
>> +                                            parent_objectid, victim_name,
>> +                                            victim_name_len);
>> +                       if (ret < 0) {
>> +                               kfree(victim_name);
>> +                               return ret;
>> +                       } else if (!ret) {
>>                                 inc_nlink(&inode->vfs_inode);
>>                                 btrfs_release_path(path);
>>
>> @@ -1091,10 +1096,12 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
>>                         search_key.offset = btrfs_extref_hash(parent_objectid,
>>                                                               victim_name,
>>                                                               victim_name_len);
>> -                       ret = 0;
>> -                       if (!backref_in_log(log_root, &search_key,
>> -                                           parent_objectid, victim_name,
>> -                                           victim_name_len)) {
>> +                       ret = backref_in_log(log_root, &search_key,
>> +                                            parent_objectid, victim_name,
>> +                                            victim_name_len);
>> +                       if (ret < 0) {
>> +                               return ret;
>> +                       } else if (!ret) {
>>                                 ret = -ENOENT;
>>                                 victim_parent = read_one_inode(root,
>>                                                 parent_objectid);
>> @@ -1869,16 +1876,19 @@ static bool name_in_log_ref(struct btrfs_root *log_root,
>>                             const u64 dirid, const u64 ino)
>>  {
>>         struct btrfs_key search_key;
>> +       int ret;
>>
>>         search_key.objectid = ino;
>>         search_key.type = BTRFS_INODE_REF_KEY;
>>         search_key.offset = dirid;
>> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
>> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
>> +       if (ret == 1)
>>                 return true;
>>
>>         search_key.type = BTRFS_INODE_EXTREF_KEY;
>>         search_key.offset = btrfs_extref_hash(dirid, name, name_len);
>> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
>> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
>> +       if (ret == 1)
>>                 return true;
> 
> This function also needs to be able to return errors and its caller
> check for errors.

Yes but this is for a follow up patch. The current patch does not make
the code any more broken than it currently is.

> 
> Thanks.
> 
>>
>>         return false;
>> --
>> 2.17.1
>>
> 
>
David Sterba Oct. 3, 2019, 12:55 p.m. UTC | #3
On Thu, Sep 26, 2019 at 01:39:58PM +0300, Nikolay Borisov wrote:
> >> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
> >> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
> >> +       if (ret == 1)
> >>                 return true;
> > 
> > This function also needs to be able to return errors and its caller
> > check for errors.
> 
> Yes but this is for a follow up patch. The current patch does not make
> the code any more broken than it currently is.

I'm going to merge the patches, please send the followup patch soon, so
we don't forget about adding the proper error handling. Thanks.
David Sterba Oct. 3, 2019, 1:07 p.m. UTC | #4
On Thu, Oct 03, 2019 at 02:55:59PM +0200, David Sterba wrote:
> On Thu, Sep 26, 2019 at 01:39:58PM +0300, Nikolay Borisov wrote:
> > >> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
> > >> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
> > >> +       if (ret == 1)
> > >>                 return true;
> > > 
> > > This function also needs to be able to return errors and its caller
> > > check for errors.
> > 
> > Yes but this is for a follow up patch. The current patch does not make
> > the code any more broken than it currently is.
> 
> I'm going to merge the patches, please send the followup patch soon, so
> we don't forget about adding the proper error handling. Thanks.

Never mind, the patch 3/3 inlines the function and the error handling is
integrated to the caller.

Patch
diff mbox series

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 7cac09a6f007..7332f7a00790 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -952,7 +952,9 @@  static noinline int backref_in_log(struct btrfs_root *log,
 		return -ENOMEM;
 
 	ret = btrfs_search_slot(NULL, log, key, path, 0, 0);
-	if (ret != 0) {
+	if (ret < 0) {
+		goto out;
+	} else if (ret == 1) {
 		ret = 0;
 		goto out;
 	}
@@ -1026,10 +1028,13 @@  static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 					   (unsigned long)(victim_ref + 1),
 					   victim_name_len);
 
-			if (!backref_in_log(log_root, &search_key,
-					    parent_objectid,
-					    victim_name,
-					    victim_name_len)) {
+			ret = backref_in_log(log_root, &search_key,
+					     parent_objectid, victim_name,
+					     victim_name_len);
+			if (ret < 0) {
+				kfree(victim_name);
+				return ret;
+			} else if (!ret) {
 				inc_nlink(&inode->vfs_inode);
 				btrfs_release_path(path);
 
@@ -1091,10 +1096,12 @@  static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 			search_key.offset = btrfs_extref_hash(parent_objectid,
 							      victim_name,
 							      victim_name_len);
-			ret = 0;
-			if (!backref_in_log(log_root, &search_key,
-					    parent_objectid, victim_name,
-					    victim_name_len)) {
+			ret = backref_in_log(log_root, &search_key,
+					     parent_objectid, victim_name,
+					     victim_name_len);
+			if (ret < 0) {
+				return ret;
+			} else if (!ret) {
 				ret = -ENOENT;
 				victim_parent = read_one_inode(root,
 						parent_objectid);
@@ -1869,16 +1876,19 @@  static bool name_in_log_ref(struct btrfs_root *log_root,
 			    const u64 dirid, const u64 ino)
 {
 	struct btrfs_key search_key;
+	int ret;
 
 	search_key.objectid = ino;
 	search_key.type = BTRFS_INODE_REF_KEY;
 	search_key.offset = dirid;
-	if (backref_in_log(log_root, &search_key, dirid, name, name_len))
+	ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
+	if (ret == 1)
 		return true;
 
 	search_key.type = BTRFS_INODE_EXTREF_KEY;
 	search_key.offset = btrfs_extref_hash(dirid, name, name_len);
-	if (backref_in_log(log_root, &search_key, dirid, name, name_len))
+	ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
+	if (ret == 1)
 		return true;
 
 	return false;