| Message ID | 1478597458-29287-1-git-send-email-wangxg.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Nov 08, 2016 at 05:30:58PM +0800, Wang Xiaoguang wrote: > The current limit of number of asynchronous delalloc pages is (10 * SZ_1M). > For 4K page, the total ram bytes would be 40G, very big value, I think in > most cases, this limit will not work, here I set limit of the number of > asynchronous delalloc pages to SZ_1M(4GB ram bytes). > > Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com> > --- > fs/btrfs/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 8e3a5a2..3a910f6 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page, > struct btrfs_root *root = BTRFS_I(inode)->root; > unsigned long nr_pages; > u64 cur_end; > - int limit = 10 * SZ_1M; > + int limit = SZ_1M; This looks like mismatch in units, the limit seems to be "10MiB" but it's used to compare async_delalloc_pages, which is in pages. I'm not sure what's the expected value for limit, as 10MiB is too small. -- 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 11/08/2016 04:30 AM, Wang Xiaoguang wrote: > The current limit of number of asynchronous delalloc pages is (10 * SZ_1M). > For 4K page, the total ram bytes would be 40G, very big value, I think in > most cases, this limit will not work, here I set limit of the number of > asynchronous delalloc pages to SZ_1M(4GB ram bytes). > > Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com> > --- > fs/btrfs/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 8e3a5a2..3a910f6 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page, > struct btrfs_root *root = BTRFS_I(inode)->root; > unsigned long nr_pages; > u64 cur_end; > - int limit = 10 * SZ_1M; > + int limit = SZ_1M; > > clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED, > 1, 0, NULL, GFP_NOFS); > As Dave points out, I didn't use the right units for this, so even though we definitely waited on this limit while it was in development, that was probably a different bug. Do you have a test case where the regular writeback throttling isn't enough to also throttle the async delalloc pages? It might be better to just delete the limit entirely. -chris -- 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
hello, On 11/22/2016 12:39 AM, David Sterba wrote: > On Tue, Nov 08, 2016 at 05:30:58PM +0800, Wang Xiaoguang wrote: >> The current limit of number of asynchronous delalloc pages is (10 * SZ_1M). >> For 4K page, the total ram bytes would be 40G, very big value, I think in >> most cases, this limit will not work, here I set limit of the number of >> asynchronous delalloc pages to SZ_1M(4GB ram bytes). >> >> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com> >> --- >> fs/btrfs/inode.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index 8e3a5a2..3a910f6 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page, >> struct btrfs_root *root = BTRFS_I(inode)->root; >> unsigned long nr_pages; >> u64 cur_end; >> - int limit = 10 * SZ_1M; >> + int limit = SZ_1M; > This looks like mismatch in units, the limit seems to be "10MiB" but > it's used to compare async_delalloc_pages, which is in pages. I'm not > sure what's the expected value for limit, as 10MiB is too small. There are such codes in cow_file_range_async() nr_pages = (cur_end - start + PAGE_SIZE) >> PAGE_SHIFT; atomic_add(nr_pages, &root->fs_info->async_delalloc_pages); So here the real limit is 10 * SZ_1M * 4096, 40GB memory, I think this value is too big, so modify it to 4GB. Regards, Xiaoguang Wang > > -- 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
hello, On 11/22/2016 01:59 AM, Chris Mason wrote: > On 11/08/2016 04:30 AM, Wang Xiaoguang wrote: >> The current limit of number of asynchronous delalloc pages is (10 * >> SZ_1M). >> For 4K page, the total ram bytes would be 40G, very big value, I >> think in >> most cases, this limit will not work, here I set limit of the number of >> asynchronous delalloc pages to SZ_1M(4GB ram bytes). >> >> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com> >> --- >> fs/btrfs/inode.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index 8e3a5a2..3a910f6 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode >> *inode, struct page *locked_page, >> struct btrfs_root *root = BTRFS_I(inode)->root; >> unsigned long nr_pages; >> u64 cur_end; >> - int limit = 10 * SZ_1M; >> + int limit = SZ_1M; >> >> clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, >> EXTENT_LOCKED, >> 1, 0, NULL, GFP_NOFS); >> > > As Dave points out, I didn't use the right units for this, so even > though we definitely waited on this limit while it was in development, > that was probably a different bug. > > Do you have a test case where the regular writeback throttling isn't > enough to also throttle the async delalloc pages? It might be better > to just delete the limit entirely. Sorry, I don't have one. The real limit is 10 * SZ_1M * 4096, 40GB memory, too big value, and I don't see this limit actually takes effect in my test environment. I'm OK with removing this limit :) Regards, Xiaoguang Wang > > -chris > > -- 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 Tue, Nov 22, 2016 at 05:20:10PM +0800, Wang Xiaoguang wrote: > hello, > > On 11/22/2016 12:39 AM, David Sterba wrote: > > On Tue, Nov 08, 2016 at 05:30:58PM +0800, Wang Xiaoguang wrote: > >> The current limit of number of asynchronous delalloc pages is (10 * SZ_1M). > >> For 4K page, the total ram bytes would be 40G, very big value, I think in > >> most cases, this limit will not work, here I set limit of the number of > >> asynchronous delalloc pages to SZ_1M(4GB ram bytes). > >> > >> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com> > >> --- > >> fs/btrfs/inode.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > >> index 8e3a5a2..3a910f6 100644 > >> --- a/fs/btrfs/inode.c > >> +++ b/fs/btrfs/inode.c > >> @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page, > >> struct btrfs_root *root = BTRFS_I(inode)->root; > >> unsigned long nr_pages; > >> u64 cur_end; > >> - int limit = 10 * SZ_1M; > >> + int limit = SZ_1M; > > This looks like mismatch in units, the limit seems to be "10MiB" but > > it's used to compare async_delalloc_pages, which is in pages. I'm not > > sure what's the expected value for limit, as 10MiB is too small. > There are such codes in cow_file_range_async() > nr_pages = (cur_end - start + PAGE_SIZE) >> PAGE_SHIFT; > atomic_add(nr_pages, &root->fs_info->async_delalloc_pages); > > So here the real limit is 10 * SZ_1M * 4096, 40GB memory, I think this > value is too big, so modify it to 4GB. Yeah, but I don't see why 4GB would be a good value either. -- 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 11/22/2016 04:20 AM, Wang Xiaoguang wrote: > hello, > > On 11/22/2016 12:39 AM, David Sterba wrote: >> On Tue, Nov 08, 2016 at 05:30:58PM +0800, Wang Xiaoguang wrote: >>> The current limit of number of asynchronous delalloc pages is (10 * >>> SZ_1M). >>> For 4K page, the total ram bytes would be 40G, very big value, I >>> think in >>> most cases, this limit will not work, here I set limit of the number of >>> asynchronous delalloc pages to SZ_1M(4GB ram bytes). >>> >>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com> >>> --- >>> fs/btrfs/inode.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >>> index 8e3a5a2..3a910f6 100644 >>> --- a/fs/btrfs/inode.c >>> +++ b/fs/btrfs/inode.c >>> @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode >>> *inode, struct page *locked_page, >>> struct btrfs_root *root = BTRFS_I(inode)->root; >>> unsigned long nr_pages; >>> u64 cur_end; >>> - int limit = 10 * SZ_1M; >>> + int limit = SZ_1M; >> This looks like mismatch in units, the limit seems to be "10MiB" but >> it's used to compare async_delalloc_pages, which is in pages. I'm not >> sure what's the expected value for limit, as 10MiB is too small. > There are such codes in cow_file_range_async() > nr_pages = (cur_end - start + PAGE_SIZE) >> PAGE_SHIFT; > atomic_add(nr_pages, &root->fs_info->async_delalloc_pages); > > So here the real limit is 10 * SZ_1M * 4096, 40GB memory, I think this > value is too big, so modify it to 4GB. I don't disagree that its too big, but if the rest of the dirty/writeback throttling infra in the kernel is enough, we can just delete this limit completely. -chris -- 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/inode.c b/fs/btrfs/inode.c index 8e3a5a2..3a910f6 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page, struct btrfs_root *root = BTRFS_I(inode)->root; unsigned long nr_pages; u64 cur_end; - int limit = 10 * SZ_1M; + int limit = SZ_1M; clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED, 1, 0, NULL, GFP_NOFS);
The current limit of number of asynchronous delalloc pages is (10 * SZ_1M). For 4K page, the total ram bytes would be 40G, very big value, I think in most cases, this limit will not work, here I set limit of the number of asynchronous delalloc pages to SZ_1M(4GB ram bytes). Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com> --- fs/btrfs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)