From: Chao Yu <chao2.yu@samsung.com>
To: "'Jaegeuk Kim'" <jaegeuk@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net
Subject: RE: [f2fs-dev] [PATCH v2] f2fs: fix to release inode page in get_new_data_page
Date: Wed, 05 Aug 2015 17:25:12 +0800 [thread overview]
Message-ID: <009f01d0cf60$b96a5cf0$2c3f16d0$@samsung.com> (raw)
In-Reply-To: <001701d0cdec$d74ba820$85e2f860$@samsung.com>
Hi Jaegeuk,
How about using v3 instead of v2 since it fixes the same type issue
in f2fs_convert_inline_dir?
Thanks,
> -----Original Message-----
> From: Chao Yu [mailto:chao2.yu@samsung.com]
> Sent: Monday, August 03, 2015 9:03 PM
> To: 'Jaegeuk Kim'
> Cc: linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH v2] f2fs: fix to release inode page in get_new_data_page
>
> >From c0f2abda489150d5d458aaae9026f1243daea604 Mon Sep 17 00:00:00 2001
> From: Chao Yu <chao2.yu@samsung.com>
> Date: Tue, 14 Jul 2015 18:14:06 +0800
> Subject: [PATCH v3] f2fs: fix to release inode page correctly
>
> In following call path, we will pass a locked and referenced ipage
> pointer to get_new_data_page:
> - init_inode_metadata
> - make_empty_dir
> - get_new_data_page
>
> There are two exit paths in get_new_data_page when error occurs:
> 1) grab_cache_page fails, ipage will not be released;
> 2) f2fs_reserve_block fails, ipage will be released in callee.
>
> So, it's not consistent for error handling in get_new_data_page.
>
> For f2fs_reserve_block, it's not very easy to change the rule
> of error handling, since it's already complicated.
>
> Here we deside to choose an easy way to fix this issue:
> If any error occur in get_new_data_page, we will ensure releasing
> ipage in this function.
>
> The same issue is in f2fs_convert_inline_dir, fix that too.
>
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
> v3:
> * fix the same issue in f2fs_convert_inline_dir.
>
> fs/f2fs/data.c | 11 +++++++++--
> fs/f2fs/inline.c | 13 ++++++++++---
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index e58562e..5da0529 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -387,7 +387,8 @@ repeat:
> *
> * Also, caller should grab and release a rwsem by calling f2fs_lock_op() and
> * f2fs_unlock_op().
> - * Note that, ipage is set only by make_empty_dir.
> + * Note that, ipage is set only by make_empty_dir, and if any error occur,
> + * ipage should be released by this function.
> */
> struct page *get_new_data_page(struct inode *inode,
> struct page *ipage, pgoff_t index, bool new_i_size)
> @@ -398,8 +399,14 @@ struct page *get_new_data_page(struct inode *inode,
> int err;
> repeat:
> page = grab_cache_page(mapping, index);
> - if (!page)
> + if (!page) {
> + /*
> + * before exiting, we should make sure ipage will be released
> + * if any error occur.
> + */
> + f2fs_put_page(ipage, 1);
> return ERR_PTR(-ENOMEM);
> + }
>
> set_new_dnode(&dn, inode, ipage, NULL, 0);
> err = f2fs_reserve_block(&dn, index);
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index a13ffcc..79d18d5 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -360,6 +360,10 @@ int make_empty_inline_dir(struct inode *inode, struct inode *parent,
> return 0;
> }
>
> +/*
> + * NOTE: ipage is grabbed by caller, but if any error occurs, we should
> + * release ipage in this function.
> + */
> static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
> struct f2fs_inline_dentry *inline_dentry)
> {
> @@ -369,8 +373,10 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
> int err;
>
> page = grab_cache_page(dir->i_mapping, 0);
> - if (!page)
> + if (!page) {
> + f2fs_put_page(ipage, 1);
> return -ENOMEM;
> + }
>
> set_new_dnode(&dn, dir, ipage, NULL, 0);
> err = f2fs_reserve_block(&dn, 0);
> @@ -434,8 +440,9 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *name,
> slots, NR_INLINE_DENTRY);
> if (bit_pos >= NR_INLINE_DENTRY) {
> err = f2fs_convert_inline_dir(dir, ipage, dentry_blk);
> - if (!err)
> - err = -EAGAIN;
> + if (err)
> + return err;
> + err = -EAGAIN;
> goto out;
> }
>
> --
> 2.4.2
>
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
WARNING: multiple messages have this Message-ID (diff)
From: Chao Yu <chao2.yu@samsung.com>
To: 'Jaegeuk Kim' <jaegeuk@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [PATCH v2] f2fs: fix to release inode page in get_new_data_page
Date: Wed, 05 Aug 2015 17:25:12 +0800 [thread overview]
Message-ID: <009f01d0cf60$b96a5cf0$2c3f16d0$@samsung.com> (raw)
In-Reply-To: <001701d0cdec$d74ba820$85e2f860$@samsung.com>
Hi Jaegeuk,
How about using v3 instead of v2 since it fixes the same type issue
in f2fs_convert_inline_dir?
Thanks,
> -----Original Message-----
> From: Chao Yu [mailto:chao2.yu@samsung.com]
> Sent: Monday, August 03, 2015 9:03 PM
> To: 'Jaegeuk Kim'
> Cc: linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH v2] f2fs: fix to release inode page in get_new_data_page
>
> >From c0f2abda489150d5d458aaae9026f1243daea604 Mon Sep 17 00:00:00 2001
> From: Chao Yu <chao2.yu@samsung.com>
> Date: Tue, 14 Jul 2015 18:14:06 +0800
> Subject: [PATCH v3] f2fs: fix to release inode page correctly
>
> In following call path, we will pass a locked and referenced ipage
> pointer to get_new_data_page:
> - init_inode_metadata
> - make_empty_dir
> - get_new_data_page
>
> There are two exit paths in get_new_data_page when error occurs:
> 1) grab_cache_page fails, ipage will not be released;
> 2) f2fs_reserve_block fails, ipage will be released in callee.
>
> So, it's not consistent for error handling in get_new_data_page.
>
> For f2fs_reserve_block, it's not very easy to change the rule
> of error handling, since it's already complicated.
>
> Here we deside to choose an easy way to fix this issue:
> If any error occur in get_new_data_page, we will ensure releasing
> ipage in this function.
>
> The same issue is in f2fs_convert_inline_dir, fix that too.
>
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
> v3:
> * fix the same issue in f2fs_convert_inline_dir.
>
> fs/f2fs/data.c | 11 +++++++++--
> fs/f2fs/inline.c | 13 ++++++++++---
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index e58562e..5da0529 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -387,7 +387,8 @@ repeat:
> *
> * Also, caller should grab and release a rwsem by calling f2fs_lock_op() and
> * f2fs_unlock_op().
> - * Note that, ipage is set only by make_empty_dir.
> + * Note that, ipage is set only by make_empty_dir, and if any error occur,
> + * ipage should be released by this function.
> */
> struct page *get_new_data_page(struct inode *inode,
> struct page *ipage, pgoff_t index, bool new_i_size)
> @@ -398,8 +399,14 @@ struct page *get_new_data_page(struct inode *inode,
> int err;
> repeat:
> page = grab_cache_page(mapping, index);
> - if (!page)
> + if (!page) {
> + /*
> + * before exiting, we should make sure ipage will be released
> + * if any error occur.
> + */
> + f2fs_put_page(ipage, 1);
> return ERR_PTR(-ENOMEM);
> + }
>
> set_new_dnode(&dn, inode, ipage, NULL, 0);
> err = f2fs_reserve_block(&dn, index);
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index a13ffcc..79d18d5 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -360,6 +360,10 @@ int make_empty_inline_dir(struct inode *inode, struct inode *parent,
> return 0;
> }
>
> +/*
> + * NOTE: ipage is grabbed by caller, but if any error occurs, we should
> + * release ipage in this function.
> + */
> static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
> struct f2fs_inline_dentry *inline_dentry)
> {
> @@ -369,8 +373,10 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
> int err;
>
> page = grab_cache_page(dir->i_mapping, 0);
> - if (!page)
> + if (!page) {
> + f2fs_put_page(ipage, 1);
> return -ENOMEM;
> + }
>
> set_new_dnode(&dn, dir, ipage, NULL, 0);
> err = f2fs_reserve_block(&dn, 0);
> @@ -434,8 +440,9 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *name,
> slots, NR_INLINE_DENTRY);
> if (bit_pos >= NR_INLINE_DENTRY) {
> err = f2fs_convert_inline_dir(dir, ipage, dentry_blk);
> - if (!err)
> - err = -EAGAIN;
> + if (err)
> + return err;
> + err = -EAGAIN;
> goto out;
> }
>
> --
> 2.4.2
>
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
------------------------------------------------------------------------------
next prev parent reply other threads:[~2015-08-05 9:25 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-14 10:14 [PATCH v2] f2fs: fix to release inode page in get_new_data_page Chao Yu
2015-08-03 13:03 ` [f2fs-dev] " Chao Yu
2015-08-05 9:25 ` Chao Yu [this message]
2015-08-05 9:25 ` Chao Yu
2015-08-05 15:12 ` [f2fs-dev] " Jaegeuk Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='009f01d0cf60$b96a5cf0$2c3f16d0$@samsung.com' \
--to=chao2.yu@samsung.com \
--cc=jaegeuk@kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.