On Thu, Aug 27, 2015 at 11:39:00PM +0530, Chandan Rajendra wrote:
> 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 | |
> | | Start direct i/o write on inode X |
> | | reserve space |
> | | dio_refill_pages() |
> | | - sdio->blocks_available == 0 |
> | | - Return -EFAULT |
> | | Since BTRFS_INODE_DIO_READY is set, |
> | | we don't release reserved space. |
> | | Clear BTRFS_INODE_DIO_READY bit. |
> | -EIOCBQUEUED is returned. | |
> |-----------------------------------+-------------------------------------|
This doesn't explain why dio_refill_pages() returns -EFAULT when
normally it doesn't, and I think it's important to save reviewers' time to
understand the failure.
Also it'd be better to add a changelog for this v2 patch.
Thanks,
-liubo
>
> 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: Chandan Rajendra <chan...@linux.vnet.ibm.com>
> ---
> fs/btrfs/btrfs_inode.h | 2 --
> fs/btrfs/inode.c | 42 +++++++++++++++++++++---------------------
> 2 files changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index 81220b2..0ef5cc1 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -44,8 +44,6 @@
> #define BTRFS_INODE_IN_DELALLOC_LIST 9
> #define BTRFS_INODE_READDIO_NEED_LOCK 10
> #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.
> * When any of them is set, it means an error happened while writing an
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index bda3c41..83571603 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7405,6 +7405,10 @@ static struct extent_map *create_pinned_em(struct
> inode *inode, u64 start,
> return em;
> }
>
> +struct btrfs_dio_data {
> + u64 outstanding_extents;
> + u64 reserve;
> +};
>
> static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
> struct buffer_head *bh_result, int create)
> @@ -7412,10 +7416,10 @@ static int btrfs_get_blocks_direct(struct inode
> *inode, sector_t iblock,
> struct extent_map *em;
> struct btrfs_root *root = BTRFS_I(inode)->root;
> struct extent_state *cached_state = NULL;
> + struct btrfs_dio_data *dio_data = NULL;
> u64 start = iblock << inode->i_blkbits;
> u64 lockstart, lockend;
> u64 len = bh_result->b_size;
> - u64 *outstanding_extents = NULL;
> int unlock_bits = EXTENT_LOCKED;
> int ret = 0;
>
> @@ -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
> * confused.
> */
> - outstanding_extents = current->journal_info;
> + dio_data = current->journal_info;
> current->journal_info = NULL;
> }
>
> @@ -7565,17 +7569,18 @@ unlock:
> * within our reservation, otherwise we need to adjust our inode
> * counter appropriately.
> */
> - if (*outstanding_extents) {
> - (*outstanding_extents)--;
> + if (dio_data->outstanding_extents) {
> + (dio_data->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);
> - 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 @@ 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;
> + if (dio_data)
> + current->journal_info = dio_data;
> return ret;
> }
>
> @@ -8329,7 +8334,8 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb,
> struct iov_iter *iter,
> {
> struct file *file = iocb->ki_filp;
> 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;
> int flags = 0;
> bool wakeup = true;
> @@ -8367,7 +8373,7 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb,
> struct iov_iter *iter,
> ret = btrfs_delalloc_reserve_space(inode, count);
> if (ret)
> goto out;
> - outstanding_extents = div64_u64(count +
> + dio_data.outstanding_extents = div64_u64(count +
> BTRFS_MAX_EXTENT_SIZE - 1,
> BTRFS_MAX_EXTENT_SIZE);
>
> @@ -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
> * 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,
> &BTRFS_I(inode)->runtime_flags)) {
> inode_dio_end(inode);
> @@ -8391,16 +8398,9 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb,
> struct iov_iter *iter,
> if (iov_iter_rw(iter) == WRITE) {
> current->journal_info = NULL;
> if (ret < 0 && ret != -EIOCBQUEUED) {
> - /*
> - * If the error comes from submitting stage,
> - * btrfs_get_blocsk_direct() has free'd data space,
> - * 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);
> + if (dio_data.reserve)
> + btrfs_delalloc_release_space(inode,
> + dio_data.reserve);
> } else if (ret >= 0 && (size_t)ret < count)
> btrfs_delalloc_release_space(inode,
> count - (size_t)ret);
> --
> 2.1.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
