1. 25 Aug, 2016 9 commits
    • Qu Wenruo's avatar
      btrfs: qgroup: Fix qgroup incorrectness caused by log replay · df2c95f3
      Qu Wenruo authored
      When doing log replay at mount time(after power loss), qgroup will leak
      numbers of replayed data extents.
      
      The cause is almost the same of balance.
      So fix it by manually informing qgroup for owner changed extents.
      
      The bug can be detected by btrfs/119 test case.
      
      Cc: Mark Fasheh <mfasheh@suse.de>
      Signed-off-by: default avatarQu Wenruo <quwenruo@cn.fujitsu.com>
      Reviewed-and-Tested-by: default avatarGoldwyn Rodrigues <rgoldwyn@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarChris Mason <clm@fb.com>
      df2c95f3
    • Qu Wenruo's avatar
      btrfs: relocation: Fix leaking qgroups numbers on data extents · 62b99540
      Qu Wenruo authored
      This patch fixes a REGRESSION introduced in 4.2, caused by the big quota
      rework.
      
      When balancing data extents, qgroup will leak all its numbers for
      relocated data extents.
      
      The relocation is done in the following steps for data extents:
      1) Create data reloc tree and inode
      2) Copy all data extents to data reloc tree
         And commit transaction
      3) Create tree reloc tree(special snapshot) for any related subvolumes
      4) Replace file extent in tree reloc tree with new extents in data reloc
         tree
         And commit transaction
      5) Merge tree reloc tree with original fs, by swapping tree blocks
      
      For 1)~4), since tree reloc tree and data reloc tree doesn't count to
      qgroup, everything is OK.
      
      But for 5), the swapping of tree blocks will only info qgroup to track
      metadata extents.
      
      If metadata extents contain file extents, qgroup number for file extents
      will get lost, leading to corrupted qgroup accounting.
      
      The fix is, before commit transaction of step 5), manually info qgroup to
      track all file extents in data reloc tree.
      Since at commit transaction time, the tree swapping is done, and qgroup
      will account these data extents correctly.
      
      Cc: Mark Fasheh <mfasheh@suse.de>
      Reported-by: default avatarMark Fasheh <mfasheh@suse.de>
      Reported-by: default avatarFilipe Manana <fdmanana@gmail.com>
      Signed-off-by: default avatarQu Wenruo <quwenruo@cn.fujitsu.com>
      Tested-by: default avatarGoldwyn Rodrigues <rgoldwyn@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarChris Mason <clm@fb.com>
      62b99540
    • Qu Wenruo's avatar
      btrfs: qgroup: Refactor btrfs_qgroup_insert_dirty_extent() · cb93b52c
      Qu Wenruo authored
      Refactor btrfs_qgroup_insert_dirty_extent() function, to two functions:
      1. btrfs_qgroup_insert_dirty_extent_nolock()
         Almost the same with original code.
         For delayed_ref usage, which has delayed refs locked.
      
         Change the return value type to int, since caller never needs the
         pointer, but only needs to know if they need to free the allocated
         memory.
      
      2. btrfs_qgroup_insert_dirty_extent()
         The more encapsulated version.
      
         Will do the delayed_refs lock, memory allocation, quota enabled check
         and other things.
      
      The original design is to keep exported functions to minimal, but since
      more btrfs hacks exposed, like replacing path in balance, we need to
      record dirty extents manually, so we have to add such functions.
      
      Also, add comment for both functions, to info developers how to keep
      qgroup correct when doing hacks.
      
      Cc: Mark Fasheh <mfasheh@suse.de>
      Signed-off-by: default avatarQu Wenruo <quwenruo@cn.fujitsu.com>
      Reviewed-and-Tested-by: default avatarGoldwyn Rodrigues <rgoldwyn@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarChris Mason <clm@fb.com>
      cb93b52c
    • Jeff Mahoney's avatar
      btrfs: waiting on qgroup rescan should not always be interruptible · d06f23d6
      Jeff Mahoney authored
      We wait on qgroup rescan completion in three places: file system
      shutdown, the quota disable ioctl, and the rescan wait ioctl.  If the
      user sends a signal while we're waiting, we continue happily along.  This
      is expected behavior for the rescan wait ioctl.  It's racy in the shutdown
      path but mostly works due to other unrelated synchronization points.
      In the quota disable path, it Oopses the kernel pretty much immediately.
      
      Cc: <stable@vger.kernel.org> # v4.4+
      Signed-off-by: default avatarJeff Mahoney <jeffm@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarChris Mason <clm@fb.com>
      d06f23d6
    • Jeff Mahoney's avatar
      btrfs: properly track when rescan worker is running · d2c609b8
      Jeff Mahoney authored
      The qgroup_flags field is overloaded such that it reflects the on-disk
      status of qgroups and the runtime state.  The BTRFS_QGROUP_STATUS_FLAG_RESCAN
      flag is used to indicate that a rescan operation is in progress, but if
      the file system is unmounted while a rescan is running, the rescan
      operation is paused.  If the file system is then mounted read-only,
      the flag will still be present but the rescan operation will not have
      been resumed.  When we go to umount, btrfs_qgroup_wait_for_completion
      will see the flag and interpret it to mean that the rescan worker is
      still running and will wait for a completion that will never come.
      
      This patch uses a separate flag to indicate when the worker is
      running.  The locking and state surrounding the qgroup rescan worker
      needs a lot of attention beyond this patch but this is enough to
      avoid a hung umount.
      
      Cc: <stable@vger.kernel.org> # v4.4+
      Signed-off-by; Jeff Mahoney <jeffm@suse.com>
      Reviewed-by: default avatarQu Wenruo <quwenruo@cn.fujitsu.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarChris Mason <clm@fb.com>
      d2c609b8
    • Alex Lyakas's avatar
      btrfs: flush_space: treat return value of do_chunk_alloc properly · eecba891
      Alex Lyakas authored
      do_chunk_alloc returns 1 when it succeeds to allocate a new chunk.
      But flush_space will not convert this to 0, and will also return 1.
      As a result, reserve_metadata_bytes will think that flush_space failed,
      and may potentially return this value "1" to the caller (depends how
      reserve_metadata_bytes was called). The caller will also treat this as an error.
      For example, btrfs_block_rsv_refill does:
      
      int ret = -ENOSPC;
      ...
      ret = reserve_metadata_bytes(root, block_rsv, num_bytes, flush);
      if (!ret) {
              block_rsv_add_bytes(block_rsv, num_bytes, 0);
              return 0;
      }
      
      return ret;
      
      So it will return -ENOSPC.
      Signed-off-by: default avatarAlex Lyakas <alex@zadarastorage.com>
      Reviewed-by: default avatarJosef Bacik <jbacik@fb.com>
      Reviewed-by: default avatarLiu Bo <bo.li.liu@oracle.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarChris Mason <clm@fb.com>
      eecba891
    • Liu Bo's avatar
      Btrfs: add ASSERT for block group's memory leak · f3bca802
      Liu Bo authored
      This adds several ASSERT()' s to report memory leak of block group cache.
      Signed-off-by: default avatarLiu Bo <bo.li.liu@oracle.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarChris Mason <clm@fb.com>
      f3bca802
    • Qu Wenruo's avatar
      btrfs: backref: Fix soft lockup in __merge_refs function · d8422ba3
      Qu Wenruo authored
      When over 1000 file extents refers to one extent, find_parent_nodes()
      will be obviously slow, due to the O(n^2)~O(n^3) loops inside
      __merge_refs().
      
      The following ftrace shows the cubic growth of execution time:
      
      256 refs
       5) + 91.768 us   |  __add_keyed_refs.isra.12 [btrfs]();
       5)   1.447 us    |  __add_missing_keys.isra.13 [btrfs]();
       5) ! 114.544 us  |  __merge_refs [btrfs]();
       5) ! 136.399 us  |  __merge_refs [btrfs]();
      
      512 refs
       6) ! 279.859 us  |  __add_keyed_refs.isra.12 [btrfs]();
       6)   3.164 us    |  __add_missing_keys.isra.13 [btrfs]();
       6) ! 442.498 us  |  __merge_refs [btrfs]();
       6) # 2091.073 us |  __merge_refs [btrfs]();
      
      and 1024 refs
       7) ! 368.683 us  |  __add_keyed_refs.isra.12 [btrfs]();
       7)   4.810 us    |  __add_missing_keys.isra.13 [btrfs]();
       7) # 2043.428 us |  __merge_refs [btrfs]();
       7) * 18964.23 us |  __merge_refs [btrfs]();
      
      And sort them into the following char:
      (Unit: us)
      ------------------------------------------------------------------------
       Trace function        | 256 ref        | 512 refs      | 1024 refs    |
      ------------------------------------------------------------------------
       __add_keyed_refs      | 91             | 249           | 368          |
       __add_missing_keys    | 1              | 3             | 4            |
       __merge_refs 1st call | 114            | 442           | 2043         |
       __merge_refs 2nd call | 136            | 2091          | 18964        |
      ------------------------------------------------------------------------
      
      We can see the that __add_keyed_refs() grows almost in linear behavior.
      And __add_missing_keys() in this case doesn't change much or takes much
      time.
      
      While for the 1st __merge_refs() it's square growth
      for the 2nd __merge_refs() call it's cubic growth.
      
      It's no doubt that merge_refs() will take a long long time to execute if
      the number of refs continues its grows.
      
      So add a cond_resced() into the loop of __merge_refs().
      
      Although this will solve the problem of soft lockup, we need to use the
      new rb_tree based structure introduced by Lu Fengqi to really solve the
      long execution time.
      Signed-off-by: default avatarQu Wenruo <quwenruo@cn.fujitsu.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarChris Mason <clm@fb.com>
      d8422ba3
    • Liu Bo's avatar
      Btrfs: fix memory leak of reloc_root · 1c1ea4f7
      Liu Bo authored
      When some critical errors occur and FS would be flipped into RO,
      if we have an on-going balance, we can end up with a memory leak
      of root->reloc_root since btrfs_drop_snapshots() bails out
      without freeing reloc_root at the very early start.
      
      However, we're not able to free reloc_root in btrfs_drop_snapshots()
      because its caller, merge_reloc_roots(), still needs to access it to
      cleanup reloc_root's rbtree.
      
      This makes us free reloc_root when we're going to free fs/file roots.
      Signed-off-by: default avatarLiu Bo <bo.li.liu@oracle.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarChris Mason <clm@fb.com>
      1c1ea4f7
  2. 05 Aug, 2016 1 commit
  3. 03 Aug, 2016 2 commits
  4. 01 Aug, 2016 11 commits
    • Filipe Manana's avatar
      Btrfs: improve performance on fsync against new inode after rename/unlink · 44f714da
      Filipe Manana authored
      With commit 56f23fdb ("Btrfs: fix file/data loss caused by fsync after
      rename and new inode") we got simple fix for a functional issue when the
      following sequence of actions is done:
      
        at transaction N
        create file A at directory D
        at transaction N + M (where M >= 1)
        move/rename existing file A from directory D to directory E
        create a new file named A at directory D
        fsync the new file
        power fail
      
      The solution was to simply detect such scenario and fallback to a full
      transaction commit when we detect it. However this turned out to had a
      significant impact on throughput (and a bit on latency too) for benchmarks
      using the dbench tool, which simulates real workloads from smbd (Samba)
      servers. For example on a test vm (with a debug kernel):
      
      Unpatched:
      Throughput 19.1572 MB/sec  32 clients  32 procs  max_latency=1005.229 ms
      
      Patched:
      Throughput 23.7015 MB/sec  32 clients  32 procs  max_latency=809.206 ms
      
      The patched results (this patch is applied) are similar to the results of
      a kernel with the commit 56f23fdb ("Btrfs: fix file/data loss caused
      by fsync after rename and new inode") reverted.
      
      This change avoids the fallback to a transaction commit and instead makes
      sure all the names of the conflicting inode (the one that had a name in a
      past transaction that matches the name of the new file in the same parent
      directory) are logged so that at log replay time we don't lose neither the
      new file nor the old file, and the old file gets the name it was renamed
      to.
      
      This also ends up avoiding a full transaction commit for a similar case
      that involves an unlink instead of a rename of the old file:
      
        at transaction N
        create file A at directory D
        at transaction N + M (where M >= 1)
        remove file A
        create a new file named A at directory D
        fsync the new file
        power fail
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      44f714da
    • Filipe Manana's avatar
      Btrfs: be more precise on errors when getting an inode from disk · 67710892
      Filipe Manana authored
      When we attempt to read an inode from disk, we end up always returning an
      -ESTALE error to the caller regardless of the actual failure reason, which
      can be an out of memory problem (when allocating a path), some error found
      when reading from the fs/subvolume btree (like a genuine IO error) or the
      inode does not exists. So lets start returning the real error code to the
      callers so that they don't treat all -ESTALE errors as meaning that the
      inode does not exists (such as during orphan cleanup). This will also be
      needed for a subsequent patch in the same series dealing with a special
      fsync case.
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      67710892
    • Filipe Manana's avatar
      Btrfs: send, don't bug on inconsistent snapshots · 95155585
      Filipe Manana authored
      When doing an incremental send, if we find a new/modified/deleted extent,
      reference or xattr without having previously processed the corresponding
      inode item we end up exexuting a BUG_ON(). This is because whenever an
      extent, xattr or reference is added, modified or deleted, we always expect
      to have the corresponding inode item updated. However there are situations
      where this will not happen due to transient -ENOMEM or -ENOSPC errors when
      doing delayed inode updates.
      
      For example, when punching holes we can succeed in deleting and modifying
      (shrinking) extents but later fail to do the delayed inode update. So after
      such failure we close our transaction handle and right after a snapshot of
      the fs/subvol tree can be made and used later for a send operation. The
      same thing can happen during truncate, link, unlink, and xattr related
      operations.
      
      So instead of executing a BUG_ON, make send return an -EIO error and print
      an informative error message do dmesg/syslog.
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      95155585
    • Filipe Manana's avatar
      Btrfs: send, avoid incorrect leaf accesses when sending utimes operations · 15b253ea
      Filipe Manana authored
      The caller of send_utimes() is supposed to be sure that the inode number
      it passes to this function does actually exists in the send snapshot.
      However due to logic/algorithm bugs (such as the one fixed by the patch
      titled "Btrfs: send, fix invalid leaf accesses due to incorrect utimes
      operations"), this might not be the case and when that happens it makes
      send_utimes() access use an unrelated leaf item as the target inode item
      or access beyond a leaf's boundaries (when the leaf is full and
      path->slots[0] matches the number of items in the leaf).
      
      So if the call to btrfs_search_slot() done by send_utimes() does not find
      the inode item, just make sure send_utimes() returns -ENOENT and does not
      silently accesses unrelated leaf items or does invalid leaf accesses, also
      allowing us to easialy and deterministically catch such algorithmic/logic
      bugs.
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      15b253ea
    • Robbie Ko's avatar
      Btrfs: send, fix invalid leaf accesses due to incorrect utimes operations · 764433a1
      Robbie Ko authored
      During an incremental send, if we have delayed rename operations for inodes
      that were children of directories which were removed in the send snapshot,
      we can end up accessing incorrect items in a leaf or accessing beyond the
      last item of the leaf due to issuing utimes operations for the removed
      inodes. Consider the following example:
      
        Parent snapshot:
        .                                                             (ino 256)
        |--- a/                                                       (ino 257)
        |    |--- c/                                                  (ino 262)
        |
        |--- b/                                                       (ino 258)
        |    |--- d/                                                  (ino 263)
        |
        |--- del/                                                     (ino 261)
              |--- x/                                                 (ino 259)
              |--- y/                                                 (ino 260)
      
        Send snapshot:
      
        .                                                             (ino 256)
        |--- a/                                                       (ino 257)
        |
        |--- b/                                                       (ino 258)
        |
        |--- c/                                                       (ino 262)
        |    |--- y/                                                  (ino 260)
        |
        |--- d/                                                       (ino 263)
             |--- x/                                                  (ino 259)
      
      1) When processing inodes 259 and 260, we end up delaying their rename
         operations because their parents, inodes 263 and 262 respectively, were
         not yet processed and therefore not yet renamed;
      
      2) When processing inode 262, its rename operation is issued and right
         after the rename operation for inode 260 is issued. However right after
         issuing the rename operation for inode 260, at send.c:apply_dir_move(),
         we issue utimes operations for all current and past parents of inode
         260. This means we try to send a utimes operation for its old parent,
         inode 261 (deleted in the send snapshot), which does not cause any
         immediate and deterministic failure, because when the target inode is
         not found in the send snapshot, the send.c:send_utimes() function
         ignores it and uses the leaf region pointed to by path->slots[0],
         which can be any unrelated item (belonging to other inode) or it can
         be a region outside the leaf boundaries, if the leaf is full and
         path->slots[0] matches the number of items in the leaf. So we end
         up either successfully sending a utimes operation, which is fine
         and irrelevant because the old parent (inode 261) will end up being
         deleted later, or we end up doing an invalid memory access tha
         crashes the kernel.
      
      So fix this by making apply_dir_move() issue utimes operations only for
      parents that still exist in the send snapshot. In a separate patch we
      will make send_utimes() return an error (-ENOENT) if the given inode
      does not exists in the send snapshot.
      Signed-off-by: default avatarRobbie Ko <robbieko@synology.com>
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      [Rewrote change log to be more detailed and better organized]
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      764433a1
    • Robbie Ko's avatar
      Btrfs: send, fix warning due to late freeing of orphan_dir_info structures · 443f9d26
      Robbie Ko authored
      Under certain situations, when doing an incremental send, we can end up
      not freeing orphan_dir_info structures as soon as they are no longer
      needed. Instead we end up freeing them only after finishing the send
      stream, which causes a warning to be emitted:
      
      [282735.229200] ------------[ cut here ]------------
      [282735.229968] WARNING: CPU: 9 PID: 10588 at fs/btrfs/send.c:6298 btrfs_ioctl_send+0xe2f/0xe51 [btrfs]
      [282735.231282] Modules linked in: btrfs crc32c_generic xor raid6_pq acpi_cpufreq tpm_tis ppdev tpm parport_pc psmouse parport sg pcspkr i2c_piix4 i2c_core evdev processor serio_raw button loop autofs4 ext4 crc16 jbd2 mbcache sr_mod cdrom sd_mod ata_generic virtio_scsi ata_piix libata virtio_pci virtio_ring virtio e1000 scsi_mod floppy [last unloaded: btrfs]
      [282735.237130] CPU: 9 PID: 10588 Comm: btrfs Tainted: G        W       4.6.0-rc7-btrfs-next-31+ #1
      [282735.239309] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS by qemu-project.org 04/01/2014
      [282735.240160]  0000000000000000 ffff880224273ca8 ffffffff8126b42c 0000000000000000
      [282735.240160]  0000000000000000 ffff880224273ce8 ffffffff81052b14 0000189a24273ac8
      [282735.240160]  ffff8802210c9800 0000000000000000 0000000000000001 0000000000000000
      [282735.240160] Call Trace:
      [282735.240160]  [<ffffffff8126b42c>] dump_stack+0x67/0x90
      [282735.240160]  [<ffffffff81052b14>] __warn+0xc2/0xdd
      [282735.240160]  [<ffffffff81052beb>] warn_slowpath_null+0x1d/0x1f
      [282735.240160]  [<ffffffffa03c99d5>] btrfs_ioctl_send+0xe2f/0xe51 [btrfs]
      [282735.240160]  [<ffffffffa0398358>] btrfs_ioctl+0x14f/0x1f81 [btrfs]
      [282735.240160]  [<ffffffff8108e456>] ? arch_local_irq_save+0x9/0xc
      [282735.240160]  [<ffffffff8118da05>] vfs_ioctl+0x18/0x34
      [282735.240160]  [<ffffffff8118e00c>] do_vfs_ioctl+0x550/0x5be
      [282735.240160]  [<ffffffff81196f0c>] ? __fget+0x6b/0x77
      [282735.240160]  [<ffffffff81196fa1>] ? __fget_light+0x62/0x71
      [282735.240160]  [<ffffffff8118e0d1>] SyS_ioctl+0x57/0x79
      [282735.240160]  [<ffffffff8149e025>] entry_SYSCALL_64_fastpath+0x18/0xa8
      [282735.240160]  [<ffffffff81100c6b>] ? time_hardirqs_off+0x9/0x14
      [282735.240160]  [<ffffffff8108e87d>] ? trace_hardirqs_off_caller+0x1f/0xaa
      [282735.256343] ---[ end trace a4539270c8056f93 ]---
      
      Consider the following example:
      
        Parent snapshot:
      
        .                                                             (ino 256)
        |--- a/                                                       (ino 257)
        |    |--- c/                                                  (ino 260)
        |
        |--- del/                                                     (ino 259)
              |--- tmp/                                               (ino 258)
              |--- x/                                                 (ino 261)
              |--- y/                                                 (ino 262)
      
        Send snapshot:
      
        .                                                             (ino 256)
        |--- a/                                                       (ino 257)
        |    |--- x/                                                  (ino 261)
        |    |--- y/                                                  (ino 262)
        |
        |--- c/                                                       (ino 260)
             |--- tmp/                                                (ino 258)
      
      1) When processing inode 258, we end up delaying its rename operation
         because it has an ancestor (in the send snapshot) that has a higher
         inode number (inode 260) which was also renamed in the send snapshot,
         therefore we delay the rename of inode 258 so that it happens after
         inode 260 is renamed;
      
      2) When processing inode 259, we end up delaying its deletion (rmdir
         operation) because it has a child inode (258) that has its rename
         operation delayed. At this point we allocate an orphan_dir_info
         structure and tag inode 258 so that we later attempt to see if we
         can delete (rmdir) inode 259 once inode 258 is renamed;
      
      3) When we process inode 260, after renaming it we finally do the rename
         operation for inode 258. Once we issue the rename operation for inode
         258 we notice that this inode was tagged so that we attempt to see
         if at this point we can delete (rmdir) inode 259. But at this point
         we can not still delete inode 259 because it has 2 children, inodes
         261 and 262, that were not yet processed and therefore not yet
         moved (renamed) away from inode 259. We end up not freeing the
         orphan_dir_info structure allocated in step 2;
      
      4) We process inodes 261 and 262, and once we move/rename inode 262
         we issue the rmdir operation for inode 260;
      
      5) We finish the send stream and notice that red black tree that
         contains orphan_dir_info structures is not empty, so we emit
         a warning and then free any orphan_dir_structures left.
      
      So fix this by freeing an orphan_dir_info structure once we try to
      apply a pending rename operation if we can not delete yet the tagged
      directory.
      
      A test case for fstests follows soon.
      Signed-off-by: default avatarRobbie Ko <robbieko@synology.com>
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      [Modified changelog to be more detailed and easier to understand]
      443f9d26
    • Robbie Ko's avatar
      Btrfs: incremental send, fix premature rmdir operations · 99ea42dd
      Robbie Ko authored
      Under certain situations, an incremental send operation can contain
      a rmdir operation that will make the receiving end fail when attempting
      to execute it, because the target directory is not yet empty.
      
      Consider the following example:
      
        Parent snapshot:
      
        .                                                             (ino 256)
        |--- a/                                                       (ino 257)
        |    |--- c/                                                  (ino 260)
        |
        |--- del/                                                     (ino 259)
              |--- tmp/                                               (ino 258)
              |--- x/                                                 (ino 261)
      
        Send snapshot:
      
        .                                                             (ino 256)
        |--- a/                                                       (ino 257)
        |    |--- x/                                                  (ino 261)
        |
        |--- c/                                                       (ino 260)
             |--- tmp/                                                (ino 258)
      
      1) When processing inode 258, we delay its rename operation because inode
         260 is its new parent in the send snapshot and it was not yet renamed
         (since 260 > 258, that is, beyond the current progress);
      
      2) When processing inode 259, we realize we can not yet send an rmdir
         operation (against inode 259) because inode 258 was still not yet
         renamed/moved away from inode 259. Therefore we update data structures
         so that after inode 258 is renamed, we try again to see if we can
         finally send an rmdir operation for inode 259;
      
      3) When we process inode 260, we send a rename operation for it followed
         by a rename operation for inode 258. Once we send the rename operation
         for inode 258 we then check if we can finally issue an rmdir for its
         previous parent, inode 259, by calling the can_rmdir() function with
         a value of sctx->cur_ino + 1 (260 + 1 = 261) for its "progress"
         argument. This makes can_rmdir() return true (value 1) because even
         though there's still a child inode of inode 259 that was not yet
         renamed/moved, which is inode 261, the given value of progress (261)
         is not lower then 261 (that is, not lower than the inode number of
         some child of inode 259). So we end up sending a rmdir operation for
         inode 259 before its child inode 261 is processed and renamed.
      
      So fix this by passing the correct progress value to the call to
      can_rmdir() from within apply_dir_move() (where we issue delayed rename
      operations), which should match stcx->cur_ino (the number of the inode
      currently being processed) and not sctx->cur_ino + 1.
      
      A test case for fstests follows soon.
      Signed-off-by: default avatarRobbie Ko <robbieko@synology.com>
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      [Rewrote change log to be more detailed, clear and well formatted]
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      99ea42dd
    • Filipe Manana's avatar
      Btrfs: incremental send, fix invalid paths for rename operations · 4122ea64
      Filipe Manana authored
      Example scenario:
      
        Parent snapshot:
      
        .                                                       (ino 277)
        |---- tmp/                                              (ino 278)
        |---- pre/                                              (ino 280)
        |      |---- wait_dir/                                  (ino 281)
        |
        |---- desc/                                             (ino 282)
        |---- ance/                                             (ino 283)
        |       |---- below_ance/                               (ino 279)
        |
        |---- other_dir/                                        (ino 284)
      
        Send snapshot:
      
        .                                                       (ino 277)
        |---- tmp/                                              (ino 278)
               |---- other_dir/                                 (ino 284)
                         |---- below_ance/                      (ino 279)
                         |            |---- pre/                (ino 280)
                         |
                         |---- wait_dir/                        (ino 281)
                                    |---- desc/                 (ino 282)
                                            |---- ance/         (ino 283)
      
      While computing the send stream the following steps happen:
      
      1) While processing inode 279 we end up delaying its rename operation
         because its new parent in the send snapshot, inode 284, was not
         yet processed and therefore not yet renamed;
      
      2) Later when processing inode 280 we end up renaming it immediately to
         "ance/below_once/pre" and not delay its rename operation because its
         new parent (inode 279 in the send snapshot) has its rename operation
         delayed and inode 280 is not an encestor of inode 279 (its parent in
         the send snapshot) in the parent snapshot;
      
      3) When processing inode 281 we end up delaying its rename operation
         because its new parent in the send snapshot, inode 284, was not yet
         processed and therefore not yet renamed;
      
      4) When processing inode 282 we do not delay its rename operation because
         its parent in the send snapshot, inode 281, already has its own rename
         operation delayed and our current inode (282) is not an ancestor of
         inode 281 in the parent snapshot. Therefore inode 282 is renamed to
         "ance/below_ance/pre/wait_dir";
      
      5) When processing inode 283 we realize that we can rename it because one
         of its ancestors in the send snapshot, inode 281, has its rename
         operation delayed and inode 283 is not an ancestor of inode 281 in the
         parent snapshot. So a rename operation to rename inode 283 to
         "ance/below_ance/pre/wait_dir/desc/ance" is issued. This path is
         invalid due to a missing path building loop that was undetected by
         the incremental send implementation, as inode 283 ends up getting
         included twice in the path (once with its path in the parent snapshot).
         Therefore its rename operation must wait before the ancestor inode 284
         is renamed.
      
      Fix this by not terminating the rename dependency checks when we find an
      ancestor, in the send snapshot, that has its rename operation delayed. So
      that we continue doing the same checks if the current inode is not an
      ancestor, in the parent snapshot, of an ancestor in the send snapshot we
      are processing in the loop.
      
      The problem and reproducer were reported by Robbie Ko, as part of a patch
      titled "Btrfs: incremental send, avoid ancestor rename to descendant".
      However the fix was unnecessarily complicated and can be addressed with
      much less code and effort.
      Reported-by: default avatarRobbie Ko <robbieko@synology.com>
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      4122ea64
    • Filipe Manana's avatar
      Btrfs: send, add missing error check for calls to path_loop() · 7969e77a
      Filipe Manana authored
      The function path_loop() can return a negative integer, signaling an
      error, 0 if there's no path loop and 1 if there's a path loop. We were
      treating any non zero values as meaning that a path loop exists. Fix
      this by explicitly checking for errors and gracefully return them to
      user space.
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      7969e77a
    • Robbie Ko's avatar
      Btrfs: send, fix failure to move directories with the same name around · 801bec36
      Robbie Ko authored
      When doing an incremental send we can end up not moving directories that
      have the same name. This happens when the same parent directory has
      different child directories with the same name in the parent and send
      snapshots.
      
      For example, consider the following scenario:
      
        Parent snapshot:
      
        .                   (ino 256)
        |---- d/            (ino 257)
        |     |--- p1/      (ino 258)
        |
        |---- p1/           (ino 259)
      
        Send snapshot:
      
        .                    (ino 256)
        |--- d/              (ino 257)
             |--- p1/        (ino 259)
                   |--- p1/  (ino 258)
      
      The directory named "d" (inode 257) has in both snapshots an entry with
      the name "p1" but it refers to different inodes in both snapshots (inode
      258 in the parent snapshot and inode 259 in the send snapshot). When
      attempting to move inode 258, the operation is delayed because its new
      parent, inode 259, was not yet moved/renamed (as the stream is currently
      processing inode 258). Then when processing inode 259, we also end up
      delaying its move/rename operation so that it happens after inode 258 is
      moved/renamed. This decision to delay the move/rename rename operation
      of inode 259 is due to the fact that the new parent inode (257) still
      has inode 258 as its child, which has the same name has inode 259. So
      we end up with inode 258 move/rename operation waiting for inode's 259
      move/rename operation, which in turn it waiting for inode's 258
      move/rename. This results in ending the send stream without issuing
      move/rename operations for inodes 258 and 259 and generating the
      following warnings in syslog/dmesg:
      
      [148402.979747] ------------[ cut here ]------------
      [148402.980588] WARNING: CPU: 14 PID: 4117 at fs/btrfs/send.c:6177 btrfs_ioctl_send+0xe03/0xe51 [btrfs]
      [148402.981928] Modules linked in: btrfs crc32c_generic xor raid6_pq acpi_cpufreq tpm_tis ppdev tpm parport_pc psmouse parport sg pcspkr i2c_piix4 i2c_core evdev processor serio_raw button loop autofs4 ext4 crc16 jbd2 mbcache sr_mod cdrom sd_mod ata_generic virtio_scsi ata_piix libata virtio_pci virtio_ring virtio e1000 scsi_mod floppy [last unloaded: btrfs]
      [148402.986999] CPU: 14 PID: 4117 Comm: btrfs Tainted: G        W       4.6.0-rc7-btrfs-next-31+ #1
      [148402.988136] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS by qemu-project.org 04/01/2014
      [148402.988136]  0000000000000000 ffff88022139fca8 ffffffff8126b42c 0000000000000000
      [148402.988136]  0000000000000000 ffff88022139fce8 ffffffff81052b14 000018212139fac8
      [148402.988136]  ffff88022b0db400 0000000000000000 0000000000000001 0000000000000000
      [148402.988136] Call Trace:
      [148402.988136]  [<ffffffff8126b42c>] dump_stack+0x67/0x90
      [148402.988136]  [<ffffffff81052b14>] __warn+0xc2/0xdd
      [148402.988136]  [<ffffffff81052beb>] warn_slowpath_null+0x1d/0x1f
      [148402.988136]  [<ffffffffa04bc831>] btrfs_ioctl_send+0xe03/0xe51 [btrfs]
      [148402.988136]  [<ffffffffa048b358>] btrfs_ioctl+0x14f/0x1f81 [btrfs]
      [148402.988136]  [<ffffffff8108e456>] ? arch_local_irq_save+0x9/0xc
      [148402.988136]  [<ffffffff8108eb51>] ? __lock_is_held+0x3c/0x57
      [148402.988136]  [<ffffffff8118da05>] vfs_ioctl+0x18/0x34
      [148402.988136]  [<ffffffff8118e00c>] do_vfs_ioctl+0x550/0x5be
      [148402.988136]  [<ffffffff81196f0c>] ? __fget+0x6b/0x77
      [148402.988136]  [<ffffffff81196fa1>] ? __fget_light+0x62/0x71
      [148402.988136]  [<ffffffff8118e0d1>] SyS_ioctl+0x57/0x79
      [148402.988136]  [<ffffffff8149e025>] entry_SYSCALL_64_fastpath+0x18/0xa8
      [148402.988136]  [<ffffffff8108e89d>] ? trace_hardirqs_off_caller+0x3f/0xaa
      [148403.011373] ---[ end trace a4539270c8056f8b ]---
      [148403.012296] ------------[ cut here ]------------
      [148403.013071] WARNING: CPU: 14 PID: 4117 at fs/btrfs/send.c:6194 btrfs_ioctl_send+0xe19/0xe51 [btrfs]
      [148403.014447] Modules linked in: btrfs crc32c_generic xor raid6_pq acpi_cpufreq tpm_tis ppdev tpm parport_pc psmouse parport sg pcspkr i2c_piix4 i2c_core evdev processor serio_raw button loop autofs4 ext4 crc16 jbd2 mbcache sr_mod cdrom sd_mod ata_generic virtio_scsi ata_piix libata virtio_pci virtio_ring virtio e1000 scsi_mod floppy [last unloaded: btrfs]
      [148403.019708] CPU: 14 PID: 4117 Comm: btrfs Tainted: G        W       4.6.0-rc7-btrfs-next-31+ #1
      [148403.020104] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS by qemu-project.org 04/01/2014
      [148403.020104]  0000000000000000 ffff88022139fca8 ffffffff8126b42c 0000000000000000
      [148403.020104]  0000000000000000 ffff88022139fce8 ffffffff81052b14 000018322139fac8
      [148403.020104]  ffff88022b0db400 0000000000000000 0000000000000001 0000000000000000
      [148403.020104] Call Trace:
      [148403.020104]  [<ffffffff8126b42c>] dump_stack+0x67/0x90
      [148403.020104]  [<ffffffff81052b14>] __warn+0xc2/0xdd
      [148403.020104]  [<ffffffff81052beb>] warn_slowpath_null+0x1d/0x1f
      [148403.020104]  [<ffffffffa04bc847>] btrfs_ioctl_send+0xe19/0xe51 [btrfs]
      [148403.020104]  [<ffffffffa048b358>] btrfs_ioctl+0x14f/0x1f81 [btrfs]
      [148403.020104]  [<ffffffff8108e456>] ? arch_local_irq_save+0x9/0xc
      [148403.020104]  [<ffffffff8108eb51>] ? __lock_is_held+0x3c/0x57
      [148403.020104]  [<ffffffff8118da05>] vfs_ioctl+0x18/0x34
      [148403.020104]  [<ffffffff8118e00c>] do_vfs_ioctl+0x550/0x5be
      [148403.020104]  [<ffffffff81196f0c>] ? __fget+0x6b/0x77
      [148403.020104]  [<ffffffff81196fa1>] ? __fget_light+0x62/0x71
      [148403.020104]  [<ffffffff8118e0d1>] SyS_ioctl+0x57/0x79
      [148403.020104]  [<ffffffff8149e025>] entry_SYSCALL_64_fastpath+0x18/0xa8
      [148403.020104]  [<ffffffff8108e89d>] ? trace_hardirqs_off_caller+0x3f/0xaa
      [148403.038981] ---[ end trace a4539270c8056f8c ]---
      
      There's another issue caused by similar (but more complex) changes in the
      directory hierarchy that makes move/rename operations fail, described with
      the following example:
      
        Parent snapshot:
      
        .
        |---- a/                                                   (ino 262)
        |     |---- c/                                             (ino 268)
        |
        |---- d/                                                   (ino 263)
              |---- ance/                                          (ino 267)
                      |---- e/                                     (ino 264)
                      |---- f/                                     (ino 265)
                      |---- ance/                                  (ino 266)
      
        Send snapshot:
      
        .
        |---- a/                                                   (ino 262)
        |---- c/                                                   (ino 268)
        |     |---- ance/                                          (ino 267)
        |
        |---- d/                                                   (ino 263)
        |     |---- ance/                                          (ino 266)
        |
        |---- f/                                                   (ino 265)
              |---- e/                                             (ino 264)
      
      When the inode 265 is processed, the path for inode 267 is computed, which
      at that time corresponds to "d/ance", and it's stored in the names cache.
      Later on when processing inode 266, we end up orphanizing (renaming to a
      name matching the pattern o<ino>-<gen>-<seq>) inode 267 because it has
      the same name as inode 266 and it's currently a child of the new parent
      directory (inode 263) for inode 266. After the orphanization and while we
      are still processing inode 266, a rename operation for inode 266 is
      generated. However the source path for that rename operation is incorrect
      because it ends up using the old, pre-orphanization, name of inode 267.
      The no longer valid name for inode 267 was previously cached when
      processing inode 265 and it remains usable and considered valid until
      the inode currently being processed has a number greater than 267.
      This resulted in the receiving side failing with the following error:
      
        ERROR: rename d/ance/ance -> d/ance failed: No such file or directory
      
      So fix these issues by detecting such circular dependencies for rename
      operations and by clearing the cached name of an inode once the inode
      is orphanized.
      
      A test case for fstests will follow soon.
      Signed-off-by: default avatarRobbie Ko <robbieko@synology.com>
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      [Rewrote change log to be more detailed and organized, and improved
       comments]
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      801bec36
    • Filipe Manana's avatar
      Btrfs: add missing check for writeback errors on fsync · 0596a904
      Filipe Manana authored
      When we start an fsync we start ordered extents for all delalloc ranges.
      However before attempting to log the inode, we only wait for those ordered
      extents if we are not doing a full sync (bit BTRFS_INODE_NEEDS_FULL_SYNC
      is set in the inode's flags). This means that if an ordered extent
      completes with an IO error before we check if we can skip logging the
      inode, we will not catch and report the IO error to user space. This is
      because on an IO error, when the ordered extent completes we do not
      update the inode, so if the inode was not previously updated by the
      current transaction we end up not logging it through calls to fsync and
      therefore not check its mapping flags for the presence of IO errors.
      
      Fix this by checking for errors in the flags of the inode's mapping when
      we notice we can skip logging the inode.
      
      This caused sporadic failures in the test generic/331 (which explicitly
      tests for IO errors during an fsync call).
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      Reviewed-by: default avatarLiu Bo <bo.li.liu@oracle.com>
      0596a904
  5. 26 Jul, 2016 17 commits