2013-10-25 16:07:37

by Huajun Li

[permalink] [raw]
Subject: [f2fs-dev 0/5] f2fs: Enable f2fs support inline data

From: Huajun Li <[email protected]>

f2fs inode is so large, so small files can be stored directly in the inode,
rather than just storing a single block address and storing the data elsewhere.

This patch set makes files less than ~3.4K store directly in inode block.
a) space saving
Test with kernel src(without repo data), it can save about 10% space
with this patch set;
b) performance
Test this patch set with iozone, there is no obvious performance difference
with the results of disabling this feature.

Huajun Li (5):
f2fs: Add flags and helpers to support inline data
f2fs: Add a new mount option: inline_data
f2fs: Add a new function: f2fs_reserve_block()
f2fs: Key functions to handle inline data
f2fs: Handle inline data read and write

fs/f2fs/Makefile | 2 +-
fs/f2fs/data.c | 77 ++++++++++++++++++++-----
fs/f2fs/f2fs.h | 22 ++++++++
fs/f2fs/file.c | 42 +++++++++++++-
fs/f2fs/inline.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++
fs/f2fs/super.c | 8 ++-
include/linux/f2fs_fs.h | 8 +++
7 files changed, 283 insertions(+), 20 deletions(-)
create mode 100644 fs/f2fs/inline.c

--
1.7.9.5


2013-10-25 16:07:59

by Huajun Li

[permalink] [raw]
Subject: [f2fs-dev 1/5] f2fs: Add flags and helpers to support inline data

From: Huajun Li <[email protected]>

Add new inode flags F2FS_INLINE_DATA and FI_INLINE_DATA to indicate
whether the inode has inline data.

Inline data makes use of inode block's data indices region to save small
file. Currently there are 923 data indices in an inode block. Since
inline xattr has made use of the last 50 indices to save its data, there
are 873 indices left which can be used for inline data. When
FI_INLINE_DATA is set, the layout of inode block's indices region is
like below:
+-----------------+
| | Reserved. reserve_new_block() will make use of
| i_addr[0] | i_addr[0] when we need to reserve a new data block
| | to convert inline data into regular one's.
|-----------------|
| | Used by inline data. A file whose size is less than
| i_addr[1..872] | 3488 bytes(~3.4k) and doesn't reserve extra
| | blocks by fallocate() can be saved here.
|-----------------|
| i_addr[873-922] | Reserved for inline xattr
+-----------------+

Signed-off-by: Haicheng Li <[email protected]>
Signed-off-by: Huajun Li <[email protected]>
Signed-off-by: Weihong Xu <[email protected]>
---
fs/f2fs/f2fs.h | 14 ++++++++++++++
include/linux/f2fs_fs.h | 8 ++++++++
2 files changed, 22 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index a61cc5f..e50a8b0 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -31,6 +31,7 @@
#define F2FS_MOUNT_POSIX_ACL 0x00000020
#define F2FS_MOUNT_DISABLE_EXT_IDENTIFY 0x00000040
#define F2FS_MOUNT_INLINE_XATTR 0x00000080
+#define F2FS_MOUNT_INLINE_DATA 0x00000100

#define clear_opt(sbi, option) (sbi->mount_opt.opt &= ~F2FS_MOUNT_##option)
#define set_opt(sbi, option) (sbi->mount_opt.opt |= F2FS_MOUNT_##option)
@@ -871,6 +872,7 @@ enum {
FI_UPDATE_DIR, /* should update inode block for consistency */
FI_DELAY_IPUT, /* used for the recovery */
FI_INLINE_XATTR, /* used for inline xattr */
+ FI_INLINE_DATA, /* used for inline data*/
};

static inline void set_inode_flag(struct f2fs_inode_info *fi, int flag)
@@ -908,6 +910,8 @@ static inline void get_inline_info(struct f2fs_inode_info *fi,
{
if (ri->i_inline & F2FS_INLINE_XATTR)
set_inode_flag(fi, FI_INLINE_XATTR);
+ if (ri->i_inline & F2FS_INLINE_DATA)
+ set_inode_flag(fi, FI_INLINE_DATA);
}

static inline void set_raw_inline(struct f2fs_inode_info *fi,
@@ -917,6 +921,8 @@ static inline void set_raw_inline(struct f2fs_inode_info *fi,

if (is_inode_flag_set(fi, FI_INLINE_XATTR))
ri->i_inline |= F2FS_INLINE_XATTR;
+ if (is_inode_flag_set(fi, FI_INLINE_DATA))
+ ri->i_inline |= F2FS_INLINE_DATA;
}

static inline unsigned int addrs_per_inode(struct f2fs_inode_info *fi)
@@ -942,6 +948,13 @@ static inline int inline_xattr_size(struct inode *inode)
return 0;
}

+static inline void *inline_data_addr(struct page *page)
+{
+ struct f2fs_inode *ri;
+ ri = (struct f2fs_inode *)page_address(page);
+ return (void *)&(ri->i_addr[1]);
+}
+
static inline int f2fs_readonly(struct super_block *sb)
{
return sb->s_flags & MS_RDONLY;
@@ -1231,4 +1244,5 @@ extern const struct address_space_operations f2fs_meta_aops;
extern const struct inode_operations f2fs_dir_inode_operations;
extern const struct inode_operations f2fs_symlink_inode_operations;
extern const struct inode_operations f2fs_special_inode_operations;
+
#endif
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index bb942f6..aea5eed 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -153,6 +153,14 @@ struct f2fs_extent {
#define NODE_DIND_BLOCK (DEF_ADDRS_PER_INODE + 5)

#define F2FS_INLINE_XATTR 0x01 /* file inline xattr flag */
+#define F2FS_INLINE_DATA 0x02 /* file inline data flag */
+
+
+#define MAX_INLINE_DATA (sizeof(__le32) * (DEF_ADDRS_PER_INODE - \
+ F2FS_INLINE_XATTR_ADDRS - 1))
+
+#define INLINE_DATA_OFFSET (PAGE_CACHE_SIZE - sizeof(struct node_footer) \
+ - sizeof(__le32)*(DEF_ADDRS_PER_INODE + 5 - 1))

struct f2fs_inode {
__le16 i_mode; /* file mode */
--
1.7.9.5

2013-10-25 16:08:15

by Huajun Li

[permalink] [raw]
Subject: [f2fs-dev 2/5] f2fs: Add a new mount option: inline_data

From: Huajun Li <[email protected]>

Add a mount option: inline_data. If the mount option is set,
data of New created small files can be stored in their inode.

Signed-off-by: Huajun Li <[email protected]>
Signed-off-by: Haicheng Li <[email protected]>
Signed-off-by: Weihong Xu <[email protected]>
---
fs/f2fs/super.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index e42351c..e8ad7f2 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -50,6 +50,7 @@ enum {
Opt_active_logs,
Opt_disable_ext_identify,
Opt_inline_xattr,
+ Opt_inline_data,
Opt_err,
};

@@ -65,6 +66,7 @@ static match_table_t f2fs_tokens = {
{Opt_active_logs, "active_logs=%u"},
{Opt_disable_ext_identify, "disable_ext_identify"},
{Opt_inline_xattr, "inline_xattr"},
+ {Opt_inline_data, "inline_data"},
{Opt_err, NULL},
};

@@ -311,6 +313,9 @@ static int parse_options(struct super_block *sb, char *options)
case Opt_disable_ext_identify:
set_opt(sbi, DISABLE_EXT_IDENTIFY);
break;
+ case Opt_inline_data:
+ set_opt(sbi, INLINE_DATA);
+ break;
default:
f2fs_msg(sb, KERN_ERR,
"Unrecognized mount option \"%s\" or missing value",
@@ -508,7 +513,8 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root)
#endif
if (test_opt(sbi, DISABLE_EXT_IDENTIFY))
seq_puts(seq, ",disable_ext_identify");
-
+ if (test_opt(sbi, INLINE_DATA))
+ seq_puts(seq, ",inline_data");
seq_printf(seq, ",active_logs=%u", sbi->active_logs);

return 0;
--
1.7.9.5

2013-10-25 16:08:22

by Huajun Li

[permalink] [raw]
Subject: [f2fs-dev 3/5] f2fs: Add a new function: f2fs_reserve_block()

From: Huajun Li <[email protected]>

Add the function f2fs_reserve_block() to easily reserve new blocks.

Signed-off-by: Huajun Li <[email protected]>
Signed-off-by: Haicheng Li <[email protected]>
Signed-off-by: Weihong Xu <[email protected]>
---
fs/f2fs/data.c | 29 ++++++++++++++++++-----------
fs/f2fs/f2fs.h | 1 +
2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index c8887d8..7b31911 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -64,6 +64,23 @@ int reserve_new_block(struct dnode_of_data *dn)
return 0;
}

+int f2fs_reserve_block(struct inode *inode,
+ struct dnode_of_data *dn, pgoff_t index)
+{
+ int err;
+
+ set_new_dnode(dn, inode, NULL, NULL, 0);
+ err = get_dnode_of_data(dn, index, ALLOC_NODE);
+ if (err)
+ return err;
+ if (dn->data_blkaddr == NULL_ADDR)
+ err = reserve_new_block(dn);
+
+ f2fs_put_dnode(dn);
+
+ return err;
+}
+
static int check_extent_cache(struct inode *inode, pgoff_t pgofs,
struct buffer_head *bh_result)
{
@@ -644,19 +661,9 @@ repeat:
*pagep = page;

f2fs_lock_op(sbi);
-
- set_new_dnode(&dn, inode, NULL, NULL, 0);
- err = get_dnode_of_data(&dn, index, ALLOC_NODE);
- if (err)
- goto err;
-
- if (dn.data_blkaddr == NULL_ADDR)
- err = reserve_new_block(&dn);
-
- f2fs_put_dnode(&dn);
+ err = f2fs_reserve_block(inode, &dn, index);
if (err)
goto err;
-
f2fs_unlock_op(sbi);

if ((len == PAGE_CACHE_SIZE) || PageUptodate(page))
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index e50a8b0..4f34330 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1112,6 +1112,7 @@ void destroy_checkpoint_caches(void);
* data.c
*/
int reserve_new_block(struct dnode_of_data *);
+int f2fs_reserve_block(struct inode *, struct dnode_of_data *, pgoff_t);
void update_extent_cache(block_t, struct dnode_of_data *);
struct page *find_data_page(struct inode *, pgoff_t, bool);
struct page *get_lock_data_page(struct inode *, pgoff_t);
--
1.7.9.5

2013-10-25 16:08:49

by Huajun Li

[permalink] [raw]
Subject: [f2fs-dev 4/5] f2fs: Key functions to handle inline data

From: Huajun Li <[email protected]>

Functions to implement inline data read/write, and move inline data to
normal data block when file size exceeds inline data limitation.

Signed-off-by: Huajun Li <[email protected]>
Signed-off-by: Haicheng Li <[email protected]>
Signed-off-by: Weihong Xu <[email protected]>
---
fs/f2fs/Makefile | 2 +-
fs/f2fs/f2fs.h | 7 +++
fs/f2fs/inline.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 152 insertions(+), 1 deletion(-)
create mode 100644 fs/f2fs/inline.c

diff --git a/fs/f2fs/Makefile b/fs/f2fs/Makefile
index 27a0820..2e35da1 100644
--- a/fs/f2fs/Makefile
+++ b/fs/f2fs/Makefile
@@ -1,6 +1,6 @@
obj-$(CONFIG_F2FS_FS) += f2fs.o

-f2fs-y := dir.o file.o inode.o namei.o hash.o super.o
+f2fs-y := dir.o file.o inode.o namei.o hash.o super.o inline.o
f2fs-y += checkpoint.o gc.o data.o node.o segment.o recovery.o
f2fs-$(CONFIG_F2FS_STAT_FS) += debug.o
f2fs-$(CONFIG_F2FS_FS_XATTR) += xattr.o
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 4f34330..904ae3d 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1246,4 +1246,11 @@ extern const struct inode_operations f2fs_dir_inode_operations;
extern const struct inode_operations f2fs_symlink_inode_operations;
extern const struct inode_operations f2fs_special_inode_operations;

+/*
+ * inline.c
+ */
+inline int f2fs_has_inline_data(struct inode *);
+int f2fs_read_inline_data(struct inode *, struct page *);
+int f2fs_convert_inline_data(struct inode *, struct page *, unsigned);
+int f2fs_write_inline_data(struct inode *, struct page *, unsigned int);
#endif
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
new file mode 100644
index 0000000..ec0d00c
--- /dev/null
+++ b/fs/f2fs/inline.c
@@ -0,0 +1,144 @@
+/*
+ * fs/f2fs/inline.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/fs.h>
+#include <linux/f2fs_fs.h>
+
+#include "f2fs.h"
+
+inline int f2fs_has_inline_data(struct inode *inode)
+{
+ return is_inode_flag_set(F2FS_I(inode), FI_INLINE_DATA);
+}
+
+int f2fs_read_inline_data(struct inode *inode, struct page *page)
+{
+ struct page *ipage;
+ void *src_addr, *dst_addr;
+
+ BUG_ON(page->index);
+ ipage = get_node_page(F2FS_SB(inode->i_sb), inode->i_ino);
+ if (IS_ERR(ipage))
+ return PTR_ERR(ipage);
+
+ src_addr = inline_data_addr(ipage);
+ dst_addr = page_address(page);
+
+ zero_user_segment(page, INLINE_DATA_OFFSET,
+ INLINE_DATA_OFFSET + MAX_INLINE_DATA);
+ /* Copy the whole inline data block */
+ memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
+ f2fs_put_page(ipage, 1);
+
+ SetPageUptodate(page);
+ unlock_page(page);
+
+ return 0;
+}
+
+static int __f2fs_convert_inline_data(struct inode *inode, struct page *page)
+{
+ int err;
+ struct page *ipage;
+ struct dnode_of_data dn;
+ void *src_addr, *dst_addr;
+ struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
+
+ ipage = get_node_page(sbi, inode->i_ino);
+ if (IS_ERR(ipage))
+ return PTR_ERR(ipage);
+
+ f2fs_lock_op(sbi);
+ /* i_addr[0] is not used for inline data,
+ * so reserving new block will not destroy inline data */
+ err = f2fs_reserve_block(inode, &dn, 0);
+ if (err) {
+ f2fs_put_page(ipage, 1);
+ f2fs_unlock_op(sbi);
+ return err;
+ }
+ f2fs_unlock_op(sbi);
+
+ src_addr = inline_data_addr(ipage);
+ dst_addr = page_address(page);
+ zero_user_segment(page, 0, PAGE_CACHE_SIZE);
+ /* Copy the whole inline data block */
+ memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
+ zero_user_segment(ipage, INLINE_DATA_OFFSET,
+ INLINE_DATA_OFFSET + MAX_INLINE_DATA);
+ clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
+ set_raw_inline(F2FS_I(inode),
+ (struct f2fs_inode *)page_address(ipage));
+
+ set_page_dirty(ipage);
+ f2fs_put_page(ipage, 1);
+ set_page_dirty(page);
+
+ return 0;
+}
+
+int f2fs_convert_inline_data(struct inode *inode,
+ struct page *p, unsigned flags)
+{
+ int err;
+ struct page *page;
+
+ if (p && !p->index) {
+ page = p;
+ } else {
+ page = grab_cache_page_write_begin(inode->i_mapping, 0, flags);
+ if (IS_ERR(page))
+ return PTR_ERR(page);
+ }
+
+ err = __f2fs_convert_inline_data(inode, page);
+
+ if (p && !p->index)
+ f2fs_put_page(page, 1);
+
+ return err;
+}
+
+int f2fs_write_inline_data(struct inode *inode,
+ struct page *page, unsigned size)
+{
+ void *src_addr, *dst_addr;
+ struct page *ipage;
+ struct dnode_of_data dn;
+ int err;
+
+ set_new_dnode(&dn, inode, NULL, NULL, 0);
+ err = get_dnode_of_data(&dn, 0, LOOKUP_NODE);
+ if (err)
+ return err;
+ ipage = dn.inode_page;
+
+ src_addr = page_address(page);
+ dst_addr = inline_data_addr(ipage);
+
+ zero_user_segment(ipage, INLINE_DATA_OFFSET,
+ INLINE_DATA_OFFSET + MAX_INLINE_DATA);
+ memcpy(dst_addr, src_addr, size);
+ if (!f2fs_has_inline_data(inode))
+ set_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
+ set_raw_inline(F2FS_I(inode),
+ (struct f2fs_inode *)page_address(ipage));
+ set_page_dirty(ipage);
+
+ if ((F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 2) ||
+ (!F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 1)) {
+ f2fs_put_dnode(&dn);
+ return 0;
+ }
+
+ /* Release the first data block if it is allocated */
+ truncate_data_blocks_range(&dn, 1);
+ f2fs_put_dnode(&dn);
+
+ return 0;
+}
--
1.7.9.5

2013-10-25 16:08:58

by Huajun Li

[permalink] [raw]
Subject: [f2fs-dev 5/5] f2fs: Handle inline data operations

From: Huajun Li <[email protected]>

Hook inline data read/write, truncate, fallocate, setattr, etc.

Files need meet following 2 requirement to inline:
1) file size is not greater than MAX_INLINE_DATA;
2) file doesn't pre-allocate data blocks by fallocate().

FI_INLINE_DATA will not be set while creating a new regular inode because
most of the files are bigger than ~3.4K. Set FI_INLINE_DATA only when
data is submitted to block layer, ranther than set it while creating a new
inode, this also avoids converting data from inline to normal data block
and vice versa.

While writting inline data to inode block, the first data block should be
released if the file has a block indexed by i_addr[0].

On the other hand, when a file operation is appied to a file with inline
data, we need to test if this file can remain inline by doing this
operation, otherwise it should be convert into normal file by reserving
a new data block, copying inline data to this new block and clear
FI_INLINE_DATA flag. Because reserve a new data block here will make use
of i_addr[0], if we save inline data in i_addr[0..822], then the first
4 bytes would be overwriten. This problem can be avoided simply by
not using i_addr[0] for inline data.

Signed-off-by: Huajun Li <[email protected]>
Signed-off-by: Haicheng Li <[email protected]>
Signed-off-by: Weihong Xu <[email protected]>
---
fs/f2fs/data.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
fs/f2fs/file.c | 42 +++++++++++++++++++++++++++++++++++++++---
2 files changed, 83 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 7b31911..73ef248 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -481,13 +481,28 @@ static int get_data_block_ro(struct inode *inode, sector_t iblock,

static int f2fs_read_data_page(struct file *file, struct page *page)
{
- return mpage_readpage(page, get_data_block_ro);
+ int ret;
+ struct inode *inode = file->f_mapping->host;
+
+ /* If the file has inline data, try to read it directlly */
+ if (f2fs_has_inline_data(inode))
+ ret = f2fs_read_inline_data(inode, page);
+ else
+ ret = mpage_readpage(page, get_data_block_ro);
+
+ return ret;
}

static int f2fs_read_data_pages(struct file *file,
struct address_space *mapping,
struct list_head *pages, unsigned nr_pages)
{
+ struct inode *inode = file->f_mapping->host;
+
+ /* If the file has inline data, skip readpages */
+ if (f2fs_has_inline_data(inode))
+ return 0;
+
return mpage_readpages(mapping, pages, nr_pages, get_data_block_ro);
}

@@ -538,7 +553,7 @@ static int f2fs_write_data_page(struct page *page,
loff_t i_size = i_size_read(inode);
const pgoff_t end_index = ((unsigned long long) i_size)
>> PAGE_CACHE_SHIFT;
- unsigned offset;
+ unsigned offset = 0;
bool need_balance_fs = false;
int err = 0;

@@ -572,7 +587,14 @@ write:
err = do_write_data_page(page);
} else {
f2fs_lock_op(sbi);
- err = do_write_data_page(page);
+ if (test_opt(sbi, INLINE_DATA) && (i_size <= MAX_INLINE_DATA)) {
+ err = f2fs_write_inline_data(inode, page, offset);
+ ClearPageDirty(page);
+ f2fs_unlock_op(sbi);
+ goto out;
+ } else {
+ err = do_write_data_page(page);
+ }
f2fs_unlock_op(sbi);
need_balance_fs = true;
}
@@ -660,12 +682,22 @@ repeat:
return -ENOMEM;
*pagep = page;

+ if ((pos + len) < MAX_INLINE_DATA) {
+ if (f2fs_has_inline_data(inode))
+ goto inline_data;
+ } else if (f2fs_has_inline_data(inode)) {
+ err = f2fs_convert_inline_data(inode, page, flags);
+ if (err)
+ return err;
+ }
+
f2fs_lock_op(sbi);
err = f2fs_reserve_block(inode, &dn, index);
if (err)
goto err;
f2fs_unlock_op(sbi);

+inline_data:
if ((len == PAGE_CACHE_SIZE) || PageUptodate(page))
return 0;

@@ -681,7 +713,11 @@ repeat:
if (dn.data_blkaddr == NEW_ADDR) {
zero_user_segment(page, 0, PAGE_CACHE_SIZE);
} else {
- err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
+ if (f2fs_has_inline_data(inode))
+ err = f2fs_read_inline_data(inode, page);
+ else
+ err = f2fs_readpage(sbi, page,
+ dn.data_blkaddr, READ_SYNC);
if (err)
return err;
lock_page(page);
@@ -735,6 +771,10 @@ static ssize_t f2fs_direct_IO(int rw, struct kiocb *iocb,
if (rw == WRITE)
return 0;

+ /* Let buffer I/O handle the inline data case. */
+ if (f2fs_has_inline_data(inode))
+ return 0;
+
/* Needs synchronization with the cleaner */
return blockdev_direct_IO(rw, iocb, inode, iov, offset, nr_segs,
get_data_block_ro);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 2d4190a..b38118e 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -305,7 +305,8 @@ static int truncate_blocks(struct inode *inode, u64 from)

f2fs_put_dnode(&dn);
free_next:
- err = truncate_inode_blocks(inode, free_from);
+ if (!f2fs_has_inline_data(inode))
+ err = truncate_inode_blocks(inode, free_from);
f2fs_unlock_op(sbi);

/* lastly zero out the first data page */
@@ -381,8 +382,17 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)

if ((attr->ia_valid & ATTR_SIZE) &&
attr->ia_size != i_size_read(inode)) {
+ if (f2fs_has_inline_data(inode) &&
+ (attr->ia_size > MAX_INLINE_DATA)) {
+ unsigned flags = AOP_FLAG_NOFS;
+ err = f2fs_convert_inline_data(inode, NULL, flags);
+ if (err)
+ return err;
+ }
+
truncate_setsize(inode, attr->ia_size);
- f2fs_truncate(inode);
+ if (!f2fs_has_inline_data(inode))
+ f2fs_truncate(inode);
f2fs_balance_fs(F2FS_SB(inode->i_sb));
}

@@ -464,6 +474,26 @@ static int punch_hole(struct inode *inode, loff_t offset, loff_t len, int mode)
loff_t off_start, off_end;
int ret = 0;

+ if (f2fs_has_inline_data(inode)) {
+ struct page *page;
+ unsigned flags = AOP_FLAG_NOFS;
+ page = grab_cache_page_write_begin(inode->i_mapping, 0, flags);
+ if (IS_ERR(page))
+ return PTR_ERR(page);
+ if (offset + len > MAX_INLINE_DATA) {
+ ret = f2fs_convert_inline_data(inode, page, flags);
+ f2fs_put_page(page, 1);
+ if (ret)
+ return ret;
+ } else {
+ zero_user_segment(page, offset, offset + len);
+ SetPageUptodate(page);
+ set_page_dirty(page);
+ f2fs_put_page(page, 1);
+ goto out;
+ }
+ }
+
pg_start = ((unsigned long long) offset) >> PAGE_CACHE_SHIFT;
pg_end = ((unsigned long long) offset + len) >> PAGE_CACHE_SHIFT;

@@ -497,7 +527,7 @@ static int punch_hole(struct inode *inode, loff_t offset, loff_t len, int mode)
f2fs_unlock_op(sbi);
}
}
-
+out:
if (!(mode & FALLOC_FL_KEEP_SIZE) &&
i_size_read(inode) <= (offset + len)) {
i_size_write(inode, offset);
@@ -520,6 +550,12 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
if (ret)
return ret;

+ if (f2fs_has_inline_data(inode) && (offset + len > MAX_INLINE_DATA)) {
+ ret = f2fs_convert_inline_data(inode, NULL, AOP_FLAG_NOFS);
+ if (ret)
+ return ret;
+ }
+
pg_start = ((unsigned long long) offset) >> PAGE_CACHE_SHIFT;
pg_end = ((unsigned long long) offset + len) >> PAGE_CACHE_SHIFT;

--
1.7.9.5

2013-10-28 12:28:56

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev 3/5] f2fs: Add a new function: f2fs_reserve_block()

2013-10-28 (월), 21:16 +0900, Jaegeuk Kim:
Hi,

>
> 2013-10-26 (토), 00:01 +0800, Huajun Li:
> > From: Huajun Li <[email protected]>
> >
> > Add the function f2fs_reserve_block() to easily reserve new blocks.
> >
> > Signed-off-by: Huajun Li <[email protected]>
> > Signed-off-by: Haicheng Li <[email protected]>
> > Signed-off-by: Weihong Xu <[email protected]>
> > ---
> > fs/f2fs/data.c | 29 ++++++++++++++++++-----------
> > fs/f2fs/f2fs.h | 1 +
> > 2 files changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index c8887d8..7b31911 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -64,6 +64,23 @@ int reserve_new_block(struct dnode_of_data *dn)
> > return 0;
> > }
> >
> > +int f2fs_reserve_block(struct inode *inode,
> > + struct dnode_of_data *dn, pgoff_t index)
>

We don't need to get dnode_of_data from parameters, since it is
used by this function only.

>
> > +{
> > + int err;

+ struct dnode_of_data dn;

>
> > +
> > + set_new_dnode(dn, inode, NULL, NULL, 0);
> > + err = get_dnode_of_data(dn, index, ALLOC_NODE);
> > + if (err)
> > + return err;
> > + if (dn->data_blkaddr == NULL_ADDR)
> > + err = reserve_new_block(dn);
> > +
> > + f2fs_put_dnode(dn);
> > +
> > + return err;
> > +}
> > +
>

--
Jaegeuk Kim
Samsung

2013-10-28 12:43:58

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev 4/5] f2fs: Key functions to handle inline data

Hi,

2013-10-26 (토), 00:01 +0800, Huajun Li:
> From: Huajun Li <[email protected]>
>
> Functions to implement inline data read/write, and move inline data to
> normal data block when file size exceeds inline data limitation.
>
> Signed-off-by: Huajun Li <[email protected]>
> Signed-off-by: Haicheng Li <[email protected]>
> Signed-off-by: Weihong Xu <[email protected]>
> ---
> fs/f2fs/Makefile | 2 +-
> fs/f2fs/f2fs.h | 7 +++
> fs/f2fs/inline.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 152 insertions(+), 1 deletion(-)
> create mode 100644 fs/f2fs/inline.c
>

[snip]

> +static int __f2fs_convert_inline_data(struct inode *inode, struct page *page)
> +{
> + int err;
> + struct page *ipage;
> + struct dnode_of_data dn;
> + void *src_addr, *dst_addr;
> + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> +

Here.. f2fs_lock_op(sbi);

> + ipage = get_node_page(sbi, inode->i_ino);
> + if (IS_ERR(ipage))
> + return PTR_ERR(ipage);
> +

Need to move f2fs_lock_op prior to get_node_page.

> + /* i_addr[0] is not used for inline data,

Coding style.
/*
* ...
*/

> + * so reserving new block will not destroy inline data */
> + err = f2fs_reserve_block(inode, &dn, 0);
> + if (err) {
> + f2fs_put_page(ipage, 1);
> + f2fs_unlock_op(sbi);
> + return err;
> + }

Need to move this too.

> +
> + src_addr = inline_data_addr(ipage);
> + dst_addr = page_address(page);
> + zero_user_segment(page, 0, PAGE_CACHE_SIZE);

+ space for readability.

> + /* Copy the whole inline data block */
> + memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
> + zero_user_segment(ipage, INLINE_DATA_OFFSET,
> + INLINE_DATA_OFFSET + MAX_INLINE_DATA);
> + clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
> + set_raw_inline(F2FS_I(inode),
> + (struct f2fs_inode *)page_address(ipage));

--> sync_inode_page()?

> +
> + set_page_dirty(ipage);
> + f2fs_put_page(ipage, 1);
> + set_page_dirty(page);

Here... f2fs_unlock_op(sbi);

Here, we need to consider data consistency.
Let's think about the below scenario.
1. inline_data was written.
2. sync_fs is done.
3. additional data were written.
: this triggers f2fs_convert_inline_data(), produces a page, and then
unsets the inline flag.
4. checkpoint was done with updated inode page. Note that, checkpoint
doesn't guarantee any user data.
5. sudden power-off is occurred.
-> Once power-off-recovery is done, we loose the inline_data which was
written successfully at step #2.

So, I think we need to do f2fs_sync_file() procedure at this moment.
Any idea?

> +
> + return 0;
> +}
> +
> +int f2fs_convert_inline_data(struct inode *inode,
> + struct page *p, unsigned flags)
> +{
> + int err;
> + struct page *page;
> +
> + if (p && !p->index) {
> + page = p;
> + } else {
> + page = grab_cache_page_write_begin(inode->i_mapping, 0, flags);
> + if (IS_ERR(page))
> + return PTR_ERR(page);
> + }
> +
> + err = __f2fs_convert_inline_data(inode, page);
> +
> + if (p && !p->index)
> + f2fs_put_page(page, 1);
> +
> + return err;
> +}
> +
> +int f2fs_write_inline_data(struct inode *inode,
> + struct page *page, unsigned size)
> +{
> + void *src_addr, *dst_addr;
> + struct page *ipage;
> + struct dnode_of_data dn;
> + int err;
> +
> + set_new_dnode(&dn, inode, NULL, NULL, 0);
> + err = get_dnode_of_data(&dn, 0, LOOKUP_NODE);
> + if (err)
> + return err;
> + ipage = dn.inode_page;
> +
> + src_addr = page_address(page);
> + dst_addr = inline_data_addr(ipage);
> +
> + zero_user_segment(ipage, INLINE_DATA_OFFSET,
> + INLINE_DATA_OFFSET + MAX_INLINE_DATA);
> + memcpy(dst_addr, src_addr, size);
> + if (!f2fs_has_inline_data(inode))
> + set_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
> + set_raw_inline(F2FS_I(inode),
> + (struct f2fs_inode *)page_address(ipage));
> + set_page_dirty(ipage);
> +
> + if ((F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 2) ||
> + (!F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 1)) {
> + f2fs_put_dnode(&dn);
> + return 0;
> + }
> +
> + /* Release the first data block if it is allocated */
> + truncate_data_blocks_range(&dn, 1);
> + f2fs_put_dnode(&dn);
> +
> + return 0;
> +}

--
Jaegeuk Kim
Samsung

2013-10-28 12:45:35

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev 5/5] f2fs: Handle inline data operations

Hi,

2013-10-26 (토), 00:01 +0800, Huajun Li:
> From: Huajun Li <[email protected]>
>

[snip]

>
> @@ -538,7 +553,7 @@ static int f2fs_write_data_page(struct page *page,
> loff_t i_size = i_size_read(inode);
> const pgoff_t end_index = ((unsigned long long) i_size)
> >> PAGE_CACHE_SHIFT;
> - unsigned offset;
> + unsigned offset = 0;
> bool need_balance_fs = false;
> int err = 0;
>
> @@ -572,7 +587,14 @@ write:
> err = do_write_data_page(page);
> } else {
> f2fs_lock_op(sbi);
> - err = do_write_data_page(page);
> + if (test_opt(sbi, INLINE_DATA) && (i_size <= MAX_INLINE_DATA)) {
> + err = f2fs_write_inline_data(inode, page, offset);
> + ClearPageDirty(page);

Don't need to call ClearPageDirty(page).

> + f2fs_unlock_op(sbi);
> + goto out;
> + } else {
> + err = do_write_data_page(page);
> + }
> f2fs_unlock_op(sbi);
> need_balance_fs = true;
>
--
Jaegeuk Kim
Samsung

2013-10-28 16:53:55

by Huajun Li

[permalink] [raw]
Subject: Re: [f2fs-dev 3/5] f2fs: Add a new function: f2fs_reserve_block()

Hi Jaegeuk,

Thanks for your kindly review and comments.

On Mon, Oct 28, 2013 at 8:28 PM, Jaegeuk Kim <[email protected]> wrote:
> 2013-10-28 (월), 21:16 +0900, Jaegeuk Kim:
> Hi,
>
>>
>> 2013-10-26 (토), 00:01 +0800, Huajun Li:
>> > From: Huajun Li <[email protected]>
>> >
>> > Add the function f2fs_reserve_block() to easily reserve new blocks.
>> >
>> > Signed-off-by: Huajun Li <[email protected]>
>> > Signed-off-by: Haicheng Li <[email protected]>
>> > Signed-off-by: Weihong Xu <[email protected]>
>> > ---
>> > fs/f2fs/data.c | 29 ++++++++++++++++++-----------
>> > fs/f2fs/f2fs.h | 1 +
>> > 2 files changed, 19 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> > index c8887d8..7b31911 100644
>> > --- a/fs/f2fs/data.c
>> > +++ b/fs/f2fs/data.c
>> > @@ -64,6 +64,23 @@ int reserve_new_block(struct dnode_of_data *dn)
>> > return 0;
>> > }
>> >
>> > +int f2fs_reserve_block(struct inode *inode,
>> > + struct dnode_of_data *dn, pgoff_t index)
>>
>
> We don't need to get dnode_of_data from parameters, since it is
> used by this function only.

After calling f2fs_reserve_block(), we need dn.data_blkaddr to check
whether it is NEW_ADDR. So IMO, it's nice to keep this parameter.

>
>>
>> > +{
>> > + int err;
>
> + struct dnode_of_data dn;
>
>>
>> > +
>> > + set_new_dnode(dn, inode, NULL, NULL, 0);
>> > + err = get_dnode_of_data(dn, index, ALLOC_NODE);
>> > + if (err)
>> > + return err;
>> > + if (dn->data_blkaddr == NULL_ADDR)
>> > + err = reserve_new_block(dn);
>> > +
>> > + f2fs_put_dnode(dn);
>> > +
>> > + return err;
>> > +}
>> > +
>>
>
> --
> Jaegeuk Kim
> Samsung
>

2013-10-28 16:56:57

by Huajun Li

[permalink] [raw]
Subject: Re: [f2fs-dev 5/5] f2fs: Handle inline data operations

On Mon, Oct 28, 2013 at 8:44 PM, Jaegeuk Kim <[email protected]> wrote:
> Hi,
>
> 2013-10-26 (토), 00:01 +0800, Huajun Li:
>> From: Huajun Li <[email protected]>
>>
>
> [snip]
>
>>
>> @@ -538,7 +553,7 @@ static int f2fs_write_data_page(struct page *page,
>> loff_t i_size = i_size_read(inode);
>> const pgoff_t end_index = ((unsigned long long) i_size)
>> >> PAGE_CACHE_SHIFT;
>> - unsigned offset;
>> + unsigned offset = 0;
>> bool need_balance_fs = false;
>> int err = 0;
>>
>> @@ -572,7 +587,14 @@ write:
>> err = do_write_data_page(page);
>> } else {
>> f2fs_lock_op(sbi);
>> - err = do_write_data_page(page);
>> + if (test_opt(sbi, INLINE_DATA) && (i_size <= MAX_INLINE_DATA)) {
>> + err = f2fs_write_inline_data(inode, page, offset);
>> + ClearPageDirty(page);
>
> Don't need to call ClearPageDirty(page).
>

Yes. I will remove it in next version, thanks.

>> + f2fs_unlock_op(sbi);
>> + goto out;
>> + } else {
>> + err = do_write_data_page(page);
>> + }
>> f2fs_unlock_op(sbi);
>> need_balance_fs = true;
>>
> --
> Jaegeuk Kim
> Samsung
>

2013-10-28 17:20:17

by Huajun Li

[permalink] [raw]
Subject: Re: [f2fs-dev 4/5] f2fs: Key functions to handle inline data

On Mon, Oct 28, 2013 at 8:43 PM, Jaegeuk Kim <[email protected]> wrote:
> Hi,
>
> 2013-10-26 (토), 00:01 +0800, Huajun Li:
>> From: Huajun Li <[email protected]>
>>
>> Functions to implement inline data read/write, and move inline data to
>> normal data block when file size exceeds inline data limitation.
>>
>> Signed-off-by: Huajun Li <[email protected]>
>> Signed-off-by: Haicheng Li <[email protected]>
>> Signed-off-by: Weihong Xu <[email protected]>
>> ---
>> fs/f2fs/Makefile | 2 +-
>> fs/f2fs/f2fs.h | 7 +++
>> fs/f2fs/inline.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 152 insertions(+), 1 deletion(-)
>> create mode 100644 fs/f2fs/inline.c
>>
>
> [snip]
>
>> +static int __f2fs_convert_inline_data(struct inode *inode, struct page *page)
>> +{
>> + int err;
>> + struct page *ipage;
>> + struct dnode_of_data dn;
>> + void *src_addr, *dst_addr;
>> + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
>> +
>
> Here.. f2fs_lock_op(sbi);
>
>> + ipage = get_node_page(sbi, inode->i_ino);
>> + if (IS_ERR(ipage))
>> + return PTR_ERR(ipage);
>> +
>
> Need to move f2fs_lock_op prior to get_node_page.
>
>> + /* i_addr[0] is not used for inline data,
>
> Coding style.
> /*
> * ...
> */
>
>> + * so reserving new block will not destroy inline data */
>> + err = f2fs_reserve_block(inode, &dn, 0);
>> + if (err) {
>> + f2fs_put_page(ipage, 1);
>> + f2fs_unlock_op(sbi);
>> + return err;
>> + }
>
> Need to move this too.
>
>> +
>> + src_addr = inline_data_addr(ipage);
>> + dst_addr = page_address(page);
>> + zero_user_segment(page, 0, PAGE_CACHE_SIZE);
>
> + space for readability.
>
>> + /* Copy the whole inline data block */
>> + memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
>> + zero_user_segment(ipage, INLINE_DATA_OFFSET,
>> + INLINE_DATA_OFFSET + MAX_INLINE_DATA);
>> + clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
>> + set_raw_inline(F2FS_I(inode),
>> + (struct f2fs_inode *)page_address(ipage));

Thanks for your comments, I will update them according to above comments.

>
> --> sync_inode_page()?
>
IMO, we just handle file data, so do we still need call sync_inode_page() ?

>> +
>> + set_page_dirty(ipage);
>> + f2fs_put_page(ipage, 1);
>> + set_page_dirty(page);
>
> Here... f2fs_unlock_op(sbi);
>
> Here, we need to consider data consistency.
> Let's think about the below scenario.
> 1. inline_data was written.
> 2. sync_fs is done.
> 3. additional data were written.
> : this triggers f2fs_convert_inline_data(), produces a page, and then
> unsets the inline flag.
> 4. checkpoint was done with updated inode page. Note that, checkpoint
> doesn't guarantee any user data.
> 5. sudden power-off is occurred.
> -> Once power-off-recovery is done, we loose the inline_data which was
> written successfully at step #2.
>
> So, I think we need to do f2fs_sync_file() procedure at this moment.
> Any idea?
>

Yes, need consider this case carefully, thanks for your reminder.

Considering sudden power-off may happen before f2fs_sync_file() is
called, so how about following solutions:
...
/* Copy the whole inline data block */
memcpy(dst_addr, src_addr, MAX_INLINE_DATA);

do f2fs_sync_file() procedure ( Or do write_data_page() procedure ? )

zero_user_segment(ipage, INLINE_DATA_OFFSET, INLINE_DATA_OFFSET +
MAX_INLINE_DATA);
clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
set_raw_inline(F2FS_I(inode), (struct f2fs_inode *)page_address(ipage));
...

>> +
>> + return 0;
>> +}
>> +
>> +int f2fs_convert_inline_data(struct inode *inode,
>> + struct page *p, unsigned flags)
>> +{
>> + int err;
>> + struct page *page;
>> +
>> + if (p && !p->index) {
>> + page = p;
>> + } else {
>> + page = grab_cache_page_write_begin(inode->i_mapping, 0, flags);
>> + if (IS_ERR(page))
>> + return PTR_ERR(page);
>> + }
>> +
>> + err = __f2fs_convert_inline_data(inode, page);
>> +
>> + if (p && !p->index)
>> + f2fs_put_page(page, 1);
>> +
>> + return err;
>> +}
>> +
>> +int f2fs_write_inline_data(struct inode *inode,
>> + struct page *page, unsigned size)
>> +{
>> + void *src_addr, *dst_addr;
>> + struct page *ipage;
>> + struct dnode_of_data dn;
>> + int err;
>> +
>> + set_new_dnode(&dn, inode, NULL, NULL, 0);
>> + err = get_dnode_of_data(&dn, 0, LOOKUP_NODE);
>> + if (err)
>> + return err;
>> + ipage = dn.inode_page;
>> +
>> + src_addr = page_address(page);
>> + dst_addr = inline_data_addr(ipage);
>> +
>> + zero_user_segment(ipage, INLINE_DATA_OFFSET,
>> + INLINE_DATA_OFFSET + MAX_INLINE_DATA);
>> + memcpy(dst_addr, src_addr, size);
>> + if (!f2fs_has_inline_data(inode))
>> + set_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
>> + set_raw_inline(F2FS_I(inode),
>> + (struct f2fs_inode *)page_address(ipage));
>> + set_page_dirty(ipage);
>> +
>> + if ((F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 2) ||
>> + (!F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 1)) {
>> + f2fs_put_dnode(&dn);
>> + return 0;
>> + }
>> +
>> + /* Release the first data block if it is allocated */
>> + truncate_data_blocks_range(&dn, 1);
>> + f2fs_put_dnode(&dn);
>> +
>> + return 0;
>> +}
>
> --
> Jaegeuk Kim
> Samsung
>

2013-10-29 00:57:01

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev 3/5] f2fs: Add a new function: f2fs_reserve_block()

Hi Huajun,

2013-10-29 (화), 00:53 +0800, Huajun Li:
> Hi Jaegeuk,
>
> Thanks for your kindly review and comments.
>
> On Mon, Oct 28, 2013 at 8:28 PM, Jaegeuk Kim <[email protected]> wrote:
> > 2013-10-28 (월), 21:16 +0900, Jaegeuk Kim:
> > Hi,
> >
> >>
> >> 2013-10-26 (토), 00:01 +0800, Huajun Li:
> >> > From: Huajun Li <[email protected]>
> >> >
> >> > Add the function f2fs_reserve_block() to easily reserve new blocks.
> >> >
> >> > Signed-off-by: Huajun Li <[email protected]>
> >> > Signed-off-by: Haicheng Li <[email protected]>
> >> > Signed-off-by: Weihong Xu <[email protected]>
> >> > ---
> >> > fs/f2fs/data.c | 29 ++++++++++++++++++-----------
> >> > fs/f2fs/f2fs.h | 1 +
> >> > 2 files changed, 19 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >> > index c8887d8..7b31911 100644
> >> > --- a/fs/f2fs/data.c
> >> > +++ b/fs/f2fs/data.c
> >> > @@ -64,6 +64,23 @@ int reserve_new_block(struct dnode_of_data *dn)
> >> > return 0;
> >> > }
> >> >
> >> > +int f2fs_reserve_block(struct inode *inode,
> >> > + struct dnode_of_data *dn, pgoff_t index)
> >>
> >
> > We don't need to get dnode_of_data from parameters, since it is
> > used by this function only.
>
> After calling f2fs_reserve_block(), we need dn.data_blkaddr to check
> whether it is NEW_ADDR. So IMO, it's nice to keep this parameter.
>

Ah, got it.
BTW, I found another flows we can clean up wrt this issue.
How about this?

---
fs/f2fs/data.c | 51 +++++++++++++++++++++++----------------------------
fs/f2fs/f2fs.h | 1 +
fs/f2fs/file.c | 39 ++++++---------------------------------
3 files changed, 30 insertions(+), 61 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index c8887d8..b8c4f76d 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -64,6 +64,22 @@ int reserve_new_block(struct dnode_of_data *dn)
return 0;
}

+int f2fs_reserve_block(struct dnode_of_data *dn, pgoff_t index)
+{
+ bool need_put = dn->inode_page ? false : true;
+ int err;
+
+ err = get_dnode_of_data(dn, index, ALLOC_NODE);
+ if (err)
+ return err;
+ if (dn->data_blkaddr == NULL_ADDR)
+ err = reserve_new_block(dn);
+
+ if (need_put)
+ f2fs_put_dnode(dn);
+ return err;
+}
+
static int check_extent_cache(struct inode *inode, pgoff_t pgofs,
struct buffer_head *bh_result)
{
@@ -300,19 +316,9 @@ struct page *get_new_data_page(struct inode *inode,
int err;

set_new_dnode(&dn, inode, npage, npage, 0);
- err = get_dnode_of_data(&dn, index, ALLOC_NODE);
+ err = f2fs_reserve_block(&dn, index);
if (err)
return ERR_PTR(err);
-
- if (dn.data_blkaddr == NULL_ADDR) {
- if (reserve_new_block(&dn)) {
- if (!npage)
- f2fs_put_dnode(&dn);
- return ERR_PTR(-ENOSPC);
- }
- }
- if (!npage)
- f2fs_put_dnode(&dn);
repeat:
page = grab_cache_page(mapping, index);
if (!page)
@@ -644,21 +650,15 @@ repeat:
*pagep = page;

f2fs_lock_op(sbi);
-
set_new_dnode(&dn, inode, NULL, NULL, 0);
- err = get_dnode_of_data(&dn, index, ALLOC_NODE);
- if (err)
- goto err;
-
- if (dn.data_blkaddr == NULL_ADDR)
- err = reserve_new_block(&dn);
-
- f2fs_put_dnode(&dn);
- if (err)
- goto err;
-
+ err = f2fs_reserve_block(&dn, index);
f2fs_unlock_op(sbi);

+ if (err) {
+ f2fs_put_page(page, 1);
+ return err;
+ }
+
if ((len == PAGE_CACHE_SIZE) || PageUptodate(page))
return 0;

@@ -691,11 +691,6 @@ out:
SetPageUptodate(page);
clear_cold_data(page);
return 0;
-
-err:
- f2fs_unlock_op(sbi);
- f2fs_put_page(page, 1);
- return err;
}

static int f2fs_write_end(struct file *file,
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index c9d394c..e77ca20 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1112,6 +1112,7 @@ void destroy_checkpoint_caches(void);
* data.c
*/
int reserve_new_block(struct dnode_of_data *);
+int f2fs_reserve_block(struct dnode_of_data *, pgoff_t);
void update_extent_cache(block_t, struct dnode_of_data *);
struct page *find_data_page(struct inode *, pgoff_t, bool);
struct page *get_lock_data_page(struct inode *, pgoff_t);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 2d4190a..21ef0d1 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -33,7 +33,6 @@ static int f2fs_vm_page_mkwrite(struct vm_area_struct
*vma,
struct page *page = vmf->page;
struct inode *inode = file_inode(vma->vm_file);
struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
- block_t old_blk_addr;
struct dnode_of_data dn;
int err;

@@ -44,24 +43,10 @@ static int f2fs_vm_page_mkwrite(struct
vm_area_struct *vma,
/* block allocation */
f2fs_lock_op(sbi);
set_new_dnode(&dn, inode, NULL, NULL, 0);
- err = get_dnode_of_data(&dn, page->index, ALLOC_NODE);
- if (err) {
- f2fs_unlock_op(sbi);
- goto out;
- }
-
- old_blk_addr = dn.data_blkaddr;
-
- if (old_blk_addr == NULL_ADDR) {
- err = reserve_new_block(&dn);
- if (err) {
- f2fs_put_dnode(&dn);
- f2fs_unlock_op(sbi);
- goto out;
- }
- }
- f2fs_put_dnode(&dn);
+ err = f2fs_reserve_block(&dn, page->index);
f2fs_unlock_op(sbi);
+ if (err)
+ goto out;

file_update_time(vma->vm_file);
lock_page(page);
@@ -531,22 +516,10 @@ static int expand_inode_data(struct inode *inode,
loff_t offset,

f2fs_lock_op(sbi);
set_new_dnode(&dn, inode, NULL, NULL, 0);
- ret = get_dnode_of_data(&dn, index, ALLOC_NODE);
- if (ret) {
- f2fs_unlock_op(sbi);
- break;
- }
-
- if (dn.data_blkaddr == NULL_ADDR) {
- ret = reserve_new_block(&dn);
- if (ret) {
- f2fs_put_dnode(&dn);
- f2fs_unlock_op(sbi);
- break;
- }
- }
- f2fs_put_dnode(&dn);
+ ret = f2fs_reserve_block(&dn, index);
f2fs_unlock_op(sbi);
+ if (ret)
+ break;

if (pg_start == pg_end)
new_size = offset + len;
--
1.8.4.474.g128a96c

--
Jaegeuk Kim
Samsung

2013-10-29 01:07:09

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev 4/5] f2fs: Key functions to handle inline data

Hi,

2013-10-29 (화), 01:20 +0800, Huajun Li:
> On Mon, Oct 28, 2013 at 8:43 PM, Jaegeuk Kim <[email protected]> wrote:
> > Hi,
> >
> > 2013-10-26 (토), 00:01 +0800, Huajun Li:
> >> From: Huajun Li <[email protected]>
> >>
> >> Functions to implement inline data read/write, and move inline data to
> >> normal data block when file size exceeds inline data limitation.
> >>
> >> Signed-off-by: Huajun Li <[email protected]>
> >> Signed-off-by: Haicheng Li <[email protected]>
> >> Signed-off-by: Weihong Xu <[email protected]>
> >> ---
> >> fs/f2fs/Makefile | 2 +-
> >> fs/f2fs/f2fs.h | 7 +++
> >> fs/f2fs/inline.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 152 insertions(+), 1 deletion(-)
> >> create mode 100644 fs/f2fs/inline.c
> >>
> >
> > [snip]
> >
> >> +static int __f2fs_convert_inline_data(struct inode *inode, struct page *page)
> >> +{
> >> + int err;
> >> + struct page *ipage;
> >> + struct dnode_of_data dn;
> >> + void *src_addr, *dst_addr;
> >> + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> >> +
> >
> > Here.. f2fs_lock_op(sbi);
> >
> >> + ipage = get_node_page(sbi, inode->i_ino);
> >> + if (IS_ERR(ipage))
> >> + return PTR_ERR(ipage);
> >> +
> >
> > Need to move f2fs_lock_op prior to get_node_page.
> >
> >> + /* i_addr[0] is not used for inline data,
> >
> > Coding style.
> > /*
> > * ...
> > */
> >
> >> + * so reserving new block will not destroy inline data */
> >> + err = f2fs_reserve_block(inode, &dn, 0);
> >> + if (err) {
> >> + f2fs_put_page(ipage, 1);
> >> + f2fs_unlock_op(sbi);
> >> + return err;
> >> + }
> >
> > Need to move this too.
> >
> >> +
> >> + src_addr = inline_data_addr(ipage);
> >> + dst_addr = page_address(page);
> >> + zero_user_segment(page, 0, PAGE_CACHE_SIZE);
> >
> > + space for readability.
> >
> >> + /* Copy the whole inline data block */
> >> + memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
> >> + zero_user_segment(ipage, INLINE_DATA_OFFSET,
> >> + INLINE_DATA_OFFSET + MAX_INLINE_DATA);
> >> + clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
> >> + set_raw_inline(F2FS_I(inode),
> >> + (struct f2fs_inode *)page_address(ipage));
>
> Thanks for your comments, I will update them according to above comments.
>
> >
> > --> sync_inode_page()?
> >
> IMO, we just handle file data, so do we still need call sync_inode_page() ?

I think sync_inode_page() is more suitable, since we need to sync
between i_size, i_blocks and its i_flag, inline_data, at this moment.

>
> >> +
> >> + set_page_dirty(ipage);

Need to consider skipping set_page_dirty(ipage).

> >> + f2fs_put_page(ipage, 1);
> >> + set_page_dirty(page);
> >
> > Here... f2fs_unlock_op(sbi);
> >
> > Here, we need to consider data consistency.
> > Let's think about the below scenario.
> > 1. inline_data was written.
> > 2. sync_fs is done.
> > 3. additional data were written.
> > : this triggers f2fs_convert_inline_data(), produces a page, and then
> > unsets the inline flag.
> > 4. checkpoint was done with updated inode page. Note that, checkpoint
> > doesn't guarantee any user data.
> > 5. sudden power-off is occurred.
> > -> Once power-off-recovery is done, we loose the inline_data which was
> > written successfully at step #2.
> >
> > So, I think we need to do f2fs_sync_file() procedure at this moment.
> > Any idea?
> >
>
> Yes, need consider this case carefully, thanks for your reminder.
>
> Considering sudden power-off may happen before f2fs_sync_file() is
> called, so how about following solutions:
> ...
> /* Copy the whole inline data block */
> memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
>
> do f2fs_sync_file() procedure ( Or do write_data_page() procedure ? )

Ya, but it may still have some conditions to do this or not.
One of them is checking whether previous inline data should be recovered
or not, for example.

>
> zero_user_segment(ipage, INLINE_DATA_OFFSET, INLINE_DATA_OFFSET +
> MAX_INLINE_DATA);
> clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
> set_raw_inline(F2FS_I(inode), (struct f2fs_inode *)page_address(ipage));
> ...
>
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +int f2fs_convert_inline_data(struct inode *inode,
> >> + struct page *p, unsigned flags)
> >> +{
> >> + int err;
> >> + struct page *page;
> >> +
> >> + if (p && !p->index) {
> >> + page = p;
> >> + } else {
> >> + page = grab_cache_page_write_begin(inode->i_mapping, 0, flags);
> >> + if (IS_ERR(page))
> >> + return PTR_ERR(page);
> >> + }
> >> +
> >> + err = __f2fs_convert_inline_data(inode, page);
> >> +
> >> + if (p && !p->index)
> >> + f2fs_put_page(page, 1);
> >> +
> >> + return err;
> >> +}
> >> +
> >> +int f2fs_write_inline_data(struct inode *inode,
> >> + struct page *page, unsigned size)
> >> +{
> >> + void *src_addr, *dst_addr;
> >> + struct page *ipage;
> >> + struct dnode_of_data dn;
> >> + int err;
> >> +
> >> + set_new_dnode(&dn, inode, NULL, NULL, 0);
> >> + err = get_dnode_of_data(&dn, 0, LOOKUP_NODE);
> >> + if (err)
> >> + return err;
> >> + ipage = dn.inode_page;
> >> +
> >> + src_addr = page_address(page);
> >> + dst_addr = inline_data_addr(ipage);
> >> +
> >> + zero_user_segment(ipage, INLINE_DATA_OFFSET,
> >> + INLINE_DATA_OFFSET + MAX_INLINE_DATA);
> >> + memcpy(dst_addr, src_addr, size);
> >> + if (!f2fs_has_inline_data(inode))
> >> + set_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
> >> + set_raw_inline(F2FS_I(inode),
> >> + (struct f2fs_inode *)page_address(ipage));
> >> + set_page_dirty(ipage);
> >> +
> >> + if ((F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 2) ||
> >> + (!F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 1)) {
> >> + f2fs_put_dnode(&dn);
> >> + return 0;
> >> + }
> >> +
> >> + /* Release the first data block if it is allocated */
> >> + truncate_data_blocks_range(&dn, 1);
> >> + f2fs_put_dnode(&dn);
> >> +
> >> + return 0;
> >> +}
> >
> > --
> > Jaegeuk Kim
> > Samsung
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Jaegeuk Kim
Samsung

2013-10-29 15:27:36

by Huajun Li

[permalink] [raw]
Subject: Re: [f2fs-dev 3/5] f2fs: Add a new function: f2fs_reserve_block()

On Tue, Oct 29, 2013 at 8:56 AM, Jaegeuk Kim <[email protected]> wrote:
> Hi Huajun,
>
> 2013-10-29 (화), 00:53 +0800, Huajun Li:
>> Hi Jaegeuk,
>>
>> Thanks for your kindly review and comments.
>>
>> On Mon, Oct 28, 2013 at 8:28 PM, Jaegeuk Kim <[email protected]> wrote:
>> > 2013-10-28 (월), 21:16 +0900, Jaegeuk Kim:
>> > Hi,
>> >
>> >>
>> >> 2013-10-26 (토), 00:01 +0800, Huajun Li:
>> >> > From: Huajun Li <[email protected]>
>> >> >
>> >> > Add the function f2fs_reserve_block() to easily reserve new blocks.
>> >> >
>> >> > Signed-off-by: Huajun Li <[email protected]>
>> >> > Signed-off-by: Haicheng Li <[email protected]>
>> >> > Signed-off-by: Weihong Xu <[email protected]>
>> >> > ---
>> >> > fs/f2fs/data.c | 29 ++++++++++++++++++-----------
>> >> > fs/f2fs/f2fs.h | 1 +
>> >> > 2 files changed, 19 insertions(+), 11 deletions(-)
>> >> >
>> >> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> >> > index c8887d8..7b31911 100644
>> >> > --- a/fs/f2fs/data.c
>> >> > +++ b/fs/f2fs/data.c
>> >> > @@ -64,6 +64,23 @@ int reserve_new_block(struct dnode_of_data *dn)
>> >> > return 0;
>> >> > }
>> >> >
>> >> > +int f2fs_reserve_block(struct inode *inode,
>> >> > + struct dnode_of_data *dn, pgoff_t index)
>> >>
>> >
>> > We don't need to get dnode_of_data from parameters, since it is
>> > used by this function only.
>>
>> After calling f2fs_reserve_block(), we need dn.data_blkaddr to check
>> whether it is NEW_ADDR. So IMO, it's nice to keep this parameter.
>>
>
> Ah, got it.
> BTW, I found another flows we can clean up wrt this issue.
> How about this?
>

This can clean up more codes, thanks a lot.

> ---
> fs/f2fs/data.c | 51 +++++++++++++++++++++++----------------------------
> fs/f2fs/f2fs.h | 1 +
> fs/f2fs/file.c | 39 ++++++---------------------------------
> 3 files changed, 30 insertions(+), 61 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index c8887d8..b8c4f76d 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -64,6 +64,22 @@ int reserve_new_block(struct dnode_of_data *dn)
> return 0;
> }
>
> +int f2fs_reserve_block(struct dnode_of_data *dn, pgoff_t index)
> +{
> + bool need_put = dn->inode_page ? false : true;
> + int err;
> +
> + err = get_dnode_of_data(dn, index, ALLOC_NODE);
> + if (err)
> + return err;
> + if (dn->data_blkaddr == NULL_ADDR)
> + err = reserve_new_block(dn);
> +
> + if (need_put)
> + f2fs_put_dnode(dn);
> + return err;
> +}
> +
> static int check_extent_cache(struct inode *inode, pgoff_t pgofs,
> struct buffer_head *bh_result)
> {
> @@ -300,19 +316,9 @@ struct page *get_new_data_page(struct inode *inode,
> int err;
>
> set_new_dnode(&dn, inode, npage, npage, 0);
> - err = get_dnode_of_data(&dn, index, ALLOC_NODE);
> + err = f2fs_reserve_block(&dn, index);
> if (err)
> return ERR_PTR(err);
> -
> - if (dn.data_blkaddr == NULL_ADDR) {
> - if (reserve_new_block(&dn)) {
> - if (!npage)
> - f2fs_put_dnode(&dn);
> - return ERR_PTR(-ENOSPC);
> - }
> - }
> - if (!npage)
> - f2fs_put_dnode(&dn);
> repeat:
> page = grab_cache_page(mapping, index);
> if (!page)
> @@ -644,21 +650,15 @@ repeat:
> *pagep = page;
>
> f2fs_lock_op(sbi);
> -
> set_new_dnode(&dn, inode, NULL, NULL, 0);
> - err = get_dnode_of_data(&dn, index, ALLOC_NODE);
> - if (err)
> - goto err;
> -
> - if (dn.data_blkaddr == NULL_ADDR)
> - err = reserve_new_block(&dn);
> -
> - f2fs_put_dnode(&dn);
> - if (err)
> - goto err;
> -
> + err = f2fs_reserve_block(&dn, index);
> f2fs_unlock_op(sbi);
>
> + if (err) {
> + f2fs_put_page(page, 1);
> + return err;
> + }
> +
> if ((len == PAGE_CACHE_SIZE) || PageUptodate(page))
> return 0;
>
> @@ -691,11 +691,6 @@ out:
> SetPageUptodate(page);
> clear_cold_data(page);
> return 0;
> -
> -err:
> - f2fs_unlock_op(sbi);
> - f2fs_put_page(page, 1);
> - return err;
> }
>
> static int f2fs_write_end(struct file *file,
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index c9d394c..e77ca20 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1112,6 +1112,7 @@ void destroy_checkpoint_caches(void);
> * data.c
> */
> int reserve_new_block(struct dnode_of_data *);
> +int f2fs_reserve_block(struct dnode_of_data *, pgoff_t);
> void update_extent_cache(block_t, struct dnode_of_data *);
> struct page *find_data_page(struct inode *, pgoff_t, bool);
> struct page *get_lock_data_page(struct inode *, pgoff_t);
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 2d4190a..21ef0d1 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -33,7 +33,6 @@ static int f2fs_vm_page_mkwrite(struct vm_area_struct
> *vma,
> struct page *page = vmf->page;
> struct inode *inode = file_inode(vma->vm_file);
> struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> - block_t old_blk_addr;
> struct dnode_of_data dn;
> int err;
>
> @@ -44,24 +43,10 @@ static int f2fs_vm_page_mkwrite(struct
> vm_area_struct *vma,
> /* block allocation */
> f2fs_lock_op(sbi);
> set_new_dnode(&dn, inode, NULL, NULL, 0);
> - err = get_dnode_of_data(&dn, page->index, ALLOC_NODE);
> - if (err) {
> - f2fs_unlock_op(sbi);
> - goto out;
> - }
> -
> - old_blk_addr = dn.data_blkaddr;
> -
> - if (old_blk_addr == NULL_ADDR) {
> - err = reserve_new_block(&dn);
> - if (err) {
> - f2fs_put_dnode(&dn);
> - f2fs_unlock_op(sbi);
> - goto out;
> - }
> - }
> - f2fs_put_dnode(&dn);
> + err = f2fs_reserve_block(&dn, page->index);
> f2fs_unlock_op(sbi);
> + if (err)
> + goto out;
>
> file_update_time(vma->vm_file);
> lock_page(page);
> @@ -531,22 +516,10 @@ static int expand_inode_data(struct inode *inode,
> loff_t offset,
>
> f2fs_lock_op(sbi);
> set_new_dnode(&dn, inode, NULL, NULL, 0);
> - ret = get_dnode_of_data(&dn, index, ALLOC_NODE);
> - if (ret) {
> - f2fs_unlock_op(sbi);
> - break;
> - }
> -
> - if (dn.data_blkaddr == NULL_ADDR) {
> - ret = reserve_new_block(&dn);
> - if (ret) {
> - f2fs_put_dnode(&dn);
> - f2fs_unlock_op(sbi);
> - break;
> - }
> - }
> - f2fs_put_dnode(&dn);
> + ret = f2fs_reserve_block(&dn, index);
> f2fs_unlock_op(sbi);
> + if (ret)
> + break;
>
> if (pg_start == pg_end)
> new_size = offset + len;
> --
> 1.8.4.474.g128a96c
>
> --
> Jaegeuk Kim
> Samsung
>

2013-10-29 15:33:14

by Huajun Li

[permalink] [raw]
Subject: Re: [f2fs-dev 4/5] f2fs: Key functions to handle inline data

Yes, will think over these cases carefully, and send out new version. thanks.

On Tue, Oct 29, 2013 at 9:06 AM, Jaegeuk Kim <[email protected]> wrote:
> Hi,
>
> 2013-10-29 (화), 01:20 +0800, Huajun Li:
>> On Mon, Oct 28, 2013 at 8:43 PM, Jaegeuk Kim <[email protected]> wrote:
>> > Hi,
>> >
>> > 2013-10-26 (토), 00:01 +0800, Huajun Li:
>> >> From: Huajun Li <[email protected]>
>> >>
>> >> Functions to implement inline data read/write, and move inline data to
>> >> normal data block when file size exceeds inline data limitation.
>> >>
>> >> Signed-off-by: Huajun Li <[email protected]>
>> >> Signed-off-by: Haicheng Li <[email protected]>
>> >> Signed-off-by: Weihong Xu <[email protected]>
>> >> ---
>> >> fs/f2fs/Makefile | 2 +-
>> >> fs/f2fs/f2fs.h | 7 +++
>> >> fs/f2fs/inline.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> 3 files changed, 152 insertions(+), 1 deletion(-)
>> >> create mode 100644 fs/f2fs/inline.c
>> >>
>> >
>> > [snip]
>> >
>> >> +static int __f2fs_convert_inline_data(struct inode *inode, struct page *page)
>> >> +{
>> >> + int err;
>> >> + struct page *ipage;
>> >> + struct dnode_of_data dn;
>> >> + void *src_addr, *dst_addr;
>> >> + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
>> >> +
>> >
>> > Here.. f2fs_lock_op(sbi);
>> >
>> >> + ipage = get_node_page(sbi, inode->i_ino);
>> >> + if (IS_ERR(ipage))
>> >> + return PTR_ERR(ipage);
>> >> +
>> >
>> > Need to move f2fs_lock_op prior to get_node_page.
>> >
>> >> + /* i_addr[0] is not used for inline data,
>> >
>> > Coding style.
>> > /*
>> > * ...
>> > */
>> >
>> >> + * so reserving new block will not destroy inline data */
>> >> + err = f2fs_reserve_block(inode, &dn, 0);
>> >> + if (err) {
>> >> + f2fs_put_page(ipage, 1);
>> >> + f2fs_unlock_op(sbi);
>> >> + return err;
>> >> + }
>> >
>> > Need to move this too.
>> >
>> >> +
>> >> + src_addr = inline_data_addr(ipage);
>> >> + dst_addr = page_address(page);
>> >> + zero_user_segment(page, 0, PAGE_CACHE_SIZE);
>> >
>> > + space for readability.
>> >
>> >> + /* Copy the whole inline data block */
>> >> + memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
>> >> + zero_user_segment(ipage, INLINE_DATA_OFFSET,
>> >> + INLINE_DATA_OFFSET + MAX_INLINE_DATA);
>> >> + clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
>> >> + set_raw_inline(F2FS_I(inode),
>> >> + (struct f2fs_inode *)page_address(ipage));
>>
>> Thanks for your comments, I will update them according to above comments.
>>
>> >
>> > --> sync_inode_page()?
>> >
>> IMO, we just handle file data, so do we still need call sync_inode_page() ?
>
> I think sync_inode_page() is more suitable, since we need to sync
> between i_size, i_blocks and its i_flag, inline_data, at this moment.
>
>>
>> >> +
>> >> + set_page_dirty(ipage);
>
> Need to consider skipping set_page_dirty(ipage).
>
>> >> + f2fs_put_page(ipage, 1);
>> >> + set_page_dirty(page);
>> >
>> > Here... f2fs_unlock_op(sbi);
>> >
>> > Here, we need to consider data consistency.
>> > Let's think about the below scenario.
>> > 1. inline_data was written.
>> > 2. sync_fs is done.
>> > 3. additional data were written.
>> > : this triggers f2fs_convert_inline_data(), produces a page, and then
>> > unsets the inline flag.
>> > 4. checkpoint was done with updated inode page. Note that, checkpoint
>> > doesn't guarantee any user data.
>> > 5. sudden power-off is occurred.
>> > -> Once power-off-recovery is done, we loose the inline_data which was
>> > written successfully at step #2.
>> >
>> > So, I think we need to do f2fs_sync_file() procedure at this moment.
>> > Any idea?
>> >
>>
>> Yes, need consider this case carefully, thanks for your reminder.
>>
>> Considering sudden power-off may happen before f2fs_sync_file() is
>> called, so how about following solutions:
>> ...
>> /* Copy the whole inline data block */
>> memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
>>
>> do f2fs_sync_file() procedure ( Or do write_data_page() procedure ? )
>
> Ya, but it may still have some conditions to do this or not.
> One of them is checking whether previous inline data should be recovered
> or not, for example.
>
>>
>> zero_user_segment(ipage, INLINE_DATA_OFFSET, INLINE_DATA_OFFSET +
>> MAX_INLINE_DATA);
>> clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
>> set_raw_inline(F2FS_I(inode), (struct f2fs_inode *)page_address(ipage));
>> ...
>>
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +int f2fs_convert_inline_data(struct inode *inode,
>> >> + struct page *p, unsigned flags)
>> >> +{
>> >> + int err;
>> >> + struct page *page;
>> >> +
>> >> + if (p && !p->index) {
>> >> + page = p;
>> >> + } else {
>> >> + page = grab_cache_page_write_begin(inode->i_mapping, 0, flags);
>> >> + if (IS_ERR(page))
>> >> + return PTR_ERR(page);
>> >> + }
>> >> +
>> >> + err = __f2fs_convert_inline_data(inode, page);
>> >> +
>> >> + if (p && !p->index)
>> >> + f2fs_put_page(page, 1);
>> >> +
>> >> + return err;
>> >> +}
>> >> +
>> >> +int f2fs_write_inline_data(struct inode *inode,
>> >> + struct page *page, unsigned size)
>> >> +{
>> >> + void *src_addr, *dst_addr;
>> >> + struct page *ipage;
>> >> + struct dnode_of_data dn;
>> >> + int err;
>> >> +
>> >> + set_new_dnode(&dn, inode, NULL, NULL, 0);
>> >> + err = get_dnode_of_data(&dn, 0, LOOKUP_NODE);
>> >> + if (err)
>> >> + return err;
>> >> + ipage = dn.inode_page;
>> >> +
>> >> + src_addr = page_address(page);
>> >> + dst_addr = inline_data_addr(ipage);
>> >> +
>> >> + zero_user_segment(ipage, INLINE_DATA_OFFSET,
>> >> + INLINE_DATA_OFFSET + MAX_INLINE_DATA);
>> >> + memcpy(dst_addr, src_addr, size);
>> >> + if (!f2fs_has_inline_data(inode))
>> >> + set_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
>> >> + set_raw_inline(F2FS_I(inode),
>> >> + (struct f2fs_inode *)page_address(ipage));
>> >> + set_page_dirty(ipage);
>> >> +
>> >> + if ((F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 2) ||
>> >> + (!F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 1)) {
>> >> + f2fs_put_dnode(&dn);
>> >> + return 0;
>> >> + }
>> >> +
>> >> + /* Release the first data block if it is allocated */
>> >> + truncate_data_blocks_range(&dn, 1);
>> >> + f2fs_put_dnode(&dn);
>> >> +
>> >> + return 0;
>> >> +}
>> >
>> > --
>> > Jaegeuk Kim
>> > Samsung
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Jaegeuk Kim
> Samsung
>