Re: [Btrfs-devel] cloning file data

["Yan Zheng" <yanzheng@21cn.com>]

2008/5/3, Sage Weil <sage@newdream.net>:
> Hi Yan-
>
> On Sat, 3 May 2008, Yan Zheng wrote:
> > I think the clone ioctl won't work in some corner case. The big loop
> > in btrfs_ioctl_clone uses path->slots[0]++ and btrfs_next_leaf to get
> > next item in the tree. However, this approach works only when the
> > layout of tree keeps unchangeed. In btrfs_ioctl_clone, both
> > btrfs_insert_file_extent and dup_item_to_inode may change the layout
> > of tree.
> >
> > To be safe, I think the codes should:
> >  use btrfs_search_slot to find next item.
> >  use a intermediate buffer when coping item between two extent buffer.
>
> Oh, right.  I think for the item copy, though, we just need to re-search
> for the source key again after doing the insert_empty_item.  Then we can
> still use copy_extent_buffer, since at that point both paths will be
> valid?
>
> Something like the below (untested) patch.  I suspect I didn't hit this
> because I was always cloning 'file' to something like 'file2' that always
> sorted after the src file, and didn't shift its position in the leaf. I'll
> try to test this in the next few days.
>
> Thanks-
> sage
>
>
> diff -r f6ba18a50ad7 inode.c
> --- a/inode.c   Fri May 02 16:13:49 2008 -0400
> +++ b/inode.c   Fri May 02 23:11:49 2008 -0700
> @@ -3103,25 +3103,27 @@ out:
>
>  void dup_item_to_inode(struct btrfs_trans_handle *trans,
>                       struct btrfs_root *root,
> -                      struct btrfs_path *path,
> -                      struct extent_buffer *leaf,
> -                      int slot,
> -                      struct btrfs_key *key,
> +                      struct btrfs_path *srcpath,
> +                      struct btrfs_key *srckey,
>                       u64 destino)
>  {
> -       struct btrfs_path *cpath = btrfs_alloc_path();
> -       int len = btrfs_item_size_nr(leaf, slot);
> -       int dstoff;
> -       struct btrfs_key ckey = *key;
> +       struct btrfs_key dstkey = *srckey;
> +       struct btrfs_path *dstpath = btrfs_alloc_path();
> +       int len = btrfs_item_size_nr(srcpath->nodes[0],
> +                                    srcpath->slots[0]);
>        int ret;
>
> -       ckey.objectid = destino;
> -       ret = btrfs_insert_empty_item(trans, root, cpath, &ckey, len);
> -       dstoff = btrfs_item_ptr_offset(cpath->nodes[0], cpath->slots[0]);
> -       copy_extent_buffer(cpath->nodes[0], leaf, dstoff,
> -                          btrfs_item_ptr_offset(leaf, slot),
> +       dstkey.objectid = destino;
> +       ret = btrfs_insert_empty_item(trans, root, dstpath, &dstkey, len);
> +       /* re-search for src key, in case we changed srcpath */
> +       ret = btrfs_search_slot(trans, root, srckey, srcpath, 0, 0);
> +       copy_extent_buffer(dstpath->nodes[0], srcpath->nodes[0],
> +                          btrfs_item_ptr_offset(dstpath->nodes[0],
> +                                                dstpath->slots[0]),
> +                          btrfs_item_ptr_offset(srcpath->nodes[0],
> +                                                srcpath->slots[0]),
>                           len);
> -       btrfs_release_path(root, cpath);
> +       btrfs_release_path(root, dstpath);
>  }
>
>  long btrfs_ioctl_clone(struct file *file, unsigned long src_fd)
> @@ -3137,7 +3139,6 @@ long btrfs_ioctl_clone(struct file *file
>        struct btrfs_key key;
>        struct extent_buffer *leaf;
>        u32 nritems;
> -       int nextret;
>        int slot;
>
>        src_file = fget(src_fd);
> @@ -3178,6 +3179,8 @@ long btrfs_ioctl_clone(struct file *file
>        while (1) {
>                ret = btrfs_lookup_file_extent(trans, root, path, src->i_ino,
>                                               pos, 0);
> +
> +next_slot:
>                if (ret < 0)
>                        goto out;
>                if (ret > 0) {
> @@ -3187,7 +3190,7 @@ long btrfs_ioctl_clone(struct file *file
>                        }
>                        path->slots[0]--;
>                }
> -next_slot:
> +
>                leaf = path->nodes[0];
>                slot = path->slots[0];
>                btrfs_item_key_to_cpu(leaf, &key, slot);
> @@ -3225,22 +3228,19 @@ next_slot:
>                                }
>                                pos = key.offset + len;
>                        } else if (found_type == BTRFS_FILE_EXTENT_INLINE) {
> -                               dup_item_to_inode(trans, root, path, leaf, slot,
> -                                                 &key, inode->i_ino);
> +                               dup_item_to_inode(trans, root, path, &key,
> +                                                 inode->i_ino);
>                                pos = key.offset + btrfs_item_size_nr(leaf,
>                                                                      slot);
>                        }
>                } else if (btrfs_key_type(&key) == BTRFS_CSUM_ITEM_KEY)
> -                       dup_item_to_inode(trans, root, path, leaf, slot, &key,
> +                       dup_item_to_inode(trans, root, path, &key,
>                                          inode->i_ino);
>
> -               if (slot >= nritems - 1) {
> -                       nextret = btrfs_next_leaf(root, path);
> -                       if (nextret)
> -                               goto out;
> -               } else {
> -                       path->slots[0]++;
> -               }
> +               /* path may not still be valid, so explicitly search
> +                * for the next key */
> +               key.offset = pos;
> +               ret = btrfs_search_slot(trans, root, &key, path, 0, 0);
>                goto next_slot;
>        }

key.offset isn't updated properly. you should differentiate extent
item, inline extent item and csum item.

For extent item:
key.offset += btrfs_file_extent_num_bytes(...);

For inline extent item:
u32 len = btrfs_file_extent_inline_len(...);
key.offset += ALIGN(len, root->sectorsize);

For csum item
key.offset += btrfs_item_size_nr(...) / BTRFS_CRC32_SIZE * root->sectorsize;

Regards
YZ