[PATCH 1/2] btrfs-progs: set generation_v2 any time we write a new root

[PATCH 1/2] btrfs-progs: set generation_v2 any time we write a new root

[Eric Sandeen]

With this integration branch commit in place:

2bd1169 btrfs-progs: root_item generation_v2 is out of sync after btrfsck

I started seeing generation mismatch messages from the kernel
at mount time, after a fresh mkfs(!):

  btrfs: mismatching generation and generation_v2 found in root item...

This is because the code which emits the warning does not do so if
there is a mismatch but generation_v2 is 0; the above commit began
setting generation_v2 to something non-zero, so the warning was emitted.

The reason there is a mismatch at all is because mkfs.btrfs calls
create_data_reloc_tree(), which copies a root, and then calls
btrfs_set_root_generation(), bumping the original copied generation.
But nothing updated generation_v2 to match on the way to disk.

Fix this by updating generation_v2 in btrfs_insert_root(),
as is done in the kernel.

This is safe because it's a new root created by userspace, so
the btrfs_root_item is guaranteed to be big enough to contain
generation_v2.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Another example of why we need to get userspace in sync with kernelspace...

diff --git a/root-tree.c b/root-tree.c
index 4454147..1823918 100644
--- a/root-tree.c
+++ b/root-tree.c
@@ -105,6 +105,11 @@ int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root
 		      *item)
 {
 	int ret;
+
+	/*
+	 * Make sure generation v1 and v2 match. See update_root for details.
+	 */
+	btrfs_set_root_generation_v2(item, btrfs_root_generation(item));
 	ret = btrfs_insert_item(trans, root, key, item, sizeof(*item));
 	return ret;
 }

[PATCH 2/2] btrfs-progs: enlarge root item if needed in btrfs_update_root

[Eric Sandeen]

This addresses the same issue as did:

2bd1169 btrfs-progs: root_item generation_v2 is out of sync after btrfsck

but rather than optionally updating generation_v2 based
on the size of the existing item, increase the size of the
item as needed, and unconditionally set generation_v2.
This matches the kernel code, and keeping things in sync is a
Good Thing.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

kdave - since you already had Anand's patch on the integration
branch, I dunno if you want to add this on top, or just replace
it with a similar patch to this, up to you.

Thanks!
-Eric

diff --git a/root-tree.c b/root-tree.c
index 1823918..902d75d 100644
--- a/root-tree.c
+++ b/root-tree.c
@@ -69,7 +69,7 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
 	int ret;
 	int slot;
 	unsigned long ptr;
-	u32 old_size;
+	u32 old_len;
 
 	path = btrfs_alloc_path();
 	BUG_ON(!path);
@@ -80,18 +80,42 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
 	l = path->nodes[0];
 	slot = path->slots[0];
 	ptr = btrfs_item_ptr_offset(l, slot);
+	old_len = btrfs_item_size_nr(l, slot);
+
 	/*
-	* If the btrfs-progs is newer and kernel is at
-	* generation_v1 then we don't touch v2 items
-	* otherwise when kernel is at same or greater
-	* version compared with btrfs-progs then update
-	* the needed
-	*/
-	old_size = btrfs_item_size_nr(l, slot);
-	if (old_size >= sizeof(*item)) {
-		btrfs_set_root_generation_v2(item,
-			btrfs_root_generation(item));
+	 * If this is the first time we update the root item which originated
+	 * from an older kernel, we need to enlarge the item size to make room
+	 * for the added fields.
+	 */
+	if (old_len < sizeof(*item)) {
+		btrfs_release_path(root, path);
+		ret = btrfs_search_slot(trans, root, key, path,
+				-1, 1);
+		if (ret < 0) {
+			goto out;
+		}
+
+		ret = btrfs_del_item(trans, root, path);
+		if (ret < 0) {
+			goto out;
+		}
+		btrfs_release_path(root, path);
+		ret = btrfs_insert_empty_item(trans, root, path,
+				key, sizeof(*item));
+		if (ret < 0) {
+			goto out;
+		}
+		l = path->nodes[0];
+		slot = path->slots[0];
+		ptr = btrfs_item_ptr_offset(l, slot);
 	}
+
+	/*
+	 * Update generation_v2 so at the next mount we know the new root
+	 * fields are valid.
+	 */
+	btrfs_set_root_generation_v2(item, btrfs_root_generation(item));
+
 	write_extent_buffer(l, item, ptr, sizeof(*item));
 	btrfs_mark_buffer_dirty(path->nodes[0]);
 out:

[PATCH 2/2 V2] btrfs-progs: update generation_v2 in btrfs_update_root

[Eric Sandeen]

This addresses the same issue as did:

2bd1169 btrfs-progs: root_item generation_v2 is out of sync after btrfsck

but rather than optionally updating generation_v2 based
on the size of the existing item, increase the size of the
item as needed, and unconditionally set generation_v2.
This matches the kernel code, and keeping things in sync is a
Good Thing.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: give it a better, more descriptive subject/summary

kdave - since you already had Anand's patch on the integration
branch, I dunno if you want to add this on top, or just replace
it with a similar patch to this, up to you.

Thanks!
-Eric

diff --git a/root-tree.c b/root-tree.c
index 1823918..902d75d 100644
--- a/root-tree.c
+++ b/root-tree.c
@@ -69,7 +69,7 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
 	int ret;
 	int slot;
 	unsigned long ptr;
-	u32 old_size;
+	u32 old_len;
 
 	path = btrfs_alloc_path();
 	BUG_ON(!path);
@@ -80,18 +80,42 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
 	l = path->nodes[0];
 	slot = path->slots[0];
 	ptr = btrfs_item_ptr_offset(l, slot);
+	old_len = btrfs_item_size_nr(l, slot);
+
 	/*
-	* If the btrfs-progs is newer and kernel is at
-	* generation_v1 then we don't touch v2 items
-	* otherwise when kernel is at same or greater
-	* version compared with btrfs-progs then update
-	* the needed
-	*/
-	old_size = btrfs_item_size_nr(l, slot);
-	if (old_size >= sizeof(*item)) {
-		btrfs_set_root_generation_v2(item,
-			btrfs_root_generation(item));
+	 * If this is the first time we update the root item which originated
+	 * from an older kernel, we need to enlarge the item size to make room
+	 * for the added fields.
+	 */
+	if (old_len < sizeof(*item)) {
+		btrfs_release_path(root, path);
+		ret = btrfs_search_slot(trans, root, key, path,
+				-1, 1);
+		if (ret < 0) {
+			goto out;
+		}
+
+		ret = btrfs_del_item(trans, root, path);
+		if (ret < 0) {
+			goto out;
+		}
+		btrfs_release_path(root, path);
+		ret = btrfs_insert_empty_item(trans, root, path,
+				key, sizeof(*item));
+		if (ret < 0) {
+			goto out;
+		}
+		l = path->nodes[0];
+		slot = path->slots[0];
+		ptr = btrfs_item_ptr_offset(l, slot);
 	}
+
+	/*
+	 * Update generation_v2 so at the next mount we know the new root
+	 * fields are valid.
+	 */
+	btrfs_set_root_generation_v2(item, btrfs_root_generation(item));
+
 	write_extent_buffer(l, item, ptr, sizeof(*item));
 	btrfs_mark_buffer_dirty(path->nodes[0]);
 out:

Re: [PATCH 2/2 V2] btrfs-progs: update generation_v2 in btrfs_update_root

[Anand Jain]

>   	/*
> -	* If the btrfs-progs is newer and kernel is at
> -	* generation_v1 then we don't touch v2 items
> -	* otherwise when kernel is at same or greater
> -	* version compared with btrfs-progs then update
> -	* the needed
> -	*/
> -	old_size = btrfs_item_size_nr(l, slot);
> -	if (old_size >= sizeof(*item)) {
> -		btrfs_set_root_generation_v2(item,
> -			btrfs_root_generation(item));
> +	 * If this is the first time we update the root item which originated
> +	 * from an older kernel, we need to enlarge the item size to make room
> +	 * for the added fields.
> +	 */
> +	if (old_len < sizeof(*item)) {

  With this fix. when kernel is using v1 and btrfs-progs is
  using v2, the disk structure will be changed to v2 after
  running btrfs check. I would rather take a conservative
  approach, by maintaining kernel intended version.

  Thanks, Anand

Re: [PATCH 2/2 V2] btrfs-progs: update generation_v2 in btrfs_update_root

[Eric Sandeen]

On 4/22/13 4:31 AM, Anand Jain wrote:
> 
>>       /*
>> -    * If the btrfs-progs is newer and kernel is at
>> -    * generation_v1 then we don't touch v2 items
>> -    * otherwise when kernel is at same or greater
>> -    * version compared with btrfs-progs then update
>> -    * the needed
>> -    */
>> -    old_size = btrfs_item_size_nr(l, slot);
>> -    if (old_size >= sizeof(*item)) {
>> -        btrfs_set_root_generation_v2(item,
>> -            btrfs_root_generation(item));
>> +     * If this is the first time we update the root item which originated
>> +     * from an older kernel, we need to enlarge the item size to make room
>> +     * for the added fields.
>> +     */
>> +    if (old_len < sizeof(*item)) {
> 
>  With this fix. when kernel is using v1 and btrfs-progs is
>  using v2, the disk structure will be changed to v2 after
>  running btrfs check. I would rather take a conservative
>  approach, by maintaining kernel intended version.

If the kernel can do it, it should be safe for userspace to do it
as well.  An old kernel will have a smaller btrfs_root_item size,
and won't even see the new fields when it reads the roots, I think.

And this has the advantage of not further diverging userspace
from kernelspace which, as we've seen, leads to bugs.  :)

Thanks,
-Eric

>  Thanks, Anand

Re: [PATCH 2/2 V2] btrfs-progs: update generation_v2 in btrfs_update_root

[David Sterba]

On Mon, Apr 22, 2013 at 12:16:41AM -0500, Eric Sandeen wrote:
> This addresses the same issue as did:
> 
> 2bd1169 btrfs-progs: root_item generation_v2 is out of sync after btrfsck
> 
> but rather than optionally updating generation_v2 based
> on the size of the existing item, increase the size of the
> item as needed, and unconditionally set generation_v2.
> This matches the kernel code, and keeping things in sync is a
> Good Thing.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: give it a better, more descriptive subject/summary
> 
> kdave - since you already had Anand's patch on the integration
> branch, I dunno if you want to add this on top, or just replace
> it with a similar patch to this, up to you.

I'm going to replace the patch (the integration branches are not linear
and could be rebased, the purpose is to catch such issues earlier so the
patch queue is moderately clean when it gets pulled).

thanks,
david

Re: [PATCH 1/2] btrfs-progs: set generation_v2 any time we write a new root

[Anand Jain]

  yeah we should set the v2 parameter at all the place where
  we call btrfs_set_root_generation. Sorry it slipped my mind.
  Thanks for the fix.

Thanks, Anand


On 04/22/2013 01:01 PM, Eric Sandeen wrote:
> With this integration branch commit in place:
>
> 2bd1169 btrfs-progs: root_item generation_v2 is out of sync after btrfsck
>
> I started seeing generation mismatch messages from the kernel
> at mount time, after a fresh mkfs(!):
>
>    btrfs: mismatching generation and generation_v2 found in root item...
>
> This is because the code which emits the warning does not do so if
> there is a mismatch but generation_v2 is 0; the above commit began
> setting generation_v2 to something non-zero, so the warning was emitted.
>
> The reason there is a mismatch at all is because mkfs.btrfs calls
> create_data_reloc_tree(), which copies a root, and then calls
> btrfs_set_root_generation(), bumping the original copied generation.
> But nothing updated generation_v2 to match on the way to disk.
>
> Fix this by updating generation_v2 in btrfs_insert_root(),
> as is done in the kernel.
>
> This is safe because it's a new root created by userspace, so
> the btrfs_root_item is guaranteed to be big enough to contain
> generation_v2.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> Another example of why we need to get userspace in sync with kernelspace...
>
> diff --git a/root-tree.c b/root-tree.c
> index 4454147..1823918 100644
> --- a/root-tree.c
> +++ b/root-tree.c
> @@ -105,6 +105,11 @@ int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root
>   		      *item)
>   {
>   	int ret;
> +
> +	/*
> +	 * Make sure generation v1 and v2 match. See update_root for details.
> +	 */
> +	btrfs_set_root_generation_v2(item, btrfs_root_generation(item));
>   	ret = btrfs_insert_item(trans, root, key, item, sizeof(*item));
>   	return ret;
>   }
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>