Commit 50745b0a authored by chandan's avatar chandan Committed by Chris Mason

Btrfs: Direct I/O: Fix space accounting

The following call trace is seen when generic/095 test is executed,

WARNING: CPU: 3 PID: 2769 at /home/chandan/code/repos/linux/fs/btrfs/inode.c:8967 btrfs_destroy_inode+0x284/0x2a0()
Modules linked in:
CPU: 3 PID: 2769 Comm: umount Not tainted 4.2.0-rc5+ #31
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20150306_163512-brownie 04/01/2014
 ffffffff81c08150 ffff8802ec9cbce8 ffffffff81984058 ffff8802ffd8feb0
 0000000000000000 ffff8802ec9cbd28 ffffffff81050385 ffff8802ec9cbd38
 ffff8802d12f8588 ffff8802d12f8588 ffff8802f15ab000 ffff8800bb96c0b0
Call Trace:
 [<ffffffff81984058>] dump_stack+0x45/0x57
 [<ffffffff81050385>] warn_slowpath_common+0x85/0xc0
 [<ffffffff81050465>] warn_slowpath_null+0x15/0x20
 [<ffffffff81340294>] btrfs_destroy_inode+0x284/0x2a0
 [<ffffffff8117ce07>] destroy_inode+0x37/0x60
 [<ffffffff8117cf39>] evict+0x109/0x170
 [<ffffffff8117cfd5>] dispose_list+0x35/0x50
 [<ffffffff8117dd3a>] evict_inodes+0xaa/0x100
 [<ffffffff81165667>] generic_shutdown_super+0x47/0xf0
 [<ffffffff81165951>] kill_anon_super+0x11/0x20
 [<ffffffff81302093>] btrfs_kill_super+0x13/0x110
 [<ffffffff81165c99>] deactivate_locked_super+0x39/0x70
 [<ffffffff811660cf>] deactivate_super+0x5f/0x70
 [<ffffffff81180e1e>] cleanup_mnt+0x3e/0x90
 [<ffffffff81180ebd>] __cleanup_mnt+0xd/0x10
 [<ffffffff81069c06>] task_work_run+0x96/0xb0
 [<ffffffff81003a3d>] do_notify_resume+0x3d/0x50
 [<ffffffff8198cbc2>] int_signal+0x12/0x17

This means that the inode had non-zero "outstanding extents" during
eviction. This occurs because, during direct I/O a task which successfully
used up its reserved data space would set BTRFS_INODE_DIO_READY bit and does
not clear the bit after finishing the DIO write. A future DIO write could
actually fail and the unused reserve space won't be freed because of the
previously set BTRFS_INODE_DIO_READY bit.

Clearing the BTRFS_INODE_DIO_READY bit in btrfs_direct_IO() caused the
following issue,
|-----------------------------------+-------------------------------------|
| Task A                            | Task B                              |
|-----------------------------------+-------------------------------------|
| Start direct i/o write on inode X.|                                     |
| reserve space                     |                                     |
| Allocate ordered extent           |                                     |
| release reserved space            |                                     |
| Set BTRFS_INODE_DIO_READY bit.    |                                     |
|                                   | splice()                            |
|                                   | Transfer data from pipe buffer to   |
|                                   | destination file.                   |
|                                   | - kmap(pipe buffer page)            |
|                                   | - Start direct i/o write on         |
|                                   |   inode X.                          |
|                                   |   - reserve space                   |
|                                   |   - dio_refill_pages()              |
|                                   |     - sdio->blocks_available == 0   |
|                                   |     - Since a kernel address is     |
|                                   |       being passed instead of a     |
|                                   |       user space address,           |
|                                   |       iov_iter_get_pages() returns  |
|                                   |       -EFAULT.                      |
|                                   |   - Since BTRFS_INODE_DIO_READY is  |
|                                   |     set, we don't release reserved  |
|                                   |     space.                          |
|                                   |   - Clear BTRFS_INODE_DIO_READY bit.|
| -EIOCBQUEUED is returned.         |                                     |
|-----------------------------------+-------------------------------------|

Hence this commit introduces "struct btrfs_dio_data" to track the usage of
reserved data space. The remaining unused "reserve space" can now be freed
reliably.
Signed-off-by: default avatarChandan Rajendra <chandan@linux.vnet.ibm.com>
Reviewed-by: default avatarLiu Bo <bo.li.liu@oracle.com>
Signed-off-by: default avatarChris Mason <clm@fb.com>
parent a30e577c
...@@ -44,8 +44,6 @@ ...@@ -44,8 +44,6 @@
#define BTRFS_INODE_IN_DELALLOC_LIST 9 #define BTRFS_INODE_IN_DELALLOC_LIST 9
#define BTRFS_INODE_READDIO_NEED_LOCK 10 #define BTRFS_INODE_READDIO_NEED_LOCK 10
#define BTRFS_INODE_HAS_PROPS 11 #define BTRFS_INODE_HAS_PROPS 11
/* DIO is ready to submit */
#define BTRFS_INODE_DIO_READY 12
/* /*
* The following 3 bits are meant only for the btree inode. * The following 3 bits are meant only for the btree inode.
* When any of them is set, it means an error happened while writing an * When any of them is set, it means an error happened while writing an
......
...@@ -7405,6 +7405,10 @@ static struct extent_map *create_pinned_em(struct inode *inode, u64 start, ...@@ -7405,6 +7405,10 @@ static struct extent_map *create_pinned_em(struct inode *inode, u64 start,
return em; return em;
} }
struct btrfs_dio_data {
u64 outstanding_extents;
u64 reserve;
};
static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock, static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
struct buffer_head *bh_result, int create) struct buffer_head *bh_result, int create)
...@@ -7412,10 +7416,10 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock, ...@@ -7412,10 +7416,10 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
struct extent_map *em; struct extent_map *em;
struct btrfs_root *root = BTRFS_I(inode)->root; struct btrfs_root *root = BTRFS_I(inode)->root;
struct extent_state *cached_state = NULL; struct extent_state *cached_state = NULL;
struct btrfs_dio_data *dio_data = NULL;
u64 start = iblock << inode->i_blkbits; u64 start = iblock << inode->i_blkbits;
u64 lockstart, lockend; u64 lockstart, lockend;
u64 len = bh_result->b_size; u64 len = bh_result->b_size;
u64 *outstanding_extents = NULL;
int unlock_bits = EXTENT_LOCKED; int unlock_bits = EXTENT_LOCKED;
int ret = 0; int ret = 0;
...@@ -7433,7 +7437,7 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock, ...@@ -7433,7 +7437,7 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
* that anything that needs to check if there's a transction doesn't get * that anything that needs to check if there's a transction doesn't get
* confused. * confused.
*/ */
outstanding_extents = current->journal_info; dio_data = current->journal_info;
current->journal_info = NULL; current->journal_info = NULL;
} }
...@@ -7565,17 +7569,18 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock, ...@@ -7565,17 +7569,18 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
* within our reservation, otherwise we need to adjust our inode * within our reservation, otherwise we need to adjust our inode
* counter appropriately. * counter appropriately.
*/ */
if (*outstanding_extents) { if (dio_data->outstanding_extents) {
(*outstanding_extents)--; (dio_data->outstanding_extents)--;
} else { } else {
spin_lock(&BTRFS_I(inode)->lock); spin_lock(&BTRFS_I(inode)->lock);
BTRFS_I(inode)->outstanding_extents++; BTRFS_I(inode)->outstanding_extents++;
spin_unlock(&BTRFS_I(inode)->lock); spin_unlock(&BTRFS_I(inode)->lock);
} }
current->journal_info = outstanding_extents;
btrfs_free_reserved_data_space(inode, len); btrfs_free_reserved_data_space(inode, len);
set_bit(BTRFS_INODE_DIO_READY, &BTRFS_I(inode)->runtime_flags); WARN_ON(dio_data->reserve < len);
dio_data->reserve -= len;
current->journal_info = dio_data;
} }
/* /*
...@@ -7598,8 +7603,8 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock, ...@@ -7598,8 +7603,8 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
unlock_err: unlock_err:
clear_extent_bit(&BTRFS_I(inode)->io_tree, lockstart, lockend, clear_extent_bit(&BTRFS_I(inode)->io_tree, lockstart, lockend,
unlock_bits, 1, 0, &cached_state, GFP_NOFS); unlock_bits, 1, 0, &cached_state, GFP_NOFS);
if (outstanding_extents) if (dio_data)
current->journal_info = outstanding_extents; current->journal_info = dio_data;
return ret; return ret;
} }
...@@ -8329,7 +8334,8 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter, ...@@ -8329,7 +8334,8 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
{ {
struct file *file = iocb->ki_filp; struct file *file = iocb->ki_filp;
struct inode *inode = file->f_mapping->host; struct inode *inode = file->f_mapping->host;
u64 outstanding_extents = 0; struct btrfs_root *root = BTRFS_I(inode)->root;
struct btrfs_dio_data dio_data = { 0 };
size_t count = 0; size_t count = 0;
int flags = 0; int flags = 0;
bool wakeup = true; bool wakeup = true;
...@@ -8367,7 +8373,7 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter, ...@@ -8367,7 +8373,7 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
ret = btrfs_delalloc_reserve_space(inode, count); ret = btrfs_delalloc_reserve_space(inode, count);
if (ret) if (ret)
goto out; goto out;
outstanding_extents = div64_u64(count + dio_data.outstanding_extents = div64_u64(count +
BTRFS_MAX_EXTENT_SIZE - 1, BTRFS_MAX_EXTENT_SIZE - 1,
BTRFS_MAX_EXTENT_SIZE); BTRFS_MAX_EXTENT_SIZE);
...@@ -8376,7 +8382,8 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter, ...@@ -8376,7 +8382,8 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
* do the accounting properly if we go over the number we * do the accounting properly if we go over the number we
* originally calculated. Abuse current->journal_info for this. * originally calculated. Abuse current->journal_info for this.
*/ */
current->journal_info = &outstanding_extents; dio_data.reserve = round_up(count, root->sectorsize);
current->journal_info = &dio_data;
} else if (test_bit(BTRFS_INODE_READDIO_NEED_LOCK, } else if (test_bit(BTRFS_INODE_READDIO_NEED_LOCK,
&BTRFS_I(inode)->runtime_flags)) { &BTRFS_I(inode)->runtime_flags)) {
inode_dio_end(inode); inode_dio_end(inode);
...@@ -8391,16 +8398,9 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter, ...@@ -8391,16 +8398,9 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
if (iov_iter_rw(iter) == WRITE) { if (iov_iter_rw(iter) == WRITE) {
current->journal_info = NULL; current->journal_info = NULL;
if (ret < 0 && ret != -EIOCBQUEUED) { if (ret < 0 && ret != -EIOCBQUEUED) {
/* if (dio_data.reserve)
* If the error comes from submitting stage, btrfs_delalloc_release_space(inode,
* btrfs_get_blocsk_direct() has free'd data space, dio_data.reserve);
* and metadata space will be handled by
* finish_ordered_fn, don't do that again to make
* sure bytes_may_use is correct.
*/
if (!test_and_clear_bit(BTRFS_INODE_DIO_READY,
&BTRFS_I(inode)->runtime_flags))
btrfs_delalloc_release_space(inode, count);
} else if (ret >= 0 && (size_t)ret < count) } else if (ret >= 0 && (size_t)ret < count)
btrfs_delalloc_release_space(inode, btrfs_delalloc_release_space(inode,
count - (size_t)ret); count - (size_t)ret);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment