Skip to content Skip to conversation

Btrfs design defect in extent backref ?

RFC In review Li Zefan 2 participants 5 messages Aug 25, 2011 – Aug 26, 2011 Btrfs file system

Share & tools

Li Zefan

author

2011-08-25 07:56 UTC #0 lore raw

RFC 1/1 Btrfs design defect in extent backref ?

We have an offset in file extent to indicate its position in the corresponding extent item in extent tree. We also have an offset in extent item to indicate the start position of the file extent that uses this item.

The math is:

extent_item.extent_data_ref.offset = file_pos - file_extent.extent_offset.

e1
disk extents: |--------------|
^
| e2
| |-----------------|
| | ^
| | |
v v |
file extents: |----- f1 -----|----- f2 -----|

So it looks like e2.offset points to f1 not f2. Therefore given an extent item, we'll have to search through all the file extents in an inode to find the relative file extent in the worst case, which makes this field somewhat useless.

What makes things worse is the above fomula can make the offset a negative value (cast to u64):

    # touch /mnt/dst
    # clone_range -s 8192 -d 4096 /mnt/src /mnt/dst
    # umount /mnt
    # btrfs-debug-tree /dev/sda7
    ...
        item 2 key (12582912 EXTENT_ITEM 49152) itemoff 3865 itemsize 82
                extent refs 2 gen 8 flags 1
                extent data backref root 5 objectid 258 offset 18446744073709543424 count 1
                extent data backref root 5 objectid 257 offset 0 count 1
    ...

and relocation won't work in this case:

    # mount /dev/sda7 /mnt
    # rm /mnt/src
    # sync
    # btrfs fi bal /mnt
    (kernel warning !!)
    (hung up !!)

I don't see the necessity or benefit of the substraction in the fomula, and I think the correct one is:

extent_item.extent_data_ref.offset = file_pos

(As a side effect thereafter we don't need extent_data_ref.count)

That's what this patch does. Unfornately it is an incompatable change in disk format.

So I think we have to live with this defect, just fix relocation for the negative offset case ?

Signed-off-by Li Zefan <[email protected]>

---
 fs/btrfs/extent-tree.c |    1 -
 fs/btrfs/file.c        |   11 +++++------
 fs/btrfs/inode.c       |    7 +++----
 fs/btrfs/ioctl.c       |    2 +-
 fs/btrfs/relocation.c  |    1 -
 5 files changed, 9 insertions(+), 13 deletions(-)

fs/btrfs/extent-tree.c +0 -1

index f5be06a..3924e03 100644
@@ -2578,7 +2578,6 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 				continue;
 
 			num_bytes = btrfs_file_extent_disk_num_bytes(buf, fi);
-			key.offset -= btrfs_file_extent_offset(buf, fi);
 			ret = process_func(trans, root, bytenr, num_bytes,
 					   parent, ref_root, key.objectid,
 					   key.offset);

fs/btrfs/file.c +5 -6

index e7872e4..7f65a27 100644
@@ -678,7 +678,7 @@ next_slot:
 						disk_bytenr, num_bytes, 0,
 						root->root_key.objectid,
 						new_key.objectid,
-						start - extent_offset);
+						start);
 				BUG_ON(ret);
 				*hint_byte = disk_bytenr;
 			}

@@ -752,8 +752,7 @@ next_slot:
 				ret = btrfs_free_extent(trans, root,
 						disk_bytenr, num_bytes, 0,
 						root->root_key.objectid,
-						key.objectid, key.offset -
-						extent_offset);
+						key.objectid, key.offset);
 				BUG_ON(ret);
 				inode_sub_bytes(inode,
 						extent_end - key.offset);

@@ -962,7 +961,7 @@ again:
 
 		ret = btrfs_inc_extent_ref(trans, root, bytenr, num_bytes, 0,
 					   root->root_key.objectid,
-					   ino, orig_offset);
+					   ino, split);
 		BUG_ON(ret);
 
 		if (split == start) {

@@ -989,7 +988,7 @@ again:
 		del_nr++;
 		ret = btrfs_free_extent(trans, root, bytenr, num_bytes,
 					0, root->root_key.objectid,
-					ino, orig_offset);
+					ino, other_start);
 		BUG_ON(ret);
 	}
 	other_start = 0;

@@ -1006,7 +1005,7 @@ again:
 		del_nr++;
 		ret = btrfs_free_extent(trans, root, bytenr, num_bytes,
 					0, root->root_key.objectid,
-					ino, orig_offset);
+					ino, other_end);
 		BUG_ON(ret);
 	}
 	if (del_nr == 0) {

fs/btrfs/inode.c +3 -4

index 0ccc743..0158652 100644
@@ -3135,7 +3135,7 @@ int btrfs_truncate_inode_items(struct btrfs_trans_handle *trans,
 	struct btrfs_key found_key;
 	u64 extent_start = 0;
 	u64 extent_num_bytes = 0;
-	u64 extent_offset = 0;
+	u64 offset = 0;
 	u64 item_end = 0;
 	u64 mask = root->sectorsize - 1;
 	u32 found_type = (u8)-1;

@@ -3256,8 +3256,7 @@ search_again:
 				extent_num_bytes =
 					btrfs_file_extent_disk_num_bytes(leaf,
 									 fi);
-				extent_offset = found_key.offset -
-					btrfs_file_extent_offset(leaf, fi);
+				offset = found_key.offset;
 
 				/* FIXME blocksize != 4096 */
 				num_dec = btrfs_file_extent_num_bytes(leaf, fi);

@@ -3314,7 +3313,7 @@ delete:
 			ret = btrfs_free_extent(trans, root, extent_start,
 						extent_num_bytes, 0,
 						btrfs_header_owner(leaf),
-						ino, extent_offset);
+						ino, offset);
 			BUG_ON(ret);
 		}

fs/btrfs/ioctl.c +1 -1

index 3351b1b..87e126f 100644
@@ -2379,7 +2379,7 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,
 							disko, diskl, 0,
 							root->root_key.objectid,
 							btrfs_ino(inode),
-							new_key.offset - datao);
+							new_key.offset);
 					BUG_ON(ret);
 				}
 			} else if (type == BTRFS_FILE_EXTENT_INLINE) {

fs/btrfs/relocation.c +0 -1

index 59bb176..a8d0089 100644
@@ -1598,7 +1598,6 @@ int replace_file_extents(struct btrfs_trans_handle *trans,
 		btrfs_set_file_extent_disk_bytenr(leaf, fi, new_bytenr);
 		dirty = 1;
 
-		key.offset -= btrfs_file_extent_offset(leaf, fi);
 		ret = btrfs_inc_extent_ref(trans, root, new_bytenr,
 					   num_bytes, parent,
 					   btrfs_header_owner(leaf),

Signature · 1 line

1.7.3.1

Yan, Zheng

2011-08-25 08:47 UTC #1 lore raw

Quoted 8 lines · Li Zefan

We have an offset in file extent to indicate its position in the corresponding extent item in extent tree. We also have an offset in extent item to indicate the start position of the file extent that uses this item.
The math is:
=A0 =A0extent_item.extent_data_ref.offset =3D file_pos - file_extent.=

extent_offset.

Quoted 5 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 e1
disk extents: =A0 =A0|--------------|
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ^
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

e2

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0|---------------=

--|

Quoted 6 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0| =A0 ^
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0| =A0 |
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 v =A0 =A0 =A0 =A0 =A0v =A0 | file extents: =A0 =A0|----- f1 -----|----- f2 -----|
So it looks like e2.offset points to f1 not f2. Therefore given an ex=

tent item,

Quoted 1 line

we'll have to search through all the file extents in an inode to find=

the

Quoted 1 line

relative file extent in the worst case, which makes this field somewh=

at useless.

Quoted 1 line

The reason for this is reducing number of file extent backref itmes. we don't have to search all the file extents because the file extent si= ze
is limited and we have extent_data_ref.count.

Quoted 1 line

What makes things worse is the above fomula can make the offset a neg=

ative

Quoted 8 lines

value (cast to u64):
=A0 =A0# touch /mnt/dst
=A0 =A0# clone_range -s 8192 -d 4096 /mnt/src /mnt/dst
=A0 =A0# umount /mnt
=A0 =A0# btrfs-debug-tree /dev/sda7
=A0 =A0...
=A0 =A0 =A0 =A0item 2 key (12582912 EXTENT_ITEM 49152) itemoff 3865 i=

temsize 82

Quoted 2 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0extent refs 2 gen 8 flags 1 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0extent data backref root 5 objectid 25=

8 offset 18446744073709543424 count 1

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0extent data backref root 5 objectid 25=

7 offset 0 count 1

Quoted 4 lines

=A0 =A0...
and relocation won't work in this case:

Quoted 8 lines

=A0 =A0# mount /dev/sda7 /mnt
=A0 =A0# rm /mnt/src
=A0 =A0# sync
=A0 =A0# btrfs fi bal /mnt
=A0 =A0(kernel warning !!)
=A0 =A0(hung up !!)
I don't see the necessity or benefit of the substraction in the fomul=

a,

Quoted 11 lines

and I think the correct one is:
=A0 =A0extent_item.extent_data_ref.offset =3D file_pos
(As a side effect thereafter we don't need extent_data_ref.count)
That's what this patch does. Unfornately it is an incompatable change in disk format.
So I think we have to live with this defect, just fix relocation for the negative offset case ?

I prefer fixing relocation.

fs/btrfs/extent-tree.c static int __btrfs_mod_ref(struct btrfs_trans_h=

Quoted 15 lines

Signed-off-by Li Zefan <[email protected]>
---
=A0fs/btrfs/extent-tree.c | =A0 =A01 - =A0fs/btrfs/file.c =A0 =A0 =A0 =A0| =A0 11 +++++------ =A0fs/btrfs/inode.c =A0 =A0 =A0 | =A0 =A07 +++---- =A0fs/btrfs/ioctl.c =A0 =A0 =A0 | =A0 =A02 +- =A0fs/btrfs/relocation.c =A0| =A0 =A01 -
=A05 files changed, 9 insertions(+), 13 deletions(-)
fs/btrfs/extent-tree.c +0 -0
index f5be06a..3924e03 100644
@@ -2578,7 +2578,6 @@ static int __btrfs_mod_ref(struct btrfs_trans_h=

andle *trans,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0contin=

ue;

Quoted 2 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0num_bytes =3D btrfs_fi=

le_extent_disk_num_bytes(buf, fi);

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 key.offset -=3D btrfs_f=

ile_extent_offset(buf, fi);

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D process_func(t=

rans, root, bytenr, num_bytes,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 parent, ref_root, key.objectid,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 key.offset);

Quoted 6 lines

fs/btrfs/file.c +0 -0
index e7872e4..7f65a27 100644
@@ -678,7 +678,7 @@ next_slot:
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0disk_bytenr, num_bytes, 0,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0root->root_key.objectid,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0new_key.objectid,

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 start - extent_offset);

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 start);

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BUG_ON=

(ret);

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*hint_=

byte =3D disk_bytenr;

Quoted 3 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
fs/btrfs/file.c +0 -0
@@ -752,8 +752,7 @@ next_slot:
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D=

btrfs_free_extent(trans, root,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0disk_bytenr, num_bytes, 0,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0root->root_key.objectid,

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 key.objectid, key.offset -

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 extent_offset);

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 key.objectid, key.offset);

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BUG_ON=

(ret);

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0inode_=

sub_bytes(inode,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0extent_end - key.offset);

Quoted 3 lines

fs/btrfs/file.c +0 -0
@@ -962,7 +961,7 @@ again:
 
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D btrfs_inc_extent_ref(trans, ro=

ot, bytenr, num_bytes, 0,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 root->root_key.objectid,

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0ino, orig_offset);

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0ino, split);

Quoted 6 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BUG_ON(ret);
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (split =3D=3D start) {
fs/btrfs/file.c +0 -0
@@ -989,7 +988,7 @@ again:
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0del_nr++;
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D btrfs_free_extent(trans, root,=

bytenr, num_bytes,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A00, root->root_key.objectid,

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 ino, orig_offset);

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 ino, other_start);

Quoted 6 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BUG_ON(ret);
=A0 =A0 =A0 =A0}
=A0 =A0 =A0 =A0other_start =3D 0;
fs/btrfs/file.c +0 -0
@@ -1006,7 +1005,7 @@ again:
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0del_nr++;
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D btrfs_free_extent(trans, root,=

bytenr, num_bytes,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A00, root->root_key.objectid,

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 ino, orig_offset);

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 ino, other_end);

fs/btrfs/inode.c int btrfs_truncate_inode_items(struct btrfs_tra=

Quoted 8 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BUG_ON(ret);
=A0 =A0 =A0 =A0}
=A0 =A0 =A0 =A0if (del_nr =3D=3D 0) {
fs/btrfs/inode.c +0 -0
index 0ccc743..0158652 100644
@@ -3135,7 +3135,7 @@ int btrfs_truncate_inode_items(struct btrfs_tra=

ns_handle *trans,

Quoted 10 lines

fs/btrfs/inode.c +1 -1

 =A0 =A0 =A0 =A0struct btrfs_key found_key;
 =A0 =A0 =A0 =A0u64 extent_start =3D 0;
 =A0 =A0 =A0 =A0u64 extent_num_bytes =3D 0;
- =A0 =A0 =A0 u64 extent_offset =3D 0;
+ =A0 =A0 =A0 u64 offset =3D 0;
 =A0 =A0 =A0 =A0u64 item_end =3D 0;
 =A0 =A0 =A0 =A0u64 mask =3D root->sectorsize - 1;
 =A0 =A0 =A0 =A0u32 found_type =3D (u8)-1;

@@ -3256,8 +3256,7 @@ search_again:

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0extent=

_num_bytes =3D

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0btrfs_file_extent_disk_num_bytes(leaf,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= fi);

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 extent_=

offset =3D found_key.offset -

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 btrfs_file_extent_offset(leaf, fi);

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 offset =

=3D found_key.offset;

Quoted 2 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* FIX=

ME blocksize !=3D 4096 */

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0num_de=

c =3D btrfs_file_extent_num_bytes(leaf, fi);

Quoted 2 lines

fs/btrfs/inode.c +0 -0
@@ -3314,7 +3313,7 @@ delete:
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D btrfs_free_ext=

ent(trans, root, extent_start,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0extent_num_bytes, 0,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0btrfs_header_owner(leaf),

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 ino, extent_offset);

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 ino, offset);

fs/btrfs/ioctl.c static noinline long btrfs_ioctl_clone(struct f=

Quoted 8 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BUG_ON(ret); =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
fs/btrfs/ioctl.c +0 -0
index 3351b1b..87e126f 100644
@@ -2379,7 +2379,7 @@ static noinline long btrfs_ioctl_clone(struct f=

ile *file, unsigned long srcfd,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0disko, diskl, 0,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0root->root_key.objectid,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0btrfs_ino(inode),

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 new_key.offset - datao);

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 new_key.offset);

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0BUG_ON(ret);

Quoted 2 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else if (type =3D=3D=

BTRFS_FILE_EXTENT_INLINE) {

fs/btrfs/relocation.c int replace_file_extents(struct btrfs_trans_han=

Quoted 5 lines

fs/btrfs/relocation.c +0 -0

index 59bb176..a8d0089 100644
@@ -1598,7 +1598,6 @@ int replace_file_extents(struct btrfs_trans_han=

dle *trans,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0btrfs_set_file_extent_disk_bytenr(leaf=

, fi, new_bytenr);

Quoted 3 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dirty =3D 1;
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 key.offset -=3D btrfs_file_extent_offse=

t(leaf, fi);

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D btrfs_inc_extent_ref(trans, ro=

ot, new_bytenr,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 num_bytes, parent,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 btrfs_header_owner(leaf),

Quoted 6 lines

--
1.7.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs=

" in

Quoted 3 lines

the body of a message to [email protected]
More majordomo info at =A0http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" = in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Li Zefan

author

2011-08-26 02:00 UTC #2 lore raw

Quoted 20 lines · Zheng

Quoted 19 lines · Li Zefan
We have an offset in file extent to indicate its position in the corresponding extent item in extent tree. We also have an offset in extent item to indicate the start position of the file extent that uses this item.
The math is:
extent_item.extent_data_ref.offset = file_pos - file_extent.extent_offset.
                      e1
disk extents: |--------------|
^
| e2
| |-----------------|
| | ^
| | |
v v |
file extents: |----- f1 -----|----- f2 -----|

Quoted 6 lines

Quoted 4 lines
So it looks like e2.offset points to f1 not f2. Therefore given an extent item, we'll have to search through all the file extents in an inode to find the relative file extent in the worst case, which makes this field somewhat useless.
The reason for this is reducing number of file extent backref itmes.

It seems to me a rare case, which isn't worth the complexity and inconvenience it brings, and it requires an extra field (.count).

Quoted 2 lines

we don't have to search all the file extents because the file extent size is limited and we have extent_data_ref.count.

Yes we have to, and for a big file with many small file extents, the extent number is not trivial.

Quoted 31 lines

Quoted 31 lines
What makes things worse is the above fomula can make the offset a negative value (cast to u64):
   # touch /mnt/dst
   # clone_range -s 8192 -d 4096 /mnt/src /mnt/dst
   # umount /mnt
   # btrfs-debug-tree /dev/sda7
   ...
       item 2 key (12582912 EXTENT_ITEM 49152) itemoff 3865 itemsize 82
               extent refs 2 gen 8 flags 1
               extent data backref root 5 objectid 258 offset 18446744073709543424 count 1
               extent data backref root 5 objectid 257 offset 0 count 1
   ...
and relocation won't work in this case:
   # mount /dev/sda7 /mnt
   # rm /mnt/src
   # sync
   # btrfs fi bal /mnt
   (kernel warning !!)
   (hung up !!)
I don't see the necessity or benefit of the substraction in the fomula, and I think the correct one is:
extent_item.extent_data_ref.offset = file_pos
(As a side effect thereafter we don't need extent_data_ref.count)

Quoted 8 lines

Quoted 5 lines
That's what this patch does. Unfornately it is an incompatable change in disk format.
So I think we have to live with this defect, just fix relocation for the negative offset case ?
I prefer fixing relocation.

Sure, though I would prefer the alternative if not for the stablity of disk format.

Yan, Zheng

2011-08-26 02:38 UTC #3 lore raw

Quoted 2 lines · Li Zefan

Quoted 1 line · Zheng
On Thu, Aug 25, 2011 at 3:56 PM, Li Zefan <[email protected]> wrot=

e:

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
We have an offset in file extent to indicate its position in the corresponding extent item in extent tree. We also have an offset in extent item to indicate the start position of the file extent that uses this item.
The math is:
=A0 =A0extent_item.extent_data_ref.offset =3D file_pos - file_exten=

t.extent_offset.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 e1
disk extents: =A0 =A0|--------------|
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ^
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

e2

Quoted 1 line

Quoted 1 line
Quoted 1 line
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0|-------------=

----|

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0| =A0 ^
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0| =A0 |
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 v =A0 =A0 =A0 =A0 =A0v =A0 | file extents: =A0 =A0|----- f1 -----|----- f2 -----|
So it looks like e2.offset points to f1 not f2. Therefore given an =

extent item,

Quoted 1 line

Quoted 1 line
Quoted 1 line
we'll have to search through all the file extents in an inode to fi=

nd the

Quoted 1 line

Quoted 1 line
Quoted 1 line
relative file extent in the worst case, which makes this field some=

what useless.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
The reason for this is reducing number of file extent backref itmes.
It seems to me a rare case, which isn't worth the complexity and inco=

nvenience

Quoted 2 lines

it brings, and it requires an extra field (.count).

Random write workload isn't a rare case.

Quoted 1 line

Quoted 1 line
we don't have to search all the file extents because the file extent=

size

Quoted 3 lines

Quoted 1 line
is limited and we have extent_data_ref.count.
Yes we have to, and for a big file with many small file extents, the =

extent

Quoted 2 lines

number is not trivial.

Max file extent size is 128M, so only need to scan a 128M range in the worst case.

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
What makes things worse is the above fomula can make the offset a n=

egative

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
value (cast to u64):
=A0 =A0# touch /mnt/dst
=A0 =A0# clone_range -s 8192 -d 4096 /mnt/src /mnt/dst =A0 =A0# umount /mnt
=A0 =A0# btrfs-debug-tree /dev/sda7
=A0 =A0...
=A0 =A0 =A0 =A0item 2 key (12582912 EXTENT_ITEM 49152) itemoff 3865=

itemsize 82

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0extent refs 2 gen 8 flags 1 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0extent data backref root 5 objectid =

258 offset 18446744073709543424 count 1

Quoted 1 line

Quoted 1 line
Quoted 1 line
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0extent data backref root 5 objectid =

257 offset 0 count 1

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
=A0 =A0...
and relocation won't work in this case:

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
=A0 =A0# mount /dev/sda7 /mnt
=A0 =A0# rm /mnt/src
=A0 =A0# sync
=A0 =A0# btrfs fi bal /mnt
=A0 =A0(kernel warning !!)
=A0 =A0(hung up !!)
I don't see the necessity or benefit of the substraction in the fom=

ula,

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
and I think the correct one is:
=A0 =A0extent_item.extent_data_ref.offset =3D file_pos
(As a side effect thereafter we don't need extent_data_ref.count)
That's what this patch does. Unfornately it is an incompatable chan=

ge

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
in disk format.
So I think we have to live with this defect, just fix relocation fo=

r

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
the negative offset case ?
I prefer fixing relocation.
Sure, though I would prefer the alternative if not for the stablity o=

f

Quoted 2 lines

disk format.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" = in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Li Zefan

author

2011-08-26 03:04 UTC #4 lore raw

Quoted 27 lines · Zheng

Quoted 26 lines · Li Zefan
Quoted 25 lines · Zheng
Quoted 23 lines · Li Zefan
We have an offset in file extent to indicate its position in the corresponding extent item in extent tree. We also have an offset in extent item to indicate the start position of the file extent that uses this item.
The math is:
extent_item.extent_data_ref.offset = file_pos - file_extent.extent_offset.
                      e1
disk extents: |--------------|
^
| e2
| |-----------------|
| | ^
| | |
v v |
file extents: |----- f1 -----|----- f2 -----|
So it looks like e2.offset points to f1 not f2. Therefore given an extent item, we'll have to search through all the file extents in an inode to find the relative file extent in the worst case, which makes this field somewhat useless.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
The reason for this is reducing number of file extent backref itmes.
It seems to me a rare case, which isn't worth the complexity and inconvenience it brings, and it requires an extra field (.count).
Random write workload isn't a rare case.

Ah, I was thinking about the clone ioctl, and ignoring other situations.

Thanks for your clarification.