Skip to contentSkip to conversation

Btrfs-progs: fix check in btrfs_lookup_extent_info()

PATCH New Filipe David Borba Manana 2 participants 6 messages Jul 3, 2013 – Jul 5, 2013 Btrfs file system

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Btrfs-progs: fix check in btrfs_lookup_extent_info()

We want to test if path->slots[0] is greater than zero. Testing for path->slots was a logical error, as it corresponds to a memory address (start of fixed array) and therefore is always non-zero.

---
 extent-tree.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
extent-tree.c +1 -1
index b0cfe0a..f094266 100644
@@ -1515,7 +1515,7 @@ again:
 	 * to make sure.
 	 */
 	if (ret > 0 && metadata) {
-		if (path->slots) {
+		if (path->slots[0]) {
 			path->slots[0]--;
 			btrfs_item_key_to_cpu(path->nodes[0], &key,
 					      path->slots[0]);
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH 1/1 Btrfs-progs: fix optimization in btrfs_lookup_extent_info

If we did a tree search with the goal to find a metadata item but the search failed with return value 1, we attempt to see if in the same leaf there's a corresponding extent item, and if there's one, just use it instead of doing another tree search for this extent item. The check in the leaf was wrong because it was seeking for a metadata item instead of an extent item.

This optimization was also being triggered incorrectly, as it was evaluating path->slots which always evaluates to true. The goal was to see if the leaf level slot was greater than zero (i.e. not the first item in the leaf).

---
 extent-tree.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
extent-tree.c +3 -2
index b0cfe0a..22e6247 100644
@@ -1515,12 +1515,13 @@ again:
 	 * to make sure.
 	 */
 	if (ret > 0 && metadata) {
-		if (path->slots) {
+		if (path->slots[0]) {
 			path->slots[0]--;
 			btrfs_item_key_to_cpu(path->nodes[0], &key,
 					      path->slots[0]);
 			if (key.objectid == bytenr &&
-			    key.type == BTRFS_METADATA_ITEM_KEY)
+			    key.type == BTRFS_EXTENT_ITEM_KEY &&
+			    key.offset == root->leafsize)
 				ret = 0;
 		}
 
Signature · 1 line
1.7.9.5
#2 lore raw

Re: [PATCH] Btrfs-progs: fix optimization in btrfs_lookup_extent_info

Quoted 38 lines · Filipe David Borba Manana

If we did a tree search with the goal to find a metadata item but the search failed with return value 1, we attempt to see if in the same leaf there's a corresponding extent item, and if there's one, just use it instead of doing another tree search for this extent item. The check in the leaf was wrong because it was seeking for a metadata item instead of an extent item.

This optimization was also being triggered incorrectly, as it was evaluating path->slots which always evaluates to true. The goal was to see if the leaf level slot was greater than zero (i.e. not the first item in the leaf).

---
 extent-tree.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
extent-tree.c +3 -3
index b0cfe0a..22e6247 100644
@@ -1515,12 +1515,13 @@ again:
         * to make sure.
         */
        if (ret > 0 && metadata) {
-               if (path->slots) {
+               if (path->slots[0]) {
                        path->slots[0]--;
                        btrfs_item_key_to_cpu(path->nodes[0], &key,
                                              path->slots[0]);
                        if (key.objectid == bytenr &&
-                           key.type == BTRFS_METADATA_ITEM_KEY)
+                           key.type == BTRFS_EXTENT_ITEM_KEY &&
+                           key.offset == root->leafsize)
                                ret = 0;
                }
 
--
Quoted 2 lines

1.7.9.5

Josef, since git suggests you are the author of this code piece, can you please review this and comment?
Thanks.

--
Filipe David Manana,

"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men."

#3 lore raw

PATCH v2 1/1 Btrfs-progs: fix optimization in btrfs_lookup_extent_info

In reply to Filipe David Borba Manana

If we did a tree search with the goal to find a metadata item but the search failed with return value 1, we attempt to see if in the same leaf there's a corresponding extent item, and if there's one, just use it instead of doing another tree search for this extent item. The check in the leaf was wrong because it was seeking for a metadata item instead of an extent item.

This optimization was also being triggered incorrectly, as it was evaluating path->slots which always evaluates to true. The goal was to see if the leaf level slot was greater than zero (i.e. not the first item in the leaf).

V2: If previous leaf item is for a different object, ensure the search key has the target object id.

---
 extent-tree.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
extent-tree.c +4 -2
index b0cfe0a..875dea9 100644
@@ -1515,17 +1515,19 @@ again:
 	 * to make sure.
 	 */
 	if (ret > 0 && metadata) {
-		if (path->slots) {
+		if (path->slots[0]) {
 			path->slots[0]--;
 			btrfs_item_key_to_cpu(path->nodes[0], &key,
 					      path->slots[0]);
 			if (key.objectid == bytenr &&
-			    key.type == BTRFS_METADATA_ITEM_KEY)
+			    key.type == BTRFS_EXTENT_ITEM_KEY &&
+			    key.offset == root->leafsize)
 				ret = 0;
 		}
 
 		if (ret) {
 			btrfs_release_path(root, path);
+			key.objectid = bytenr;
 			key.type = BTRFS_EXTENT_ITEM_KEY;
 			key.offset = root->leafsize;
 			metadata = 0;
Signature · 1 line
1.7.9.5
Reviewed-by on v2 1/1
#4 lore raw

Re: [PATCH v2] Btrfs-progs: fix optimization in btrfs_lookup_extent_info

Quoted 36 lines · Filipe David Borba Manana

If we did a tree search with the goal to find a metadata item but the search failed with return value 1, we attempt to see if in the same leaf there's a corresponding extent item, and if there's one, just use it instead of doing another tree search for this extent item. The check in the leaf was wrong because it was seeking for a metadata item instead of an extent item.

This optimization was also being triggered incorrectly, as it was evaluating path->slots which always evaluates to true. The goal was to see if the leaf level slot was greater than zero (i.e. not the first item in the leaf).

V2: If previous leaf item is for a different object, ensure the search key has the target object id.

---
 extent-tree.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
extent-tree.c +2 -2
index b0cfe0a..875dea9 100644
@@ -1515,17 +1515,19 @@ again:
 	 * to make sure.
 	 */
 	if (ret > 0 && metadata) {
-		if (path->slots) {
+		if (path->slots[0]) {
 			path->slots[0]--;
 			btrfs_item_key_to_cpu(path->nodes[0], &key,
 					      path->slots[0]);
 			if (key.objectid == bytenr &&
-			    key.type == BTRFS_METADATA_ITEM_KEY)
+			    key.type == BTRFS_EXTENT_ITEM_KEY &&

extent-tree.c again: · line 1533

Quoted 10 lines
extent-tree.c +2 -0
+			    key.offset == root->leafsize)
 				ret = 0;
 		}
 
 		if (ret) {
 			btrfs_release_path(root, path);
+			key.objectid = bytenr;
 			key.type = BTRFS_EXTENT_ITEM_KEY;
 			key.offset = root->leafsize;
 			metadata = 0;

Thanks,

Josef

#5 lore raw

PATCH v3 1/1 Btrfs-progs: fix optimization in btrfs_lookup_extent_info

In reply to Filipe David Borba Manana

If we did a tree search with the goal to find a metadata item but the search failed with return value 1, we attempt to see if in the same leaf there's a corresponding extent item, and if there's one, just use it instead of doing another tree search for this extent item. The check in the leaf was wrong because it was seeking for a metadata item instead of an extent item.

This optimization was also being triggered incorrectly, as it was evaluating path->slots which always evaluates to true. The goal was to see if the leaf level slot was greater than zero (i.e. not the first item in the leaf).

V2: If previous leaf item is for a different object, ensure the search key has the target object id.

V3: Added Josef Bacik's review mention.

---
 extent-tree.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
extent-tree.c +4 -2
index b0cfe0a..875dea9 100644
@@ -1515,17 +1515,19 @@ again:
 	 * to make sure.
 	 */
 	if (ret > 0 && metadata) {
-		if (path->slots) {
+		if (path->slots[0]) {
 			path->slots[0]--;
 			btrfs_item_key_to_cpu(path->nodes[0], &key,
 					      path->slots[0]);
 			if (key.objectid == bytenr &&
-			    key.type == BTRFS_METADATA_ITEM_KEY)
+			    key.type == BTRFS_EXTENT_ITEM_KEY &&
+			    key.offset == root->leafsize)
 				ret = 0;
 		}
 
 		if (ret) {
 			btrfs_release_path(root, path);
+			key.objectid = bytenr;
 			key.type = BTRFS_EXTENT_ITEM_KEY;
 			key.offset = root->leafsize;
 			metadata = 0;
Signature · 1 line
1.7.9.5