| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 08 May 2013 11:42:15 +0900
From: Jaegeuk Kim <jaegeuk.kim@...sung.com>
To: linux-fsdevel@...r.kernel.org
Cc: linux-kernel@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [PATCH 2/2] f2fs: remove alloc_nid_done/fail for performance
Sorry, please ignore this patch.
2013-05-08 (수), 09:56 +0900, Jaegeuk Kim:
> This mechanism revealed two issues: bug and performance.
>
> Now, iput() doesn't guarantee that the inode is freed completely due to the
> linked f2fs_drop_inode().
> So, in the case of failure on f2fs_new_inode(), we should not add the free nid
> again to the list even though iput() is called before then.
>
> Second thing is for performance.
> When allocating a free nid, we scan to find a new NID_NEW candidate, and after
> allocation, we need to scan again to remove the free nid.
>
> So, this patch resolves the issues by removing this mechanism.
> Just do this with a simple basic list control.
>
> Signed-off-by: Jaegeuk Kim <jaegeuk.kim@...sung.com>
> ---
> fs/f2fs/f2fs.h | 2 --
> fs/f2fs/namei.c | 15 ------------
> fs/f2fs/node.c | 76 +++++++++++++--------------------------------------------
> fs/f2fs/node.h | 6 -----
> fs/f2fs/xattr.c | 2 --
> 5 files changed, 17 insertions(+), 84 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 20aab02..2fe2567 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -956,8 +956,6 @@ struct page *get_node_page_ra(struct page *, int);
> void sync_inode_page(struct dnode_of_data *);
> int sync_node_pages(struct f2fs_sb_info *, nid_t, struct writeback_control *);
> bool alloc_nid(struct f2fs_sb_info *, nid_t *);
> -void alloc_nid_done(struct f2fs_sb_info *, nid_t);
> -void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
> void recover_node_page(struct f2fs_sb_info *, struct page *,
> struct f2fs_summary *, struct node_info *, block_t);
> int recover_inode_page(struct f2fs_sb_info *, struct page *);
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 4aa26e5..077ead1 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -26,7 +26,6 @@ static struct inode *f2fs_new_inode(struct inode *dir, umode_t mode)
> struct f2fs_sb_info *sbi = F2FS_SB(sb);
> nid_t ino;
> struct inode *inode;
> - bool nid_free = false;
> int err, ilock;
>
> inode = new_inode(sb);
> @@ -60,7 +59,6 @@ static struct inode *f2fs_new_inode(struct inode *dir, umode_t mode)
> err = insert_inode_locked(inode);
> if (err) {
> err = -EINVAL;
> - nid_free = true;
> goto out;
> }
> trace_f2fs_new_inode(inode, 0);
> @@ -73,8 +71,6 @@ out:
> fail:
> trace_f2fs_new_inode(inode, err);
> iput(inode);
> - if (nid_free)
> - alloc_nid_failed(sbi, ino);
> return ERR_PTR(err);
> }
>
> @@ -146,8 +142,6 @@ static int f2fs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
> if (err)
> goto out;
>
> - alloc_nid_done(sbi, ino);
> -
> if (!sbi->por_doing)
> d_instantiate(dentry, inode);
> unlock_new_inode(inode);
> @@ -156,7 +150,6 @@ out:
> clear_nlink(inode);
> unlock_new_inode(inode);
> iput(inode);
> - alloc_nid_failed(sbi, ino);
> return err;
> }
>
> @@ -287,8 +280,6 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> goto out;
>
> err = page_symlink(inode, symname, symlen);
> - alloc_nid_done(sbi, inode->i_ino);
> -
> d_instantiate(dentry, inode);
> unlock_new_inode(inode);
> return err;
> @@ -296,7 +287,6 @@ out:
> clear_nlink(inode);
> unlock_new_inode(inode);
> iput(inode);
> - alloc_nid_failed(sbi, inode->i_ino);
> return err;
> }
>
> @@ -324,8 +314,6 @@ static int f2fs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
> if (err)
> goto out_fail;
>
> - alloc_nid_done(sbi, inode->i_ino);
> -
> d_instantiate(dentry, inode);
> unlock_new_inode(inode);
>
> @@ -336,7 +324,6 @@ out_fail:
> clear_nlink(inode);
> unlock_new_inode(inode);
> iput(inode);
> - alloc_nid_failed(sbi, inode->i_ino);
> return err;
> }
>
> @@ -375,7 +362,6 @@ static int f2fs_mknod(struct inode *dir, struct dentry *dentry,
> if (err)
> goto out;
>
> - alloc_nid_done(sbi, inode->i_ino);
> d_instantiate(dentry, inode);
> unlock_new_inode(inode);
> return 0;
> @@ -383,7 +369,6 @@ out:
> clear_nlink(inode);
> unlock_new_inode(inode);
> iput(inode);
> - alloc_nid_failed(sbi, inode->i_ino);
> return err;
> }
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 3df43b4..fa5e8dc 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -432,13 +432,11 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t index, int mode)
> dn->nid = nids[i];
> npage[i] = new_node_page(dn, noffset[i]);
> if (IS_ERR(npage[i])) {
> - alloc_nid_failed(sbi, nids[i]);
> err = PTR_ERR(npage[i]);
> goto release_pages;
> }
>
> set_nid(parent, offset[i - 1], nids[i], i == 1);
> - alloc_nid_done(sbi, nids[i]);
> done = true;
> } else if (mode == LOOKUP_NODE_RA && i == level && level > 1) {
> npage[i] = get_node_page_ra(parent, offset[i - 1]);
> @@ -1243,10 +1241,18 @@ static struct free_nid *__lookup_free_nid_list(nid_t n, struct list_head *head)
> return NULL;
> }
>
> -static void __del_from_free_nid_list(struct free_nid *i)
> +/*
> + * should be covered by free_nid_list_lock
> + */
> +static void __del_from_free_nid_list(struct f2fs_nm_info *nm_i,
> + struct free_nid *i)
> {
> + if (!i)
> + return;
> +
> list_del(&i->list);
> kmem_cache_free(free_nid_slab, i);
> + nm_i->fcnt--;
> }
>
> static int add_free_nid(struct f2fs_nm_info *nm_i, nid_t nid, bool build)
> @@ -1280,7 +1286,6 @@ retry:
> goto retry;
> }
> i->nid = nid;
> - i->state = NID_NEW;
>
> spin_lock(&nm_i->free_nid_list_lock);
> if (__lookup_free_nid_list(nid, &nm_i->free_nid_list)) {
> @@ -1299,10 +1304,7 @@ static void remove_free_nid(struct f2fs_nm_info *nm_i, nid_t nid)
> struct free_nid *i;
> spin_lock(&nm_i->free_nid_list_lock);
> i = __lookup_free_nid_list(nid, &nm_i->free_nid_list);
> - if (i && i->state == NID_NEW) {
> - __del_from_free_nid_list(i);
> - nm_i->fcnt--;
> - }
> + __del_from_free_nid_list(nm_i, i);
> spin_unlock(&nm_i->free_nid_list_lock);
> }
>
> @@ -1382,8 +1384,6 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
> bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
> {
> struct f2fs_nm_info *nm_i = NM_I(sbi);
> - struct free_nid *i = NULL;
> - struct list_head *this;
> retry:
> if (sbi->total_valid_node_count + 1 >= nm_i->max_nid)
> return false;
> @@ -1392,17 +1392,13 @@ retry:
>
> /* We should not use stale free nids created by build_free_nids */
> if (nm_i->fcnt && !sbi->on_build_free_nids) {
> - BUG_ON(list_empty(&nm_i->free_nid_list));
> - list_for_each(this, &nm_i->free_nid_list) {
> - i = list_entry(this, struct free_nid, list);
> - if (i->state == NID_NEW)
> - break;
> - }
> + struct list_head *flist = &nm_i->free_nid_list;
> + struct free_nid *i;
>
> - BUG_ON(i->state != NID_NEW);
> + BUG_ON(list_empty(flist));
> + i = list_first_entry(flist, struct free_nid, list);
> *nid = i->nid;
> - i->state = NID_ALLOC;
> - nm_i->fcnt--;
> + __del_from_free_nid_list(nm_i, i);
> spin_unlock(&nm_i->free_nid_list_lock);
> return true;
> }
> @@ -1417,41 +1413,6 @@ retry:
> goto retry;
> }
>
> -/*
> - * alloc_nid() should be called prior to this function.
> - */
> -void alloc_nid_done(struct f2fs_sb_info *sbi, nid_t nid)
> -{
> - struct f2fs_nm_info *nm_i = NM_I(sbi);
> - struct free_nid *i;
> -
> - spin_lock(&nm_i->free_nid_list_lock);
> - i = __lookup_free_nid_list(nid, &nm_i->free_nid_list);
> - BUG_ON(!i || i->state != NID_ALLOC);
> - __del_from_free_nid_list(i);
> - spin_unlock(&nm_i->free_nid_list_lock);
> -}
> -
> -/*
> - * alloc_nid() should be called prior to this function.
> - */
> -void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid)
> -{
> - struct f2fs_nm_info *nm_i = NM_I(sbi);
> - struct free_nid *i;
> -
> - spin_lock(&nm_i->free_nid_list_lock);
> - i = __lookup_free_nid_list(nid, &nm_i->free_nid_list);
> - BUG_ON(!i || i->state != NID_ALLOC);
> - if (nm_i->fcnt > 2 * MAX_FREE_NIDS) {
> - __del_from_free_nid_list(i);
> - } else {
> - i->state = NID_NEW;
> - nm_i->fcnt++;
> - }
> - spin_unlock(&nm_i->free_nid_list_lock);
> -}
> -
> void recover_node_page(struct f2fs_sb_info *sbi, struct page *page,
> struct f2fs_summary *sum, struct node_info *ni,
> block_t new_blkaddr)
> @@ -1747,11 +1708,8 @@ void destroy_node_manager(struct f2fs_sb_info *sbi)
>
> /* destroy free nid list */
> spin_lock(&nm_i->free_nid_list_lock);
> - list_for_each_entry_safe(i, next_i, &nm_i->free_nid_list, list) {
> - BUG_ON(i->state == NID_ALLOC);
> - __del_from_free_nid_list(i);
> - nm_i->fcnt--;
> - }
> + list_for_each_entry_safe(i, next_i, &nm_i->free_nid_list, list)
> + __del_from_free_nid_list(nm_i, i);
> BUG_ON(nm_i->fcnt);
> spin_unlock(&nm_i->free_nid_list_lock);
>
> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> index 0a2d72f..2f226a4 100644
> --- a/fs/f2fs/node.h
> +++ b/fs/f2fs/node.h
> @@ -74,15 +74,9 @@ static inline void node_info_from_raw_nat(struct node_info *ni,
> /*
> * For free nid mangement
> */
> -enum nid_state {
> - NID_NEW, /* newly added to free nid list */
> - NID_ALLOC /* it is allocated */
> -};
> -
> struct free_nid {
> struct list_head list; /* for free node id list */
> nid_t nid; /* node id */
> - int state; /* in use or not: NID_NEW or NID_ALLOC */
> };
>
> static inline int next_free_nid(struct f2fs_sb_info *sbi, nid_t *nid)
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 0b02dce..5675920 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -337,13 +337,11 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
>
> page = new_node_page(&dn, XATTR_NODE_OFFSET);
> if (IS_ERR(page)) {
> - alloc_nid_failed(sbi, fi->i_xattr_nid);
> fi->i_xattr_nid = 0;
> error = PTR_ERR(page);
> goto exit;
> }
>
> - alloc_nid_done(sbi, fi->i_xattr_nid);
> base_addr = page_address(page);
> header = XATTR_HDR(base_addr);
> header->h_magic = cpu_to_le32(F2FS_XATTR_MAGIC);
--
Jaegeuk Kim
Samsung
Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)
Powered by blists - more mailing lists