2015-12-23 00:59:53

by Jaegeuk Kim

[permalink] [raw]
Subject: [PATCH 1/4] f2fs: check inline_data flag at converting time

We can check inode's inline_data flag when calling to convert it.

Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/data.c | 8 +++-----
fs/f2fs/file.c | 58 ++++++++++++++++++++++----------------------------------
fs/f2fs/inline.c | 3 +++
3 files changed, 29 insertions(+), 40 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index e34b1bd..cf0c9dd 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1573,11 +1573,9 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
int err;

/* we don't need to use inline_data strictly */
- if (f2fs_has_inline_data(inode)) {
- err = f2fs_convert_inline_inode(inode);
- if (err)
- return err;
- }
+ err = f2fs_convert_inline_inode(inode);
+ if (err)
+ return err;

if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode))
return 0;
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 7f8ca47..f2effe1 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -418,19 +418,18 @@ static loff_t f2fs_llseek(struct file *file, loff_t offset, int whence)
static int f2fs_file_mmap(struct file *file, struct vm_area_struct *vma)
{
struct inode *inode = file_inode(file);
+ int err;

if (f2fs_encrypted_inode(inode)) {
- int err = f2fs_get_encryption_info(inode);
+ err = f2fs_get_encryption_info(inode);
if (err)
return 0;
}

/* we don't need to use inline_data strictly */
- if (f2fs_has_inline_data(inode)) {
- int err = f2fs_convert_inline_inode(inode);
- if (err)
- return err;
- }
+ err = f2fs_convert_inline_inode(inode);
+ if (err)
+ return err;

file_accessed(file);
vma->vm_ops = &f2fs_file_vm_ops;
@@ -604,7 +603,7 @@ int f2fs_truncate(struct inode *inode, bool lock)
trace_f2fs_truncate(inode);

/* we should check inline_data size */
- if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
+ if (!f2fs_may_inline_data(inode)) {
err = f2fs_convert_inline_inode(inode);
if (err)
return err;
@@ -688,8 +687,7 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
truncate_setsize(inode, attr->ia_size);

/* should convert inline inode here */
- if (f2fs_has_inline_data(inode) &&
- !f2fs_may_inline_data(inode)) {
+ if (!f2fs_may_inline_data(inode)) {
err = f2fs_convert_inline_inode(inode);
if (err)
return err;
@@ -786,13 +784,11 @@ static int punch_hole(struct inode *inode, loff_t offset, loff_t len)
{
pgoff_t pg_start, pg_end;
loff_t off_start, off_end;
- int ret = 0;
+ int ret;

- if (f2fs_has_inline_data(inode)) {
- ret = f2fs_convert_inline_inode(inode);
- if (ret)
- return ret;
- }
+ ret = f2fs_convert_inline_inode(inode);
+ if (ret)
+ return ret;

pg_start = ((unsigned long long) offset) >> PAGE_CACHE_SHIFT;
pg_end = ((unsigned long long) offset + len) >> PAGE_CACHE_SHIFT;
@@ -951,11 +947,9 @@ static int f2fs_collapse_range(struct inode *inode, loff_t offset, loff_t len)

f2fs_balance_fs(F2FS_I_SB(inode));

- if (f2fs_has_inline_data(inode)) {
- ret = f2fs_convert_inline_inode(inode);
- if (ret)
- return ret;
- }
+ ret = f2fs_convert_inline_inode(inode);
+ if (ret)
+ return ret;

pg_start = offset >> PAGE_CACHE_SHIFT;
pg_end = (offset + len) >> PAGE_CACHE_SHIFT;
@@ -1001,11 +995,9 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,

f2fs_balance_fs(sbi);

- if (f2fs_has_inline_data(inode)) {
- ret = f2fs_convert_inline_inode(inode);
- if (ret)
- return ret;
- }
+ ret = f2fs_convert_inline_inode(inode);
+ if (ret)
+ return ret;

ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
if (ret)
@@ -1114,11 +1106,9 @@ static int f2fs_insert_range(struct inode *inode, loff_t offset, loff_t len)

f2fs_balance_fs(sbi);

- if (f2fs_has_inline_data(inode)) {
- ret = f2fs_convert_inline_inode(inode);
- if (ret)
- return ret;
- }
+ ret = f2fs_convert_inline_inode(inode);
+ if (ret)
+ return ret;

ret = truncate_blocks(inode, i_size_read(inode), true);
if (ret)
@@ -1168,11 +1158,9 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
if (ret)
return ret;

- if (f2fs_has_inline_data(inode)) {
- ret = f2fs_convert_inline_inode(inode);
- if (ret)
- return ret;
- }
+ ret = f2fs_convert_inline_inode(inode);
+ if (ret)
+ return ret;

pg_start = ((unsigned long long) offset) >> PAGE_CACHE_SHIFT;
pg_end = ((unsigned long long) offset + len) >> PAGE_CACHE_SHIFT;
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index bda7126..8090854 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -177,6 +177,9 @@ int f2fs_convert_inline_inode(struct inode *inode)
struct page *ipage, *page;
int err = 0;

+ if (!f2fs_has_inline_data(inode))
+ return 0;
+
page = grab_cache_page(inode->i_mapping, 0);
if (!page)
return -ENOMEM;
--
2.5.4 (Apple Git-61)


2015-12-23 01:00:55

by Jaegeuk Kim

[permalink] [raw]
Subject: [PATCH 2/4] f2fs: avoid unnecessary f2fs_gc for dir operations

The f2fs_balance_fs doesn't need to cover f2fs_new_inode or f2fs_find_entry
works.

Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/namei.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 2c32110..8d2616f 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -128,8 +128,6 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
nid_t ino = 0;
int err;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -142,6 +140,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
inode->i_mapping->a_ops = &f2fs_dblock_aops;
ino = inode->i_ino;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
if (err)
@@ -288,12 +288,13 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
int err = -ENOENT;

trace_f2fs_unlink_enter(dir, dentry);
- f2fs_balance_fs(sbi);

de = f2fs_find_entry(dir, &dentry->d_name, &page);
if (!de)
goto fail;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = acquire_orphan_inode(sbi);
if (err) {
@@ -341,8 +342,6 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
if (len > dir->i_sb->s_blocksize)
return -ENAMETOOLONG;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, S_IFLNK | S_IRWXUGO);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -353,6 +352,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
inode->i_op = &f2fs_symlink_inode_operations;
inode->i_mapping->a_ops = &f2fs_dblock_aops;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
if (err)
@@ -433,8 +434,6 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
struct inode *inode;
int err;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, S_IFDIR | mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -444,6 +443,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
inode->i_mapping->a_ops = &f2fs_dblock_aops;
mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);

+ f2fs_balance_fs(sbi);
+
set_inode_flag(F2FS_I(inode), FI_INC_LINK);
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
--
2.5.4 (Apple Git-61)

2015-12-23 00:59:57

by Jaegeuk Kim

[permalink] [raw]
Subject: [PATCH 3/4] f2fs: record node block allocation in dnode_of_data

This patch introduces recording node block allocation in dnode_of_data.
This information helps to figure out whether any node block is allocated during
specific file operations.

Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/data.c | 1 +
fs/f2fs/f2fs.h | 1 +
fs/f2fs/file.c | 1 +
fs/f2fs/node.c | 4 ++++
4 files changed, 7 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index cf0c9dd..a7a9a05 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -226,6 +226,7 @@ void set_data_blkaddr(struct dnode_of_data *dn)
addr_array = blkaddr_in_node(rn);
addr_array[ofs_in_node] = cpu_to_le32(dn->data_blkaddr);
set_page_dirty(node_page);
+ dn->node_changed = true;
}

int reserve_new_block(struct dnode_of_data *dn)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 90fb970..0f4d329 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -547,6 +547,7 @@ struct dnode_of_data {
unsigned int ofs_in_node; /* data offset in the node page */
bool inode_page_locked; /* inode page is locked or not */
block_t data_blkaddr; /* block address of the node block */
+ bool node_changed; /* is node block changed */
};

static inline void set_new_dnode(struct dnode_of_data *dn, struct inode *inode,
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index f2effe1..10ed357 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -484,6 +484,7 @@ int truncate_data_blocks_range(struct dnode_of_data *dn, int count)
dec_valid_block_count(sbi, dn->inode, nr_free);
set_page_dirty(dn->node_page);
sync_inode_page(dn);
+ dn->node_changed = true;
}
dn->ofs_in_node = ofs;

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 6cc8ac7..ff2acb1 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -542,6 +542,7 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t index, int mode)

set_nid(parent, offset[i - 1], nids[i], i == 1);
alloc_nid_done(sbi, nids[i]);
+ dn->node_changed = true;
done = true;
} else if (mode == LOOKUP_NODE_RA && i == level && level > 1) {
npage[i] = get_node_page_ra(parent, offset[i - 1]);
@@ -678,6 +679,7 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
if (ret < 0)
goto out_err;
set_nid(page, i, 0, false);
+ dn->node_changed = true;
}
} else {
child_nofs = nofs + ofs * (NIDS_PER_BLOCK + 1) + 1;
@@ -691,6 +693,7 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
ret = truncate_nodes(&rdn, child_nofs, 0, depth - 1);
if (ret == (NIDS_PER_BLOCK + 1)) {
set_nid(page, i, 0, false);
+ dn->node_changed = true;
child_nofs += ret;
} else if (ret < 0 && ret != -ENOENT) {
goto out_err;
@@ -752,6 +755,7 @@ static int truncate_partial_nodes(struct dnode_of_data *dn,
if (err < 0)
goto fail;
set_nid(pages[idx], i, 0, false);
+ dn->node_changed = true;
}

if (offset[idx + 1] == 0) {
--
2.5.4 (Apple Git-61)

2015-12-23 01:00:27

by Jaegeuk Kim

[permalink] [raw]
Subject: [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed

If user tries to update or read data, we don't need to call f2fs_balance_fs
which triggers f2fs_gc, which increases unnecessary long latency.

Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/data.c | 19 ++++++++++++++++---
fs/f2fs/file.c | 26 +++++++++-----------------
fs/f2fs/inline.c | 4 ++++
3 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index a7a9a05..8f8f8b0 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -509,7 +509,6 @@ static void __allocate_data_blocks(struct inode *inode, loff_t offset,
u64 end_offset;

while (len) {
- f2fs_balance_fs(sbi);
f2fs_lock_op(sbi);

/* When reading holes, we need its node page */
@@ -542,6 +541,9 @@ static void __allocate_data_blocks(struct inode *inode, loff_t offset,

f2fs_put_dnode(&dn);
f2fs_unlock_op(sbi);
+
+ if (dn.node_changed)
+ f2fs_balance_fs(sbi);
}
return;

@@ -551,6 +553,8 @@ sync_out:
f2fs_put_dnode(&dn);
out:
f2fs_unlock_op(sbi);
+ if (dn.node_changed)
+ f2fs_balance_fs(sbi);
return;
}

@@ -1410,8 +1414,6 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,

trace_f2fs_write_begin(inode, pos, len, flags);

- f2fs_balance_fs(sbi);
-
/*
* We should check this at this moment to avoid deadlock on inode page
* and #0 page. The locking rule for inline_data conversion should be:
@@ -1461,6 +1463,17 @@ put_next:
f2fs_put_dnode(&dn);
f2fs_unlock_op(sbi);

+ if (dn.node_changed && has_not_enough_free_secs(sbi, 0)) {
+ unlock_page(page);
+ f2fs_balance_fs(sbi);
+ lock_page(page);
+ if (page->mapping != mapping) {
+ /* The page got truncated from under us */
+ f2fs_put_page(page, 1);
+ goto repeat;
+ }
+ }
+
f2fs_wait_on_page_writeback(page, DATA);

/* wait for GCed encrypted page writeback */
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 10ed357..dbc08bb 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -40,8 +40,6 @@ static int f2fs_vm_page_mkwrite(struct vm_area_struct *vma,
struct dnode_of_data dn;
int err;

- f2fs_balance_fs(sbi);
-
sb_start_pagefault(inode->i_sb);

f2fs_bug_on(sbi, f2fs_has_inline_data(inode));
@@ -57,6 +55,9 @@ static int f2fs_vm_page_mkwrite(struct vm_area_struct *vma,
f2fs_put_dnode(&dn);
f2fs_unlock_op(sbi);

+ if (dn.node_changed)
+ f2fs_balance_fs(sbi);
+
file_update_time(vma->vm_file);
lock_page(page);
if (unlikely(page->mapping != inode->i_mapping ||
@@ -233,9 +234,6 @@ int f2fs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
goto out;
}
go_write:
- /* guarantee free sections for fsync */
- f2fs_balance_fs(sbi);
-
/*
* Both of fdatasync() and fsync() are able to be recovered from
* sudden-power-off.
@@ -267,6 +265,8 @@ sync_nodes:
if (need_inode_block_update(sbi, ino)) {
mark_inode_dirty_sync(inode);
f2fs_write_inode(inode, NULL);
+
+ f2fs_balance_fs(sbi);
goto sync_nodes;
}

@@ -946,8 +946,6 @@ static int f2fs_collapse_range(struct inode *inode, loff_t offset, loff_t len)
if (offset & (F2FS_BLKSIZE - 1) || len & (F2FS_BLKSIZE - 1))
return -EINVAL;

- f2fs_balance_fs(F2FS_I_SB(inode));
-
ret = f2fs_convert_inline_inode(inode);
if (ret)
return ret;
@@ -994,8 +992,6 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
if (ret)
return ret;

- f2fs_balance_fs(sbi);
-
ret = f2fs_convert_inline_inode(inode);
if (ret)
return ret;
@@ -1105,12 +1101,12 @@ static int f2fs_insert_range(struct inode *inode, loff_t offset, loff_t len)
if (offset & (F2FS_BLKSIZE - 1) || len & (F2FS_BLKSIZE - 1))
return -EINVAL;

- f2fs_balance_fs(sbi);
-
ret = f2fs_convert_inline_inode(inode);
if (ret)
return ret;

+ f2fs_balance_fs(sbi);
+
ret = truncate_blocks(inode, i_size_read(inode), true);
if (ret)
return ret;
@@ -1153,8 +1149,6 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
loff_t off_start, off_end;
int ret = 0;

- f2fs_balance_fs(sbi);
-
ret = inode_newsize_ok(inode, (len + offset));
if (ret)
return ret;
@@ -1163,6 +1157,8 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
if (ret)
return ret;

+ f2fs_balance_fs(sbi);
+
pg_start = ((unsigned long long) offset) >> PAGE_CACHE_SHIFT;
pg_end = ((unsigned long long) offset + len) >> PAGE_CACHE_SHIFT;

@@ -1350,8 +1346,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
if (!inode_owner_or_capable(inode))
return -EACCES;

- f2fs_balance_fs(F2FS_I_SB(inode));
-
if (f2fs_is_atomic_file(inode))
return 0;

@@ -1438,8 +1432,6 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
if (ret)
return ret;

- f2fs_balance_fs(F2FS_I_SB(inode));
-
clear_inode_flag(F2FS_I(inode), FI_ATOMIC_FILE);
clear_inode_flag(F2FS_I(inode), FI_VOLATILE_FILE);
commit_inmem_pages(inode, true);
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 8090854..c24e5d9 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -202,6 +202,10 @@ out:
f2fs_unlock_op(sbi);

f2fs_put_page(page, 1);
+
+ if (dn.node_changed)
+ f2fs_balance_fs(sbi);
+
return err;
}

--
2.5.4 (Apple Git-61)

2015-12-23 03:20:50

by Chao Yu

[permalink] [raw]
Subject: RE: [f2fs-dev] [PATCH 1/4] f2fs: check inline_data flag at converting time

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Wednesday, December 23, 2015 9:00 AM
> To: [email protected]; [email protected];
> [email protected]
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 1/4] f2fs: check inline_data flag at converting time
>
> We can check inode's inline_data flag when calling to convert it.
>
> Signed-off-by: Jaegeuk Kim <[email protected]>

Reviewed-by: Chao Yu <[email protected]>

2015-12-23 03:27:12

by Chao Yu

[permalink] [raw]
Subject: RE: [f2fs-dev] [PATCH 2/4] f2fs: avoid unnecessary f2fs_gc for dir operations

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Wednesday, December 23, 2015 9:00 AM
> To: [email protected]; [email protected];
> [email protected]
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 2/4] f2fs: avoid unnecessary f2fs_gc for dir operations
>
> The f2fs_balance_fs doesn't need to cover f2fs_new_inode or f2fs_find_entry
> works.

So this avoids some unneeded overhead in f2fs_balance_fs for scenario where
f2fs_new_inode and f2fs_find_entry will fail for most of the time?

Shouldn't cover f2fs_find_entry in rename or rename2 case?

Thanks,

>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> fs/f2fs/namei.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 2c32110..8d2616f 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -128,8 +128,6 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t
> mode,
> nid_t ino = 0;
> int err;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, mode);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -142,6 +140,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t
> mode,
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> ino = inode->i_ino;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> if (err)
> @@ -288,12 +288,13 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
> int err = -ENOENT;
>
> trace_f2fs_unlink_enter(dir, dentry);
> - f2fs_balance_fs(sbi);
>
> de = f2fs_find_entry(dir, &dentry->d_name, &page);
> if (!de)
> goto fail;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = acquire_orphan_inode(sbi);
> if (err) {
> @@ -341,8 +342,6 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> if (len > dir->i_sb->s_blocksize)
> return -ENAMETOOLONG;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, S_IFLNK | S_IRWXUGO);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -353,6 +352,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> inode->i_op = &f2fs_symlink_inode_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> if (err)
> @@ -433,8 +434,6 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t
> mode)
> struct inode *inode;
> int err;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, S_IFDIR | mode);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -444,6 +443,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t
> mode)
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);
>
> + f2fs_balance_fs(sbi);
> +
> set_inode_flag(F2FS_I(inode), FI_INC_LINK);
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> --
> 2.5.4 (Apple Git-61)
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2015-12-23 08:01:30

by Chao Yu

[permalink] [raw]
Subject: RE: [f2fs-dev] [PATCH 3/4] f2fs: record node block allocation in dnode_of_data

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Wednesday, December 23, 2015 9:00 AM
> To: [email protected]; [email protected];
> [email protected]
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 3/4] f2fs: record node block allocation in dnode_of_data
>
> This patch introduces recording node block allocation in dnode_of_data.
> This information helps to figure out whether any node block is allocated during
> specific file operations.
>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> fs/f2fs/data.c | 1 +
> fs/f2fs/f2fs.h | 1 +
> fs/f2fs/file.c | 1 +
> fs/f2fs/node.c | 4 ++++
> 4 files changed, 7 insertions(+)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index cf0c9dd..a7a9a05 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -226,6 +226,7 @@ void set_data_blkaddr(struct dnode_of_data *dn)
> addr_array = blkaddr_in_node(rn);
> addr_array[ofs_in_node] = cpu_to_le32(dn->data_blkaddr);
> set_page_dirty(node_page);
> + dn->node_changed = true;
> }
>
> int reserve_new_block(struct dnode_of_data *dn)
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 90fb970..0f4d329 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -547,6 +547,7 @@ struct dnode_of_data {
> unsigned int ofs_in_node; /* data offset in the node page */
> bool inode_page_locked; /* inode page is locked or not */

Better to add node_changed here to avoid holes generated by compiler due
to alignment.

> block_t data_blkaddr; /* block address of the node block */
> + bool node_changed; /* is node block changed */

How about reset it in set_new_dnode?

> };
>
> static inline void set_new_dnode(struct dnode_of_data *dn, struct inode *inode,
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index f2effe1..10ed357 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -484,6 +484,7 @@ int truncate_data_blocks_range(struct dnode_of_data *dn, int count)
> dec_valid_block_count(sbi, dn->inode, nr_free);
> set_page_dirty(dn->node_page);
> sync_inode_page(dn);
> + dn->node_changed = true;

dn->node_changed should have been set in set_data_blkaddr, so no needed to
set here.

Thanks,

> }
> dn->ofs_in_node = ofs;
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 6cc8ac7..ff2acb1 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -542,6 +542,7 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t index, int mode)
>
> set_nid(parent, offset[i - 1], nids[i], i == 1);
> alloc_nid_done(sbi, nids[i]);
> + dn->node_changed = true;
> done = true;
> } else if (mode == LOOKUP_NODE_RA && i == level && level > 1) {
> npage[i] = get_node_page_ra(parent, offset[i - 1]);
> @@ -678,6 +679,7 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
> if (ret < 0)
> goto out_err;
> set_nid(page, i, 0, false);
> + dn->node_changed = true;
> }
> } else {
> child_nofs = nofs + ofs * (NIDS_PER_BLOCK + 1) + 1;
> @@ -691,6 +693,7 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
> ret = truncate_nodes(&rdn, child_nofs, 0, depth - 1);
> if (ret == (NIDS_PER_BLOCK + 1)) {
> set_nid(page, i, 0, false);
> + dn->node_changed = true;
> child_nofs += ret;
> } else if (ret < 0 && ret != -ENOENT) {
> goto out_err;
> @@ -752,6 +755,7 @@ static int truncate_partial_nodes(struct dnode_of_data *dn,
> if (err < 0)
> goto fail;
> set_nid(pages[idx], i, 0, false);
> + dn->node_changed = true;
> }
>
> if (offset[idx + 1] == 0) {
> --
> 2.5.4 (Apple Git-61)
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2015-12-23 09:48:43

by Chao Yu

[permalink] [raw]
Subject: RE: [f2fs-dev] [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Wednesday, December 23, 2015 9:00 AM
> To: [email protected]; [email protected];
> [email protected]
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed
>
> If user tries to update or read data, we don't need to call f2fs_balance_fs
> which triggers f2fs_gc, which increases unnecessary long latency.

One missing case is get_data_block_dio, how about also covering it based on
following patch?


>From 7175efac7473e7a04285055c69edfb7432f8ca4e Mon Sep 17 00:00:00 2001
From: Chao Yu <[email protected]>
Date: Wed, 23 Dec 2015 17:11:43 +0800
Subject: [PATCH] f2fs: reduce covered region of sbi->cp_rwsem in
f2fs_map_blocks

Only cover sbi->cp_rwsem on one dnode page's allocation and modification
instead of multiple's in f2fs_map_blocks, it can reduce the covered region
of cp_rwsem, then we can avoid potential long time delay for concurrent
checkpointer.

Signed-off-by: Chao Yu <[email protected]>
---
fs/f2fs/data.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 8f8f8b0..3c83b16 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -594,7 +594,7 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
}

if (create)
- f2fs_lock_op(F2FS_I_SB(inode));
+ f2fs_lock_op(sbi);

/* When reading holes, we need its node page */
set_new_dnode(&dn, inode, NULL, NULL, 0);
@@ -651,6 +651,11 @@ get_next:
allocated = false;
f2fs_put_dnode(&dn);

+ if (create) {
+ f2fs_unlock_op(sbi);
+ f2fs_lock_op(sbi);
+ }
+
set_new_dnode(&dn, inode, NULL, NULL, 0);
err = get_dnode_of_data(&dn, pgofs, mode);
if (err) {
@@ -706,7 +711,7 @@ put_out:
f2fs_put_dnode(&dn);
unlock_out:
if (create)
- f2fs_unlock_op(F2FS_I_SB(inode));
+ f2fs_unlock_op(sbi);
out:
trace_f2fs_map_blocks(inode, map, err);
return err;
--
2.6.3

2015-12-23 18:54:56

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH 2/4 v2] f2fs: avoid unnecessary f2fs_gc for dir operations

Chang log from v1:
- add more cases

>From 9fea6346f5dd2992525e853e27a4fe899d122379 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Tue, 22 Dec 2015 11:56:08 -0800
Subject: [PATCH] f2fs: avoid unnecessary f2fs_gc for dir operations

The f2fs_balance_fs doesn't need to cover f2fs_new_inode or f2fs_find_entry
works.

Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/namei.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 2c32110..e4588f7 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -128,8 +128,6 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
nid_t ino = 0;
int err;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -142,6 +140,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
inode->i_mapping->a_ops = &f2fs_dblock_aops;
ino = inode->i_ino;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
if (err)
@@ -288,12 +288,13 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
int err = -ENOENT;

trace_f2fs_unlink_enter(dir, dentry);
- f2fs_balance_fs(sbi);

de = f2fs_find_entry(dir, &dentry->d_name, &page);
if (!de)
goto fail;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = acquire_orphan_inode(sbi);
if (err) {
@@ -341,8 +342,6 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
if (len > dir->i_sb->s_blocksize)
return -ENAMETOOLONG;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, S_IFLNK | S_IRWXUGO);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -353,6 +352,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
inode->i_op = &f2fs_symlink_inode_operations;
inode->i_mapping->a_ops = &f2fs_dblock_aops;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
if (err)
@@ -433,8 +434,6 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
struct inode *inode;
int err;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, S_IFDIR | mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -444,6 +443,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
inode->i_mapping->a_ops = &f2fs_dblock_aops;
mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);

+ f2fs_balance_fs(sbi);
+
set_inode_flag(F2FS_I(inode), FI_INC_LINK);
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
@@ -481,8 +482,6 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
struct inode *inode;
int err = 0;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -490,6 +489,8 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
init_special_inode(inode, inode->i_mode, rdev);
inode->i_op = &f2fs_special_inode_operations;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
if (err)
@@ -516,9 +517,6 @@ static int __f2fs_tmpfile(struct inode *dir, struct dentry *dentry,
struct inode *inode;
int err;

- if (!whiteout)
- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -530,6 +528,8 @@ static int __f2fs_tmpfile(struct inode *dir, struct dentry *dentry,
inode->i_op = &f2fs_file_inode_operations;
inode->i_fop = &f2fs_file_operations;
inode->i_mapping->a_ops = &f2fs_dblock_aops;
+
+ f2fs_balance_fs(sbi);
}

f2fs_lock_op(sbi);
@@ -604,8 +604,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
goto out;
}

- f2fs_balance_fs(sbi);
-
old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
if (!old_entry)
goto out;
@@ -635,6 +633,8 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
if (!new_entry)
goto out_whiteout;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);

err = acquire_orphan_inode(sbi);
@@ -666,6 +666,8 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
update_inode_page(old_inode);
update_inode_page(new_inode);
} else {
+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);

err = f2fs_add_link(new_dentry, old_inode);
@@ -763,8 +765,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
new_inode)))
return -EPERM;

- f2fs_balance_fs(sbi);
-
old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
if (!old_entry)
goto out;
@@ -807,6 +807,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
goto out_new_dir;
}

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);

err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
--
2.5.4 (Apple Git-61)

2015-12-23 18:57:31

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: record node block allocation in dnode_of_data

Hi Chao,

On Wed, Dec 23, 2015 at 04:00:36PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:[email protected]]
> > Sent: Wednesday, December 23, 2015 9:00 AM
> > To: [email protected]; [email protected];
> > [email protected]
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 3/4] f2fs: record node block allocation in dnode_of_data
> >
> > This patch introduces recording node block allocation in dnode_of_data.
> > This information helps to figure out whether any node block is allocated during
> > specific file operations.
> >
> > Signed-off-by: Jaegeuk Kim <[email protected]>
> > ---
> > fs/f2fs/data.c | 1 +
> > fs/f2fs/f2fs.h | 1 +
> > fs/f2fs/file.c | 1 +
> > fs/f2fs/node.c | 4 ++++
> > 4 files changed, 7 insertions(+)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index cf0c9dd..a7a9a05 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -226,6 +226,7 @@ void set_data_blkaddr(struct dnode_of_data *dn)
> > addr_array = blkaddr_in_node(rn);
> > addr_array[ofs_in_node] = cpu_to_le32(dn->data_blkaddr);
> > set_page_dirty(node_page);
> > + dn->node_changed = true;
> > }
> >
> > int reserve_new_block(struct dnode_of_data *dn)
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 90fb970..0f4d329 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -547,6 +547,7 @@ struct dnode_of_data {
> > unsigned int ofs_in_node; /* data offset in the node page */
> > bool inode_page_locked; /* inode page is locked or not */
>
> Better to add node_changed here to avoid holes generated by compiler due
> to alignment.

Ok.

>
> > block_t data_blkaddr; /* block address of the node block */
> > + bool node_changed; /* is node block changed */
>
> How about reset it in set_new_dnode?

In set_new_dnode(), memset() is called.

>
> > };
> >
> > static inline void set_new_dnode(struct dnode_of_data *dn, struct inode *inode,
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index f2effe1..10ed357 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -484,6 +484,7 @@ int truncate_data_blocks_range(struct dnode_of_data *dn, int count)
> > dec_valid_block_count(sbi, dn->inode, nr_free);
> > set_page_dirty(dn->node_page);
> > sync_inode_page(dn);
> > + dn->node_changed = true;
>
> dn->node_changed should have been set in set_data_blkaddr, so no needed to
> set here.

Right. :)

Thanks,

>
> Thanks,
>
> > }
> > dn->ofs_in_node = ofs;
> >
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index 6cc8ac7..ff2acb1 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -542,6 +542,7 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t index, int mode)
> >
> > set_nid(parent, offset[i - 1], nids[i], i == 1);
> > alloc_nid_done(sbi, nids[i]);
> > + dn->node_changed = true;
> > done = true;
> > } else if (mode == LOOKUP_NODE_RA && i == level && level > 1) {
> > npage[i] = get_node_page_ra(parent, offset[i - 1]);
> > @@ -678,6 +679,7 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
> > if (ret < 0)
> > goto out_err;
> > set_nid(page, i, 0, false);
> > + dn->node_changed = true;
> > }
> > } else {
> > child_nofs = nofs + ofs * (NIDS_PER_BLOCK + 1) + 1;
> > @@ -691,6 +693,7 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
> > ret = truncate_nodes(&rdn, child_nofs, 0, depth - 1);
> > if (ret == (NIDS_PER_BLOCK + 1)) {
> > set_nid(page, i, 0, false);
> > + dn->node_changed = true;
> > child_nofs += ret;
> > } else if (ret < 0 && ret != -ENOENT) {
> > goto out_err;
> > @@ -752,6 +755,7 @@ static int truncate_partial_nodes(struct dnode_of_data *dn,
> > if (err < 0)
> > goto fail;
> > set_nid(pages[idx], i, 0, false);
> > + dn->node_changed = true;
> > }
> >
> > if (offset[idx + 1] == 0) {
> > --
> > 2.5.4 (Apple Git-61)
> >
> >
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2015-12-23 19:00:43

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH 3/4 v2] f2fs: record node block allocation in dnode_of_data

Change log from v1:
- remove redundant set
- adjust memory alignment

>From 60c6a898094535e850268dd77701255a37cce3d3 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Tue, 22 Dec 2015 12:59:54 -0800
Subject: [PATCH] f2fs: record node block allocation in dnode_of_data

This patch introduces recording node block allocation in dnode_of_data.
This information helps to figure out whether any node block is allocated during
specific file operations.

Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/data.c | 1 +
fs/f2fs/f2fs.h | 1 +
fs/f2fs/node.c | 4 ++++
3 files changed, 6 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index cf0c9dd..a7a9a05 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -226,6 +226,7 @@ void set_data_blkaddr(struct dnode_of_data *dn)
addr_array = blkaddr_in_node(rn);
addr_array[ofs_in_node] = cpu_to_le32(dn->data_blkaddr);
set_page_dirty(node_page);
+ dn->node_changed = true;
}

int reserve_new_block(struct dnode_of_data *dn)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 90fb970..3e4a60d 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -546,6 +546,7 @@ struct dnode_of_data {
nid_t nid; /* node id of the direct node block */
unsigned int ofs_in_node; /* data offset in the node page */
bool inode_page_locked; /* inode page is locked or not */
+ bool node_changed; /* is node block changed */
block_t data_blkaddr; /* block address of the node block */
};

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 6cc8ac7..ff2acb1 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -542,6 +542,7 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t index, int mode)

set_nid(parent, offset[i - 1], nids[i], i == 1);
alloc_nid_done(sbi, nids[i]);
+ dn->node_changed = true;
done = true;
} else if (mode == LOOKUP_NODE_RA && i == level && level > 1) {
npage[i] = get_node_page_ra(parent, offset[i - 1]);
@@ -678,6 +679,7 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
if (ret < 0)
goto out_err;
set_nid(page, i, 0, false);
+ dn->node_changed = true;
}
} else {
child_nofs = nofs + ofs * (NIDS_PER_BLOCK + 1) + 1;
@@ -691,6 +693,7 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
ret = truncate_nodes(&rdn, child_nofs, 0, depth - 1);
if (ret == (NIDS_PER_BLOCK + 1)) {
set_nid(page, i, 0, false);
+ dn->node_changed = true;
child_nofs += ret;
} else if (ret < 0 && ret != -ENOENT) {
goto out_err;
@@ -752,6 +755,7 @@ static int truncate_partial_nodes(struct dnode_of_data *dn,
if (err < 0)
goto fail;
set_nid(pages[idx], i, 0, false);
+ dn->node_changed = true;
}

if (offset[idx + 1] == 0) {
--
2.5.4 (Apple Git-61)

2015-12-23 19:13:44

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed

Hi Chao,

On Wed, Dec 23, 2015 at 05:46:59PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:[email protected]]
> > Sent: Wednesday, December 23, 2015 9:00 AM
> > To: [email protected]; [email protected];
> > [email protected]
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed
> >
> > If user tries to update or read data, we don't need to call f2fs_balance_fs
> > which triggers f2fs_gc, which increases unnecessary long latency.
>
> One missing case is get_data_block_dio, how about also covering it based on
> following patch?

It makes sense.
I'll submit v2.

Thanks,

>
>
> >From 7175efac7473e7a04285055c69edfb7432f8ca4e Mon Sep 17 00:00:00 2001
> From: Chao Yu <[email protected]>
> Date: Wed, 23 Dec 2015 17:11:43 +0800
> Subject: [PATCH] f2fs: reduce covered region of sbi->cp_rwsem in
> f2fs_map_blocks
>
> Only cover sbi->cp_rwsem on one dnode page's allocation and modification
> instead of multiple's in f2fs_map_blocks, it can reduce the covered region
> of cp_rwsem, then we can avoid potential long time delay for concurrent
> checkpointer.
>
> Signed-off-by: Chao Yu <[email protected]>
> ---
> fs/f2fs/data.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 8f8f8b0..3c83b16 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -594,7 +594,7 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
> }
>
> if (create)
> - f2fs_lock_op(F2FS_I_SB(inode));
> + f2fs_lock_op(sbi);
>
> /* When reading holes, we need its node page */
> set_new_dnode(&dn, inode, NULL, NULL, 0);
> @@ -651,6 +651,11 @@ get_next:
> allocated = false;
> f2fs_put_dnode(&dn);
>
> + if (create) {
> + f2fs_unlock_op(sbi);
> + f2fs_lock_op(sbi);
> + }
> +
> set_new_dnode(&dn, inode, NULL, NULL, 0);
> err = get_dnode_of_data(&dn, pgofs, mode);
> if (err) {
> @@ -706,7 +711,7 @@ put_out:
> f2fs_put_dnode(&dn);
> unlock_out:
> if (create)
> - f2fs_unlock_op(F2FS_I_SB(inode));
> + f2fs_unlock_op(sbi);
> out:
> trace_f2fs_map_blocks(inode, map, err);
> return err;
> --
> 2.6.3
>

2015-12-23 19:14:33

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed

Change log v2:
- add dio case

>From c2d16a526371954671f9c8cff5f09f9d230f7993 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Tue, 22 Dec 2015 13:23:35 -0800
Subject: [PATCH] f2fs: call f2fs_balance_fs only when node was changed

If user tries to update or read data, we don't need to call f2fs_balance_fs
which triggers f2fs_gc, which increases unnecessary long latency.

Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/data.c | 26 ++++++++++++++++++++++----
fs/f2fs/file.c | 26 +++++++++-----------------
fs/f2fs/inline.c | 4 ++++
3 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 82ecaa30..958d826 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -509,7 +509,6 @@ static void __allocate_data_blocks(struct inode *inode, loff_t offset,
u64 end_offset;

while (len) {
- f2fs_balance_fs(sbi);
f2fs_lock_op(sbi);

/* When reading holes, we need its node page */
@@ -542,6 +541,9 @@ static void __allocate_data_blocks(struct inode *inode, loff_t offset,

f2fs_put_dnode(&dn);
f2fs_unlock_op(sbi);
+
+ if (dn.node_changed)
+ f2fs_balance_fs(sbi);
}
return;

@@ -551,6 +553,8 @@ sync_out:
f2fs_put_dnode(&dn);
out:
f2fs_unlock_op(sbi);
+ if (dn.node_changed)
+ f2fs_balance_fs(sbi);
return;
}

@@ -649,6 +653,8 @@ get_next:

if (create) {
f2fs_unlock_op(sbi);
+ if (dn.node_changed)
+ f2fs_balance_fs(sbi);
f2fs_lock_op(sbi);
}

@@ -706,8 +712,11 @@ sync_out:
put_out:
f2fs_put_dnode(&dn);
unlock_out:
- if (create)
+ if (create) {
f2fs_unlock_op(sbi);
+ if (dn.node_changed)
+ f2fs_balance_fs(sbi);
+ }
out:
trace_f2fs_map_blocks(inode, map, err);
return err;
@@ -1415,8 +1424,6 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,

trace_f2fs_write_begin(inode, pos, len, flags);

- f2fs_balance_fs(sbi);
-
/*
* We should check this at this moment to avoid deadlock on inode page
* and #0 page. The locking rule for inline_data conversion should be:
@@ -1466,6 +1473,17 @@ put_next:
f2fs_put_dnode(&dn);
f2fs_unlock_op(sbi);

+ if (dn.node_changed && has_not_enough_free_secs(sbi, 0)) {
+ unlock_page(page);
+ f2fs_balance_fs(sbi);
+ lock_page(page);
+ if (page->mapping != mapping) {
+ /* The page got truncated from under us */
+ f2fs_put_page(page, 1);
+ goto repeat;
+ }
+ }
+
f2fs_wait_on_page_writeback(page, DATA);

/* wait for GCed encrypted page writeback */
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index f2effe1..888ce47 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -40,8 +40,6 @@ static int f2fs_vm_page_mkwrite(struct vm_area_struct *vma,
struct dnode_of_data dn;
int err;

- f2fs_balance_fs(sbi);
-
sb_start_pagefault(inode->i_sb);

f2fs_bug_on(sbi, f2fs_has_inline_data(inode));
@@ -57,6 +55,9 @@ static int f2fs_vm_page_mkwrite(struct vm_area_struct *vma,
f2fs_put_dnode(&dn);
f2fs_unlock_op(sbi);

+ if (dn.node_changed)
+ f2fs_balance_fs(sbi);
+
file_update_time(vma->vm_file);
lock_page(page);
if (unlikely(page->mapping != inode->i_mapping ||
@@ -233,9 +234,6 @@ int f2fs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
goto out;
}
go_write:
- /* guarantee free sections for fsync */
- f2fs_balance_fs(sbi);
-
/*
* Both of fdatasync() and fsync() are able to be recovered from
* sudden-power-off.
@@ -267,6 +265,8 @@ sync_nodes:
if (need_inode_block_update(sbi, ino)) {
mark_inode_dirty_sync(inode);
f2fs_write_inode(inode, NULL);
+
+ f2fs_balance_fs(sbi);
goto sync_nodes;
}

@@ -945,8 +945,6 @@ static int f2fs_collapse_range(struct inode *inode, loff_t offset, loff_t len)
if (offset & (F2FS_BLKSIZE - 1) || len & (F2FS_BLKSIZE - 1))
return -EINVAL;

- f2fs_balance_fs(F2FS_I_SB(inode));
-
ret = f2fs_convert_inline_inode(inode);
if (ret)
return ret;
@@ -993,8 +991,6 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
if (ret)
return ret;

- f2fs_balance_fs(sbi);
-
ret = f2fs_convert_inline_inode(inode);
if (ret)
return ret;
@@ -1104,12 +1100,12 @@ static int f2fs_insert_range(struct inode *inode, loff_t offset, loff_t len)
if (offset & (F2FS_BLKSIZE - 1) || len & (F2FS_BLKSIZE - 1))
return -EINVAL;

- f2fs_balance_fs(sbi);
-
ret = f2fs_convert_inline_inode(inode);
if (ret)
return ret;

+ f2fs_balance_fs(sbi);
+
ret = truncate_blocks(inode, i_size_read(inode), true);
if (ret)
return ret;
@@ -1152,8 +1148,6 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
loff_t off_start, off_end;
int ret = 0;

- f2fs_balance_fs(sbi);
-
ret = inode_newsize_ok(inode, (len + offset));
if (ret)
return ret;
@@ -1162,6 +1156,8 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
if (ret)
return ret;

+ f2fs_balance_fs(sbi);
+
pg_start = ((unsigned long long) offset) >> PAGE_CACHE_SHIFT;
pg_end = ((unsigned long long) offset + len) >> PAGE_CACHE_SHIFT;

@@ -1349,8 +1345,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
if (!inode_owner_or_capable(inode))
return -EACCES;

- f2fs_balance_fs(F2FS_I_SB(inode));
-
if (f2fs_is_atomic_file(inode))
return 0;

@@ -1437,8 +1431,6 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
if (ret)
return ret;

- f2fs_balance_fs(F2FS_I_SB(inode));
-
clear_inode_flag(F2FS_I(inode), FI_ATOMIC_FILE);
clear_inode_flag(F2FS_I(inode), FI_VOLATILE_FILE);
commit_inmem_pages(inode, true);
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 8090854..c24e5d9 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -202,6 +202,10 @@ out:
f2fs_unlock_op(sbi);

f2fs_put_page(page, 1);
+
+ if (dn.node_changed)
+ f2fs_balance_fs(sbi);
+
return err;
}

--
2.5.4 (Apple Git-61)

2015-12-24 01:34:07

by Chao Yu

[permalink] [raw]
Subject: RE: [f2fs-dev] [PATCH 2/4 v2] f2fs: avoid unnecessary f2fs_gc for dir operations

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Thursday, December 24, 2015 2:55 AM
> To: Chao Yu
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [f2fs-dev] [PATCH 2/4 v2] f2fs: avoid unnecessary f2fs_gc for dir operations
>
> Chang log from v1:
> - add more cases
>
> From 9fea6346f5dd2992525e853e27a4fe899d122379 Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <[email protected]>
> Date: Tue, 22 Dec 2015 11:56:08 -0800
> Subject: [PATCH] f2fs: avoid unnecessary f2fs_gc for dir operations
>
> The f2fs_balance_fs doesn't need to cover f2fs_new_inode or f2fs_find_entry
> works.
>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> fs/f2fs/namei.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 2c32110..e4588f7 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -128,8 +128,6 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t
> mode,
> nid_t ino = 0;
> int err;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, mode);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -142,6 +140,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t
> mode,
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> ino = inode->i_ino;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> if (err)
> @@ -288,12 +288,13 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
> int err = -ENOENT;
>
> trace_f2fs_unlink_enter(dir, dentry);
> - f2fs_balance_fs(sbi);
>
> de = f2fs_find_entry(dir, &dentry->d_name, &page);
> if (!de)
> goto fail;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = acquire_orphan_inode(sbi);
> if (err) {
> @@ -341,8 +342,6 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> if (len > dir->i_sb->s_blocksize)
> return -ENAMETOOLONG;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, S_IFLNK | S_IRWXUGO);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -353,6 +352,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> inode->i_op = &f2fs_symlink_inode_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> if (err)
> @@ -433,8 +434,6 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t
> mode)
> struct inode *inode;
> int err;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, S_IFDIR | mode);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -444,6 +443,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t
> mode)
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);
>
> + f2fs_balance_fs(sbi);
> +
> set_inode_flag(F2FS_I(inode), FI_INC_LINK);
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> @@ -481,8 +482,6 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
> struct inode *inode;
> int err = 0;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, mode);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -490,6 +489,8 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
> init_special_inode(inode, inode->i_mode, rdev);
> inode->i_op = &f2fs_special_inode_operations;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> if (err)
> @@ -516,9 +517,6 @@ static int __f2fs_tmpfile(struct inode *dir, struct dentry *dentry,
> struct inode *inode;
> int err;
>
> - if (!whiteout)
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, mode);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -530,6 +528,8 @@ static int __f2fs_tmpfile(struct inode *dir, struct dentry *dentry,
> inode->i_op = &f2fs_file_inode_operations;
> inode->i_fop = &f2fs_file_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> +
> + f2fs_balance_fs(sbi);
> }
>
> f2fs_lock_op(sbi);
> @@ -604,8 +604,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> goto out;
> }
>
> - f2fs_balance_fs(sbi);
> -
> old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
> if (!old_entry)
> goto out;
> @@ -635,6 +633,8 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> if (!new_entry)
> goto out_whiteout;
>
> + f2fs_balance_fs(sbi);

If we are in RENAME_WHITEOUT case, move f2fs_balance_fs here seems a bit late.
How about let __f2fs_tmpfile do reclaim for both whiteout and tmpfile case
itself, and do reclaim exclude whiteout case here?

Thanks,

> +
> f2fs_lock_op(sbi);
>
> err = acquire_orphan_inode(sbi);
> @@ -666,6 +666,8 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> update_inode_page(old_inode);
> update_inode_page(new_inode);
> } else {
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
>
> err = f2fs_add_link(new_dentry, old_inode);
> @@ -763,8 +765,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry
> *old_dentry,
> new_inode)))
> return -EPERM;
>
> - f2fs_balance_fs(sbi);
> -
> old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
> if (!old_entry)
> goto out;
> @@ -807,6 +807,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry
> *old_dentry,
> goto out_new_dir;
> }
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
>
> err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
> --
> 2.5.4 (Apple Git-61)

2015-12-24 01:35:47

by Chao Yu

[permalink] [raw]
Subject: RE: [f2fs-dev] [PATCH 3/4 v2] f2fs: record node block allocation in dnode_of_data

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Thursday, December 24, 2015 3:00 AM
> To: [email protected]; [email protected];
> [email protected]
> Subject: Re: [f2fs-dev] [PATCH 3/4 v2] f2fs: record node block allocation in dnode_of_data
>
> Change log from v1:
> - remove redundant set
> - adjust memory alignment
>
> >From 60c6a898094535e850268dd77701255a37cce3d3 Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <[email protected]>
> Date: Tue, 22 Dec 2015 12:59:54 -0800
> Subject: [PATCH] f2fs: record node block allocation in dnode_of_data
>
> This patch introduces recording node block allocation in dnode_of_data.
> This information helps to figure out whether any node block is allocated during
> specific file operations.
>
> Signed-off-by: Jaegeuk Kim <[email protected]>

Reviewed-by: Chao Yu <[email protected]>

2015-12-24 02:13:16

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH 2/4 v2] f2fs: avoid unnecessary f2fs_gc for dir operations

Hi Chao,

Right.
But, in the rename path, we still need to do f2fs_balance_fs, since it produces
another dirty node page in the mean time.

How about this?

>From bbc5bf8f6c940cd75a4d71ce40ce4bd3f647a823 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Tue, 22 Dec 2015 11:56:08 -0800
Subject: [PATCH] f2fs: avoid unnecessary f2fs_gc for dir operations

The f2fs_balance_fs doesn't need to cover f2fs_new_inode or f2fs_find_entry
works.

Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/namei.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 2c32110..4e27c5c 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -128,8 +128,6 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
nid_t ino = 0;
int err;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -142,6 +140,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
inode->i_mapping->a_ops = &f2fs_dblock_aops;
ino = inode->i_ino;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
if (err)
@@ -288,12 +288,13 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
int err = -ENOENT;

trace_f2fs_unlink_enter(dir, dentry);
- f2fs_balance_fs(sbi);

de = f2fs_find_entry(dir, &dentry->d_name, &page);
if (!de)
goto fail;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = acquire_orphan_inode(sbi);
if (err) {
@@ -341,8 +342,6 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
if (len > dir->i_sb->s_blocksize)
return -ENAMETOOLONG;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, S_IFLNK | S_IRWXUGO);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -353,6 +352,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
inode->i_op = &f2fs_symlink_inode_operations;
inode->i_mapping->a_ops = &f2fs_dblock_aops;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
if (err)
@@ -433,8 +434,6 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
struct inode *inode;
int err;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, S_IFDIR | mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -444,6 +443,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
inode->i_mapping->a_ops = &f2fs_dblock_aops;
mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);

+ f2fs_balance_fs(sbi);
+
set_inode_flag(F2FS_I(inode), FI_INC_LINK);
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
@@ -481,8 +482,6 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
struct inode *inode;
int err = 0;

- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -490,6 +489,8 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
init_special_inode(inode, inode->i_mode, rdev);
inode->i_op = &f2fs_special_inode_operations;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = f2fs_add_link(dentry, inode);
if (err)
@@ -516,9 +517,6 @@ static int __f2fs_tmpfile(struct inode *dir, struct dentry *dentry,
struct inode *inode;
int err;

- if (!whiteout)
- f2fs_balance_fs(sbi);
-
inode = f2fs_new_inode(dir, mode);
if (IS_ERR(inode))
return PTR_ERR(inode);
@@ -532,6 +530,8 @@ static int __f2fs_tmpfile(struct inode *dir, struct dentry *dentry,
inode->i_mapping->a_ops = &f2fs_dblock_aops;
}

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);
err = acquire_orphan_inode(sbi);
if (err)
@@ -604,8 +604,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
goto out;
}

- f2fs_balance_fs(sbi);
-
old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
if (!old_entry)
goto out;
@@ -635,6 +633,8 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
if (!new_entry)
goto out_whiteout;

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);

err = acquire_orphan_inode(sbi);
@@ -666,6 +666,8 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
update_inode_page(old_inode);
update_inode_page(new_inode);
} else {
+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);

err = f2fs_add_link(new_dentry, old_inode);
@@ -763,8 +765,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
new_inode)))
return -EPERM;

- f2fs_balance_fs(sbi);
-
old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
if (!old_entry)
goto out;
@@ -807,6 +807,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
goto out_new_dir;
}

+ f2fs_balance_fs(sbi);
+
f2fs_lock_op(sbi);

err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
--
2.5.4 (Apple Git-61)

2015-12-24 03:31:43

by Chao Yu

[permalink] [raw]
Subject: RE: [f2fs-dev] [PATCH 2/4 v2] f2fs: avoid unnecessary f2fs_gc for dir operations

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Thursday, December 24, 2015 10:13 AM
> To: Chao Yu
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [f2fs-dev] [PATCH 2/4 v2] f2fs: avoid unnecessary f2fs_gc for dir operations
>
> Hi Chao,
>
> Right.
> But, in the rename path, we still need to do f2fs_balance_fs, since it produces
> another dirty node page in the mean time.

That's right.

>
> How about this?
>
> From bbc5bf8f6c940cd75a4d71ce40ce4bd3f647a823 Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <[email protected]>
> Date: Tue, 22 Dec 2015 11:56:08 -0800
> Subject: [PATCH] f2fs: avoid unnecessary f2fs_gc for dir operations
>
> The f2fs_balance_fs doesn't need to cover f2fs_new_inode or f2fs_find_entry
> works.
>
> Signed-off-by: Jaegeuk Kim <[email protected]>

Reviewed-by: Chao Yu <[email protected]>

Thanks,

> ---
> fs/f2fs/namei.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 2c32110..4e27c5c 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -128,8 +128,6 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t
> mode,
> nid_t ino = 0;
> int err;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, mode);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -142,6 +140,8 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t
> mode,
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> ino = inode->i_ino;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> if (err)
> @@ -288,12 +288,13 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
> int err = -ENOENT;
>
> trace_f2fs_unlink_enter(dir, dentry);
> - f2fs_balance_fs(sbi);
>
> de = f2fs_find_entry(dir, &dentry->d_name, &page);
> if (!de)
> goto fail;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = acquire_orphan_inode(sbi);
> if (err) {
> @@ -341,8 +342,6 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> if (len > dir->i_sb->s_blocksize)
> return -ENAMETOOLONG;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, S_IFLNK | S_IRWXUGO);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -353,6 +352,8 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> inode->i_op = &f2fs_symlink_inode_operations;
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> if (err)
> @@ -433,8 +434,6 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t
> mode)
> struct inode *inode;
> int err;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, S_IFDIR | mode);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -444,6 +443,8 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t
> mode)
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);
>
> + f2fs_balance_fs(sbi);
> +
> set_inode_flag(F2FS_I(inode), FI_INC_LINK);
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> @@ -481,8 +482,6 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
> struct inode *inode;
> int err = 0;
>
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, mode);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -490,6 +489,8 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
> init_special_inode(inode, inode->i_mode, rdev);
> inode->i_op = &f2fs_special_inode_operations;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = f2fs_add_link(dentry, inode);
> if (err)
> @@ -516,9 +517,6 @@ static int __f2fs_tmpfile(struct inode *dir, struct dentry *dentry,
> struct inode *inode;
> int err;
>
> - if (!whiteout)
> - f2fs_balance_fs(sbi);
> -
> inode = f2fs_new_inode(dir, mode);
> if (IS_ERR(inode))
> return PTR_ERR(inode);
> @@ -532,6 +530,8 @@ static int __f2fs_tmpfile(struct inode *dir, struct dentry *dentry,
> inode->i_mapping->a_ops = &f2fs_dblock_aops;
> }
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
> err = acquire_orphan_inode(sbi);
> if (err)
> @@ -604,8 +604,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> goto out;
> }
>
> - f2fs_balance_fs(sbi);
> -
> old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
> if (!old_entry)
> goto out;
> @@ -635,6 +633,8 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> if (!new_entry)
> goto out_whiteout;
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
>
> err = acquire_orphan_inode(sbi);
> @@ -666,6 +666,8 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> update_inode_page(old_inode);
> update_inode_page(new_inode);
> } else {
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
>
> err = f2fs_add_link(new_dentry, old_inode);
> @@ -763,8 +765,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry
> *old_dentry,
> new_inode)))
> return -EPERM;
>
> - f2fs_balance_fs(sbi);
> -
> old_entry = f2fs_find_entry(old_dir, &old_dentry->d_name, &old_page);
> if (!old_entry)
> goto out;
> @@ -807,6 +807,8 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry
> *old_dentry,
> goto out_new_dir;
> }
>
> + f2fs_balance_fs(sbi);
> +
> f2fs_lock_op(sbi);
>
> err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
> --
> 2.5.4 (Apple Git-61)

2015-12-24 05:49:19

by Chao Yu

[permalink] [raw]
Subject: RE: [f2fs-dev] [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Thursday, December 24, 2015 3:14 AM
> To: [email protected]; [email protected];
> [email protected]
> Subject: Re: [f2fs-dev] [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed
>
> Change log v2:
> - add dio case
>
> >From c2d16a526371954671f9c8cff5f09f9d230f7993 Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <[email protected]>
> Date: Tue, 22 Dec 2015 13:23:35 -0800
> Subject: [PATCH] f2fs: call f2fs_balance_fs only when node was changed
>
> If user tries to update or read data, we don't need to call f2fs_balance_fs
> which triggers f2fs_gc, which increases unnecessary long latency.
>
> Signed-off-by: Jaegeuk Kim <[email protected]>

Reviewed-by: Chao Yu <[email protected]>

2015-12-25 01:39:28

by Chao Yu

[permalink] [raw]
Subject: RE: [f2fs-dev] [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed

Hi Jaegeuk,

> -----Original Message-----
> From: Chao Yu [mailto:[email protected]]
> Sent: Wednesday, December 23, 2015 5:47 PM
> To: 'Jaegeuk Kim'
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [f2fs-dev] [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed
>
> Hi Jaegeuk,
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:[email protected]]
> > Sent: Wednesday, December 23, 2015 9:00 AM
> > To: [email protected]; [email protected];
> > [email protected]
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 4/4] f2fs: call f2fs_balance_fs only when node was changed
> >
> > If user tries to update or read data, we don't need to call f2fs_balance_fs
> > which triggers f2fs_gc, which increases unnecessary long latency.
>
> One missing case is get_data_block_dio, how about also covering it based on
> following patch?
>
>
> >From 7175efac7473e7a04285055c69edfb7432f8ca4e Mon Sep 17 00:00:00 2001
> From: Chao Yu <[email protected]>
> Date: Wed, 23 Dec 2015 17:11:43 +0800
> Subject: [PATCH] f2fs: reduce covered region of sbi->cp_rwsem in
> f2fs_map_blocks

Title of this commit log was not merged into dev-test branch correctly,
could you please reedit it? :)

Thanks,