[permalink] [raw]

Subject: RE: [f2fs-dev] [PATCH 03/11] f2fs: add infra for ino management

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Saturday, July 26, 2014 6:47 AM
> To: [email protected]; [email protected];
> [email protected]
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 03/11] f2fs: add infra for ino management
>
> This patch changes the naming of orphan-related data structures to use as
> inode numbers managed globally.
> Later, we can use this facility for managing any inode number lists.
>
> Signed-off-by: Jaegeuk Kim <[email protected]>

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

> ---
> fs/f2fs/checkpoint.c | 72 +++++++++++++++++++++++++++-------------------------
> fs/f2fs/debug.c | 2 +-
> fs/f2fs/f2fs.h | 19 +++++++++-----
> fs/f2fs/super.c | 2 +-
> 4 files changed, 53 insertions(+), 42 deletions(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 3e3c2c3..f93d154 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -22,7 +22,7 @@
> #include "segment.h"
> #include <trace/events/f2fs.h>
>
> -static struct kmem_cache *orphan_entry_slab;
> +static struct kmem_cache *ino_entry_slab;
> static struct kmem_cache *inode_entry_slab;
>
> /*
> @@ -282,19 +282,18 @@ const struct address_space_operations f2fs_meta_aops = {
> .set_page_dirty = f2fs_set_meta_page_dirty,
> };
>
> -static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino)
> +static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
> {
> - struct list_head *head;
> struct ino_entry *new, *e;
>
> - new = f2fs_kmem_cache_alloc(orphan_entry_slab, GFP_ATOMIC);
> + new = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_ATOMIC);
> new->ino = ino;
>
> - spin_lock(&sbi->orphan_inode_lock);
> - list_for_each_entry(e, &sbi->orphan_inode_list, list) {
> + spin_lock(&sbi->ino_lock[type]);
> + list_for_each_entry(e, &sbi->ino_list[type], list) {
> if (e->ino == ino) {
> - spin_unlock(&sbi->orphan_inode_lock);
> - kmem_cache_free(orphan_entry_slab, new);
> + spin_unlock(&sbi->ino_lock[type]);
> + kmem_cache_free(ino_entry_slab, new);
> return;
> }
> if (e->ino > ino)
> @@ -303,58 +302,58 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino)
>
> /* add new entry into list which is sorted by inode number */
> list_add_tail(&new->list, &e->list);
> - spin_unlock(&sbi->orphan_inode_lock);
> + spin_unlock(&sbi->ino_lock[type]);
> }
>
> -static void __remove_ino_entry(struct f2fs_sb_info *sbi, nid_t ino)
> +static void __remove_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
> {
> struct ino_entry *e;
>
> - spin_lock(&sbi->orphan_inode_lock);
> - list_for_each_entry(e, &sbi->orphan_inode_list, list) {
> + spin_lock(&sbi->ino_lock[type]);
> + list_for_each_entry(e, &sbi->ino_list[type], list) {
> if (e->ino == ino) {
> list_del(&e->list);
> sbi->n_orphans--;
> - spin_unlock(&sbi->orphan_inode_lock);
> - kmem_cache_free(orphan_entry_slab, e);
> + spin_unlock(&sbi->ino_lock[type]);
> + kmem_cache_free(ino_entry_slab, e);
> return;
> }
> }
> - spin_unlock(&sbi->orphan_inode_lock);
> + spin_unlock(&sbi->ino_lock[type]);
> }
>
> int acquire_orphan_inode(struct f2fs_sb_info *sbi)
> {
> int err = 0;
>
> - spin_lock(&sbi->orphan_inode_lock);
> + spin_lock(&sbi->ino_lock[ORPHAN_INO]);
> if (unlikely(sbi->n_orphans >= sbi->max_orphans))
> err = -ENOSPC;
> else
> sbi->n_orphans++;
> - spin_unlock(&sbi->orphan_inode_lock);
> + spin_unlock(&sbi->ino_lock[ORPHAN_INO]);
>
> return err;
> }
>
> void release_orphan_inode(struct f2fs_sb_info *sbi)
> {
> - spin_lock(&sbi->orphan_inode_lock);
> + spin_lock(&sbi->ino_lock[ORPHAN_INO]);
> f2fs_bug_on(sbi->n_orphans == 0);
> sbi->n_orphans--;
> - spin_unlock(&sbi->orphan_inode_lock);
> + spin_unlock(&sbi->ino_lock[ORPHAN_INO]);
> }
>
> void add_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
> {
> /* add new orphan entry into list which is sorted by inode number */
> - __add_ino_entry(sbi, ino);
> + __add_ino_entry(sbi, ino, ORPHAN_INO);
> }
>
> void remove_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
> {
> /* remove orphan entry from orphan list */
> - __remove_ino_entry(sbi, ino);
> + __remove_ino_entry(sbi, ino, ORPHAN_INO);
> }
>
> static void recover_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
> @@ -408,14 +407,14 @@ static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t
> start_blk)
> unsigned short orphan_blocks = (unsigned short)((sbi->n_orphans +
> (F2FS_ORPHANS_PER_BLOCK - 1)) / F2FS_ORPHANS_PER_BLOCK);
> struct page *page = NULL;
> - struct orphan_inode_entry *orphan = NULL;
> + struct ino_entry *orphan = NULL;
>
> for (index = 0; index < orphan_blocks; index++)
> grab_meta_page(sbi, start_blk + index);
>
> index = 1;
> - spin_lock(&sbi->orphan_inode_lock);
> - head = &sbi->orphan_inode_list;
> + spin_lock(&sbi->ino_lock[ORPHAN_INO]);
> + head = &sbi->ino_list[ORPHAN_INO];
>
> /* loop for each orphan inode entry and write them in Jornal block */
> list_for_each_entry(orphan, head, list) {
> @@ -455,7 +454,7 @@ static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk)
> f2fs_put_page(page, 1);
> }
>
> - spin_unlock(&sbi->orphan_inode_lock);
> + spin_unlock(&sbi->ino_lock[ORPHAN_INO]);
> }
>
> static struct page *validate_checkpoint(struct f2fs_sb_info *sbi,
> @@ -939,31 +938,36 @@ void write_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> trace_f2fs_write_checkpoint(sbi->sb, is_umount, "finish checkpoint");
> }
>
> -void init_orphan_info(struct f2fs_sb_info *sbi)
> +void init_ino_entry_info(struct f2fs_sb_info *sbi)
> {
> - spin_lock_init(&sbi->orphan_inode_lock);
> - INIT_LIST_HEAD(&sbi->orphan_inode_list);
> - sbi->n_orphans = 0;
> + int i;
> +
> + for (i = 0; i < MAX_INO_ENTRY; i++) {
> + spin_lock_init(&sbi->ino_lock[i]);
> + INIT_LIST_HEAD(&sbi->ino_list[i]);
> + }
> +
> /*
> * considering 512 blocks in a segment 8 blocks are needed for cp
> * and log segment summaries. Remaining blocks are used to keep
> * orphan entries with the limitation one reserved segment
> * for cp pack we can have max 1020*504 orphan entries
> */
> + sbi->n_orphans = 0;
> sbi->max_orphans = (sbi->blocks_per_seg - 2 - NR_CURSEG_TYPE)
> * F2FS_ORPHANS_PER_BLOCK;
> }
>
> int __init create_checkpoint_caches(void)
> {
> - orphan_entry_slab = f2fs_kmem_cache_create("f2fs_orphan_entry",
> - sizeof(struct orphan_inode_entry));
> - if (!orphan_entry_slab)
> + ino_entry_slab = f2fs_kmem_cache_create("f2fs_ino_entry",
> + sizeof(struct ino_entry));
> + if (!ino_entry_slab)
> return -ENOMEM;
> inode_entry_slab = f2fs_kmem_cache_create("f2fs_dirty_dir_entry",
> sizeof(struct dir_inode_entry));
> if (!inode_entry_slab) {
> - kmem_cache_destroy(orphan_entry_slab);
> + kmem_cache_destroy(ino_entry_slab);
> return -ENOMEM;
> }
> return 0;
> @@ -971,6 +975,6 @@ int __init create_checkpoint_caches(void)
>
> void destroy_checkpoint_caches(void)
> {
> - kmem_cache_destroy(orphan_entry_slab);
> + kmem_cache_destroy(ino_entry_slab);
> kmem_cache_destroy(inode_entry_slab);
> }
> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
> index 3f99266..a441ba3 100644
> --- a/fs/f2fs/debug.c
> +++ b/fs/f2fs/debug.c
> @@ -167,7 +167,7 @@ get_cache:
> si->cache_mem += npages << PAGE_CACHE_SHIFT;
> npages = META_MAPPING(sbi)->nrpages;
> si->cache_mem += npages << PAGE_CACHE_SHIFT;
> - si->cache_mem += sbi->n_orphans * sizeof(struct orphan_inode_entry);
> + si->cache_mem += sbi->n_orphans * sizeof(struct ino_entry);
> si->cache_mem += sbi->n_dirty_dirs * sizeof(struct dir_inode_entry);
> }
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index e999eec..b6fa6ec 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -100,8 +100,13 @@ enum {
> META_SSA
> };
>
> -/* for the list of orphan inodes */
> -struct orphan_inode_entry {
> +/* for the list of ino */
> +enum {
> + ORPHAN_INO, /* for orphan ino list */
> + MAX_INO_ENTRY, /* max. list */
> +};
> +
> +struct ino_entry {
> struct list_head list; /* list head */
> nid_t ino; /* inode number */
> };
> @@ -450,9 +455,11 @@ struct f2fs_sb_info {
> bool por_doing; /* recovery is doing or not */
> wait_queue_head_t cp_wait;
>
> - /* for orphan inode management */
> - struct list_head orphan_inode_list; /* orphan inode list */
> - spinlock_t orphan_inode_lock; /* for orphan inode list */
> + /* for inode management */
> + spinlock_t ino_lock[MAX_INO_ENTRY]; /* for ino entry lock */
> + struct list_head ino_list[MAX_INO_ENTRY]; /* inode list head */
> +
> + /* for orphan inode, use 0'th array */
> unsigned int n_orphans; /* # of orphan inodes */
> unsigned int max_orphans; /* max orphan inodes */
>
> @@ -1255,7 +1262,7 @@ void add_dirty_dir_inode(struct inode *);
> void remove_dirty_dir_inode(struct inode *);
> void sync_dirty_dir_inodes(struct f2fs_sb_info *);
> void write_checkpoint(struct f2fs_sb_info *, bool);
> -void init_orphan_info(struct f2fs_sb_info *);
> +void init_ino_entry_info(struct f2fs_sb_info *);
> int __init create_checkpoint_caches(void);
> void destroy_checkpoint_caches(void);
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index eec89a2..5a80755 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1003,7 +1003,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> INIT_LIST_HEAD(&sbi->dir_inode_list);
> spin_lock_init(&sbi->dir_inode_lock);
>
> - init_orphan_info(sbi);
> + init_ino_entry_info(sbi);
>
> /* setup f2fs internal modules */
> err = build_segment_manager(sbi);
> --
> 1.8.5.2 (Apple Git-48)
>
>
> ------------------------------------------------------------------------------
> Want fast and easy access to all the code in your enterprise? Index and
> search up to 200,000 lines of code with a free copy of Black Duck
> Code Sight - the same software that powers the world's largest code
> search on Ohloh, the Black Duck Open Hub! Try it now.
> http://p.sf.net/sfu/bds
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2014-07-29 11:32:57

by Chao Yu

[permalink] [raw]

Subject: RE: [f2fs-dev] [PATCH 04/11] f2fs: use radix_tree for ino management

Hi,

One comment as following.

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Saturday, July 26, 2014 6:47 AM
> To: [email protected]; [email protected];
> [email protected]
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 04/11] f2fs: use radix_tree for ino management
>
> For better ino management, this patch replaces the data structure from list
> to radix tree.
>
> Signed-off-by: Jaegeuk Kim <[email protected]>

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

> ---
> fs/f2fs/checkpoint.c | 48 ++++++++++++++++++++++++++----------------------
> fs/f2fs/f2fs.h | 1 +
> 2 files changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index f93d154..d35094a 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -284,24 +284,26 @@ const struct address_space_operations f2fs_meta_aops = {
>
> static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
> {
> - struct ino_entry *new, *e;
> -
> - new = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_ATOMIC);
> - new->ino = ino;
> -
> + struct ino_entry *e;
> +retry:
> spin_lock(&sbi->ino_lock[type]);
> - list_for_each_entry(e, &sbi->ino_list[type], list) {
> - if (e->ino == ino) {
> +
> + e = radix_tree_lookup(&sbi->ino_root[type], ino);
> + if (!e) {
> + e = kmem_cache_alloc(ino_entry_slab, GFP_ATOMIC);
> + if (!e) {
> spin_unlock(&sbi->ino_lock[type]);
> - kmem_cache_free(ino_entry_slab, new);
> - return;
> + goto retry;
> }
> - if (e->ino > ino)
> - break;
> - }
> + if (radix_tree_insert(&sbi->ino_root[type], ino, e)) {
> + spin_unlock(&sbi->ino_lock[type]);

we should add kmem_cache_free(ino_entry_slab, e) here to avoid memory leak.

> + goto retry;
> + }
> + memset(e, 0, sizeof(struct ino_entry));
> + e->ino = ino;
>
> - /* add new entry into list which is sorted by inode number */
> - list_add_tail(&new->list, &e->list);
> + list_add_tail(&e->list, &sbi->ino_list[type]);
> + }
> spin_unlock(&sbi->ino_lock[type]);
> }
>
> @@ -310,14 +312,15 @@ static void __remove_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int
> type)
> struct ino_entry *e;
>
> spin_lock(&sbi->ino_lock[type]);
> - list_for_each_entry(e, &sbi->ino_list[type], list) {
> - if (e->ino == ino) {
> - list_del(&e->list);
> + e = radix_tree_lookup(&sbi->ino_root[type], ino);
> + if (e) {
> + list_del(&e->list);
> + radix_tree_delete(&sbi->ino_root[type], ino);
> + if (type == ORPHAN_INO)
> sbi->n_orphans--;
> - spin_unlock(&sbi->ino_lock[type]);
> - kmem_cache_free(ino_entry_slab, e);
> - return;
> - }
> + spin_unlock(&sbi->ino_lock[type]);
> + kmem_cache_free(ino_entry_slab, e);
> + return;
> }
> spin_unlock(&sbi->ino_lock[type]);
> }
> @@ -346,7 +349,7 @@ void release_orphan_inode(struct f2fs_sb_info *sbi)
>
> void add_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
> {
> - /* add new orphan entry into list which is sorted by inode number */
> + /* add new orphan ino entry into list */
> __add_ino_entry(sbi, ino, ORPHAN_INO);
> }
>
> @@ -943,6 +946,7 @@ void init_ino_entry_info(struct f2fs_sb_info *sbi)
> int i;
>
> for (i = 0; i < MAX_INO_ENTRY; i++) {
> + INIT_RADIX_TREE(&sbi->ino_root[i], GFP_ATOMIC);
> spin_lock_init(&sbi->ino_lock[i]);
> INIT_LIST_HEAD(&sbi->ino_list[i]);
> }
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index b6fa6ec..4454caa 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -456,6 +456,7 @@ struct f2fs_sb_info {
> wait_queue_head_t cp_wait;
>
> /* for inode management */
> + struct radix_tree_root ino_root[MAX_INO_ENTRY]; /* ino entry array */
> spinlock_t ino_lock[MAX_INO_ENTRY]; /* for ino entry lock */
> struct list_head ino_list[MAX_INO_ENTRY]; /* inode list head */
>
> --
> 1.8.5.2 (Apple Git-48)
>
>
> ------------------------------------------------------------------------------
> Want fast and easy access to all the code in your enterprise? Index and
> search up to 200,000 lines of code with a free copy of Black Duck
> Code Sight - the same software that powers the world's largest code
> search on Ohloh, the Black Duck Open Hub! Try it now.
> http://p.sf.net/sfu/bds
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2014-07-29 11:39:15

by Chao Yu

[permalink] [raw]

On Wed, Jul 30, 2014 at 08:54:55AM +0900, Changman Lee wrote:
> On Tue, Jul 29, 2014 at 05:22:15AM -0700, Jaegeuk Kim wrote:
> > Hi Changman,
> >
> > On Tue, Jul 29, 2014 at 09:41:11AM +0900, Changman Lee wrote:
> > > Hi Jaegeuk,
> > >
> > > On Fri, Jul 25, 2014 at 03:47:21PM -0700, Jaegeuk Kim wrote:
> > > > This patch enforces in-place-updates only when fdatasync is requested.
> > > > If we adopt this in-place-updates for the fdatasync, we can skip to write the
> > > > recovery information.
> > >
> > > But, as you know, random write occurs when changing into in-place-updates.
> > > It will degrade write performance. Is there any case in-place-updates is
> > > better, except recovery or high utilization?
> >
> > As I described, you can easily imagine, if users requested small amount of data
> > writes with fdatasync, we should do data writes + node writes.
> > But, if we can do in-place-update, we don't need to write node blocks.
> > Surely it triggers random writes, however, the amount of data is preety small
> > and the device handles them very fast by its inside cache, so that it can
> > enhance the performance.
> >
> > Thanks,
>
> Partially agree. Sometimes, I see that SSR shows lower performance than
> IPU. One of the reasons might be node writes.

What did you mean? That's why I consider IPU eagarly instead of SSR and LFS
under the very strict cases.

> Anyway, if so, we should know total dirty pages for fdatasync and it's very
> tunable according to a random write performance of device.

Agreed. We can do that either by comparing the number of dirty pages,
additional data/node writes, and cost of checkpoint at the same time.
And there is another thing is that we need to consider the number of
waiting time for end_io.
I'll look into this at some time.

Thanks,

>
> Thanks,
>
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Signed-off-by: Jaegeuk Kim <[email protected]>
> > > > ---
> > > > fs/f2fs/f2fs.h | 1 +
> > > > fs/f2fs/file.c | 7 +++++++
> > > > fs/f2fs/segment.h | 4 ++++
> > > > 3 files changed, 12 insertions(+)
> > > >
> > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > index ab36025..8f8685e 100644
> > > > --- a/fs/f2fs/f2fs.h
> > > > +++ b/fs/f2fs/f2fs.h
> > > > @@ -998,6 +998,7 @@ enum {
> > > > FI_INLINE_DATA, /* used for inline data*/
> > > > FI_APPEND_WRITE, /* inode has appended data */
> > > > FI_UPDATE_WRITE, /* inode has in-place-update data */
> > > > + FI_NEED_IPU, /* used fo ipu for fdatasync */
> > > > };
> > > >
> > > > static inline void set_inode_flag(struct f2fs_inode_info *fi, int flag)
> > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > > index 121689a..e339856 100644
> > > > --- a/fs/f2fs/file.c
> > > > +++ b/fs/f2fs/file.c
> > > > @@ -127,11 +127,18 @@ int f2fs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> > > > return 0;
> > > >
> > > > trace_f2fs_sync_file_enter(inode);
> > > > +
> > > > + /* if fdatasync is triggered, let's do in-place-update */
> > > > + if (datasync)
> > > > + set_inode_flag(fi, FI_NEED_IPU);
> > > > +
> > > > ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
> > > > if (ret) {
> > > > trace_f2fs_sync_file_exit(inode, need_cp, datasync, ret);
> > > > return ret;
> > > > }
> > > > + if (datasync)
> > > > + clear_inode_flag(fi, FI_NEED_IPU);
> > > >
> > > > /*
> > > > * if there is no written data, don't waste time to write recovery info.
> > > > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > > > index ee5c75e..55973f7 100644
> > > > --- a/fs/f2fs/segment.h
> > > > +++ b/fs/f2fs/segment.h
> > > > @@ -486,6 +486,10 @@ static inline bool need_inplace_update(struct inode *inode)
> > > > if (S_ISDIR(inode->i_mode))
> > > > return false;
> > > >
> > > > + /* this is only set during fdatasync */
> > > > + if (is_inode_flag_set(F2FS_I(inode), FI_NEED_IPU))
> > > > + return true;
> > > > +
> > > > switch (SM_I(sbi)->ipu_policy) {
> > > > case F2FS_IPU_FORCE:
> > > > return true;
> > > > --
> > > > 1.8.5.2 (Apple Git-48)
> > > >
> > > >
> > > > ------------------------------------------------------------------------------
> > > > Want fast and easy access to all the code in your enterprise? Index and
> > > > search up to 200,000 lines of code with a free copy of Black Duck
> > > > Code Sight - the same software that powers the world's largest code
> > > > search on Ohloh, the Black Duck Open Hub! Try it now.
> > > > http://p.sf.net/sfu/bds
> > > > _______________________________________________
> > > > Linux-f2fs-devel mailing list
> > > > [email protected]
> > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2014-07-30 01:45:41

by Chao Yu

[permalink] [raw]

Hi,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Wednesday, July 30, 2014 11:18 AM
> To: Chao Yu
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [f2fs-dev] [PATCH 08/11] f2fs: fix wrong condition for unlikely
>
> On Wed, Jul 30, 2014 at 09:44:43AM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:[email protected]]
> > > Sent: Saturday, July 26, 2014 6:47 AM
> > > To: [email protected]; [email protected];
> > > [email protected]
> > > Cc: Jaegeuk Kim
> > > Subject: [f2fs-dev] [PATCH 08/11] f2fs: fix wrong condition for unlikely
> > >
> > > This patch fixes the wrongly used unlikely condition.
> > >
> > > Signed-off-by: Jaegeuk Kim <[email protected]>

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

> > > ---
> > > fs/f2fs/checkpoint.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > > index 42a16c1..36b0d47 100644
> > > --- a/fs/f2fs/checkpoint.c
> > > +++ b/fs/f2fs/checkpoint.c
> > > @@ -932,7 +932,7 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> > > /* Here, we only have one bio having CP pack */
> > > sync_meta_pages(sbi, META_FLUSH, LONG_MAX);
> > >
> > > - if (unlikely(!is_set_ckpt_flags(ckpt, CP_ERROR_FLAG))) {
> > > + if (!is_set_ckpt_flags(ckpt, CP_ERROR_FLAG)) {
> >
> > Maybe use likely(!is_set_ckpt_flags(ckpt, CP_ERROR_FLAG)) or
> >
> > if (unlikely(is_set_ckpt_flags(ckpt, CP_ERROR_FLAG)))
> > return;
> >
> > is more appropriate. How do you think?
>
> Currently I'd like to put this without any likely or unlikely.
> Best thing is to measure some performance how this would make effect on.

I think we could get little improvement of performance if we use likely or unlikely
here. So if you'd like leave it without pre-judgment, it will be ok.
Let's just leave it as it was. :)

Thanks,

> Until then, it'd be better to do without it, since apparently this should
> not be unlikely.
>
> How about you?
> Can we compare both of them explicitly?
>
> Thanks,
>
> >
> > > clear_prefree_segments(sbi);
> > > release_dirty_inode(sbi);
> > > F2FS_RESET_SB_DIRT(sbi);
> > > --
> > > 1.8.5.2 (Apple Git-48)
> > >
> > >
> > > ------------------------------------------------------------------------------
> > > Want fast and easy access to all the code in your enterprise? Index and
> > > search up to 200,000 lines of code with a free copy of Black Duck
> > > Code Sight - the same software that powers the world's largest code
> > > search on Ohloh, the Black Duck Open Hub! Try it now.
> > > http://p.sf.net/sfu/bds
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > [email protected]
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel