Message ID | 20170712085002.23241-2-lufq.fnst@cn.fujitsu.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, Jul 12, 2017 at 04:49:48PM +0800, Lu Fengqi wrote: > From: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com> > > This issue was revealed by modifying BTRFS_MAX_EXTENT_SIZE(128MB) to 64KB, > When modifying BTRFS_MAX_EXTENT_SIZE(128MB) to 64KB, fsstress test often > gets these warnings from btrfs_destroy_inode(): > WARN_ON(BTRFS_I(inode)->outstanding_extents); > WARN_ON(BTRFS_I(inode)->reserved_extents); > > Simple test program below can reproduce this issue steadily. > Note: you need to modify BTRFS_MAX_EXTENT_SIZE to 64KB to have test, > otherwise there won't be such WARNING. > #include <string.h> > #include <unistd.h> > #include <sys/types.h> > #include <sys/stat.h> > #include <fcntl.h> > > int main(void) > { > int fd; > char buf[68 *1024]; > > memset(buf, 0, 68 * 1024); > fd = open("testfile", O_CREAT | O_EXCL | O_RDWR); > pwrite(fd, buf, 68 * 1024, 64 * 1024); > return; > } > > When BTRFS_MAX_EXTENT_SIZE is 64KB, and buffered data range is: > 64KB 128K 132KB > |-----------------------------------------------|---------------| > 64 + 4KB > > 1) for above data range, btrfs_delalloc_reserve_metadata() will reserve > metadata and set BTRFS_I(inode)->outstanding_extents to 2. > (68KB + 64KB - 1) / 64KB == 2 > > Outstanding_extents: 2 > > 2) then btrfs_dirty_page() will be called to dirty pages and set > EXTENT_DELALLOC flag. In this case, btrfs_set_bit_hook() will be called > twice. > The 1st set_bit_hook() call will set DEALLOC flag for the first 64K. > 64KB 128KB > |-----------------------------------------------| > 64KB DELALLOC > Outstanding_extents: 2 > > Set_bit_hooks() uses FIRST_DELALLOC flag to avoid re-increase > outstanding_extents counter. > So for 1st set_bit_hooks() call, it won't modify outstanding_extents, > it's still 2. > > Then FIRST_DELALLOC flag is *CLEARED*. > > 3) 2nd btrfs_set_bit_hook() call. > Because FIRST_DELALLOC have been cleared by previous set_bit_hook(), > btrfs_set_bit_hook() will increase BTRFS_I(inode)->outstanding_extents by > one, so now BTRFS_I(inode)->outstanding_extents is 3. > 64KB 128KB 132KB > |-----------------------------------------------|----------------| > 64K DELALLOC 4K DELALLOC > Outstanding_extents: 3 > > But the correct outstanding_extents number should be 2, not 3. > The 2nd btrfs_set_bit_hook() call just screwed up this, and leads to the > WARN_ON(). > > Normally, we can solve it by only increasing outstanding_extents in > set_bit_hook(). > But the problem is for delalloc_reserve/release_metadata(), we only have > a 'length' parameter, and calculate in-accurate outstanding_extents. > If we only rely on set_bit_hook() release_metadata() will crew things up > as it will decrease inaccurate number. > > So the fix we use is: > 1) Increase *INACCURATE* outstanding_extents at delalloc_reserve_meta > Just as a place holder. > 2) Increase *accurate* outstanding_extents at set_bit_hooks() > This is the real increaser. > 3) Decrease *INACCURATE* outstanding_extents before returning > This makes outstanding_extents to correct value. > > For 128M BTRFS_MAX_EXTENT_SIZE, due to limitation of > __btrfs_buffered_write(), each iteration will only handle about 2MB > data. > So btrfs_dirty_pages() won't need to handle cases cross 2 extents. > NACK on this, it just makes a crappy problem harder to understand. We need to stop using outstanding_extents as both a reservation thing _and_ an accurate count. I'll rework this and fix the other issues related to over-reservation with compression. Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017年07月25日 04:00, Josef Bacik wrote: > On Wed, Jul 12, 2017 at 04:49:48PM +0800, Lu Fengqi wrote: >> From: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com> >> >> This issue was revealed by modifying BTRFS_MAX_EXTENT_SIZE(128MB) to 64KB, >> When modifying BTRFS_MAX_EXTENT_SIZE(128MB) to 64KB, fsstress test often >> gets these warnings from btrfs_destroy_inode(): >> WARN_ON(BTRFS_I(inode)->outstanding_extents); >> WARN_ON(BTRFS_I(inode)->reserved_extents); >> >> Simple test program below can reproduce this issue steadily. >> Note: you need to modify BTRFS_MAX_EXTENT_SIZE to 64KB to have test, >> otherwise there won't be such WARNING. >> #include <string.h> >> #include <unistd.h> >> #include <sys/types.h> >> #include <sys/stat.h> >> #include <fcntl.h> >> >> int main(void) >> { >> int fd; >> char buf[68 *1024]; >> >> memset(buf, 0, 68 * 1024); >> fd = open("testfile", O_CREAT | O_EXCL | O_RDWR); >> pwrite(fd, buf, 68 * 1024, 64 * 1024); >> return; >> } >> >> When BTRFS_MAX_EXTENT_SIZE is 64KB, and buffered data range is: >> 64KB 128K 132KB >> |-----------------------------------------------|---------------| >> 64 + 4KB >> >> 1) for above data range, btrfs_delalloc_reserve_metadata() will reserve >> metadata and set BTRFS_I(inode)->outstanding_extents to 2. >> (68KB + 64KB - 1) / 64KB == 2 >> >> Outstanding_extents: 2 >> >> 2) then btrfs_dirty_page() will be called to dirty pages and set >> EXTENT_DELALLOC flag. In this case, btrfs_set_bit_hook() will be called >> twice. >> The 1st set_bit_hook() call will set DEALLOC flag for the first 64K. >> 64KB 128KB >> |-----------------------------------------------| >> 64KB DELALLOC >> Outstanding_extents: 2 >> >> Set_bit_hooks() uses FIRST_DELALLOC flag to avoid re-increase >> outstanding_extents counter. >> So for 1st set_bit_hooks() call, it won't modify outstanding_extents, >> it's still 2. >> >> Then FIRST_DELALLOC flag is *CLEARED*. >> >> 3) 2nd btrfs_set_bit_hook() call. >> Because FIRST_DELALLOC have been cleared by previous set_bit_hook(), >> btrfs_set_bit_hook() will increase BTRFS_I(inode)->outstanding_extents by >> one, so now BTRFS_I(inode)->outstanding_extents is 3. >> 64KB 128KB 132KB >> |-----------------------------------------------|----------------| >> 64K DELALLOC 4K DELALLOC >> Outstanding_extents: 3 >> >> But the correct outstanding_extents number should be 2, not 3. >> The 2nd btrfs_set_bit_hook() call just screwed up this, and leads to the >> WARN_ON(). >> >> Normally, we can solve it by only increasing outstanding_extents in >> set_bit_hook(). >> But the problem is for delalloc_reserve/release_metadata(), we only have >> a 'length' parameter, and calculate in-accurate outstanding_extents. >> If we only rely on set_bit_hook() release_metadata() will crew things up >> as it will decrease inaccurate number. >> >> So the fix we use is: >> 1) Increase *INACCURATE* outstanding_extents at delalloc_reserve_meta >> Just as a place holder. >> 2) Increase *accurate* outstanding_extents at set_bit_hooks() >> This is the real increaser. >> 3) Decrease *INACCURATE* outstanding_extents before returning >> This makes outstanding_extents to correct value. >> >> For 128M BTRFS_MAX_EXTENT_SIZE, due to limitation of >> __btrfs_buffered_write(), each iteration will only handle about 2MB >> data. >> So btrfs_dirty_pages() won't need to handle cases cross 2 extents. >> > > NACK on this, it just makes a crappy problem harder to understand. We need to > stop using outstanding_extents as both a reservation thing _and_ an accurate > count. Indeed, outstanding_extent is hard to understand and causing tons of problems. > I'll rework this and fix the other issues related to over-reservation > with compression. Thanks, Looking forward to it. Thanks, Qu > > Josef > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 5bdd36664421..1c52d5a3544a 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3175,6 +3175,8 @@ int btrfs_start_delalloc_roots(struct btrfs_fs_info *fs_info, int delay_iput, int nr); int btrfs_set_extent_delalloc(struct inode *inode, u64 start, u64 end, struct extent_state **cached_state, int dedupe); +int btrfs_set_extent_defrag(struct inode *inode, u64 start, u64 end, + struct extent_state **cached_state); int btrfs_create_subvol_root(struct btrfs_trans_handle *trans, struct btrfs_root *new_root, struct btrfs_root *parent_root, diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 5d3c6ac960fd..e644f936449b 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -1601,6 +1601,9 @@ static void btrfs_split_extent_hook(void *private_data, if (!(orig->state & EXTENT_DELALLOC)) return; + if (btrfs_is_free_space_inode(BTRFS_I(inode))) + return; + size = orig->end - orig->start + 1; if (size > BTRFS_MAX_EXTENT_SIZE) { u32 num_extents; @@ -1641,6 +1644,9 @@ static void btrfs_merge_extent_hook(void *private_data, if (!(other->state & EXTENT_DELALLOC)) return; + if (btrfs_is_free_space_inode(BTRFS_I(inode))) + return; + if (new->start > other->start) new_size = new->end - other->start + 1; else @@ -1737,7 +1743,6 @@ static void btrfs_set_bit_hook(void *private_data, struct extent_state *state, unsigned *bits) { struct inode *inode = private_data; - struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); if ((*bits & EXTENT_DEFRAG) && !(*bits & EXTENT_DELALLOC)) @@ -1750,13 +1755,16 @@ static void btrfs_set_bit_hook(void *private_data, if (!(state->state & EXTENT_DELALLOC) && (*bits & EXTENT_DELALLOC)) { struct btrfs_root *root = BTRFS_I(inode)->root; u64 len = state->end + 1 - state->start; + u64 num_extents = div64_u64(len + BTRFS_MAX_EXTENT_SIZE - 1, + BTRFS_MAX_EXTENT_SIZE); bool do_list = !btrfs_is_free_space_inode(BTRFS_I(inode)); - if (*bits & EXTENT_FIRST_DELALLOC) { + if (*bits & EXTENT_FIRST_DELALLOC) *bits &= ~EXTENT_FIRST_DELALLOC; - } else { + + if (do_list) { spin_lock(&BTRFS_I(inode)->lock); - BTRFS_I(inode)->outstanding_extents++; + BTRFS_I(inode)->outstanding_extents += num_extents; spin_unlock(&BTRFS_I(inode)->lock); } @@ -2021,9 +2029,52 @@ static noinline int add_pending_csums(struct btrfs_trans_handle *trans, int btrfs_set_extent_delalloc(struct inode *inode, u64 start, u64 end, struct extent_state **cached_state, int dedupe) { + int ret; + u64 num_extents = div64_u64(end - start + BTRFS_MAX_EXTENT_SIZE, + BTRFS_MAX_EXTENT_SIZE); + + WARN_ON((end & (PAGE_SIZE - 1)) == 0); + ret = set_extent_delalloc(&BTRFS_I(inode)->io_tree, start, end, + cached_state); + + /* + * btrfs_delalloc_reserve_metadata() will first add number of + * outstanding extents according to data length, which is inaccurate + * for case like dirtying already dirty pages. + * so here we will decrease such inaccurate numbers, to make + * outstanding_extents only rely on the correct values added by + * set_bit_hook() + * + * Also, we skipped the metadata space reserve for space cache inodes, + * so don't modify the outstanding_extents value. + */ + if (ret == 0 && !btrfs_is_free_space_inode(BTRFS_I(inode))) { + spin_lock(&BTRFS_I(inode)->lock); + BTRFS_I(inode)->outstanding_extents -= num_extents; + spin_unlock(&BTRFS_I(inode)->lock); + } + + return ret; +} + +int btrfs_set_extent_defrag(struct inode *inode, u64 start, u64 end, + struct extent_state **cached_state) +{ + int ret; + u64 num_extents = div64_u64(end - start + BTRFS_MAX_EXTENT_SIZE, + BTRFS_MAX_EXTENT_SIZE); + WARN_ON((end & (PAGE_SIZE - 1)) == 0); - return set_extent_delalloc(&BTRFS_I(inode)->io_tree, start, end, - cached_state); + ret = set_extent_defrag(&BTRFS_I(inode)->io_tree, start, end, + cached_state); + + if (ret == 0 && !btrfs_is_free_space_inode(BTRFS_I(inode))) { + spin_lock(&BTRFS_I(inode)->lock); + BTRFS_I(inode)->outstanding_extents -= num_extents; + spin_unlock(&BTRFS_I(inode)->lock); + } + + return ret; } /* see btrfs_writepage_start_hook for details on why this is required */ diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index fa1b78cf25f6..faa0e7cc0e2c 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -1232,10 +1232,8 @@ static int cluster_pages_for_defrag(struct inode *inode, (page_cnt - i_done) << PAGE_SHIFT); } - - set_extent_defrag(&BTRFS_I(inode)->io_tree, page_start, page_end - 1, - &cached_state); - + btrfs_set_extent_defrag(inode, page_start, + page_end - 1, &cached_state); unlock_extent_cached(&BTRFS_I(inode)->io_tree, page_start, page_end - 1, &cached_state, GFP_NOFS);