2017-07-26 12:40:28

by Yunlong Song

[permalink] [raw]
Subject: [PATCH] f2fs: provide f2fs_balance_fs to __write_node_page

Signed-off-by: Yunlong Song <[email protected]>
---
fs/f2fs/checkpoint.c | 2 +-
fs/f2fs/f2fs.h | 2 +-
fs/f2fs/node.c | 16 ++++++++++------
3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 5b876f6..3c84a25 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1017,7 +1017,7 @@ static int block_operations(struct f2fs_sb_info *sbi)

if (get_pages(sbi, F2FS_DIRTY_NODES)) {
up_write(&sbi->node_write);
- err = sync_node_pages(sbi, &wbc);
+ err = sync_node_pages(sbi, &wbc, false);
if (err) {
up_write(&sbi->node_change);
f2fs_unlock_all(sbi);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 94a88b2..f69051b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2293,7 +2293,7 @@ struct page *new_node_page(struct dnode_of_data *dn,
void move_node_page(struct page *node_page, int gc_type);
int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
struct writeback_control *wbc, bool atomic);
-int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc);
+int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need);
void build_free_nids(struct f2fs_sb_info *sbi, bool sync, bool mount);
bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid);
void alloc_nid_done(struct f2fs_sb_info *sbi, nid_t nid);
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index d53fe62..b5c0ce3 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1326,7 +1326,7 @@ static struct page *last_fsync_dnode(struct f2fs_sb_info *sbi, nid_t ino)
}

static int __write_node_page(struct page *page, bool atomic, bool *submitted,
- struct writeback_control *wbc)
+ struct writeback_control *wbc, bool need)
{
struct f2fs_sb_info *sbi = F2FS_P_SB(page);
nid_t nid;
@@ -1387,6 +1387,10 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
}

unlock_page(page);
+ if (need)
+ f2fs_balance_fs(sbi, false);
+ else
+ f2fs_balance_fs_bg(sbi);

if (unlikely(f2fs_cp_error(sbi))) {
f2fs_submit_merged_write(sbi, NODE);
@@ -1405,7 +1409,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
static int f2fs_write_node_page(struct page *page,
struct writeback_control *wbc)
{
- return __write_node_page(page, false, NULL, wbc);
+ return __write_node_page(page, false, NULL, wbc, true);
}

int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
@@ -1493,7 +1497,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,

ret = __write_node_page(page, atomic &&
page == last_page,
- &submitted, wbc);
+ &submitted, wbc, true);
if (ret) {
unlock_page(page);
f2fs_put_page(last_page, 0);
@@ -1530,7 +1534,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
return ret ? -EIO: 0;
}

-int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
+int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need)
{
pgoff_t index, end;
struct pagevec pvec;
@@ -1608,7 +1612,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
set_fsync_mark(page, 0);
set_dentry_mark(page, 0);

- ret = __write_node_page(page, false, &submitted, wbc);
+ ret = __write_node_page(page, false, &submitted, wbc, need);
if (ret)
unlock_page(page);
else if (submitted)
@@ -1697,7 +1701,7 @@ static int f2fs_write_node_pages(struct address_space *mapping,
diff = nr_pages_to_write(sbi, NODE, wbc);
wbc->sync_mode = WB_SYNC_NONE;
blk_start_plug(&plug);
- sync_node_pages(sbi, wbc);
+ sync_node_pages(sbi, wbc, true);
blk_finish_plug(&plug);
wbc->nr_to_write = max((long)0, wbc->nr_to_write - diff);
return 0;
--
1.8.5.2


2017-07-27 12:11:32

by Yunlong Song

[permalink] [raw]
Subject: [PATCH v2] f2fs: provide f2fs_balance_fs to __write_node_page

Let node writeback also do f2fs_balance_fs to ensure there are always enough free
segments.

Signed-off-by: Yunlong Song <[email protected]>
---
fs/f2fs/checkpoint.c | 2 +-
fs/f2fs/f2fs.h | 2 +-
fs/f2fs/node.c | 14 ++++++++------
3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 5b876f6..3c84a25 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1017,7 +1017,7 @@ static int block_operations(struct f2fs_sb_info *sbi)

if (get_pages(sbi, F2FS_DIRTY_NODES)) {
up_write(&sbi->node_write);
- err = sync_node_pages(sbi, &wbc);
+ err = sync_node_pages(sbi, &wbc, false);
if (err) {
up_write(&sbi->node_change);
f2fs_unlock_all(sbi);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 94a88b2..f69051b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2293,7 +2293,7 @@ struct page *new_node_page(struct dnode_of_data *dn,
void move_node_page(struct page *node_page, int gc_type);
int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
struct writeback_control *wbc, bool atomic);
-int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc);
+int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need);
void build_free_nids(struct f2fs_sb_info *sbi, bool sync, bool mount);
bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid);
void alloc_nid_done(struct f2fs_sb_info *sbi, nid_t nid);
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index d53fe62..210ec25 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1326,7 +1326,7 @@ static struct page *last_fsync_dnode(struct f2fs_sb_info *sbi, nid_t ino)
}

static int __write_node_page(struct page *page, bool atomic, bool *submitted,
- struct writeback_control *wbc)
+ struct writeback_control *wbc, bool need)
{
struct f2fs_sb_info *sbi = F2FS_P_SB(page);
nid_t nid;
@@ -1387,6 +1387,8 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
}

unlock_page(page);
+ if (need)
+ f2fs_balance_fs(sbi, false);

if (unlikely(f2fs_cp_error(sbi))) {
f2fs_submit_merged_write(sbi, NODE);
@@ -1405,7 +1407,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
static int f2fs_write_node_page(struct page *page,
struct writeback_control *wbc)
{
- return __write_node_page(page, false, NULL, wbc);
+ return __write_node_page(page, false, NULL, wbc, false);
}

int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
@@ -1493,7 +1495,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,

ret = __write_node_page(page, atomic &&
page == last_page,
- &submitted, wbc);
+ &submitted, wbc, true);
if (ret) {
unlock_page(page);
f2fs_put_page(last_page, 0);
@@ -1530,7 +1532,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
return ret ? -EIO: 0;
}

-int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
+int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need)
{
pgoff_t index, end;
struct pagevec pvec;
@@ -1608,7 +1610,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
set_fsync_mark(page, 0);
set_dentry_mark(page, 0);

- ret = __write_node_page(page, false, &submitted, wbc);
+ ret = __write_node_page(page, false, &submitted, wbc, need);
if (ret)
unlock_page(page);
else if (submitted)
@@ -1697,7 +1699,7 @@ static int f2fs_write_node_pages(struct address_space *mapping,
diff = nr_pages_to_write(sbi, NODE, wbc);
wbc->sync_mode = WB_SYNC_NONE;
blk_start_plug(&plug);
- sync_node_pages(sbi, wbc);
+ sync_node_pages(sbi, wbc, true);
blk_finish_plug(&plug);
wbc->nr_to_write = max((long)0, wbc->nr_to_write - diff);
return 0;
--
1.8.5.2

2017-07-27 12:17:06

by Yunlong Song

[permalink] [raw]
Subject: Re: [PATCH v2] f2fs: provide f2fs_balance_fs to __write_node_page

v1->v2, fix some dead lock problems under some heavy load test

On 2017/7/27 20:11, Yunlong Song wrote:
> Let node writeback also do f2fs_balance_fs to ensure there are always enough free
> segments.
>
> Signed-off-by: Yunlong Song <[email protected]>
> ---
> fs/f2fs/checkpoint.c | 2 +-
> fs/f2fs/f2fs.h | 2 +-
> fs/f2fs/node.c | 14 ++++++++------
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 5b876f6..3c84a25 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -1017,7 +1017,7 @@ static int block_operations(struct f2fs_sb_info *sbi)
>
> if (get_pages(sbi, F2FS_DIRTY_NODES)) {
> up_write(&sbi->node_write);
> - err = sync_node_pages(sbi, &wbc);
> + err = sync_node_pages(sbi, &wbc, false);
> if (err) {
> up_write(&sbi->node_change);
> f2fs_unlock_all(sbi);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 94a88b2..f69051b 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2293,7 +2293,7 @@ struct page *new_node_page(struct dnode_of_data *dn,
> void move_node_page(struct page *node_page, int gc_type);
> int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
> struct writeback_control *wbc, bool atomic);
> -int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc);
> +int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need);
> void build_free_nids(struct f2fs_sb_info *sbi, bool sync, bool mount);
> bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid);
> void alloc_nid_done(struct f2fs_sb_info *sbi, nid_t nid);
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index d53fe62..210ec25 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1326,7 +1326,7 @@ static struct page *last_fsync_dnode(struct f2fs_sb_info *sbi, nid_t ino)
> }
>
> static int __write_node_page(struct page *page, bool atomic, bool *submitted,
> - struct writeback_control *wbc)
> + struct writeback_control *wbc, bool need)
> {
> struct f2fs_sb_info *sbi = F2FS_P_SB(page);
> nid_t nid;
> @@ -1387,6 +1387,8 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
> }
>
> unlock_page(page);
> + if (need)
> + f2fs_balance_fs(sbi, false);
>
> if (unlikely(f2fs_cp_error(sbi))) {
> f2fs_submit_merged_write(sbi, NODE);
> @@ -1405,7 +1407,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
> static int f2fs_write_node_page(struct page *page,
> struct writeback_control *wbc)
> {
> - return __write_node_page(page, false, NULL, wbc);
> + return __write_node_page(page, false, NULL, wbc, false);
> }
>
> int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
> @@ -1493,7 +1495,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>
> ret = __write_node_page(page, atomic &&
> page == last_page,
> - &submitted, wbc);
> + &submitted, wbc, true);
> if (ret) {
> unlock_page(page);
> f2fs_put_page(last_page, 0);
> @@ -1530,7 +1532,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
> return ret ? -EIO: 0;
> }
>
> -int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
> +int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need)
> {
> pgoff_t index, end;
> struct pagevec pvec;
> @@ -1608,7 +1610,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
> set_fsync_mark(page, 0);
> set_dentry_mark(page, 0);
>
> - ret = __write_node_page(page, false, &submitted, wbc);
> + ret = __write_node_page(page, false, &submitted, wbc, need);
> if (ret)
> unlock_page(page);
> else if (submitted)
> @@ -1697,7 +1699,7 @@ static int f2fs_write_node_pages(struct address_space *mapping,
> diff = nr_pages_to_write(sbi, NODE, wbc);
> wbc->sync_mode = WB_SYNC_NONE;
> blk_start_plug(&plug);
> - sync_node_pages(sbi, wbc);
> + sync_node_pages(sbi, wbc, true);
> blk_finish_plug(&plug);
> wbc->nr_to_write = max((long)0, wbc->nr_to_write - diff);
> return 0;

--
Thanks,
Yunlong Song


2017-07-28 16:20:21

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH v2] f2fs: provide f2fs_balance_fs to __write_node_page

Hi Yunlong,

On 07/27, Yunlong Song wrote:
> v1->v2, fix some dead lock problems under some heavy load test

So, does this patch resolve the previous GC problem?

Thanks,

>
> On 2017/7/27 20:11, Yunlong Song wrote:
> > Let node writeback also do f2fs_balance_fs to ensure there are always enough free
> > segments.
> >
> > Signed-off-by: Yunlong Song <[email protected]>
> > ---
> > fs/f2fs/checkpoint.c | 2 +-
> > fs/f2fs/f2fs.h | 2 +-
> > fs/f2fs/node.c | 14 ++++++++------
> > 3 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 5b876f6..3c84a25 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -1017,7 +1017,7 @@ static int block_operations(struct f2fs_sb_info *sbi)
> > if (get_pages(sbi, F2FS_DIRTY_NODES)) {
> > up_write(&sbi->node_write);
> > - err = sync_node_pages(sbi, &wbc);
> > + err = sync_node_pages(sbi, &wbc, false);
> > if (err) {
> > up_write(&sbi->node_change);
> > f2fs_unlock_all(sbi);
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 94a88b2..f69051b 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2293,7 +2293,7 @@ struct page *new_node_page(struct dnode_of_data *dn,
> > void move_node_page(struct page *node_page, int gc_type);
> > int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
> > struct writeback_control *wbc, bool atomic);
> > -int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc);
> > +int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need);
> > void build_free_nids(struct f2fs_sb_info *sbi, bool sync, bool mount);
> > bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid);
> > void alloc_nid_done(struct f2fs_sb_info *sbi, nid_t nid);
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index d53fe62..210ec25 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -1326,7 +1326,7 @@ static struct page *last_fsync_dnode(struct f2fs_sb_info *sbi, nid_t ino)
> > }
> > static int __write_node_page(struct page *page, bool atomic, bool *submitted,
> > - struct writeback_control *wbc)
> > + struct writeback_control *wbc, bool need)
> > {
> > struct f2fs_sb_info *sbi = F2FS_P_SB(page);
> > nid_t nid;
> > @@ -1387,6 +1387,8 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
> > }
> > unlock_page(page);
> > + if (need)
> > + f2fs_balance_fs(sbi, false);
> > if (unlikely(f2fs_cp_error(sbi))) {
> > f2fs_submit_merged_write(sbi, NODE);
> > @@ -1405,7 +1407,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
> > static int f2fs_write_node_page(struct page *page,
> > struct writeback_control *wbc)
> > {
> > - return __write_node_page(page, false, NULL, wbc);
> > + return __write_node_page(page, false, NULL, wbc, false);
> > }
> > int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
> > @@ -1493,7 +1495,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
> > ret = __write_node_page(page, atomic &&
> > page == last_page,
> > - &submitted, wbc);
> > + &submitted, wbc, true);
> > if (ret) {
> > unlock_page(page);
> > f2fs_put_page(last_page, 0);
> > @@ -1530,7 +1532,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
> > return ret ? -EIO: 0;
> > }
> > -int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
> > +int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need)
> > {
> > pgoff_t index, end;
> > struct pagevec pvec;
> > @@ -1608,7 +1610,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
> > set_fsync_mark(page, 0);
> > set_dentry_mark(page, 0);
> > - ret = __write_node_page(page, false, &submitted, wbc);
> > + ret = __write_node_page(page, false, &submitted, wbc, need);
> > if (ret)
> > unlock_page(page);
> > else if (submitted)
> > @@ -1697,7 +1699,7 @@ static int f2fs_write_node_pages(struct address_space *mapping,
> > diff = nr_pages_to_write(sbi, NODE, wbc);
> > wbc->sync_mode = WB_SYNC_NONE;
> > blk_start_plug(&plug);
> > - sync_node_pages(sbi, wbc);
> > + sync_node_pages(sbi, wbc, true);
> > blk_finish_plug(&plug);
> > wbc->nr_to_write = max((long)0, wbc->nr_to_write - diff);
> > return 0;
>
> --
> Thanks,
> Yunlong Song
>

2017-07-29 00:20:03

by Chao Yu

[permalink] [raw]
Subject: Re: [PATCH v2] f2fs: provide f2fs_balance_fs to __write_node_page

Hi Yunlong,

On 2017/7/27 20:13, Yunlong Song wrote:
> v1->v2, fix some dead lock problems under some heavy load test
>
> On 2017/7/27 20:11, Yunlong Song wrote:
>> Let node writeback also do f2fs_balance_fs to ensure there are always enough free
>> segments.

Could we cover __write_data_page of directory inode with this logic?

Thanks,

>>
>> Signed-off-by: Yunlong Song <[email protected]>
>> ---
>> fs/f2fs/checkpoint.c | 2 +-
>> fs/f2fs/f2fs.h | 2 +-
>> fs/f2fs/node.c | 14 ++++++++------
>> 3 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 5b876f6..3c84a25 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -1017,7 +1017,7 @@ static int block_operations(struct f2fs_sb_info *sbi)
>>
>> if (get_pages(sbi, F2FS_DIRTY_NODES)) {
>> up_write(&sbi->node_write);
>> - err = sync_node_pages(sbi, &wbc);
>> + err = sync_node_pages(sbi, &wbc, false);
>> if (err) {
>> up_write(&sbi->node_change);
>> f2fs_unlock_all(sbi);
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 94a88b2..f69051b 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -2293,7 +2293,7 @@ struct page *new_node_page(struct dnode_of_data *dn,
>> void move_node_page(struct page *node_page, int gc_type);
>> int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>> struct writeback_control *wbc, bool atomic);
>> -int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc);
>> +int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need);
>> void build_free_nids(struct f2fs_sb_info *sbi, bool sync, bool mount);
>> bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid);
>> void alloc_nid_done(struct f2fs_sb_info *sbi, nid_t nid);
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index d53fe62..210ec25 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -1326,7 +1326,7 @@ static struct page *last_fsync_dnode(struct f2fs_sb_info *sbi, nid_t ino)
>> }
>>
>> static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>> - struct writeback_control *wbc)
>> + struct writeback_control *wbc, bool need)
>> {
>> struct f2fs_sb_info *sbi = F2FS_P_SB(page);
>> nid_t nid;
>> @@ -1387,6 +1387,8 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>> }
>>
>> unlock_page(page);
>> + if (need)
>> + f2fs_balance_fs(sbi, false);
>>
>> if (unlikely(f2fs_cp_error(sbi))) {
>> f2fs_submit_merged_write(sbi, NODE);
>> @@ -1405,7 +1407,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>> static int f2fs_write_node_page(struct page *page,
>> struct writeback_control *wbc)
>> {
>> - return __write_node_page(page, false, NULL, wbc);
>> + return __write_node_page(page, false, NULL, wbc, false);
>> }
>>
>> int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>> @@ -1493,7 +1495,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>>
>> ret = __write_node_page(page, atomic &&
>> page == last_page,
>> - &submitted, wbc);
>> + &submitted, wbc, true);
>> if (ret) {
>> unlock_page(page);
>> f2fs_put_page(last_page, 0);
>> @@ -1530,7 +1532,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>> return ret ? -EIO: 0;
>> }
>>
>> -int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
>> +int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need)
>> {
>> pgoff_t index, end;
>> struct pagevec pvec;
>> @@ -1608,7 +1610,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
>> set_fsync_mark(page, 0);
>> set_dentry_mark(page, 0);
>>
>> - ret = __write_node_page(page, false, &submitted, wbc);
>> + ret = __write_node_page(page, false, &submitted, wbc, need);
>> if (ret)
>> unlock_page(page);
>> else if (submitted)
>> @@ -1697,7 +1699,7 @@ static int f2fs_write_node_pages(struct address_space *mapping,
>> diff = nr_pages_to_write(sbi, NODE, wbc);
>> wbc->sync_mode = WB_SYNC_NONE;
>> blk_start_plug(&plug);
>> - sync_node_pages(sbi, wbc);
>> + sync_node_pages(sbi, wbc, true);
>> blk_finish_plug(&plug);
>> wbc->nr_to_write = max((long)0, wbc->nr_to_write - diff);
>> return 0;
>

2017-07-29 13:20:42

by Yunlong Song

[permalink] [raw]
Subject: Re: [PATCH v2] f2fs: provide f2fs_balance_fs to __write_node_page

Hi, Jay,
Not sure yet, whether it is just the case or not, at least we
strengthen f2fs_balance_fs in some
corner cases. And maybe there are still some corner cases which we have
not considered yet. See
my new patch "provide f2fs_balance_fs to __write_data_page for dentry
pages", which is reported
by Chao. This is another corner case.

On 2017/7/29 0:20, Jaegeuk Kim wrote:
> Hi Yunlong,
>
> On 07/27, Yunlong Song wrote:
>> v1->v2, fix some dead lock problems under some heavy load test
> So, does this patch resolve the previous GC problem?
>
> Thanks,
>
>> On 2017/7/27 20:11, Yunlong Song wrote:
>>> Let node writeback also do f2fs_balance_fs to ensure there are always enough free
>>> segments.
>>>
>>> Signed-off-by: Yunlong Song <[email protected]>
>>> ---
>>> fs/f2fs/checkpoint.c | 2 +-
>>> fs/f2fs/f2fs.h | 2 +-
>>> fs/f2fs/node.c | 14 ++++++++------
>>> 3 files changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 5b876f6..3c84a25 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -1017,7 +1017,7 @@ static int block_operations(struct f2fs_sb_info *sbi)
>>> if (get_pages(sbi, F2FS_DIRTY_NODES)) {
>>> up_write(&sbi->node_write);
>>> - err = sync_node_pages(sbi, &wbc);
>>> + err = sync_node_pages(sbi, &wbc, false);
>>> if (err) {
>>> up_write(&sbi->node_change);
>>> f2fs_unlock_all(sbi);
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 94a88b2..f69051b 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -2293,7 +2293,7 @@ struct page *new_node_page(struct dnode_of_data *dn,
>>> void move_node_page(struct page *node_page, int gc_type);
>>> int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>>> struct writeback_control *wbc, bool atomic);
>>> -int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc);
>>> +int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need);
>>> void build_free_nids(struct f2fs_sb_info *sbi, bool sync, bool mount);
>>> bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid);
>>> void alloc_nid_done(struct f2fs_sb_info *sbi, nid_t nid);
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index d53fe62..210ec25 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -1326,7 +1326,7 @@ static struct page *last_fsync_dnode(struct f2fs_sb_info *sbi, nid_t ino)
>>> }
>>> static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>> - struct writeback_control *wbc)
>>> + struct writeback_control *wbc, bool need)
>>> {
>>> struct f2fs_sb_info *sbi = F2FS_P_SB(page);
>>> nid_t nid;
>>> @@ -1387,6 +1387,8 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>> }
>>> unlock_page(page);
>>> + if (need)
>>> + f2fs_balance_fs(sbi, false);
>>> if (unlikely(f2fs_cp_error(sbi))) {
>>> f2fs_submit_merged_write(sbi, NODE);
>>> @@ -1405,7 +1407,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>> static int f2fs_write_node_page(struct page *page,
>>> struct writeback_control *wbc)
>>> {
>>> - return __write_node_page(page, false, NULL, wbc);
>>> + return __write_node_page(page, false, NULL, wbc, false);
>>> }
>>> int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>>> @@ -1493,7 +1495,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>>> ret = __write_node_page(page, atomic &&
>>> page == last_page,
>>> - &submitted, wbc);
>>> + &submitted, wbc, true);
>>> if (ret) {
>>> unlock_page(page);
>>> f2fs_put_page(last_page, 0);
>>> @@ -1530,7 +1532,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>>> return ret ? -EIO: 0;
>>> }
>>> -int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
>>> +int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need)
>>> {
>>> pgoff_t index, end;
>>> struct pagevec pvec;
>>> @@ -1608,7 +1610,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
>>> set_fsync_mark(page, 0);
>>> set_dentry_mark(page, 0);
>>> - ret = __write_node_page(page, false, &submitted, wbc);
>>> + ret = __write_node_page(page, false, &submitted, wbc, need);
>>> if (ret)
>>> unlock_page(page);
>>> else if (submitted)
>>> @@ -1697,7 +1699,7 @@ static int f2fs_write_node_pages(struct address_space *mapping,
>>> diff = nr_pages_to_write(sbi, NODE, wbc);
>>> wbc->sync_mode = WB_SYNC_NONE;
>>> blk_start_plug(&plug);
>>> - sync_node_pages(sbi, wbc);
>>> + sync_node_pages(sbi, wbc, true);
>>> blk_finish_plug(&plug);
>>> wbc->nr_to_write = max((long)0, wbc->nr_to_write - diff);
>>> return 0;
>> --
>> Thanks,
>> Yunlong Song
>>
> .
>

--
Thanks,
Yunlong Song


2017-07-29 13:23:03

by Yunlong Song

[permalink] [raw]
Subject: Re: [PATCH v2] f2fs: provide f2fs_balance_fs to __write_node_page

Hi Chao,
Nice suggestion, I have sent out a new patch to implement it.

On 2017/7/29 8:19, Chao Yu wrote:
> Hi Yunlong,
>
> On 2017/7/27 20:13, Yunlong Song wrote:
>> v1->v2, fix some dead lock problems under some heavy load test
>>
>> On 2017/7/27 20:11, Yunlong Song wrote:
>>> Let node writeback also do f2fs_balance_fs to ensure there are always enough free
>>> segments.
> Could we cover __write_data_page of directory inode with this logic?
>
> Thanks,
>
>>> Signed-off-by: Yunlong Song <[email protected]>
>>> ---
>>> fs/f2fs/checkpoint.c | 2 +-
>>> fs/f2fs/f2fs.h | 2 +-
>>> fs/f2fs/node.c | 14 ++++++++------
>>> 3 files changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 5b876f6..3c84a25 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -1017,7 +1017,7 @@ static int block_operations(struct f2fs_sb_info *sbi)
>>>
>>> if (get_pages(sbi, F2FS_DIRTY_NODES)) {
>>> up_write(&sbi->node_write);
>>> - err = sync_node_pages(sbi, &wbc);
>>> + err = sync_node_pages(sbi, &wbc, false);
>>> if (err) {
>>> up_write(&sbi->node_change);
>>> f2fs_unlock_all(sbi);
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 94a88b2..f69051b 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -2293,7 +2293,7 @@ struct page *new_node_page(struct dnode_of_data *dn,
>>> void move_node_page(struct page *node_page, int gc_type);
>>> int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>>> struct writeback_control *wbc, bool atomic);
>>> -int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc);
>>> +int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need);
>>> void build_free_nids(struct f2fs_sb_info *sbi, bool sync, bool mount);
>>> bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid);
>>> void alloc_nid_done(struct f2fs_sb_info *sbi, nid_t nid);
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index d53fe62..210ec25 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -1326,7 +1326,7 @@ static struct page *last_fsync_dnode(struct f2fs_sb_info *sbi, nid_t ino)
>>> }
>>>
>>> static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>> - struct writeback_control *wbc)
>>> + struct writeback_control *wbc, bool need)
>>> {
>>> struct f2fs_sb_info *sbi = F2FS_P_SB(page);
>>> nid_t nid;
>>> @@ -1387,6 +1387,8 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>> }
>>>
>>> unlock_page(page);
>>> + if (need)
>>> + f2fs_balance_fs(sbi, false);
>>>
>>> if (unlikely(f2fs_cp_error(sbi))) {
>>> f2fs_submit_merged_write(sbi, NODE);
>>> @@ -1405,7 +1407,7 @@ static int __write_node_page(struct page *page, bool atomic, bool *submitted,
>>> static int f2fs_write_node_page(struct page *page,
>>> struct writeback_control *wbc)
>>> {
>>> - return __write_node_page(page, false, NULL, wbc);
>>> + return __write_node_page(page, false, NULL, wbc, false);
>>> }
>>>
>>> int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>>> @@ -1493,7 +1495,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>>>
>>> ret = __write_node_page(page, atomic &&
>>> page == last_page,
>>> - &submitted, wbc);
>>> + &submitted, wbc, true);
>>> if (ret) {
>>> unlock_page(page);
>>> f2fs_put_page(last_page, 0);
>>> @@ -1530,7 +1532,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
>>> return ret ? -EIO: 0;
>>> }
>>>
>>> -int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
>>> +int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc, bool need)
>>> {
>>> pgoff_t index, end;
>>> struct pagevec pvec;
>>> @@ -1608,7 +1610,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
>>> set_fsync_mark(page, 0);
>>> set_dentry_mark(page, 0);
>>>
>>> - ret = __write_node_page(page, false, &submitted, wbc);
>>> + ret = __write_node_page(page, false, &submitted, wbc, need);
>>> if (ret)
>>> unlock_page(page);
>>> else if (submitted)
>>> @@ -1697,7 +1699,7 @@ static int f2fs_write_node_pages(struct address_space *mapping,
>>> diff = nr_pages_to_write(sbi, NODE, wbc);
>>> wbc->sync_mode = WB_SYNC_NONE;
>>> blk_start_plug(&plug);
>>> - sync_node_pages(sbi, wbc);
>>> + sync_node_pages(sbi, wbc, true);
>>> blk_finish_plug(&plug);
>>> wbc->nr_to_write = max((long)0, wbc->nr_to_write - diff);
>>> return 0;
>
> .
>

--
Thanks,
Yunlong Song