* [PATCH 0/3] Cleanup backref_in_log and its callers
@ 2019-08-30 14:44 Nikolay Borisov
2019-08-30 14:44 ` [PATCH 1/3] btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log Nikolay Borisov
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Nikolay Borisov @ 2019-08-30 14:44 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
This series cleans up backref_in_log and its callers. Patch 1 removes the
opencoding of btrfs_find_name_in_backref which greatly simplifies backref_in_log
itself.
Patch 2 properly propagates error values of the internal functions to
backref_in_log's callers and also fixes, where necessary, callers to cope with
those ret values.
Patch 3 continues in the spirit of the previous patch, in that it open codes
name_in_log_ref so that the caller can properly handle backref_in_log retvals.
Nikolay Borisov (3):
btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log
btrfs: Properly handle backref_in_log retval
btrfs: Open-code name_in_log_ref in replay_one_name
fs/btrfs/tree-log.c | 113 +++++++++++++++++---------------------------
1 file changed, 44 insertions(+), 69 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log 2019-08-30 14:44 [PATCH 0/3] Cleanup backref_in_log and its callers Nikolay Borisov @ 2019-08-30 14:44 ` Nikolay Borisov 2019-09-24 17:09 ` David Sterba 2019-08-30 14:44 ` [PATCH 2/3] btrfs: Properly handle backref_in_log retval Nikolay Borisov 2019-08-30 14:44 ` [PATCH 3/3] btrfs: Open-code name_in_log_ref in replay_one_name Nikolay Borisov 2 siblings, 1 reply; 11+ messages in thread From: Nikolay Borisov @ 2019-08-30 14:44 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/tree-log.c | 48 ++++++++++++--------------------------------- 1 file changed, 12 insertions(+), 36 deletions(-) diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 77b6797fcac3..369046a754b0 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -945,54 +945,30 @@ static noinline int backref_in_log(struct btrfs_root *log, const char *name, int namelen) { struct btrfs_path *path; - struct btrfs_inode_ref *ref; - unsigned long ptr; - unsigned long ptr_end; - unsigned long name_ptr; - int found_name_len; - int item_size; int ret; - int match = 0; path = btrfs_alloc_path(); if (!path) return -ENOMEM; ret = btrfs_search_slot(NULL, log, key, path, 0, 0); - if (ret != 0) - goto out; - - ptr = btrfs_item_ptr_offset(path->nodes[0], path->slots[0]); - - if (key->type == BTRFS_INODE_EXTREF_KEY) { - if (btrfs_find_name_in_ext_backref(path->nodes[0], - path->slots[0], - ref_objectid, - name, namelen)) - match = 1; - + if (ret != 0) { + ret = 0; goto out; } - item_size = btrfs_item_size_nr(path->nodes[0], path->slots[0]); - ptr_end = ptr + item_size; - while (ptr < ptr_end) { - ref = (struct btrfs_inode_ref *)ptr; - found_name_len = btrfs_inode_ref_name_len(path->nodes[0], ref); - if (found_name_len == namelen) { - name_ptr = (unsigned long)(ref + 1); - ret = memcmp_extent_buffer(path->nodes[0], name, - name_ptr, namelen); - if (ret == 0) { - match = 1; - goto out; - } - } - ptr = (unsigned long)(ref + 1) + found_name_len; - } + if (key->type == BTRFS_INODE_EXTREF_KEY) + ret = !!btrfs_find_name_in_ext_backref(path->nodes[0], + path->slots[0], + ref_objectid, + name, namelen); + else + ret = !!btrfs_find_name_in_backref(path->nodes[0], + path->slots[0], + name, namelen); out: btrfs_free_path(path); - return match; + return ret; } static inline int __add_inode_ref(struct btrfs_trans_handle *trans, -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log 2019-08-30 14:44 ` [PATCH 1/3] btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log Nikolay Borisov @ 2019-09-24 17:09 ` David Sterba 0 siblings, 0 replies; 11+ messages in thread From: David Sterba @ 2019-09-24 17:09 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs On Fri, Aug 30, 2019 at 05:44:47PM +0300, Nikolay Borisov wrote: > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> I wrote a short changelog. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] btrfs: Properly handle backref_in_log retval 2019-08-30 14:44 [PATCH 0/3] Cleanup backref_in_log and its callers Nikolay Borisov 2019-08-30 14:44 ` [PATCH 1/3] btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log Nikolay Borisov @ 2019-08-30 14:44 ` Nikolay Borisov 2019-09-24 17:09 ` David Sterba 2019-08-30 14:44 ` [PATCH 3/3] btrfs: Open-code name_in_log_ref in replay_one_name Nikolay Borisov 2 siblings, 1 reply; 11+ messages in thread From: Nikolay Borisov @ 2019-08-30 14:44 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov 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> --- fs/btrfs/tree-log.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 369046a754b0..ca294ff463de 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -952,10 +952,8 @@ 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) { - ret = 0; + if (ret != 0) goto out; - } if (key->type == BTRFS_INODE_EXTREF_KEY) ret = !!btrfs_find_name_in_ext_backref(path->nodes[0], @@ -1026,10 +1024,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 +1092,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 +1872,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; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] btrfs: Properly handle backref_in_log retval 2019-08-30 14:44 ` [PATCH 2/3] btrfs: Properly handle backref_in_log retval Nikolay Borisov @ 2019-09-24 17:09 ` David Sterba 2019-09-25 11:03 ` [PATCH v2] " Nikolay Borisov 0 siblings, 1 reply; 11+ messages in thread From: David Sterba @ 2019-09-24 17:09 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs On Fri, Aug 30, 2019 at 05:44:48PM +0300, Nikolay Borisov 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> > --- > fs/btrfs/tree-log.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c > index 369046a754b0..ca294ff463de 100644 > --- a/fs/btrfs/tree-log.c > +++ b/fs/btrfs/tree-log.c > @@ -952,10 +952,8 @@ 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) { > - ret = 0; > + if (ret != 0) > goto out; > - } Is this correct? Before: nonzero -> zero, after the nonzero value is returned from the function. The return value is checked by the callers but this obscures the fact that it comes from btrfs_search_slot and is not just 0 or < 0. Because search slot returns 1 in case it does not find the key, which is translated to ENOENT in some contexts. Here it would be "false", as the reference is not found. So logically the return code of search slot and find backref is inverted. Due to this inverted logic I'd rather avoid propagating the semantics of search slot error value and always make it obvious that it follows the natural expectations after reading the code like: ret = backref_in_log() if (ret < 0) return ...; if (ret == 0) /* no, backref is not in log */ if (ret > 0) /* yes, backref is in the log */ > if (key->type == BTRFS_INODE_EXTREF_KEY) > ret = !!btrfs_find_name_in_ext_backref(path->nodes[0], > @@ -1026,10 +1024,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 +1092,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 +1872,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; The error handling in the code snippets above seems to follow the exptected logic but I don't see that it's correct due to the search slot value inversion. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] btrfs: Properly handle backref_in_log retval 2019-09-24 17:09 ` David Sterba @ 2019-09-25 11:03 ` Nikolay Borisov 2019-09-26 9:31 ` Filipe Manana 0 siblings, 1 reply; 11+ messages in thread From: Nikolay Borisov @ 2019-09-25 11:03 UTC (permalink / raw) To: linux-btrfs; +Cc: dsterba, Nikolay Borisov 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; return false; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] btrfs: Properly handle backref_in_log retval 2019-09-25 11:03 ` [PATCH v2] " Nikolay Borisov @ 2019-09-26 9:31 ` Filipe Manana 2019-09-26 10:39 ` Nikolay Borisov 0 siblings, 1 reply; 11+ messages in thread From: Filipe Manana @ 2019-09-26 9:31 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs, dsterba 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 > -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.” ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] btrfs: Properly handle backref_in_log retval 2019-09-26 9:31 ` Filipe Manana @ 2019-09-26 10:39 ` Nikolay Borisov 2019-10-03 12:55 ` David Sterba 0 siblings, 1 reply; 11+ messages in thread From: Nikolay Borisov @ 2019-09-26 10:39 UTC (permalink / raw) To: fdmanana; +Cc: dsterba, linux-btrfs 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 >> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] btrfs: Properly handle backref_in_log retval 2019-09-26 10:39 ` Nikolay Borisov @ 2019-10-03 12:55 ` David Sterba 2019-10-03 13:07 ` David Sterba 0 siblings, 1 reply; 11+ messages in thread From: David Sterba @ 2019-10-03 12:55 UTC (permalink / raw) To: Nikolay Borisov; +Cc: fdmanana, dsterba, linux-btrfs 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] btrfs: Properly handle backref_in_log retval 2019-10-03 12:55 ` David Sterba @ 2019-10-03 13:07 ` David Sterba 0 siblings, 0 replies; 11+ messages in thread From: David Sterba @ 2019-10-03 13:07 UTC (permalink / raw) To: dsterba, Nikolay Borisov, fdmanana, linux-btrfs 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] btrfs: Open-code name_in_log_ref in replay_one_name 2019-08-30 14:44 [PATCH 0/3] Cleanup backref_in_log and its callers Nikolay Borisov 2019-08-30 14:44 ` [PATCH 1/3] btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log Nikolay Borisov 2019-08-30 14:44 ` [PATCH 2/3] btrfs: Properly handle backref_in_log retval Nikolay Borisov @ 2019-08-30 14:44 ` Nikolay Borisov 2 siblings, 0 replies; 11+ messages in thread From: Nikolay Borisov @ 2019-08-30 14:44 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov That function adds unnecessary indirection between backref_in_log and the caller. Furthermore it also "downgrades" backref_in_log's return value to a boolean, when in fact it could very well be an error. Rectify the situation by simply opencoding name_in_log_ref in replay_one_name and properly handling possible return codes from backref_in_log. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/tree-log.c | 51 +++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 29 deletions(-) diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index ca294ff463de..822dd0486f7c 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -1863,33 +1863,6 @@ static noinline int insert_one_name(struct btrfs_trans_handle *trans, return ret; } -/* - * Return true if an inode reference exists in the log for the given name, - * inode and parent inode. - */ -static bool name_in_log_ref(struct btrfs_root *log_root, - const char *name, const int name_len, - 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; - 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); - ret = backref_in_log(log_root, &search_key, dirid, name, name_len); - if (ret == 1) - return true; - - return false; -} - /* * take a single entry in a log directory item and replay it into * the subvolume. @@ -2006,8 +1979,28 @@ static noinline int replay_one_name(struct btrfs_trans_handle *trans, return ret; insert: - if (name_in_log_ref(root->log_root, name, name_len, - key->objectid, log_key.objectid)) { + /* Ensure the to-be-added name is not already in the subvol tree */ + found_key.objectid = log_key.objectid; + found_key.type = BTRFS_INODE_REF_KEY; + found_key.offset = key->objectid; + ret = backref_in_log(root->log_root, &found_key, 0, name, name_len); + if (ret < 0) + goto out; + else if (ret) { + /* The dentry will be added later. */ + ret = 0; + update_size = false; + goto out; + } + + found_key.objectid = log_key.objectid; + found_key.type = BTRFS_INODE_EXTREF_KEY; + found_key.offset = key->objectid; + ret = backref_in_log(root->log_root, &found_key, key->objectid, name, + name_len); + if (ret < 0) { + goto out; + } else if (ret) { /* The dentry will be added later. */ ret = 0; update_size = false; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-10-03 13:07 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-30 14:44 [PATCH 0/3] Cleanup backref_in_log and its callers Nikolay Borisov 2019-08-30 14:44 ` [PATCH 1/3] btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log Nikolay Borisov 2019-09-24 17:09 ` David Sterba 2019-08-30 14:44 ` [PATCH 2/3] btrfs: Properly handle backref_in_log retval Nikolay Borisov 2019-09-24 17:09 ` David Sterba 2019-09-25 11:03 ` [PATCH v2] " Nikolay Borisov 2019-09-26 9:31 ` Filipe Manana 2019-09-26 10:39 ` Nikolay Borisov 2019-10-03 12:55 ` David Sterba 2019-10-03 13:07 ` David Sterba 2019-08-30 14:44 ` [PATCH 3/3] btrfs: Open-code name_in_log_ref in replay_one_name Nikolay Borisov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).