1. 27 Jul, 2020 40 commits
    • Josef Bacik's avatar
      btrfs: document special case error codes for fs errors · 59131393
      Josef Bacik authored
      We've had some discussions about what to do in certain scenarios for
      error codes, specifically EUCLEAN and EROFS.  Document these near the
      error handling code so its clear what their intentions are.
      Signed-off-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      59131393
    • Josef Bacik's avatar
      btrfs: don't WARN if we abort a transaction with EROFS · f95ebdbe
      Josef Bacik authored
      If we got some sort of corruption via a read and call
      btrfs_handle_fs_error() we'll set BTRFS_FS_STATE_ERROR on the fs and
      complain.  If a subsequent trans handle trips over this it'll get EROFS
      and then abort.  However at that point we're not aborting for the
      original reason, we're aborting because we've been flipped read only.
      We do not need to WARN_ON() here.
      
      CC: stable@vger.kernel.org # 5.4+
      Signed-off-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      f95ebdbe
    • Filipe Manana's avatar
      btrfs: reduce contention on log trees when logging checksums · 3ebac17c
      Filipe Manana authored
      The possibility of extents being shared (through clone and deduplication
      operations) requires special care when logging data checksums, to avoid
      having a log tree with different checksum items that cover ranges which
      overlap (which resulted in missing checksums after replaying a log tree).
      Such problems were fixed in the past by the following commits:
      
      commit 40e046ac ("Btrfs: fix missing data checksums after replaying a
                            log tree")
      
      commit e289f03e ("btrfs: fix corrupt log due to concurrent fsync of
                            inodes with shared extents")
      
      Test case generic/588 exercises the scenario solved by the first commit
      (purely sequential and deterministic) while test case generic/457 often
      triggered the case fixed by the second commit (not deterministic, requires
      specific timings under concurrency).
      
      The problems were addressed by deleting, from the log tree, any existing
      checksums before logging the new ones. And also by doing the deletion and
      logging of the cheksums while locking the checksum range in an extent io
      tree (root->log_csum_range), to deal with the case where we have concurrent
      fsyncs against files with shared extents.
      
      That however causes more contention on the leaves of a log tree where we
      store checksums (and all the nodes in the paths leading to them), even
      when we do not have shared extents, or all the shared extents were created
      by past transactions. It also adds a bit of contention on the spin lock of
      the log_csums_range extent io tree of the log root.
      
      This change adds a 'last_reflink_trans' field to the inode to keep track
      of the last transaction where a new extent was shared between inodes
      (through clone and deduplication operations). It is updated for both the
      source and destination inodes of reflink operations whenever a new extent
      (created in the current transaction) becomes shared by the inodes. This
      field is kept in memory only, not persisted in the inode item, similar
      to other existing fields (last_unlink_trans, logged_trans).
      
      When logging checksums for an extent, if the value of 'last_reflink_trans'
      is smaller then the current transaction's generation/id, we skip locking
      the extent range and deletion of checksums from the log tree, since we
      know we do not have new shared extents. This reduces contention on the
      log tree's leaves where checksums are stored.
      
      The following script, which uses fio, was used to measure the impact of
      this change:
      
        $ cat test-fsync.sh
        #!/bin/bash
      
        DEV=/dev/sdk
        MNT=/mnt/sdk
        MOUNT_OPTIONS="-o ssd"
        MKFS_OPTIONS="-d single -m single"
      
        if [ $# -ne 3 ]; then
            echo "Use $0 NUM_JOBS FILE_SIZE FSYNC_FREQ"
            exit 1
        fi
      
        NUM_JOBS=$1
        FILE_SIZE=$2
        FSYNC_FREQ=$3
      
        cat <<EOF > /tmp/fio-job.ini
        [writers]
        rw=write
        fsync=$FSYNC_FREQ
        fallocate=none
        group_reporting=1
        direct=0
        bs=64k
        ioengine=sync
        size=$FILE_SIZE
        directory=$MNT
        numjobs=$NUM_JOBS
        EOF
      
        echo "Using config:"
        echo
        cat /tmp/fio-job.ini
        echo
      
        mkfs.btrfs -f $MKFS_OPTIONS $DEV
        mount $MOUNT_OPTIONS $DEV $MNT
        fio /tmp/fio-job.ini
        umount $MNT
      
      The tests were performed for different numbers of jobs, file sizes and
      fsync frequency. A qemu VM using kvm was used, with 8 cores (the host has
      12 cores, with cpu governance set to performance mode on all cores), 16GiB
      of ram (the host has 64GiB) and using a NVMe device directly (without an
      intermediary filesystem in the host). While running the tests, the host
      was not used for anything else, to avoid disturbing the tests.
      
      The obtained results were the following (the last line of fio's output was
      pasted). Starting with 16 jobs is where a significant difference is
      observable in this particular setup and hardware (differences highlighted
      below). The very small differences for tests with less than 16 jobs are
      possibly just noise and random.
      
          **** 1 job, file size 1G, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=23.8MiB/s (24.9MB/s), 23.8MiB/s-23.8MiB/s (24.9MB/s-24.9MB/s), io=1024MiB (1074MB), run=43075-43075msec
      
      after this change:
      
      WRITE: bw=24.4MiB/s (25.6MB/s), 24.4MiB/s-24.4MiB/s (25.6MB/s-25.6MB/s), io=1024MiB (1074MB), run=41938-41938msec
      
          **** 2 jobs, file size 1G, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=37.7MiB/s (39.5MB/s), 37.7MiB/s-37.7MiB/s (39.5MB/s-39.5MB/s), io=2048MiB (2147MB), run=54351-54351msec
      
      after this change:
      
      WRITE: bw=37.7MiB/s (39.5MB/s), 37.6MiB/s-37.6MiB/s (39.5MB/s-39.5MB/s), io=2048MiB (2147MB), run=54428-54428msec
      
          **** 4 jobs, file size 1G, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=67.5MiB/s (70.8MB/s), 67.5MiB/s-67.5MiB/s (70.8MB/s-70.8MB/s), io=4096MiB (4295MB), run=60669-60669msec
      
      after this change:
      
      WRITE: bw=68.6MiB/s (71.0MB/s), 68.6MiB/s-68.6MiB/s (71.0MB/s-71.0MB/s), io=4096MiB (4295MB), run=59678-59678msec
      
          **** 8 jobs, file size 1G, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=128MiB/s (134MB/s), 128MiB/s-128MiB/s (134MB/s-134MB/s), io=8192MiB (8590MB), run=64048-64048msec
      
      after this change:
      
      WRITE: bw=129MiB/s (135MB/s), 129MiB/s-129MiB/s (135MB/s-135MB/s), io=8192MiB (8590MB), run=63405-63405msec
      
          **** 16 jobs, file size 1G, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=78.5MiB/s (82.3MB/s), 78.5MiB/s-78.5MiB/s (82.3MB/s-82.3MB/s), io=16.0GiB (17.2GB), run=208676-208676msec
      
      after this change:
      
      WRITE: bw=110MiB/s (115MB/s), 110MiB/s-110MiB/s (115MB/s-115MB/s), io=16.0GiB (17.2GB), run=149295-149295msec
      (+40.1% throughput, -28.5% runtime)
      
          **** 32 jobs, file size 1G, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=58.8MiB/s (61.7MB/s), 58.8MiB/s-58.8MiB/s (61.7MB/s-61.7MB/s), io=32.0GiB (34.4GB), run=557134-557134msec
      
      after this change:
      
      WRITE: bw=76.1MiB/s (79.8MB/s), 76.1MiB/s-76.1MiB/s (79.8MB/s-79.8MB/s), io=32.0GiB (34.4GB), run=430550-430550msec
      (+29.4% throughput, -22.7% runtime)
      
          **** 64 jobs, file size 512M, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=65.8MiB/s (68.0MB/s), 65.8MiB/s-65.8MiB/s (68.0MB/s-68.0MB/s), io=32.0GiB (34.4GB), run=498055-498055msec
      
      after this change:
      
      WRITE: bw=85.1MiB/s (89.2MB/s), 85.1MiB/s-85.1MiB/s (89.2MB/s-89.2MB/s), io=32.0GiB (34.4GB), run=385116-385116msec
      (+29.3% throughput, -22.7% runtime)
      
          **** 128 jobs, file size 256M, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=54.7MiB/s (57.3MB/s), 54.7MiB/s-54.7MiB/s (57.3MB/s-57.3MB/s), io=32.0GiB (34.4GB), run=599373-599373msec
      
      after this change:
      
      WRITE: bw=121MiB/s (126MB/s), 121MiB/s-121MiB/s (126MB/s-126MB/s), io=32.0GiB (34.4GB), run=271907-271907msec
      (+121.2% throughput, -54.6% runtime)
      
          **** 256 jobs, file size 256M, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=69.2MiB/s (72.5MB/s), 69.2MiB/s-69.2MiB/s (72.5MB/s-72.5MB/s), io=64.0GiB (68.7GB), run=947536-947536msec
      
      after this change:
      
      WRITE: bw=121MiB/s (127MB/s), 121MiB/s-121MiB/s (127MB/s-127MB/s), io=64.0GiB (68.7GB), run=541916-541916msec
      (+74.9% throughput, -42.8% runtime)
      
          **** 512 jobs, file size 128M, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=85.4MiB/s (89.5MB/s), 85.4MiB/s-85.4MiB/s (89.5MB/s-89.5MB/s), io=64.0GiB (68.7GB), run=767734-767734msec
      
      after this change:
      
      WRITE: bw=141MiB/s (147MB/s), 141MiB/s-141MiB/s (147MB/s-147MB/s), io=64.0GiB (68.7GB), run=466022-466022msec
      (+65.1% throughput, -39.3% runtime)
      
          **** 1024 jobs, file size 128M, fsync frequency 1 ****
      
      before this change:
      
      WRITE: bw=115MiB/s (120MB/s), 115MiB/s-115MiB/s (120MB/s-120MB/s), io=128GiB (137GB), run=1143775-1143775msec
      
      after this change:
      
      WRITE: bw=171MiB/s (180MB/s), 171MiB/s-171MiB/s (180MB/s-180MB/s), io=128GiB (137GB), run=764843-764843msec
      (+48.7% throughput, -33.1% runtime)
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      3ebac17c
    • Nikolay Borisov's avatar
      btrfs: remove done label in writepage_delalloc · b69d1ee9
      Nikolay Borisov authored
      Since there is not common cleanup run after the label it makes it
      somewhat redundant.
      Reviewed-by: default avatarJohannes Thumshirn <johannes.thumshirn@wdc.com>
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      b69d1ee9
    • Qu Wenruo's avatar
      btrfs: add comments for btrfs_reserve_flush_enum · fd7fb634
      Qu Wenruo authored
      This enum is the interface exposed to developers.
      
      Although we have a detailed comment explaining the whole idea of space
      flushing at the beginning of space-info.c, the exposed enum interface
      doesn't have any comment.
      
      Some corner cases, like BTRFS_RESERVE_FLUSH_ALL and
      BTRFS_RESERVE_FLUSH_ALL_STEAL can be interrupted by fatal signals, are
      not explained at all.
      
      So add some simple comments for these enums as a quick reference.
      Signed-off-by: default avatarQu Wenruo <wqu@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      fd7fb634
    • Qu Wenruo's avatar
      btrfs: relocation: review the call sites which can be interrupted by signal · 44d354ab
      Qu Wenruo authored
      Since most metadata reservation calls can return -EINTR when get
      interrupted by fatal signal, we need to review the all the metadata
      reservation call sites.
      
      In relocation code, the metadata reservation happens in the following
      sites:
      
      - btrfs_block_rsv_refill() in merge_reloc_root()
        merge_reloc_root() is a pretty critical section, we don't want to be
        interrupted by signal, so change the flush status to
        BTRFS_RESERVE_FLUSH_LIMIT, so it won't get interrupted by signal.
        Since such change can be ENPSPC-prone, also shrink the amount of
        metadata to reserve least amount avoid deadly ENOSPC there.
      
      - btrfs_block_rsv_refill() in reserve_metadata_space()
        It calls with BTRFS_RESERVE_FLUSH_LIMIT, which won't get interrupted
        by signal.
      
      - btrfs_block_rsv_refill() in prepare_to_relocate()
      
      - btrfs_block_rsv_add() in prepare_to_relocate()
      
      - btrfs_block_rsv_refill() in relocate_block_group()
      
      - btrfs_delalloc_reserve_metadata() in relocate_file_extent_cluster()
      
      - btrfs_start_transaction() in relocate_block_group()
      
      - btrfs_start_transaction() in create_reloc_inode()
        Can be interrupted by fatal signal and we can handle it easily.
        For these call sites, just catch the -EINTR value in btrfs_balance()
        and count them as canceled.
      
      CC: stable@vger.kernel.org # 5.4+
      Signed-off-by: default avatarQu Wenruo <wqu@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      44d354ab
    • Qu Wenruo's avatar
      btrfs: avoid possible signal interruption of btrfs_drop_snapshot() on relocation tree · f3e3d9cc
      Qu Wenruo authored
      [BUG]
      There is a bug report about bad signal timing could lead to read-only
      fs during balance:
      
        BTRFS info (device xvdb): balance: start -d -m -s
        BTRFS info (device xvdb): relocating block group 73001861120 flags metadata
        BTRFS info (device xvdb): found 12236 extents, stage: move data extents
        BTRFS info (device xvdb): relocating block group 71928119296 flags data
        BTRFS info (device xvdb): found 3 extents, stage: move data extents
        BTRFS info (device xvdb): found 3 extents, stage: update data pointers
        BTRFS info (device xvdb): relocating block group 60922265600 flags metadata
        BTRFS: error (device xvdb) in btrfs_drop_snapshot:5505: errno=-4 unknown
        BTRFS info (device xvdb): forced readonly
        BTRFS info (device xvdb): balance: ended with status: -4
      
      [CAUSE]
      The direct cause is the -EINTR from the following call chain when a
      fatal signal is pending:
      
       relocate_block_group()
       |- clean_dirty_subvols()
          |- btrfs_drop_snapshot()
             |- btrfs_start_transaction()
                |- btrfs_delayed_refs_rsv_refill()
                   |- btrfs_reserve_metadata_bytes()
                      |- __reserve_metadata_bytes()
                         |- wait_reserve_ticket()
                            |- prepare_to_wait_event();
                            |- ticket->error = -EINTR;
      
      Normally this behavior is fine for most btrfs_start_transaction()
      callers, as they need to catch any other error, same for the signal, and
      exit ASAP.
      
      However for balance, especially for the clean_dirty_subvols() case, we're
      already doing cleanup works, getting -EINTR from btrfs_drop_snapshot()
      could cause a lot of unexpected problems.
      
      From the mentioned forced read-only report, to later balance error due
      to half dropped reloc trees.
      
      [FIX]
      Fix this problem by using btrfs_join_transaction() if
      btrfs_drop_snapshot() is called from relocation context.
      
      Since btrfs_join_transaction() won't get interrupted by signal, we can
      continue the cleanup.
      
      CC: stable@vger.kernel.org # 5.4+
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarQu Wenruo <wqu@suse.com>
      Reviewed-by: David Sterba <dsterba@suse.com>3
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      f3e3d9cc
    • Qu Wenruo's avatar
      btrfs: relocation: allow signal to cancel balance · 5cb502f4
      Qu Wenruo authored
      Although btrfs balance can be canceled with "btrfs balance cancel"
      command, it's still almost muscle memory to press Ctrl-C to cancel a
      long running btrfs balance.
      
      So allow btrfs balance to check signal to determine if it should exit.
      The cancellation points are in known location and we're only adding one
      more reason, so this should be safe.
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarQu Wenruo <wqu@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      5cb502f4
    • Nikolay Borisov's avatar
      btrfs: raid56: remove out label in __raid56_parity_recover · 813f8a0e
      Nikolay Borisov authored
      There's no cleanup that occurs so we can simply return 0 directly.
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      813f8a0e
    • David Sterba's avatar
      btrfs: add missing check for nocow and compression inode flags · f37c563b
      David Sterba authored
      User Forza reported on IRC that some invalid combinations of file
      attributes are accepted by chattr.
      
      The NODATACOW and compression file flags/attributes are mutually
      exclusive, but they could be set by 'chattr +c +C' on an empty file. The
      nodatacow will be in effect because it's checked first in
      btrfs_run_delalloc_range.
      
      Extend the flag validation to catch the following cases:
      
        - input flags are conflicting
        - old and new flags are conflicting
        - initialize the local variable with inode flags after inode ls locked
      
      Inode attributes take precedence over mount options and are an
      independent setting.
      
      Nocompress would be a no-op with nodatacow, but we don't want to mix
      any compression-related options with nodatacow.
      
      CC: stable@vger.kernel.org # 4.4+
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      f37c563b
    • Anand Jain's avatar
      btrfs: don't traverse into the seed devices in show_devname · 4faf55b0
      Anand Jain authored
      ->show_devname currently shows the lowest devid in the list. As the seed
      devices have the lowest devid in the sprouted filesystem, the userland
      tool such as findmnt end up seeing seed device instead of the device from
      the read-writable sprouted filesystem. As shown below.
      
       mount /dev/sda /btrfs
       mount: /btrfs: WARNING: device write-protected, mounted read-only.
      
       findmnt --output SOURCE,TARGET,UUID /btrfs
       SOURCE   TARGET UUID
       /dev/sda /btrfs 899f7027-3e46-4626-93e7-7d4c9ad19111
      
       btrfs dev add -f /dev/sdb /btrfs
      
       umount /btrfs
       mount /dev/sdb /btrfs
      
       findmnt --output SOURCE,TARGET,UUID /btrfs
       SOURCE   TARGET UUID
       /dev/sda /btrfs 899f7027-3e46-4626-93e7-7d4c9ad19111
      
      All sprouts from a single seed will show the same seed device and the
      same fsid. That's confusing.
      This is causing problems in our prototype as there isn't any reference
      to the sprout file-system(s) which is being used for actual read and
      write.
      
      This was added in the patch which implemented the show_devname in btrfs
      commit 9c5085c1 ("Btrfs: implement ->show_devname").
      I tried to look for any particular reason that we need to show the seed
      device, there isn't any.
      
      So instead, do not traverse through the seed devices, just show the
      lowest devid in the sprouted fsid.
      
      After the patch:
      
       mount /dev/sda /btrfs
       mount: /btrfs: WARNING: device write-protected, mounted read-only.
      
       findmnt --output SOURCE,TARGET,UUID /btrfs
       SOURCE   TARGET UUID
       /dev/sda /btrfs 899f7027-3e46-4626-93e7-7d4c9ad19111
      
       btrfs dev add -f /dev/sdb /btrfs
       mount -o rw,remount /dev/sdb /btrfs
      
       findmnt --output SOURCE,TARGET,UUID /btrfs
       SOURCE   TARGET UUID
       /dev/sdb /btrfs 595ca0e6-b82e-46b5-b9e2-c72a6928be48
      
       mount /dev/sda /btrfs1
       mount: /btrfs1: WARNING: device write-protected, mounted read-only.
      
       btrfs dev add -f /dev/sdc /btrfs1
      
       findmnt --output SOURCE,TARGET,UUID /btrfs1
       SOURCE   TARGET  UUID
       /dev/sdc /btrfs1 ca1dbb7a-8446-4f95-853c-a20f3f82bdbb
      
       cat /proc/self/mounts | grep btrfs
       /dev/sdb /btrfs btrfs rw,relatime,noacl,space_cache,subvolid=5,subvol=/ 0 0
       /dev/sdc /btrfs1 btrfs ro,relatime,noacl,space_cache,subvolid=5,subvol=/ 0 0
      Reported-by: default avatarMartin K. Petersen <martin.petersen@oracle.com>
      CC: stable@vger.kernel.org # 4.19+
      Tested-by: default avatarMartin K. Petersen <martin.petersen@oracle.com>
      Signed-off-by: default avatarAnand Jain <anand.jain@oracle.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      4faf55b0
    • Qu Wenruo's avatar
      btrfs: qgroup: free per-trans reserved space when a subvolume gets dropped · a3cf0e43
      Qu Wenruo authored
      [BUG]
      Sometime fsstress could lead to qgroup warning for case like
      generic/013:
      
        BTRFS warning (device dm-3): qgroup 0/259 has unreleased space, type 1 rsv 81920
        ------------[ cut here ]------------
        WARNING: CPU: 9 PID: 24535 at fs/btrfs/disk-io.c:4142 close_ctree+0x1dc/0x323 [btrfs]
        Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
        RIP: 0010:close_ctree+0x1dc/0x323 [btrfs]
        Call Trace:
         btrfs_put_super+0x15/0x17 [btrfs]
         generic_shutdown_super+0x72/0x110
         kill_anon_super+0x18/0x30
         btrfs_kill_super+0x17/0x30 [btrfs]
         deactivate_locked_super+0x3b/0xa0
         deactivate_super+0x40/0x50
         cleanup_mnt+0x135/0x190
         __cleanup_mnt+0x12/0x20
         task_work_run+0x64/0xb0
         __prepare_exit_to_usermode+0x1bc/0x1c0
         __syscall_return_slowpath+0x47/0x230
         do_syscall_64+0x64/0xb0
         entry_SYSCALL_64_after_hwframe+0x44/0xa9
        ---[ end trace 6c341cdf9b6cc3c1 ]---
        BTRFS error (device dm-3): qgroup reserved space leaked
      
      While that subvolume 259 is no longer in that filesystem.
      
      [CAUSE]
      Normally per-trans qgroup reserved space is freed when a transaction is
      committed, in commit_fs_roots().
      
      However for completely dropped subvolume, that subvolume is completely
      gone, thus is no longer in the fs_roots_radix, and its per-trans
      reserved qgroup will never be freed.
      
      Since the subvolume is already gone, leaked per-trans space won't cause
      any trouble for end users.
      
      [FIX]
      Just call btrfs_qgroup_free_meta_all_pertrans() before a subvolume is
      completely dropped.
      Signed-off-by: default avatarQu Wenruo <wqu@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      a3cf0e43
    • Tom Rix's avatar
      btrfs: ref-verify: fix memory leak in add_block_entry · d60ba8de
      Tom Rix authored
      clang static analysis flags this error
      
      fs/btrfs/ref-verify.c:290:3: warning: Potential leak of memory pointed to by 're' [unix.Malloc]
                      kfree(be);
                      ^~~~~
      
      The problem is in this block of code:
      
      	if (root_objectid) {
      		struct root_entry *exist_re;
      
      		exist_re = insert_root_entry(&exist->roots, re);
      		if (exist_re)
      			kfree(re);
      	}
      
      There is no 'else' block freeing when root_objectid is 0. Add the
      missing kfree to the else branch.
      
      Fixes: fd708b81 ("Btrfs: add a extent ref verify tool")
      CC: stable@vger.kernel.org # 4.19+
      Signed-off-by: default avatarTom Rix <trix@redhat.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      d60ba8de
    • David Sterba's avatar
      btrfs: prefetch chunk tree leaves at mount · d85327b1
      David Sterba authored
      The whole chunk tree is read at mount time so we can utilize readahead
      to get the tree blocks to memory before we read the items. The idea is
      from Robbie, but instead of updating search slot readahead, this patch
      implements the chunk tree readahead manually from nodes on level 1.
      
      We've decided to do specific readahead optimizations and then unify them
      under a common API so we don't break everything by changing the search
      slot readahead logic.
      
      Higher chunk trees grow on large filesystems (many terabytes), and
      prefetching just level 1 seems to be sufficient. Provided example was
      from a 200TiB filesystem with chunk tree level 2.
      
      CC: Robbie Ko <robbieko@synology.com>
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      d85327b1
    • Johannes Thumshirn's avatar
      btrfs: add metadata_uuid to FS_INFO ioctl · 49bac897
      Johannes Thumshirn authored
      Add retrieval of the filesystem's metadata UUID to the fsinfo ioctl.
      This is driven by setting the BTRFS_FS_INFO_FLAG_METADATA_UUID flag in
      btrfs_ioctl_fs_info_args::flags.
      Reviewed-by: default avatarNikolay Borisov <nborisov@suse.com>
      Signed-off-by: default avatarJohannes Thumshirn <johannes.thumshirn@wdc.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      49bac897
    • Johannes Thumshirn's avatar
      btrfs: add filesystem generation to FS_INFO ioctl · 0fb408a5
      Johannes Thumshirn authored
      Add retrieval of the filesystem's generation to the fsinfo ioctl. This is
      driven by setting the BTRFS_FS_INFO_FLAG_GENERATION flag in
      btrfs_ioctl_fs_info_args::flags.
      Reviewed-by: default avatarNikolay Borisov <nborisov@suse.com>
      Signed-off-by: default avatarJohannes Thumshirn <johannes.thumshirn@wdc.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      0fb408a5
    • Johannes Thumshirn's avatar
      btrfs: pass checksum type via BTRFS_IOC_FS_INFO ioctl · 137c5418
      Johannes Thumshirn authored
      With the recent addition of filesystem checksum types other than CRC32c,
      it is not anymore hard-coded which checksum type a btrfs filesystem uses.
      
      Up to now there is no good way to read the filesystem checksum, apart from
      reading the filesystem UUID and then query sysfs for the checksum type.
      
      Add a new csum_type and csum_size fields to the BTRFS_IOC_FS_INFO ioctl
      command which usually is used to query filesystem features. Also add a
      flags member indicating that the kernel responded with a set csum_type and
      csum_size field.
      
      For compatibility reasons, only return the csum_type and csum_size if
      the BTRFS_FS_INFO_FLAG_CSUM_INFO flag was passed to the kernel. Also
      clear any unknown flags so we don't pass false positives to user-space
      newer than the kernel.
      
      To simplify further additions to the ioctl, also switch the padding to a
      u8 array. Pahole was used to verify the result of this switch:
      
      The csum members are added before flags, which might look odd, but this
      is to keep the alignment requirements and not to introduce holes in the
      structure.
      
        $ pahole -C btrfs_ioctl_fs_info_args fs/btrfs/btrfs.ko
        struct btrfs_ioctl_fs_info_args {
      	  __u64                      max_id;               /*     0     8 */
      	  __u64                      num_devices;          /*     8     8 */
      	  __u8                       fsid[16];             /*    16    16 */
      	  __u32                      nodesize;             /*    32     4 */
      	  __u32                      sectorsize;           /*    36     4 */
      	  __u32                      clone_alignment;      /*    40     4 */
      	  __u16                      csum_type;            /*    44     2 */
      	  __u16                      csum_size;            /*    46     2 */
      	  __u64                      flags;                /*    48     8 */
      	  __u8                       reserved[968];        /*    56   968 */
      
      	  /* size: 1024, cachelines: 16, members: 10 */
        };
      
      Fixes: 3951e7f0 ("btrfs: add xxhash64 to checksumming algorithms")
      Fixes: 3831bf00 ("btrfs: add sha256 to checksumming algorithm")
      CC: stable@vger.kernel.org # 5.5+
      Signed-off-by: default avatarJohannes Thumshirn <johannes.thumshirn@wdc.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      137c5418
    • Qu Wenruo's avatar
      btrfs: qgroup: remove ASYNC_COMMIT mechanism in favor of reserve retry-after-EDQUOT · adca4d94
      Qu Wenruo authored
      commit a514d638 ("btrfs: qgroup: Commit transaction in advance to
      reduce early EDQUOT") tries to reduce the early EDQUOT problems by
      checking the qgroup free against threshold and tries to wake up commit
      kthread to free some space.
      
      The problem of that mechanism is, it can only free qgroup per-trans
      metadata space, can't do anything to data, nor prealloc qgroup space.
      
      Now since we have the ability to flush qgroup space, and implemented
      retry-after-EDQUOT behavior, such mechanism can be completely replaced.
      
      So this patch will cleanup such mechanism in favor of
      retry-after-EDQUOT.
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarQu Wenruo <wqu@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      adca4d94
    • Qu Wenruo's avatar
      btrfs: qgroup: try to flush qgroup space when we get -EDQUOT · c53e9653
      Qu Wenruo authored
      [PROBLEM]
      There are known problem related to how btrfs handles qgroup reserved
      space.  One of the most obvious case is the the test case btrfs/153,
      which do fallocate, then write into the preallocated range.
      
        btrfs/153 1s ... - output mismatch (see xfstests-dev/results//btrfs/153.out.bad)
            --- tests/btrfs/153.out     2019-10-22 15:18:14.068965341 +0800
            +++ xfstests-dev/results//btrfs/153.out.bad      2020-07-01 20:24:40.730000089 +0800
            @@ -1,2 +1,5 @@
             QA output created by 153
            +pwrite: Disk quota exceeded
            +/mnt/scratch/testfile2: Disk quota exceeded
            +/mnt/scratch/testfile2: Disk quota exceeded
             Silence is golden
            ...
            (Run 'diff -u xfstests-dev/tests/btrfs/153.out xfstests-dev/results//btrfs/153.out.bad'  to see the entire diff)
      
      [CAUSE]
      Since commit c6887cd1 ("Btrfs: don't do nocow check unless we have to"),
      we always reserve space no matter if it's COW or not.
      
      Such behavior change is mostly for performance, and reverting it is not
      a good idea anyway.
      
      For preallcoated extent, we reserve qgroup data space for it already,
      and since we also reserve data space for qgroup at buffered write time,
      it needs twice the space for us to write into preallocated space.
      
      This leads to the -EDQUOT in buffered write routine.
      
      And we can't follow the same solution, unlike data/meta space check,
      qgroup reserved space is shared between data/metadata.
      The EDQUOT can happen at the metadata reservation, so doing NODATACOW
      check after qgroup reservation failure is not a solution.
      
      [FIX]
      To solve the problem, we don't return -EDQUOT directly, but every time
      we got a -EDQUOT, we try to flush qgroup space:
      
      - Flush all inodes of the root
        NODATACOW writes will free the qgroup reserved at run_dealloc_range().
        However we don't have the infrastructure to only flush NODATACOW
        inodes, here we flush all inodes anyway.
      
      - Wait for ordered extents
        This would convert the preallocated metadata space into per-trans
        metadata, which can be freed in later transaction commit.
      
      - Commit transaction
        This will free all per-trans metadata space.
      
      Also we don't want to trigger flush multiple times, so here we introduce
      a per-root wait list and a new root status, to ensure only one thread
      starts the flushing.
      
      Fixes: c6887cd1 ("Btrfs: don't do nocow check unless we have to")
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarQu Wenruo <wqu@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      c53e9653
    • Qu Wenruo's avatar
      btrfs: qgroup: allow to unreserve range without releasing other ranges · 263da812
      Qu Wenruo authored
      [PROBLEM]
      Before this patch, when btrfs_qgroup_reserve_data() fails, we free all
      reserved space of the changeset.
      
      For example:
      	ret = btrfs_qgroup_reserve_data(inode, changeset, 0, SZ_1M);
      	ret = btrfs_qgroup_reserve_data(inode, changeset, SZ_1M, SZ_1M);
      	ret = btrfs_qgroup_reserve_data(inode, changeset, SZ_2M, SZ_1M);
      
      If the last btrfs_qgroup_reserve_data() failed, it will release the
      entire [0, 3M) range.
      
      This behavior is kind of OK for now, as when we hit -EDQUOT, we normally
      go error handling and need to release all reserved ranges anyway.
      
      But this also means the following call is not possible:
      
      	ret = btrfs_qgroup_reserve_data();
      	if (ret == -EDQUOT) {
      		/* Do something to free some qgroup space */
      		ret = btrfs_qgroup_reserve_data();
      	}
      
      As if the first btrfs_qgroup_reserve_data() fails, it will free all
      reserved qgroup space.
      
      [CAUSE]
      This is because we release all reserved ranges when
      btrfs_qgroup_reserve_data() fails.
      
      [FIX]
      This patch will implement a new function, qgroup_unreserve_range(), to
      iterate through the ulist nodes, to find any nodes in the failure range,
      and remove the EXTENT_QGROUP_RESERVED bits from the io_tree, and
      decrease the extent_changeset::bytes_changed, so that we can revert to
      previous state.
      
      This allows later patches to retry btrfs_qgroup_reserve_data() if EDQUOT
      happens.
      Suggested-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarQu Wenruo <wqu@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      263da812
    • Josef Bacik's avatar
      btrfs: convert block group refcount to refcount_t · 48aaeebe
      Josef Bacik authored
      We have refcount_t now with the associated library to handle refcounts,
      which gives us extra debugging around reference count mistakes that may
      be made.  For example it'll warn on any transition from 0->1 or 0->-1,
      which is handy for noticing cases where we've messed up reference
      counting.  Convert the block group ref counting from an atomic_t to
      refcount_t and use the appropriate helpers.
      Reviewed-by: default avatarFilipe Manana <fdmanana@suse.com>
      Signed-off-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      48aaeebe
    • Marcos Paulo de Souza's avatar
      btrfs: add multi-statement protection to btrfs_set/clear_and_info macros · 60f8667b
      Marcos Paulo de Souza authored
      Multi-statement macros should be enclosed in do/while(0) block to make
      their use safe in single statement if conditions. All current uses of
      the macros are safe, so this change is for future protection.
      Reviewed-by: default avatarAnand Jain <anand.jain@oracle.com>
      Signed-off-by: default avatarMarcos Paulo de Souza <mpdesouza@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      60f8667b
    • Nikolay Borisov's avatar
    • Nikolay Borisov's avatar
    • Nikolay Borisov's avatar
      btrfs: raid56: use in_range where applicable · 83025863
      Nikolay Borisov authored
      While at it use the opportunity to simplify find_logical_bio_stripe by
      reducing the scope of 'stripe_start' variable and squash the
      sector-to-bytes conversion on one line.
      Reviewed-by: default avatarJohannes Thumshirn <johannes.thumshirn@wdc.com>
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      83025863
    • Nikolay Borisov's avatar
      btrfs: raid56: assign bio in while() when using bio_list_pop · bf28a605
      Nikolay Borisov authored
      Unify the style in the file such that return value of bio_list_pop is
      assigned directly in the while loop. This is in line with the rest of
      the kernel.
      Reviewed-by: default avatarJohannes Thumshirn <johannes.thumshirn@wdc.com>
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      bf28a605
    • Nikolay Borisov's avatar
      btrfs: raid56: remove redundant device check in rbio_add_io_page · f90ae76a
      Nikolay Borisov authored
      The merging logic is always executed if the current stripe's device
      is not missing. So there's no point in duplicating the check. Simply
      remove it, while at it reduce the scope of the 'last_end' variable.
      If the current stripe's device is missing we fail the stripe early on.
      Reviewed-by: default avatarJohannes Thumshirn <johannes.thumshirn@wdc.com>
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      f90ae76a
    • Nikolay Borisov's avatar
      btrfs: always initialize btrfs_bio::tgtdev_map/raid_map pointers · 608769a4
      Nikolay Borisov authored
      Since btrfs_bio always contains the extra space for the tgtdev_map and
      raid_maps it's pointless to make the assignment iff specific conditions
      are met.
      
      Instead, always assign the pointers to their correct value at allocation
      time. To accommodate this change also move code a bit in
      __btrfs_map_block so that btrfs_bio::stripes array is always initialized
      before the raid_map, subsequently move the call to sort_parity_stripes
      in the 'if' building the raid_map, retaining the old behavior.
      
      To better understand the change, there are 2 aspects to this:
      
      1. The original code is harder to grasp because the calculations for
         initializing raid_map/tgtdev ponters are apart from the initial
         allocation of memory. Having them predicated on 2 separate checks
         doesn't help that either... So by moving the initialisation in
         alloc_btrfs_bio puts everything together.
      
      2. tgtdev/raid_maps are now always initialized despite sometimes they
         might be equal i.e __btrfs_map_block_for_discard calls
         alloc_btrfs_bio with tgtdev = 0 but their usage should be predicated
         on external checks i.e. just because those pointers are non-null
         doesn't mean they are valid per-se. And actually while taking another
         look at __btrfs_map_block I saw a discrepancy:
      
         Original code initialised tgtdev_map if the following check is true:
      
      	   if (dev_replace_is_ongoing && dev_replace->tgtdev != NULL)
      
         However, further down tgtdev_map is only used if the following check
         is true:
      
      	if (dev_replace_is_ongoing && dev_replace->tgtdev != NULL && need_full_stripe(op))
      
        e.g. the additional need_full_stripe(op) predicate is there.
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      [ copy more details from mail discussion ]
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      608769a4
    • Nikolay Borisov's avatar
      btrfs: sysfs: add bdi link to the fsid directory · 3092c68f
      Nikolay Borisov authored
      Since BTRFS uses a private bdi it makes sense to create a link to this
      bdi under /sys/fs/btrfs/<UUID>/bdi. This allows size of read ahead to
      be controlled. Without this patch it's not possible to uniquely identify
      which bdi pertains to which btrfs filesystem in the case of multiple
      btrfs filesystems.
      
      It's fine to simply call sysfs_remove_link without checking if the
      link indeed has been created. The call path
      
      sysfs_remove_link
       kernfs_remove_by_name
        kernfs_remove_by_name_ns
      
      will simply return -ENOENT in case it doesn't exist.
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      3092c68f
    • Nikolay Borisov's avatar
      btrfs: increment corrupt device counter during compressed read · 5a9472fe
      Nikolay Borisov authored
      If a compressed read fails due to checksum error only a line is printed
      to dmesg, device corrupt counter is not modified.
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Reviewed-by: default avatarJohannes Thumshirn <johannes.thumshirn@wdc.com>
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      5a9472fe
    • Nikolay Borisov's avatar
      btrfs: remove needless ASSERT check of orig_bio in end_compressed_bio_read · 26056eab
      Nikolay Borisov authored
      compressed_bio::orig_bio is always set in btrfs_submit_compressed_read
      before any bio submission is performed. Since that function is always
      called with a valid bio it renders the ASSERT unnecessary.
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Reviewed-by: default avatarJohannes Thumshirn <johannes.thumshirn@wdc.com>
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      26056eab
    • Nikolay Borisov's avatar
      btrfs: increment device corruption error in case of checksum error · 814723e0
      Nikolay Borisov authored
      Now that btrfs_io_bio have access to btrfs_device we can safely
      increment the device corruption counter on error. There is one notable
      exception - repair bios for raid. Since those don't go through the
      normal submit_stripe_bio callpath but through raid56_parity_recover thus
      repair bios won't have their device set.
      
      Scrub increments the corruption counter for checksum mismatch as well
      but does not call this function.
      
      Link: https://lore.kernel.org/linux-btrfs/4857863.FCrPRfMyHP@liv/Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      814723e0
    • Nikolay Borisov's avatar
      btrfs: don't check for btrfs_device::bdev in btrfs_end_bio · 3eee86c8
      Nikolay Borisov authored
      btrfs_map_bio ensures that all submitted bios to devices have valid
      btrfs_device::bdev so this check can be removed from btrfs_end_bio. This
      check was added in june 2012 597a60fa ("Btrfs: don't count I/O
      statistic read errors for missing devices") but then in October of the
      same year another commit de1ee92a ("Btrfs: recheck bio against
      block device when we map the bio") started checking for the presence of
      btrfs_device::bdev before actually issuing the bio.
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      3eee86c8
    • Nikolay Borisov's avatar
      btrfs: record btrfs_device directly in btrfs_io_bio · c31efbdf
      Nikolay Borisov authored
      Instead of recording stripe_index and using that to access correct
      btrfs_device from btrfs_bio::stripes record the btrfs_device in
      btrfs_io_bio. This will enable endio handlers to increment device
      error counters on checksum errors.
      Reviewed-by: default avatarJohannes Thumshirn <johannes.thumshirn@wdc.com>
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      c31efbdf
    • Nikolay Borisov's avatar
      btrfs: streamline btrfs_get_io_failure_record logic · 3526302f
      Nikolay Borisov authored
      Make the function directly return a pointer to a failure record and
      adjust callers to handle it. Also refactor the logic inside so that
      the case which allocates the failure record for the first time is not
      handled in an 'if' arm, saving us a level of indentation. Finally make
      the function static as it's not used outside of extent_io.c .
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      3526302f
    • Nikolay Borisov's avatar
      btrfs: make get_state_failrec return failrec directly · 2279a270
      Nikolay Borisov authored
      Only failure that get_state_failrec can get is if there is no failure
      for the given address. There is no reason why the function should return
      a status code and use a separate parameter for returning the actual
      failure rec (if one is found). Simplify it by making the return type
      a pointer and return ERR_PTR value in case of errors.
      Signed-off-by: default avatarNikolay Borisov <nborisov@suse.com>
      Reviewed-by: default avatarDavid Sterba <dsterba@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      2279a270
    • David Sterba's avatar
      btrfs: remove deprecated mount option subvolrootid · b90a4ab6
      David Sterba authored
      The option subvolrootid used to be a workaround for mounting subvolumes
      and ineffective since 5e2a4b25 ("btrfs: deprecate subvolrootid mount
      option"). We have subvol= that works and we don't need to keep the
      cruft, let's remove it.
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      b90a4ab6
    • David Sterba's avatar
      btrfs: remove deprecated mount option alloc_start · d801e7a3
      David Sterba authored
      The mount option alloc_start has no effect since 0d0c71b3 ("btrfs:
      obsolete and remove mount option alloc_start") which has details why
      it's been deprecated. We can remove it.
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      d801e7a3
    • Filipe Manana's avatar
      btrfs: remove no longer needed use of log_writers for the log root tree · a93e0168
      Filipe Manana authored
      When syncing the log, we used to update the log root tree without holding
      neither the log_mutex of the subvolume root nor the log_mutex of log root
      tree.
      
      We used to have two critical sections delimited by the log_mutex of the
      log root tree, so in the first one we incremented the log_writers of the
      log root tree and on the second one we decremented it and waited for the
      log_writers counter to go down to zero. This was because the update of
      the log root tree happened between the two critical sections.
      
      The use of two critical sections allowed a little bit more of parallelism
      and required the use of the log_writers counter, necessary to make sure
      we didn't miss any log root tree update when we have multiple tasks trying
      to sync the log in parallel.
      
      However after commit 06989c79 ("Btrfs: fix race updating log root
      item during fsync") the log root tree update was moved into a critical
      section delimited by the subvolume's log_mutex. Later another commit
      moved the log tree update from that critical section into the second
      critical section delimited by the log_mutex of the log root tree. Both
      commits addressed different bugs.
      
      The end result is that the first critical section delimited by the
      log_mutex of the log root tree became pointless, since there's nothing
      done between it and the second critical section, we just have an unlock
      of the log_mutex followed by a lock operation. This means we can merge
      both critical sections, as the first one does almost nothing now, and we
      can stop using the log_writers counter of the log root tree, which was
      incremented in the first critical section and decremented in the second
      criticial section, used to make sure no one in the second critical section
      started writeback of the log root tree before some other task updated it.
      
      So just remove the mutex_unlock() followed by mutex_lock() of the log root
      tree, as well as the use of the log_writers counter for the log root tree.
      
      This patch is part of a series that has the following patches:
      
      1/4 btrfs: only commit the delayed inode when doing a full fsync
      2/4 btrfs: only commit delayed items at fsync if we are logging a directory
      3/4 btrfs: stop incremening log_batch for the log root tree when syncing log
      4/4 btrfs: remove no longer needed use of log_writers for the log root tree
      
      After the entire patchset applied I saw about 12% decrease on max latency
      reported by dbench. The test was done on a qemu vm, with 8 cores, 16Gb of
      ram, using kvm and using a raw NVMe device directly (no intermediary fs on
      the host). The test was invoked like the following:
      
        mkfs.btrfs -f /dev/sdk
        mount -o ssd -o nospace_cache /dev/sdk /mnt/sdk
        dbench -D /mnt/sdk -t 300 8
        umount /mnt/dsk
      
      CC: stable@vger.kernel.org # 5.4+
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      a93e0168
    • Filipe Manana's avatar
      btrfs: stop incremening log_batch for the log root tree when syncing log · 28a95795
      Filipe Manana authored
      We are incrementing the log_batch atomic counter of the root log tree but
      we never use that counter, it's used only for the log trees of subvolume
      roots. We started doing it when we moved the log_batch and log_write
      counters from the global, per fs, btrfs_fs_info structure, into the
      btrfs_root structure in commit 7237f183 ("Btrfs: fix tree logs
      parallel sync").
      
      So just stop doing it for the log root tree and add a comment over the
      field declaration so inform it's used only for log trees of subvolume
      roots.
      
      This patch is part of a series that has the following patches:
      
      1/4 btrfs: only commit the delayed inode when doing a full fsync
      2/4 btrfs: only commit delayed items at fsync if we are logging a directory
      3/4 btrfs: stop incremening log_batch for the log root tree when syncing log
      4/4 btrfs: remove no longer needed use of log_writers for the log root tree
      
      After the entire patchset applied I saw about 12% decrease on max latency
      reported by dbench. The test was done on a qemu vm, with 8 cores, 16Gb of
      ram, using kvm and using a raw NVMe device directly (no intermediary fs on
      the host). The test was invoked like the following:
      
        mkfs.btrfs -f /dev/sdk
        mount -o ssd -o nospace_cache /dev/sdk /mnt/sdk
        dbench -D /mnt/sdk -t 300 8
        umount /mnt/dsk
      
      CC: stable@vger.kernel.org # 5.4+
      Reviewed-by: default avatarJosef Bacik <josef@toxicpanda.com>
      Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
      Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
      28a95795