RE: [f2fs-dev] [PATCH 2/3] f2fs: fix conditions to remain recovery information in f2fs_sync_file

From: Chao Yu
Date: Mon Sep 22 2014 - 03:25:28 EST

Next message: Namhyung Kim: "Re: [PATCH v3] perf tools: Fix build-id matching on vmlinux"
Previous message: Chris Zhong: "[PATCH] i2c-rk3x: move setup to the earlier subsys initcall"
In reply to: Jaegeuk Kim: "[PATCH 2/3] f2fs: fix conditions to remain recovery information in f2fs_sync_file"
Next in thread: Huang Ying: "Re: [f2fs-dev] [PATCH 2/3] f2fs: fix conditions to remain recovery information in f2fs_sync_file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Jaegeuk, Huang,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@xxxxxxxxxx]
> Sent: Thursday, September 18, 2014 1:51 PM
> To: linux-kernel@xxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx;
> linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: Jaegeuk Kim; Huang Ying
> Subject: [f2fs-dev] [PATCH 2/3] f2fs: fix conditions to remain recovery information in
> f2fs_sync_file
>
> This patch revisited whole the recovery information during the f2fs_sync_file.
>
> In this patch, there are three information to make a decision.
>
> a) IS_CHECKPOINTED, /* is it checkpointed before? */
> b) HAS_FSYNCED_INODE, /* is the inode fsynced before? */
> c) HAS_LAST_FSYNC, /* has the latest node fsync mark? */
>
> And, the scenarios for our rule are based on:
>
> [Term] F: fsync_mark, D: dentry_mark
>
> 1. inode(x) | CP | inode(x) | dnode(F)
> 2. inode(x) | CP | inode(F) | dnode(F)
> 3. inode(x) | CP | dnode(F) | inode(x) | inode(F)
> 4. inode(x) | CP | dnode(F) | inode(F)
> 5. CP | inode(x) | dnode(F) | inode(DF)
> 6. CP | inode(DF) | dnode(F)
> 7. CP | dnode(F) | inode(DF)
> 8. CP | dnode(F) | inode(x) | inode(DF)

No sure, do we missed these cases:
inode(x) | CP | inode(F) | dnode(x) -> write inode(F)
CP | inode(DF) | dnode(x) -> write inode(F)

In these cases we will write another inode with fsync flag because our last
dnode is written out to disk by bdi-flusher (HAS_LAST_FSYNC is not marked). But
this appended inode is not useful.

AFAIK, HAS_LAST_FSYNC(AKA fsync_done) is introduced in commit 479f40c44ae3
("f2fs: skip unnecessary node writes during fsync") to avoid writting multiple
unneeded inode page by multiple redundant fsync calls. But for now, its role can
be taken by HAS_FSYNCED_INODE.
So, can we remove this flag to simplify our logic of fsync flow?

Then in fsync flow, the rule can be:
If CPed before, there must be a inode(F) written in warm node chain;
If not CPed before, there must be a inode(DF) written in warm node chain.

Thanks,
Yu

>
> For example, #3, the three conditions should be changed as follows.
>
> inode(x) | CP | dnode(F) | inode(x) | inode(F)
> a) x o o o o
> b) x x x x o
> c) x o o x o
>
> If f2fs_sync_file stops ------^,
> it should write inode(F) --------------^
>
> So, the need_inode_block_update should return true, since
> c) get_nat_flag(e, HAS_LAST_FSYNC), is false.
>
> For example, #8,
> CP | alloc | dnode(F) | inode(x) | inode(DF)
> a) o x x x x
> b) x x x o
> c) o o x o
>
> If f2fs_sync_file stops -------^,
> it should write inode(DF) --------------^
>
> Note that, the roll-forward policy should follow this rule, which means,
> if there are any missing blocks, we doesn't need to recover that inode.
>
> Signed-off-by: Huang Ying <ying.huang@xxxxxxxxx>
> Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> ---
> fs/f2fs/data.c | 3 ---
> fs/f2fs/f2fs.h | 6 +++---
> fs/f2fs/file.c | 10 ++++++----
> fs/f2fs/node.c | 56 +++++++++++++++++++++++++++++++++++---------------------
> fs/f2fs/node.h | 21 ++++++++++++---------
> 5 files changed, 56 insertions(+), 40 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 0e37658..fdc3dbe 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1089,9 +1089,6 @@ static ssize_t f2fs_direct_IO(int rw, struct kiocb *iocb,
> if (check_direct_IO(inode, rw, iter, offset))
> return 0;
>
> - /* clear fsync mark to recover these blocks */
> - fsync_mark_clear(F2FS_I_SB(inode), inode->i_ino);
> -
> trace_f2fs_direct_IO_enter(inode, offset, count, rw);
>
> err = blockdev_direct_IO(rw, iocb, inode, iter, offset, get_data_block);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 9fc1bcd..fd44083 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1224,9 +1224,9 @@ struct dnode_of_data;
> struct node_info;
>
> bool available_free_memory(struct f2fs_sb_info *, int);
> -int is_checkpointed_node(struct f2fs_sb_info *, nid_t);
> -bool fsync_mark_done(struct f2fs_sb_info *, nid_t);
> -void fsync_mark_clear(struct f2fs_sb_info *, nid_t);
> +bool is_checkpointed_node(struct f2fs_sb_info *, nid_t);
> +bool has_fsynced_inode(struct f2fs_sb_info *, nid_t);
> +bool need_inode_block_update(struct f2fs_sb_info *, nid_t);
> void get_node_info(struct f2fs_sb_info *, nid_t, struct node_info *);
> int get_dnode_of_data(struct dnode_of_data *, pgoff_t, int);
> int truncate_inode_blocks(struct inode *, pgoff_t);
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index af06e22..3035c79 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -207,15 +207,17 @@ int f2fs_sync_file(struct file *file, loff_t start, loff_t end, int
> datasync)
> up_write(&fi->i_sem);
> }
> } else {
> - /* if there is no written node page, write its inode page */
> - while (!sync_node_pages(sbi, ino, &wbc)) {
> - if (fsync_mark_done(sbi, ino))
> - goto out;
> +sync_nodes:
> + sync_node_pages(sbi, ino, &wbc);
> +
> + if (need_inode_block_update(sbi, ino)) {
> mark_inode_dirty_sync(inode);
> ret = f2fs_write_inode(inode, NULL);
> if (ret)
> goto out;
> + goto sync_nodes;
> }
> +
> ret = wait_on_node_pages_writeback(sbi, ino);
> if (ret)
> goto out;
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index d19d6b1..7a2d9c9 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -123,44 +123,48 @@ static void __del_from_nat_cache(struct f2fs_nm_info *nm_i, struct
> nat_entry *e)
> kmem_cache_free(nat_entry_slab, e);
> }
>
> -int is_checkpointed_node(struct f2fs_sb_info *sbi, nid_t nid)
> +bool is_checkpointed_node(struct f2fs_sb_info *sbi, nid_t nid)
> {
> struct f2fs_nm_info *nm_i = NM_I(sbi);
> struct nat_entry *e;
> - int is_cp = 1;
> + bool is_cp = true;
>
> read_lock(&nm_i->nat_tree_lock);
> e = __lookup_nat_cache(nm_i, nid);
> if (e && !get_nat_flag(e, IS_CHECKPOINTED))
> - is_cp = 0;
> + is_cp = false;
> read_unlock(&nm_i->nat_tree_lock);
> return is_cp;
> }
>
> -bool fsync_mark_done(struct f2fs_sb_info *sbi, nid_t nid)
> +bool has_fsynced_inode(struct f2fs_sb_info *sbi, nid_t ino)
> {
> struct f2fs_nm_info *nm_i = NM_I(sbi);
> struct nat_entry *e;
> - bool fsync_done = false;
> + bool fsynced = false;
>
> read_lock(&nm_i->nat_tree_lock);
> - e = __lookup_nat_cache(nm_i, nid);
> - if (e)
> - fsync_done = get_nat_flag(e, HAS_FSYNC_MARK);
> + e = __lookup_nat_cache(nm_i, ino);
> + if (e && get_nat_flag(e, HAS_FSYNCED_INODE))
> + fsynced = true;
> read_unlock(&nm_i->nat_tree_lock);
> - return fsync_done;
> + return fsynced;
> }
>
> -void fsync_mark_clear(struct f2fs_sb_info *sbi, nid_t nid)
> +bool need_inode_block_update(struct f2fs_sb_info *sbi, nid_t ino)
> {
> struct f2fs_nm_info *nm_i = NM_I(sbi);
> struct nat_entry *e;
> + bool need_update = true;
>
> - write_lock(&nm_i->nat_tree_lock);
> - e = __lookup_nat_cache(nm_i, nid);
> - if (e)
> - set_nat_flag(e, HAS_FSYNC_MARK, false);
> - write_unlock(&nm_i->nat_tree_lock);
> + read_lock(&nm_i->nat_tree_lock);
> + e = __lookup_nat_cache(nm_i, ino);
> + if (e && get_nat_flag(e, HAS_LAST_FSYNC) &&
> + (get_nat_flag(e, IS_CHECKPOINTED) ||
> + get_nat_flag(e, HAS_FSYNCED_INODE)))
> + need_update = false;
> + read_unlock(&nm_i->nat_tree_lock);
> + return need_update;
> }
>
> static struct nat_entry *grab_nat_entry(struct f2fs_nm_info *nm_i, nid_t nid)
> @@ -176,7 +180,7 @@ static struct nat_entry *grab_nat_entry(struct f2fs_nm_info *nm_i, nid_t
> nid)
> }
> memset(new, 0, sizeof(struct nat_entry));
> nat_set_nid(new, nid);
> - set_nat_flag(new, IS_CHECKPOINTED, true);
> + nat_reset_flag(new);
> list_add_tail(&new->list, &nm_i->nat_entries);
> nm_i->nat_cnt++;
> return new;
> @@ -244,12 +248,17 @@ retry:
>
> /* change address */
> nat_set_blkaddr(e, new_blkaddr);
> + if (new_blkaddr == NEW_ADDR || new_blkaddr == NULL_ADDR)
> + set_nat_flag(e, IS_CHECKPOINTED, false);
> __set_nat_cache_dirty(nm_i, e);
>
> /* update fsync_mark if its inode nat entry is still alive */
> e = __lookup_nat_cache(nm_i, ni->ino);
> - if (e)
> - set_nat_flag(e, HAS_FSYNC_MARK, fsync_done);
> + if (e) {
> + if (fsync_done && ni->nid == ni->ino)
> + set_nat_flag(e, HAS_FSYNCED_INODE, true);
> + set_nat_flag(e, HAS_LAST_FSYNC, fsync_done);
> + }
> write_unlock(&nm_i->nat_tree_lock);
> }
>
> @@ -1121,10 +1130,14 @@ continue_unlock:
>
> /* called by fsync() */
> if (ino && IS_DNODE(page)) {
> - int mark = !is_checkpointed_node(sbi, ino);
> set_fsync_mark(page, 1);
> - if (IS_INODE(page))
> - set_dentry_mark(page, mark);
> + if (IS_INODE(page)) {
> + if (!is_checkpointed_node(sbi, ino) &&
> + !has_fsynced_inode(sbi, ino))
> + set_dentry_mark(page, 1);
> + else
> + set_dentry_mark(page, 0);
> + }
> nwritten++;
> } else {
> set_fsync_mark(page, 0);
> @@ -1912,6 +1925,7 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
> write_unlock(&nm_i->nat_tree_lock);
> } else {
> write_lock(&nm_i->nat_tree_lock);
> + nat_reset_flag(ne);
> __clear_nat_cache_dirty(nm_i, ne);
> write_unlock(&nm_i->nat_tree_lock);
> }
> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> index 3043778..b8ba63c 100644
> --- a/fs/f2fs/node.h
> +++ b/fs/f2fs/node.h
> @@ -41,7 +41,8 @@ struct node_info {
>
> enum {
> IS_CHECKPOINTED, /* is it checkpointed before? */
> - HAS_FSYNC_MARK, /* has the latest node fsync mark? */
> + HAS_FSYNCED_INODE, /* is the inode fsynced before? */
> + HAS_LAST_FSYNC, /* has the latest node fsync mark? */
> };
>
> struct nat_entry {
> @@ -60,15 +61,9 @@ struct nat_entry {
> #define nat_set_version(nat, v) (nat->ni.version = v)
>
> #define __set_nat_cache_dirty(nm_i, ne) \
> - do { \
> - set_nat_flag(ne, IS_CHECKPOINTED, false); \
> - list_move_tail(&ne->list, &nm_i->dirty_nat_entries); \
> - } while (0)
> + list_move_tail(&ne->list, &nm_i->dirty_nat_entries);
> #define __clear_nat_cache_dirty(nm_i, ne) \
> - do { \
> - set_nat_flag(ne, IS_CHECKPOINTED, true); \
> - list_move_tail(&ne->list, &nm_i->nat_entries); \
> - } while (0)
> + list_move_tail(&ne->list, &nm_i->nat_entries);
> #define inc_node_version(version) (++version)
>
> static inline void set_nat_flag(struct nat_entry *ne,
> @@ -87,6 +82,14 @@ static inline bool get_nat_flag(struct nat_entry *ne, unsigned int type)
> return ne->flag & mask;
> }
>
> +static inline void nat_reset_flag(struct nat_entry *ne)
> +{
> + /* these states can be set only after checkpoint was done */
> + set_nat_flag(ne, IS_CHECKPOINTED, true);
> + set_nat_flag(ne, HAS_FSYNCED_INODE, false);
> + set_nat_flag(ne, HAS_LAST_FSYNC, true);
> +}
> +
> static inline void node_info_from_raw_nat(struct node_info *ni,
> struct f2fs_nat_entry *raw_ne)
> {
> --
> 1.8.5.2 (Apple Git-48)
>
>
> ------------------------------------------------------------------------------
> Want excitement?
> Manually upgrade your production database.
> When you want reliability, choose Perforce
> Perforce version control. Predictably reliable.
> http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Namhyung Kim: "Re: [PATCH v3] perf tools: Fix build-id matching on vmlinux"
Previous message: Chris Zhong: "[PATCH] i2c-rk3x: move setup to the earlier subsys initcall"
In reply to: Jaegeuk Kim: "[PATCH 2/3] f2fs: fix conditions to remain recovery information in f2fs_sync_file"
Next in thread: Huang Ying: "Re: [f2fs-dev] [PATCH 2/3] f2fs: fix conditions to remain recovery information in f2fs_sync_file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]