Accounting fixes

My fix

Btrfs: fix merge delalloc logic

only fixed half of the problems, it didn't fix the case where we have two large extents on either side and then join them together with a new small extent. We need to instead keep track of how many extents we have accounted for with each side of the new extent, and then see how many extents we need for the new large extent. If they match then we know we need to keep our reservation, otherwise we need to drop our reservation. This shows up with a case like this

[BTRFS_MAX_EXTENT_SIZE+4K][4K HOLE][BTRFS_MAX_EXTENT_SIZE+4K]

Previously the logic would have said that the number extents required for the new size (3) is larger than the number of extents required for the largest side
(2) therefore we need to keep our reservation. But this isn't the case, since both sides require a reservation of 2 which leads to 4 for the whole range currently reserved, but we only need 3, so we need to drop one of the reservations. The same problem existed for splits, we'd think we only need 3 extents when creating the hole but in reality we need 4. Thanks,

Signed-off-by Josef Bacik <[email protected]>

---
 fs/btrfs/inode.c | 57 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

fs/btrfs/inode.c +30 -28

index 97b601b..bb74a41 100644
@@ -1542,30 +1542,17 @@ static void btrfs_split_extent_hook(struct inode *inode,
                u64 new_size;
 
                /*
-                * We need the largest size of the remaining extent to see if we
-                * need to add a new outstanding extent.  Think of the following
-                * case
-                *
-                * [MEAX_EXTENT_SIZEx2 - 4k][4k]
-                *
-                * The new_size would just be 4k and we'd think we had enough
-                * outstanding extents for this if we only took one side of the
-                * split, same goes for the other direction.  We need to see if
-                * the larger size still is the same amount of extents as the
-                * original size, because if it is we need to add a new
-                * outstanding extent.  But if we split up and the larger size
-                * is less than the original then we are good to go since we've
-                * already accounted for the extra extent in our original
-                * accounting.
+                * See the explanation in btrfs_merge_extent_hook, the same
+                * applies here, just in reverse.
                 */
                new_size = orig->end - split + 1;
-               if ((split - orig->start) > new_size)
-                       new_size = split - orig->start;
-
-               num_extents = div64_u64(size + BTRFS_MAX_EXTENT_SIZE - 1,
+               num_extents = div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
                                        BTRFS_MAX_EXTENT_SIZE);
-               if (div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
-                             BTRFS_MAX_EXTENT_SIZE) < num_extents)
+               new_size = split - orig->start;
+               num_extents += div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
+                                       BTRFS_MAX_EXTENT_SIZE);
+               if (div64_u64(size + BTRFS_MAX_EXTENT_SIZE - 1,
+                             BTRFS_MAX_EXTENT_SIZE) >= num_extents)
                        return;
        }
 

@@ -1591,9 +1578,6 @@ static void btrfs_merge_extent_hook(struct inode *inode,
        if (!(other->state & EXTENT_DELALLOC))
                return;
 
-       old_size = other->end - other->start + 1;
-       if (old_size < (new->end - new->start + 1))
-               old_size = (new->end - new->start + 1);
        if (new->start > other->start)
                new_size = new->end - other->start + 1;
        else

@@ -1608,13 +1592,32 @@ static void btrfs_merge_extent_hook(struct inode *inode,
        }
 
        /*
-        * If we grew by another max_extent, just return, we want to keep that
-        * reserved amount.
+        * We have to add up either side to figure out how many extents were
+        * accounted for before we merged into one big extent.  If the number of
+        * extents we accounted for is <= the amount we need for the new range
+        * then we can return, otherwise drop.  Think of it like this
+        *
+        * [ 4k][MAX_SIZE]
+        *
+        * So we've grown the extent by a MAX_SIZE extent, this would mean we
+        * need 2 outstanding extents, on one side we have 1 and the other side
+        * we have 1 so they are == and we can return.  But in this case
+        *
+        * [MAX_SIZE+4k][MAX_SIZE+4k]
+        *
+        * Each range on their own accounts for 2 extents, but merged together
+        * they are only 3 extents worth of accounting, so we need to drop in
+        * this case.
         */
+       old_size = other->end - other->start + 1;
        num_extents = div64_u64(old_size + BTRFS_MAX_EXTENT_SIZE - 1,
                                BTRFS_MAX_EXTENT_SIZE);
+       old_size = new->end - new->start + 1;
+       num_extents += div64_u64(old_size + BTRFS_MAX_EXTENT_SIZE - 1,
+                                BTRFS_MAX_EXTENT_SIZE);
+
        if (div64_u64(new_size + BTRFS_MAX_EXTENT_SIZE - 1,
-                     BTRFS_MAX_EXTENT_SIZE) > num_extents)
+                     BTRFS_MAX_EXTENT_SIZE) >= num_extents)
                return;
 
        spin_lock(&BTRFS_I(inode)->lock);
--

1.8.3.1

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

If we fail during our sanity tests we could get NULL deref's because we unload the module before the dummy extent buffers are free'd via RCU. So check for this case and just free the things directly. Thanks,

Signed-off-by Josef Bacik <[email protected]>

---
 fs/btrfs/extent_io.c | 6 ++++++
 1 file changed, 6 insertions(+)

fs/btrfs/extent_io.c +0 -0

index 29850d4..d13cead 100644

fs/btrfs/extent_io.c +5 -0

+		if (unlikely(test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags))) {
+			__free_extent_buffer(eb);
+			return 1;
+		}
+#endif
 		call_rcu(&eb->rcu_head, btrfs_release_extent_buffer_rcu);
 		return 1;
 	}

Quoted 15 lines · Josef Bacik

If we fail during our sanity tests we could get NULL deref's because we unload the module before the dummy extent buffers are free'd via RCU. So check for this case and just free the things directly. Thanks,

Signed-off-by Josef Bacik <[email protected]>

---
  fs/btrfs/extent_io.c | 6 ++++++
  1 file changed, 6 insertions(+)

fs/btrfs/extent_io.c +0 -0

index 29850d4..d13cead 100644
@@ -4968,6 +4968,12 @@ static int release_extent_buffer(struct extent_buffer *eb)
 

fs/btrfs/extent_io.c static int release_extent_buffer(struct extent_buffer *eb) · line 4971

Quoted 3 lines

fs/btrfs/extent_io.c +1 -0

  		/* Should be safe to release our pages at this point */
  		btrfs_release_extent_buffer_page(eb);
+#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS

Please use btrfs_test_is_dummy_root, it's there to avoid the #ifdefs

We are keeping track of how many extents we need to reserve properly based on the amount we want to write, but we were still incrementing outstanding_extents if we wrote less than what we requested. This isn't quite right since we will be limited to our max extent size. So instead lets do something horrible! Keep track of how many outstanding_extents we reserved, and decrement each time we allocate an extent. If we use our entire reserve make sure to jack up outstanding_extents on the inode so the accounting works out properly. Thanks,

Reported-by Filipe Manana <[email protected]>

Signed-off-by Josef Bacik <[email protected]>

---
 fs/btrfs/inode.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

fs/btrfs/inode.c +35 -3

index 3717b3d..aa1fb53 100644
@@ -7239,7 +7239,7 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
        u64 start = iblock << inode->i_blkbits;
        u64 lockstart, lockend;
        u64 len = bh_result->b_size;
-       u64 orig_len = len;
+       u64 *outstanding_extents = NULL;
        int unlock_bits = EXTENT_LOCKED;
        int ret = 0;
 

@@ -7251,6 +7251,16 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
        lockstart = start;
        lockend = start + len - 1;
 
+       if (current->journal_info) {
+               /*
+                * Need to pull our outstanding extents and set journal_info to NULL so
+                * that anything that needs to check if there's a transction doesn't get
+                * confused.
+                */
+               outstanding_extents = current->journal_info;
+               current->journal_info = NULL;
+       }
+
        /*
         * If this errors out it's because we couldn't invalidate pagecache for
         * this range and we need to fallback to buffered.

@@ -7374,11 +7384,20 @@ unlock:
                if (start + len > i_size_read(inode))
                        i_size_write(inode, start + len);
 
-               if (len < orig_len) {
+               /*
+                * If we have an outstanding_extents count still set then we're
+                * within our reservation, otherwise we need to adjust our inode
+                * counter appropriately.
+                */
+               if (*outstanding_extents) {
+                       (*outstanding_extents)--;
+               } else {
                        spin_lock(&BTRFS_I(inode)->lock);
                        BTRFS_I(inode)->outstanding_extents++;
                        spin_unlock(&BTRFS_I(inode)->lock);
                }
+
+               current->journal_info = outstanding_extents;
                btrfs_free_reserved_data_space(inode, len);
        }
 

@@ -7402,6 +7421,8 @@ unlock:
 unlock_err:
        clear_extent_bit(&BTRFS_I(inode)->io_tree, lockstart, lockend,
                         unlock_bits, 1, 0, &cached_state, GFP_NOFS);
+       if (outstanding_extents)
+               current->journal_info = outstanding_extents;
        return ret;
 }
 

@@ -8101,6 +8122,7 @@ static ssize_t btrfs_direct_IO(int rw, struct kiocb *iocb,
 {
        struct file *file = iocb->ki_filp;
        struct inode *inode = file->f_mapping->host;
+       u64 outstanding_extents = 0;
        size_t count = 0;
        int flags = 0;
        bool wakeup = true;

@@ -8138,6 +8160,16 @@ static ssize_t btrfs_direct_IO(int rw, struct kiocb *iocb,
                ret = btrfs_delalloc_reserve_space(inode, count);
                if (ret)
                        goto out;
+               outstanding_extents = div64_u64(count +
+                                               BTRFS_MAX_EXTENT_SIZE - 1,
+                                               BTRFS_MAX_EXTENT_SIZE);
+
+               /*
+                * We need to know how many extents we reserved so that we can
+                * do the accounting properly if we go over the number we
+                * originally calculated.  Abuse current->journal_info for this.
+                */
+               current->journal_info = &outstanding_extents;
        } else if (test_bit(BTRFS_INODE_READDIO_NEED_LOCK,
                                     &BTRFS_I(inode)->runtime_flags)) {
                inode_dio_done(inode);

@@ -8150,6 +8182,7 @@ static ssize_t btrfs_direct_IO(int rw, struct kiocb *iocb,
                        iter, offset, btrfs_get_blocks_direct, NULL,
                        btrfs_submit_direct, flags);
        if (rw & WRITE) {
+               current->journal_info = NULL;
                if (ret < 0 && ret != -EIOCBQUEUED)
                        btrfs_delalloc_release_space(inode, count);
                else if (ret >= 0 && (size_t)ret < count)
--

1.8.3.1

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

I've changed all of these a whole bunch recently, I think I've gotten all the issues worked out, please review/test to make sure there's not some other fucking corner I've missed. Thanks,

I've changed all of these a whole bunch recently, I think I've gotten all the issues worked out, please review/test to make sure there's not some other fucking corner I've missed. Thanks,