Btrfs: fix outstanding_extents accounting in DIO

PATCH In review Josef Bacik 2 participants 2 messages Mar 17, 2015 Btrfs file system

Share & tools

Josef Bacik

author

2015-03-17 15:04 UTC #0 lore raw

PATCH 1/1 Btrfs: fix outstanding_extents accounting in DIO

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. We need to fix this logic to only do this if we request less than MAX_EXTENT_SIZE or if we write less than MAX_EXTENT_SIZE when we request an amount larger than MAX_EXTENT_SIZE. This fixes the problem Filipe reported with generic/300. Thanks,

Reported-by Filipe Manana <[email protected]>

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

---
 fs/btrfs/inode.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

fs/btrfs/inode.c +18 -1

index 156d0f5..f6d2c56 100644
@@ -7387,7 +7387,24 @@ unlock:
 		if (start + len > i_size_read(inode))
 			i_size_write(inode, start + len);
 
-		if (len < orig_len) {
+		/*
+		 * direct_io can send down chunks > BTRFS_MAX_EXTENT_SIZE, so we
+		 * don't want to jack up outstanding_extents if we're just
+		 * allocating the largest extent we can for a range that we've
+		 * already reserved the approriate number of outstanding_extents
+		 * for.
+		 *
+		 * So if orig_len <= BTRFS_MAX_EXTENT_SIZE and our allocated len
+		 * is less than orig_len then we know we're going to end up with
+		 * more extents than we reserved.
+		 *
+		 * If orig_len > BTRFS_MAX_EXTENT_SIZE but we weren't able to
+		 * allocate a BTRFS_MAX_EXTENT_SIZE extent then we know we have
+		 * to add another outstanding extent.
+		 */
+		if (len < orig_len &&
+		    (orig_len <= BTRFS_MAX_EXTENT_SIZE ||
+		     len < BTRFS_MAX_EXTENT_SIZE)) {
 			spin_lock(&BTRFS_I(inode)->lock);
 			BTRFS_I(inode)->outstanding_extents++;
 			spin_unlock(&BTRFS_I(inode)->lock);

Signature · 1 line

1.8.3.1

Filipe David Manana

2015-03-17 15:37 UTC #1 lore raw

Quoted 9 lines · Josef Bacik

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. We need to fix this logic to only do this if we request less than MAX_EXTENT_SIZE or if we write less than MAX_EXTENT_SIZE when we request an amount larger than MAX_EXTENT_SIZE. This fixes the problem Filipe reported with generic/300. Thanks,
Reported-by Filipe Manana <[email protected]>
Signed-off-by Josef Bacik <[email protected]>

I hate to be the bringer of bad news, but this still causes the warnings for the modified fstests/generic/300 everytime I run it.

I have all your recent patches dealing with fixing the accounting of outstanding_extents applied:

$ git log --author='Josef Bacik' --pretty=oneline
4bfbd94312a4dc464f1827ca03faccb8da8f2aaf Btrfs: account merges properly
e0037ac670a4e462166f3ec28e691f8595fca607 Btrfs: fix merge delalloc logic
f46e5f2aebc726ed0dad860c3769a9bf63b0ea81 Btrfs: fix
outstanding_extents accounting in DIO
53f6e7d09b796371074ada19e452b7566682fe2f Btrfs: account for the
correct number of extents for delalloc reservations
b8c3ce1cfa5a5f7f6fe71b8f6ac4f601e7ccdf1f Btrfs: remove extra
run_delayed_refs in update_cowonly_root
(...)

Also, with the new fix for the merges of buffered extents ("Btrfs:
account merges properly") I get the assertion triggered again:

[13382.732589] BTRFS: assertion failed:
BTRFS_I(inode)->outstanding_extents >= num_extents, file:
fs/btrfs/extent-tree.c, line: 4983
[13382.735759] ------------[ cut here ]------------
[13382.736031] kernel BUG at fs/btrfs/ctree.h:4013!
[13382.736031] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
(...)
[13382.736031]  [<ffffffffa03be666>] drop_outstanding_extent+0x3d/0x6d [btrfs]

For this fio job (more stressful then the previous simple fio job):

[global]
ioengine=sync
direct=0
bssplit=130M/100
fallocate=none
filename=foobar

[job1]
numjobs=1
size=4G
rw=randwrite
time_based
runtime=60

Quoted 41 lines

---
 fs/btrfs/inode.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

fs/btrfs/inode.c +18 -2

index 156d0f5..f6d2c56 100644
@@ -7387,7 +7387,24 @@ unlock:
                if (start + len > i_size_read(inode))
                        i_size_write(inode, start + len);
 
-               if (len < orig_len) {
+               /*
+                * direct_io can send down chunks > BTRFS_MAX_EXTENT_SIZE, so we
+                * don't want to jack up outstanding_extents if we're just
+                * allocating the largest extent we can for a range that we've
+                * already reserved the approriate number of outstanding_extents
+                * for.
+                *
+                * So if orig_len <= BTRFS_MAX_EXTENT_SIZE and our allocated len
+                * is less than orig_len then we know we're going to end up with
+                * more extents than we reserved.
+                *
+                * If orig_len > BTRFS_MAX_EXTENT_SIZE but we weren't able to
+                * allocate a BTRFS_MAX_EXTENT_SIZE extent then we know we have
+                * to add another outstanding extent.
+                */
+               if (len < orig_len &&
+                   (orig_len <= BTRFS_MAX_EXTENT_SIZE ||
+                    len < BTRFS_MAX_EXTENT_SIZE)) {
                        spin_lock(&BTRFS_I(inode)->lock);
                        BTRFS_I(inode)->outstanding_extents++;
                        spin_unlock(&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

Signature · 5 lines

Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."