Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Mon, Sep 15, 2014 at 01:14:09PM +0800, Huang Ying wrote:
> On Sun, 2014-09-14 at 00:48 -0700, Jaegeuk Kim wrote:
> > On Sat, Sep 13, 2014 at 10:23:18PM +0800, Huang Ying wrote:
> > > On Fri, 2014-09-12 at 15:34 +0800, Huang Ying wrote:
> > > > On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
> > > > > On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
> > > > > >
> > > > > > On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
> > > > > > > On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > > > > > > > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > > > > > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > > > > > > > Hi Huang,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > For fsync, if the nid of a non-inode dnode < nid of
> > > > > > > > > > > > > > inode and the
> > > > > > > > > > > > > > inode is not checkpointed. The non-inode dnode may
> > > > > > > > > > > > > > be written
> > > > > > > > > before
> > > > > > > > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will
> > > > > > > > > > > > > > fail, cause the
> > > > > > > > > > > > > > recovery fail.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Usually, inode will be allocated before non-inode
> > > > > > > > > > > > > > dnode, so the
> > > > > > > > > nid
> > > > > > > > > > > of
> > > > > > > > > > > > > > inode < nid of non-inode dnode. But it is possible
> > > > > > > > > > > > > > for the
> > > > > > > > > reverse.
> > > > > > > > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This is fixed via ignoring non-inode dnode before
> > > > > > > > > > > > > > inode dnode in
> > > > > > > > > > > > > > find_fsync_dnodes.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > The patch was tested via allocating nid reversely
> > > > > > > > > > > > > > via a debugging
> > > > > > > > > > > > > > patch, that is, from big number to small number.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > > > > > > > @@ -172,8 +172,8 @@ static int
> > > > > > > > > > > > > > find_fsync_dnodes(struct f2fs
> > > > > > > > > > > > > > if (IS_INODE(page) &&
> > > > > > > > > > > > > > is_dent_dnode(page))
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > FI_INC_LINK);
> > > > > > > > > > > > > > - } else {
> > > > > > > > > > > > > > - if (IS_INODE(page) &&
> > > > > > > > > > > > > > is_dent_dnode(page)) {
> > > > > > > > > > > > >
> > > > > > > > > > > > > If this is not inode block, we should add this inode
> > > > > > > > > > > > > to recover its
> > > > > > > > > > > data blocks.
> > > > > > > > > > > >
> > > > > > > > > > > > Is it possible that there is only non-inode dnode but
> > > > > > > > > > > > no inode when
> > > > > > > > > > > > find_fsync_dnodes checking dnodes? Per my
> > > > > > > > > > > > understanding, any
> > > > > > > > > changes to
> > > > > > > > > > > > file will cause inode page dirty (for example, mtime
> > > > > > > > > > > > changed), so
> > > > > > > > > that
> > > > > > > > > > > > we will write inode block. Is it right? If so, the
> > > > > > > > > > > > solution in this
> > > > > > > > > > > > patch should work too.
> > > > > > > > > > >
> > > > > > > > > > > Your description says that f2fs_iget will fail, which
> > > > > > > > > > > causes the
> > > > > > > > > recovery
> > > > > > > > > > > fail.
> > > > > > > > > > > So, I thought it would be better to handle the f2fs_iget
> > > > > > > > > > > failure
> > > > > > > > > directly.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Yes. That is another way to fix the issue.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > In addition, we cannot guarantee the write order of dnode
> > > > > > > > > > > and inode.
> > > > > > > > > > > For exmaple,
> > > > > > > > > > > 1. the inode is written by flusher or kswapd, then,
> > > > > >
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Mon, Sep 15, 2014 at 01:14:09PM +0800, Huang Ying wrote:
On Sun, 2014-09-14 at 00:48 -0700, Jaegeuk Kim wrote:
On Sat, Sep 13, 2014 at 10:23:18PM +0800, Huang Ying wrote:
On Fri, 2014-09-12 at 15:34 +0800, Huang Ying wrote:
On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim
jaeg...@kernel.org wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
jaeg...@kernel.org wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying
wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying
wrote:
For fsync, if the nid of a non-inode dnode nid of
inode and the
inode is not checkpointed. The non-inode dnode may
be written
before
inode. So in find_fsync_dnodes, f2fs_iget will
fail, cause the
recovery fail.
Usually, inode will be allocated before non-inode
dnode, so the
nid
of
inode nid of non-inode dnode. But it is possible
for the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before
inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely
via a debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int
find_fsync_dnodes(struct f2fs
if (IS_INODE(page)
is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page)
is_dent_dnode(page)) {
If this is not inode block, we should add this inode
to recover its
data blocks.
Is it possible that there is only non-inode dnode but
no inode when
find_fsync_dnodes checking dnodes? Per my
understanding, any
changes to
file will cause inode page dirty (for example, mtime
changed), so
that
we will write inode block. Is it right? If so, the
solution in this
patch should work too.
Your description says that f2fs_iget will fail, which
causes the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget
failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode
and inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node
chain, since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all
scenarios. If
the inode is checkpointed, the file can be recovered,
although the inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) |
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Sun, 2014-09-14 at 00:48 -0700, Jaegeuk Kim wrote:
> On Sat, Sep 13, 2014 at 10:23:18PM +0800, Huang Ying wrote:
> > On Fri, 2014-09-12 at 15:34 +0800, Huang Ying wrote:
> > > On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
> > > > On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
> > > > >
> > > > > On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
> > > > > > On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > > > > > > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim
> > > > > > > wrote:
> > > > > > >
> > > > > > > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > > > > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > > > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > > > > > > Hi Huang,
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > For fsync, if the nid of a non-inode dnode < nid of
> > > > > > > > > > > > > inode and the
> > > > > > > > > > > > > inode is not checkpointed. The non-inode dnode may
> > > > > > > > > > > > > be written
> > > > > > > > before
> > > > > > > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail,
> > > > > > > > > > > > > cause the
> > > > > > > > > > > > > recovery fail.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Usually, inode will be allocated before non-inode
> > > > > > > > > > > > > dnode, so the
> > > > > > > > nid
> > > > > > > > > > of
> > > > > > > > > > > > > inode < nid of non-inode dnode. But it is possible
> > > > > > > > > > > > > for the
> > > > > > > > reverse.
> > > > > > > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > > > > > > >
> > > > > > > > > > > > > This is fixed via ignoring non-inode dnode before
> > > > > > > > > > > > > inode dnode in
> > > > > > > > > > > > > find_fsync_dnodes.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The patch was tested via allocating nid reversely via
> > > > > > > > > > > > > a debugging
> > > > > > > > > > > > > patch, that is, from big number to small number.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > > > > > > @@ -172,8 +172,8 @@ static int
> > > > > > > > > > > > > find_fsync_dnodes(struct f2fs
> > > > > > > > > > > > > if (IS_INODE(page) &&
> > > > > > > > > > > > > is_dent_dnode(page))
> > > > > > > > > > > > >
> > > > > > > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > > > > > >
> > > > > > > > > > > > > FI_INC_LINK);
> > > > > > > > > > > > > - } else {
> > > > > > > > > > > > > - if (IS_INODE(page) &&
> > > > > > > > > > > > > is_dent_dnode(page)) {
> > > > > > > > > > > >
> > > > > > > > > > > > If this is not inode block, we should add this inode to
> > > > > > > > > > > > recover its
> > > > > > > > > > data blocks.
> > > > > > > > > > >
> > > > > > > > > > > Is it possible that there is only non-inode dnode but no
> > > > > > > > > > > inode when
> > > > > > > > > > > find_fsync_dnodes checking dnodes? Per my understanding,
> > > > > > > > > > > any
> > > > > > > > changes to
> > > > > > > > > > > file will cause inode page dirty (for example, mtime
> > > > > > > > > > > changed), so
> > > > > > > > that
> > > > > > > > > > > we will write inode block. Is it right? If so, the
> > > > > > > > > > > solution in this
> > > > > > > > > > > patch should work too.
> > > > > > > > > >
> > > > > > > > > > Your description says that f2fs_iget will fail, which
> > > > > > > > > > causes the
> > > > > > > > recovery
> > > > > > > > > > fail.
> > > > > > > > > > So, I thought it would be better to handle the f2fs_iget
> > > > > > > > > > failure
> > > > > > > > directly.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yes. That is another way to fix the issue.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > In addition, we cannot guarantee the write order of dnode
> > > > > > > > > > and inode.
> > > > > > > > > > For exmaple,
> > > > > > > > > > 1. the inode is written by flusher or kswapd, then,
> > > > > > > > > > 2. f2fs_sync_file writes its dnode.
> > > > > > > > > >
> > > > > > > > > > In that case, we can get only non-inode dnode in the node
> > > > > > > > > > chain, since
> > > > > > > > the
> > > > > > > > > > inode
> > > > > > > > > > has not fsync_mark.
> > > > > > > > > >
>
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Sun, 2014-09-14 at 00:48 -0700, Jaegeuk Kim wrote:
On Sat, Sep 13, 2014 at 10:23:18PM +0800, Huang Ying wrote:
On Fri, 2014-09-12 at 15:34 +0800, Huang Ying wrote:
On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org
wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
jaeg...@kernel.org wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying
wrote:
For fsync, if the nid of a non-inode dnode nid of
inode and the
inode is not checkpointed. The non-inode dnode may
be written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail,
cause the
recovery fail.
Usually, inode will be allocated before non-inode
dnode, so the
nid
of
inode nid of non-inode dnode. But it is possible
for the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before
inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via
a debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int
find_fsync_dnodes(struct f2fs
if (IS_INODE(page)
is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page)
is_dent_dnode(page)) {
If this is not inode block, we should add this inode to
recover its
data blocks.
Is it possible that there is only non-inode dnode but no
inode when
find_fsync_dnodes checking dnodes? Per my understanding,
any
changes to
file will cause inode page dirty (for example, mtime
changed), so
that
we will write inode block. Is it right? If so, the
solution in this
patch should work too.
Your description says that f2fs_iget will fail, which
causes the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget
failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode
and inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node
chain, since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all
scenarios. If
the inode is checkpointed, the file can be recovered,
although the inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an
impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Sun, 2014-09-14 at 00:38 -0700, Jaegeuk Kim wrote:
> On Fri, Sep 12, 2014 at 03:34:48PM +0800, Huang Ying wrote:
> > On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
> > > On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
> > > >
> > > > On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
> > > > > On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > > > > > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim
> > > > > > wrote:
> > > > > >
> > > > > > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > > > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > > > > > Hi Huang,
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying
> > > > > > > > > > > wrote:
> > > > > > > > > > > > For fsync, if the nid of a non-inode dnode < nid of
> > > > > > > > > > > > inode and the
> > > > > > > > > > > > inode is not checkpointed. The non-inode dnode may be
> > > > > > > > > > > > written
> > > > > > > before
> > > > > > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail,
> > > > > > > > > > > > cause the
> > > > > > > > > > > > recovery fail.
> > > > > > > > > > > >
> > > > > > > > > > > > Usually, inode will be allocated before non-inode
> > > > > > > > > > > > dnode, so the
> > > > > > > nid
> > > > > > > > > of
> > > > > > > > > > > > inode < nid of non-inode dnode. But it is possible for
> > > > > > > > > > > > the
> > > > > > > reverse.
> > > > > > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > > > > > >
> > > > > > > > > > > > This is fixed via ignoring non-inode dnode before inode
> > > > > > > > > > > > dnode in
> > > > > > > > > > > > find_fsync_dnodes.
> > > > > > > > > > > >
> > > > > > > > > > > > The patch was tested via allocating nid reversely via a
> > > > > > > > > > > > debugging
> > > > > > > > > > > > patch, that is, from big number to small number.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > > > > > ---
> > > > > > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct
> > > > > > > > > > > > f2fs
> > > > > > > > > > > > if (IS_INODE(page) &&
> > > > > > > > > > > > is_dent_dnode(page))
> > > > > > > > > > > >
> > > > > > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > > > > >
> > > > > > > > > > > > FI_INC_LINK);
> > > > > > > > > > > > - } else {
> > > > > > > > > > > > - if (IS_INODE(page) &&
> > > > > > > > > > > > is_dent_dnode(page)) {
> > > > > > > > > > >
> > > > > > > > > > > If this is not inode block, we should add this inode to
> > > > > > > > > > > recover its
> > > > > > > > > data blocks.
> > > > > > > > > >
> > > > > > > > > > Is it possible that there is only non-inode dnode but no
> > > > > > > > > > inode when
> > > > > > > > > > find_fsync_dnodes checking dnodes? Per my understanding,
> > > > > > > > > > any
> > > > > > > changes to
> > > > > > > > > > file will cause inode page dirty (for example, mtime
> > > > > > > > > > changed), so
> > > > > > > that
> > > > > > > > > > we will write inode block. Is it right? If so, the
> > > > > > > > > > solution in this
> > > > > > > > > > patch should work too.
> > > > > > > > >
> > > > > > > > > Your description says that f2fs_iget will fail, which causes
> > > > > > > > > the
> > > > > > > recovery
> > > > > > > > > fail.
> > > > > > > > > So, I thought it would be better to handle the f2fs_iget
> > > > > > > > > failure
> > > > > > > directly.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Yes. That is another way to fix the issue.
> > > > > > > >
> > > > > > > >
> > > > > > > > > In addition, we cannot guarantee the write order of dnode and
> > > > > > > > > inode.
> > > > > > > > > For exmaple,
> > > > > > > > > 1. the inode is written by flusher or kswapd, then,
> > > > > > > > > 2. f2fs_sync_file writes its dnode.
> > > > > > > > >
> > > > > > > > > In that case, we can get only non-inode dnode in the node
> > > > > > > > > chain, since
> > > > > > > the
> > > > > > > > > inode
> > > > > > > > > has not fsync_mark.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I think your solution is better here, but does not fix all
> > > > > > > > scenarios. If
> > > > > > > > the inode is checkpointed, the file can be recovered, although
> > > > > > > > the inode
> > > > > > > > information may be not
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Sat, Sep 13, 2014 at 10:23:18PM +0800, Huang Ying wrote:
> On Fri, 2014-09-12 at 15:34 +0800, Huang Ying wrote:
> > On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
> > > On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
> > > >
> > > > On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
> > > > > On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > > > > > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim
> > > > > > wrote:
> > > > > >
> > > > > > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > > > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > > > > > Hi Huang,
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying
> > > > > > > > > > > wrote:
> > > > > > > > > > > > For fsync, if the nid of a non-inode dnode < nid of
> > > > > > > > > > > > inode and the
> > > > > > > > > > > > inode is not checkpointed. The non-inode dnode may be
> > > > > > > > > > > > written
> > > > > > > before
> > > > > > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail,
> > > > > > > > > > > > cause the
> > > > > > > > > > > > recovery fail.
> > > > > > > > > > > >
> > > > > > > > > > > > Usually, inode will be allocated before non-inode
> > > > > > > > > > > > dnode, so the
> > > > > > > nid
> > > > > > > > > of
> > > > > > > > > > > > inode < nid of non-inode dnode. But it is possible for
> > > > > > > > > > > > the
> > > > > > > reverse.
> > > > > > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > > > > > >
> > > > > > > > > > > > This is fixed via ignoring non-inode dnode before inode
> > > > > > > > > > > > dnode in
> > > > > > > > > > > > find_fsync_dnodes.
> > > > > > > > > > > >
> > > > > > > > > > > > The patch was tested via allocating nid reversely via a
> > > > > > > > > > > > debugging
> > > > > > > > > > > > patch, that is, from big number to small number.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > > > > > ---
> > > > > > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct
> > > > > > > > > > > > f2fs
> > > > > > > > > > > > if (IS_INODE(page) &&
> > > > > > > > > > > > is_dent_dnode(page))
> > > > > > > > > > > >
> > > > > > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > > > > >
> > > > > > > > > > > > FI_INC_LINK);
> > > > > > > > > > > > - } else {
> > > > > > > > > > > > - if (IS_INODE(page) &&
> > > > > > > > > > > > is_dent_dnode(page)) {
> > > > > > > > > > >
> > > > > > > > > > > If this is not inode block, we should add this inode to
> > > > > > > > > > > recover its
> > > > > > > > > data blocks.
> > > > > > > > > >
> > > > > > > > > > Is it possible that there is only non-inode dnode but no
> > > > > > > > > > inode when
> > > > > > > > > > find_fsync_dnodes checking dnodes? Per my understanding,
> > > > > > > > > > any
> > > > > > > changes to
> > > > > > > > > > file will cause inode page dirty (for example, mtime
> > > > > > > > > > changed), so
> > > > > > > that
> > > > > > > > > > we will write inode block. Is it right? If so, the
> > > > > > > > > > solution in this
> > > > > > > > > > patch should work too.
> > > > > > > > >
> > > > > > > > > Your description says that f2fs_iget will fail, which causes
> > > > > > > > > the
> > > > > > > recovery
> > > > > > > > > fail.
> > > > > > > > > So, I thought it would be better to handle the f2fs_iget
> > > > > > > > > failure
> > > > > > > directly.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Yes. That is another way to fix the issue.
> > > > > > > >
> > > > > > > >
> > > > > > > > > In addition, we cannot guarantee the write order of dnode and
> > > > > > > > > inode.
> > > > > > > > > For exmaple,
> > > > > > > > > 1. the inode is written by flusher or kswapd, then,
> > > > > > > > > 2. f2fs_sync_file writes its dnode.
> > > > > > > > >
> > > > > > > > > In that case, we can get only non-inode dnode in the node
> > > > > > > > > chain, since
> > > > > > > the
> > > > > > > > > inode
> > > > > > > > > has not fsync_mark.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I think your solution is better here, but does not fix all
> > > > > > > > scenarios. If
> > > > > > > > the inode is checkpointed, the file can be recovered, although
> > > > > > > > the inode
> > > > > > > > information may be not
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Fri, Sep 12, 2014 at 03:34:48PM +0800, Huang Ying wrote:
> On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
> > On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
> > >
> > > On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
> > > > On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > > > > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim
> > > > > wrote:
> > > > >
> > > > > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > > > > Hi Huang,
> > > > > > > > > >
> > > > > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > > > > > > > > > > For fsync, if the nid of a non-inode dnode < nid of inode
> > > > > > > > > > > and the
> > > > > > > > > > > inode is not checkpointed. The non-inode dnode may be
> > > > > > > > > > > written
> > > > > > before
> > > > > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail,
> > > > > > > > > > > cause the
> > > > > > > > > > > recovery fail.
> > > > > > > > > > >
> > > > > > > > > > > Usually, inode will be allocated before non-inode dnode,
> > > > > > > > > > > so the
> > > > > > nid
> > > > > > > > of
> > > > > > > > > > > inode < nid of non-inode dnode. But it is possible for
> > > > > > > > > > > the
> > > > > > reverse.
> > > > > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > > > > >
> > > > > > > > > > > This is fixed via ignoring non-inode dnode before inode
> > > > > > > > > > > dnode in
> > > > > > > > > > > find_fsync_dnodes.
> > > > > > > > > > >
> > > > > > > > > > > The patch was tested via allocating nid reversely via a
> > > > > > > > > > > debugging
> > > > > > > > > > > patch, that is, from big number to small number.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > > > > ---
> > > > > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct
> > > > > > > > > > > f2fs
> > > > > > > > > > > if (IS_INODE(page) &&
> > > > > > > > > > > is_dent_dnode(page))
> > > > > > > > > > >
> > > > > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > > > >
> > > > > > > > > > > FI_INC_LINK);
> > > > > > > > > > > - } else {
> > > > > > > > > > > - if (IS_INODE(page) &&
> > > > > > > > > > > is_dent_dnode(page)) {
> > > > > > > > > >
> > > > > > > > > > If this is not inode block, we should add this inode to
> > > > > > > > > > recover its
> > > > > > > > data blocks.
> > > > > > > > >
> > > > > > > > > Is it possible that there is only non-inode dnode but no
> > > > > > > > > inode when
> > > > > > > > > find_fsync_dnodes checking dnodes? Per my understanding, any
> > > > > > changes to
> > > > > > > > > file will cause inode page dirty (for example, mtime
> > > > > > > > > changed), so
> > > > > > that
> > > > > > > > > we will write inode block. Is it right? If so, the solution
> > > > > > > > > in this
> > > > > > > > > patch should work too.
> > > > > > > >
> > > > > > > > Your description says that f2fs_iget will fail, which causes the
> > > > > > recovery
> > > > > > > > fail.
> > > > > > > > So, I thought it would be better to handle the f2fs_iget failure
> > > > > > directly.
> > > > > > > >
> > > > > > >
> > > > > > > Yes. That is another way to fix the issue.
> > > > > > >
> > > > > > >
> > > > > > > > In addition, we cannot guarantee the write order of dnode and
> > > > > > > > inode.
> > > > > > > > For exmaple,
> > > > > > > > 1. the inode is written by flusher or kswapd, then,
> > > > > > > > 2. f2fs_sync_file writes its dnode.
> > > > > > > >
> > > > > > > > In that case, we can get only non-inode dnode in the node
> > > > > > > > chain, since
> > > > > > the
> > > > > > > > inode
> > > > > > > > has not fsync_mark.
> > > > > > > >
> > > > > > >
> > > > > > > I think your solution is better here, but does not fix all
> > > > > > > scenarios. If
> > > > > > > the inode is checkpointed, the file can be recovered, although
> > > > > > > the inode
> > > > > > > information may be not up to date. But if the inode is not
> > > > > > > checkpointed,
> > > > > > > f2fs_iget will fail too and recover will fail.
> > > > > >
> > > > > > Ok, let me consider your scenarios.
> > > > > >
> > > > > > Term: F: fsync_mark, D: dentry_mark
> > > > > >
> > > > > > 1. inode(x) | CP | inode(x) | dnode(F)
> > > > > > -> Lose the latest inode(x).
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Fri, Sep 12, 2014 at 03:34:48PM +0800, Huang Ying wrote:
On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org
wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim jaeg...@kernel.org
wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode
and the
inode is not checkpointed. The non-inode dnode may be
written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail,
cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode,
so the
nid
of
inode nid of non-inode dnode. But it is possible for
the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode
dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a
debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct
f2fs
if (IS_INODE(page)
is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page)
is_dent_dnode(page)) {
If this is not inode block, we should add this inode to
recover its
data blocks.
Is it possible that there is only non-inode dnode but no
inode when
find_fsync_dnodes checking dnodes? Per my understanding, any
changes to
file will cause inode page dirty (for example, mtime
changed), so
that
we will write inode block. Is it right? If so, the solution
in this
patch should work too.
Your description says that f2fs_iget will fail, which causes the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and
inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node
chain, since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all
scenarios. If
the inode is checkpointed, the file can be recovered, although
the inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00
2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep 2014 00:16:34 -0700
Subject: [PATCH] f2fs: fix roll-forward missing scenarios
We can summarize the roll forward recovery scenarios as follows.
[Term] F: fsync_mark, D: dentry_mark
1. inode(x) | CP |
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Sat, Sep 13, 2014 at 10:23:18PM +0800, Huang Ying wrote:
On Fri, 2014-09-12 at 15:34 +0800, Huang Ying wrote:
On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org
wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
jaeg...@kernel.org wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying
wrote:
For fsync, if the nid of a non-inode dnode nid of
inode and the
inode is not checkpointed. The non-inode dnode may be
written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail,
cause the
recovery fail.
Usually, inode will be allocated before non-inode
dnode, so the
nid
of
inode nid of non-inode dnode. But it is possible for
the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode
dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a
debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct
f2fs
if (IS_INODE(page)
is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page)
is_dent_dnode(page)) {
If this is not inode block, we should add this inode to
recover its
data blocks.
Is it possible that there is only non-inode dnode but no
inode when
find_fsync_dnodes checking dnodes? Per my understanding,
any
changes to
file will cause inode page dirty (for example, mtime
changed), so
that
we will write inode block. Is it right? If so, the
solution in this
patch should work too.
Your description says that f2fs_iget will fail, which causes
the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget
failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and
inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node
chain, since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all
scenarios. If
the inode is checkpointed, the file can be recovered, although
the inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00
2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep 2014
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Sun, 2014-09-14 at 00:38 -0700, Jaegeuk Kim wrote:
On Fri, Sep 12, 2014 at 03:34:48PM +0800, Huang Ying wrote:
On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org
wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
jaeg...@kernel.org wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying
wrote:
For fsync, if the nid of a non-inode dnode nid of
inode and the
inode is not checkpointed. The non-inode dnode may be
written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail,
cause the
recovery fail.
Usually, inode will be allocated before non-inode
dnode, so the
nid
of
inode nid of non-inode dnode. But it is possible for
the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode
dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a
debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct
f2fs
if (IS_INODE(page)
is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page)
is_dent_dnode(page)) {
If this is not inode block, we should add this inode to
recover its
data blocks.
Is it possible that there is only non-inode dnode but no
inode when
find_fsync_dnodes checking dnodes? Per my understanding,
any
changes to
file will cause inode page dirty (for example, mtime
changed), so
that
we will write inode block. Is it right? If so, the
solution in this
patch should work too.
Your description says that f2fs_iget will fail, which causes
the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget
failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and
inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node
chain, since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all
scenarios. If
the inode is checkpointed, the file can be recovered, although
the inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00
2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Fri, 2014-09-12 at 15:34 +0800, Huang Ying wrote:
> On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
> > On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
> > >
> > > On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
> > > > On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > > > > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim
> > > > > wrote:
> > > > >
> > > > > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > > > > Hi Huang,
> > > > > > > > > >
> > > > > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > > > > > > > > > > For fsync, if the nid of a non-inode dnode < nid of inode
> > > > > > > > > > > and the
> > > > > > > > > > > inode is not checkpointed. The non-inode dnode may be
> > > > > > > > > > > written
> > > > > > before
> > > > > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail,
> > > > > > > > > > > cause the
> > > > > > > > > > > recovery fail.
> > > > > > > > > > >
> > > > > > > > > > > Usually, inode will be allocated before non-inode dnode,
> > > > > > > > > > > so the
> > > > > > nid
> > > > > > > > of
> > > > > > > > > > > inode < nid of non-inode dnode. But it is possible for
> > > > > > > > > > > the
> > > > > > reverse.
> > > > > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > > > > >
> > > > > > > > > > > This is fixed via ignoring non-inode dnode before inode
> > > > > > > > > > > dnode in
> > > > > > > > > > > find_fsync_dnodes.
> > > > > > > > > > >
> > > > > > > > > > > The patch was tested via allocating nid reversely via a
> > > > > > > > > > > debugging
> > > > > > > > > > > patch, that is, from big number to small number.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > > > > ---
> > > > > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct
> > > > > > > > > > > f2fs
> > > > > > > > > > > if (IS_INODE(page) &&
> > > > > > > > > > > is_dent_dnode(page))
> > > > > > > > > > >
> > > > > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > > > >
> > > > > > > > > > > FI_INC_LINK);
> > > > > > > > > > > - } else {
> > > > > > > > > > > - if (IS_INODE(page) &&
> > > > > > > > > > > is_dent_dnode(page)) {
> > > > > > > > > >
> > > > > > > > > > If this is not inode block, we should add this inode to
> > > > > > > > > > recover its
> > > > > > > > data blocks.
> > > > > > > > >
> > > > > > > > > Is it possible that there is only non-inode dnode but no
> > > > > > > > > inode when
> > > > > > > > > find_fsync_dnodes checking dnodes? Per my understanding, any
> > > > > > changes to
> > > > > > > > > file will cause inode page dirty (for example, mtime
> > > > > > > > > changed), so
> > > > > > that
> > > > > > > > > we will write inode block. Is it right? If so, the solution
> > > > > > > > > in this
> > > > > > > > > patch should work too.
> > > > > > > >
> > > > > > > > Your description says that f2fs_iget will fail, which causes the
> > > > > > recovery
> > > > > > > > fail.
> > > > > > > > So, I thought it would be better to handle the f2fs_iget failure
> > > > > > directly.
> > > > > > > >
> > > > > > >
> > > > > > > Yes. That is another way to fix the issue.
> > > > > > >
> > > > > > >
> > > > > > > > In addition, we cannot guarantee the write order of dnode and
> > > > > > > > inode.
> > > > > > > > For exmaple,
> > > > > > > > 1. the inode is written by flusher or kswapd, then,
> > > > > > > > 2. f2fs_sync_file writes its dnode.
> > > > > > > >
> > > > > > > > In that case, we can get only non-inode dnode in the node
> > > > > > > > chain, since
> > > > > > the
> > > > > > > > inode
> > > > > > > > has not fsync_mark.
> > > > > > > >
> > > > > > >
> > > > > > > I think your solution is better here, but does not fix all
> > > > > > > scenarios. If
> > > > > > > the inode is checkpointed, the file can be recovered, although
> > > > > > > the inode
> > > > > > > information may be not up to date. But if the inode is not
> > > > > > > checkpointed,
> > > > > > > f2fs_iget will fail too and recover will fail.
> > > > > >
> > > > > > Ok, let me consider your scenarios.
> > > > > >
> > > > > > Term: F: fsync_mark, D: dentry_mark
> > > > > >
> > > > > > 1. inode(x) | CP | inode(x) | dnode(F)
> > > > > > -> Lose the latest inode(x). Need to
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Fri, 2014-09-12 at 15:34 +0800, Huang Ying wrote:
On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org
wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim jaeg...@kernel.org
wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode
and the
inode is not checkpointed. The non-inode dnode may be
written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail,
cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode,
so the
nid
of
inode nid of non-inode dnode. But it is possible for
the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode
dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a
debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct
f2fs
if (IS_INODE(page)
is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page)
is_dent_dnode(page)) {
If this is not inode block, we should add this inode to
recover its
data blocks.
Is it possible that there is only non-inode dnode but no
inode when
find_fsync_dnodes checking dnodes? Per my understanding, any
changes to
file will cause inode page dirty (for example, mtime
changed), so
that
we will write inode block. Is it right? If so, the solution
in this
patch should work too.
Your description says that f2fs_iget will fail, which causes the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and
inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node
chain, since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all
scenarios. If
the inode is checkpointed, the file can be recovered, although
the inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00
2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep 2014 00:16:34 -0700
Subject: [PATCH] f2fs: fix roll-forward missing scenarios
We can summarize the roll forward recovery scenarios as follows.
[Term] F: fsync_mark, D: dentry_mark
1. inode(x) | CP |
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
> On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
> >
> > On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
> > > On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > > > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim wrote:
> > > >
> > > > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
> > > > > > wrote:
> > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > > > Hi Huang,
> > > > > > > > >
> > > > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > > > > > > > > > For fsync, if the nid of a non-inode dnode < nid of inode
> > > > > > > > > > and the
> > > > > > > > > > inode is not checkpointed. The non-inode dnode may be
> > > > > > > > > > written
> > > > > before
> > > > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail, cause
> > > > > > > > > > the
> > > > > > > > > > recovery fail.
> > > > > > > > > >
> > > > > > > > > > Usually, inode will be allocated before non-inode dnode, so
> > > > > > > > > > the
> > > > > nid
> > > > > > > of
> > > > > > > > > > inode < nid of non-inode dnode. But it is possible for the
> > > > > reverse.
> > > > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > > > >
> > > > > > > > > > This is fixed via ignoring non-inode dnode before inode
> > > > > > > > > > dnode in
> > > > > > > > > > find_fsync_dnodes.
> > > > > > > > > >
> > > > > > > > > > The patch was tested via allocating nid reversely via a
> > > > > > > > > > debugging
> > > > > > > > > > patch, that is, from big number to small number.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > > > ---
> > > > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > > > >
> > > > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
> > > > > > > > > > if (IS_INODE(page) && is_dent_dnode(page))
> > > > > > > > > >
> > > > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > > >
> > > > > > > > > > FI_INC_LINK);
> > > > > > > > > > - } else {
> > > > > > > > > > - if (IS_INODE(page) &&
> > > > > > > > > > is_dent_dnode(page)) {
> > > > > > > > >
> > > > > > > > > If this is not inode block, we should add this inode to
> > > > > > > > > recover its
> > > > > > > data blocks.
> > > > > > > >
> > > > > > > > Is it possible that there is only non-inode dnode but no inode
> > > > > > > > when
> > > > > > > > find_fsync_dnodes checking dnodes? Per my understanding, any
> > > > > changes to
> > > > > > > > file will cause inode page dirty (for example, mtime changed),
> > > > > > > > so
> > > > > that
> > > > > > > > we will write inode block. Is it right? If so, the solution
> > > > > > > > in this
> > > > > > > > patch should work too.
> > > > > > >
> > > > > > > Your description says that f2fs_iget will fail, which causes the
> > > > > recovery
> > > > > > > fail.
> > > > > > > So, I thought it would be better to handle the f2fs_iget failure
> > > > > directly.
> > > > > > >
> > > > > >
> > > > > > Yes. That is another way to fix the issue.
> > > > > >
> > > > > >
> > > > > > > In addition, we cannot guarantee the write order of dnode and
> > > > > > > inode.
> > > > > > > For exmaple,
> > > > > > > 1. the inode is written by flusher or kswapd, then,
> > > > > > > 2. f2fs_sync_file writes its dnode.
> > > > > > >
> > > > > > > In that case, we can get only non-inode dnode in the node chain,
> > > > > > > since
> > > > > the
> > > > > > > inode
> > > > > > > has not fsync_mark.
> > > > > > >
> > > > > >
> > > > > > I think your solution is better here, but does not fix all
> > > > > > scenarios. If
> > > > > > the inode is checkpointed, the file can be recovered, although the
> > > > > > inode
> > > > > > information may be not up to date. But if the inode is not
> > > > > > checkpointed,
> > > > > > f2fs_iget will fail too and recover will fail.
> > > > >
> > > > > Ok, let me consider your scenarios.
> > > > >
> > > > > Term: F: fsync_mark, D: dentry_mark
> > > > >
> > > > > 1. inode(x) | CP | inode(x) | dnode(F)
> > > > > -> Lose the latest inode(x). Need to fix.
> > > > >
> > > > > 2. inode(x) | CP | dnode(F) | inode(x)
> > > > > -> Impossible, but recover latest dnode(F)
> > > > >
> > > > > 3. CP | inode(x) | dnode(F)
> > > > > -> Need to write inode(DF) in f2fs_sync_file.
> > > > >
> > > > > 4. CP | dnode(F) | inode(DF)
> > > > > -> If f2fs_iget fails, then goto next.
> > > > >
> > > > > 5. CP |
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Thu, 2014-09-11 at 22:13 -0700, Jaegeuk Kim wrote:
On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim jaeg...@kernel.org
wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode
and the
inode is not checkpointed. The non-inode dnode may be
written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause
the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so
the
nid
of
inode nid of non-inode dnode. But it is possible for the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode
dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a
debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page)
is_dent_dnode(page)) {
If this is not inode block, we should add this inode to
recover its
data blocks.
Is it possible that there is only non-inode dnode but no inode
when
find_fsync_dnodes checking dnodes? Per my understanding, any
changes to
file will cause inode page dirty (for example, mtime changed),
so
that
we will write inode block. Is it right? If so, the solution
in this
patch should work too.
Your description says that f2fs_iget will fail, which causes the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and
inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node chain,
since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all
scenarios. If
the inode is checkpointed, the file can be recovered, although the
inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep 2014 00:16:34 -0700
Subject: [PATCH] f2fs: fix roll-forward missing scenarios
We can summarize the roll forward recovery scenarios as follows.
[Term] F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Update the latest inode(x).
2. inode(x) | CP | inode(F) | dnode(F)
- No problem.
3. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
I think this is
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
>
> On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
> > On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim wrote:
> > >
> > > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
> > > > > wrote:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > > Hi Huang,
> > > > > > > >
> > > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > > > > > > > > For fsync, if the nid of a non-inode dnode < nid of inode and
> > > > > > > > > the
> > > > > > > > > inode is not checkpointed. The non-inode dnode may be written
> > > > before
> > > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail, cause
> > > > > > > > > the
> > > > > > > > > recovery fail.
> > > > > > > > >
> > > > > > > > > Usually, inode will be allocated before non-inode dnode, so
> > > > > > > > > the
> > > > nid
> > > > > > of
> > > > > > > > > inode < nid of non-inode dnode. But it is possible for the
> > > > reverse.
> > > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > > >
> > > > > > > > > This is fixed via ignoring non-inode dnode before inode dnode
> > > > > > > > > in
> > > > > > > > > find_fsync_dnodes.
> > > > > > > > >
> > > > > > > > > The patch was tested via allocating nid reversely via a
> > > > > > > > > debugging
> > > > > > > > > patch, that is, from big number to small number.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > > ---
> > > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > > >
> > > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
> > > > > > > > > if (IS_INODE(page) && is_dent_dnode(page))
> > > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > >
> > > > > > > > > FI_INC_LINK);
> > > > > > > > > - } else {
> > > > > > > > > - if (IS_INODE(page) && is_dent_dnode(page)) {
> > > > > > > >
> > > > > > > > If this is not inode block, we should add this inode to recover
> > > > > > > > its
> > > > > > data blocks.
> > > > > > >
> > > > > > > Is it possible that there is only non-inode dnode but no inode
> > > > > > > when
> > > > > > > find_fsync_dnodes checking dnodes? Per my understanding, any
> > > > changes to
> > > > > > > file will cause inode page dirty (for example, mtime changed), so
> > > > that
> > > > > > > we will write inode block. Is it right? If so, the solution in
> > > > > > > this
> > > > > > > patch should work too.
> > > > > >
> > > > > > Your description says that f2fs_iget will fail, which causes the
> > > > recovery
> > > > > > fail.
> > > > > > So, I thought it would be better to handle the f2fs_iget failure
> > > > directly.
> > > > > >
> > > > >
> > > > > Yes. That is another way to fix the issue.
> > > > >
> > > > >
> > > > > > In addition, we cannot guarantee the write order of dnode and inode.
> > > > > > For exmaple,
> > > > > > 1. the inode is written by flusher or kswapd, then,
> > > > > > 2. f2fs_sync_file writes its dnode.
> > > > > >
> > > > > > In that case, we can get only non-inode dnode in the node chain,
> > > > > > since
> > > > the
> > > > > > inode
> > > > > > has not fsync_mark.
> > > > > >
> > > > >
> > > > > I think your solution is better here, but does not fix all scenarios.
> > > > > If
> > > > > the inode is checkpointed, the file can be recovered, although the
> > > > > inode
> > > > > information may be not up to date. But if the inode is not
> > > > > checkpointed,
> > > > > f2fs_iget will fail too and recover will fail.
> > > >
> > > > Ok, let me consider your scenarios.
> > > >
> > > > Term: F: fsync_mark, D: dentry_mark
> > > >
> > > > 1. inode(x) | CP | inode(x) | dnode(F)
> > > > -> Lose the latest inode(x). Need to fix.
> > > >
> > > > 2. inode(x) | CP | dnode(F) | inode(x)
> > > > -> Impossible, but recover latest dnode(F)
> > > >
> > > > 3. CP | inode(x) | dnode(F)
> > > > -> Need to write inode(DF) in f2fs_sync_file.
> > > >
> > > > 4. CP | dnode(F) | inode(DF)
> > > > -> If f2fs_iget fails, then goto next.
> > > >
> > > > 5. CP | dnode(F) | inode(x)
> > > > -> If f2fs_iget fails, then goto next. But, this is an impossible
> > > > scenario.
> > > > Drop this dnode(F).
> > > >
> > > > Indeed, there were some missing scenarios.
> > > >
> > > > So, how about this patch?
> > > >
> > > > From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
> > > > From: Jaegeuk Kim
> > > > Date: Wed, 10 Sep
Re: [f2fs-dev] [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Thu, 2014-09-11 at 18:47 +0800, Chao Yu wrote:
> > -Original Message-
> > From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
> > Sent: Thursday, September 11, 2014 1:37 PM
> > To: huang ying
> > Cc: linux-f2fs-de...@lists.sourceforge.net; LKML; Huang Ying
> > Subject: Re: [f2fs-dev] [PATCH] f2fs: Fix recover when nid of non-inode
> > dnode < nid of inode
> >
> > On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim wrote:
> > >
> > > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim
> > > > > wrote:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > > Hi Huang,
> > > > > > > >
> > > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > > > > > > > > For fsync, if the nid of a non-inode dnode < nid of inode and
> > > > > > > > > the
> > > > > > > > > inode is not checkpointed. The non-inode dnode may be written
> > > > before
> > > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail, cause
> > > > > > > > > the
> > > > > > > > > recovery fail.
> > > > > > > > >
> > > > > > > > > Usually, inode will be allocated before non-inode dnode, so
> > > > > > > > > the
> > > > nid
> > > > > > of
> > > > > > > > > inode < nid of non-inode dnode. But it is possible for the
> > > > reverse.
> > > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > > >
> > > > > > > > > This is fixed via ignoring non-inode dnode before inode dnode
> > > > > > > > > in
> > > > > > > > > find_fsync_dnodes.
> > > > > > > > >
> > > > > > > > > The patch was tested via allocating nid reversely via a
> > > > > > > > > debugging
> > > > > > > > > patch, that is, from big number to small number.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > > ---
> > > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > > >
> > > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
> > > > > > > > > if (IS_INODE(page) && is_dent_dnode(page))
> > > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > >
> > > > > > > > > FI_INC_LINK);
> > > > > > > > > - } else {
> > > > > > > > > - if (IS_INODE(page) && is_dent_dnode(page)) {
> > > > > > > >
> > > > > > > > If this is not inode block, we should add this inode to recover
> > > > > > > > its
> > > > > > data blocks.
> > > > > > >
> > > > > > > Is it possible that there is only non-inode dnode but no inode
> > > > > > > when
> > > > > > > find_fsync_dnodes checking dnodes? Per my understanding, any
> > > > changes to
> > > > > > > file will cause inode page dirty (for example, mtime changed), so
> > > > that
> > > > > > > we will write inode block. Is it right? If so, the solution in
> > > > > > > this
> > > > > > > patch should work too.
> > > > > >
> > > > > > Your description says that f2fs_iget will fail, which causes the
> > > > recovery
> > > > > &g
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
> On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim wrote:
> >
> > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > Hi Huang,
> > > > > > >
> > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > > > > > > > For fsync, if the nid of a non-inode dnode < nid of inode and
> > > > > > > > the
> > > > > > > > inode is not checkpointed. The non-inode dnode may be written
> > > before
> > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
> > > > > > > > recovery fail.
> > > > > > > >
> > > > > > > > Usually, inode will be allocated before non-inode dnode, so the
> > > nid
> > > > > of
> > > > > > > > inode < nid of non-inode dnode. But it is possible for the
> > > reverse.
> > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > >
> > > > > > > > This is fixed via ignoring non-inode dnode before inode dnode in
> > > > > > > > find_fsync_dnodes.
> > > > > > > >
> > > > > > > > The patch was tested via allocating nid reversely via a
> > > > > > > > debugging
> > > > > > > > patch, that is, from big number to small number.
> > > > > > > >
> > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > ---
> > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
> > > > > > > > if (IS_INODE(page) && is_dent_dnode(page))
> > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > FI_INC_LINK);
> > > > > > > > - } else {
> > > > > > > > - if (IS_INODE(page) && is_dent_dnode(page)) {
> > > > > > >
> > > > > > > If this is not inode block, we should add this inode to recover
> > > > > > > its
> > > > > data blocks.
> > > > > >
> > > > > > Is it possible that there is only non-inode dnode but no inode when
> > > > > > find_fsync_dnodes checking dnodes? Per my understanding, any
> > > changes to
> > > > > > file will cause inode page dirty (for example, mtime changed), so
> > > that
> > > > > > we will write inode block. Is it right? If so, the solution in
> > > > > > this
> > > > > > patch should work too.
> > > > >
> > > > > Your description says that f2fs_iget will fail, which causes the
> > > recovery
> > > > > fail.
> > > > > So, I thought it would be better to handle the f2fs_iget failure
> > > directly.
> > > > >
> > > >
> > > > Yes. That is another way to fix the issue.
> > > >
> > > >
> > > > > In addition, we cannot guarantee the write order of dnode and inode.
> > > > > For exmaple,
> > > > > 1. the inode is written by flusher or kswapd, then,
> > > > > 2. f2fs_sync_file writes its dnode.
> > > > >
> > > > > In that case, we can get only non-inode dnode in the node chain, since
> > > the
> > > > > inode
> > > > > has not fsync_mark.
> > > > >
> > > >
> > > > I think your solution is better here, but does not fix all scenarios.
> > > > If
> > > > the inode is checkpointed, the file can be recovered, although the inode
> > > > information may be not up to date. But if the inode is not
> > > > checkpointed,
> > > > f2fs_iget will fail too and recover will fail.
> > >
> > > Ok, let me consider your scenarios.
> > >
> > > Term: F: fsync_mark, D: dentry_mark
> > >
> > > 1. inode(x) | CP | inode(x) | dnode(F)
> > > -> Lose the latest inode(x). Need to fix.
> > >
> > > 2. inode(x) | CP | dnode(F) | inode(x)
> > > -> Impossible, but recover latest dnode(F)
> > >
> > > 3. CP | inode(x) | dnode(F)
> > > -> Need to write inode(DF) in f2fs_sync_file.
> > >
> > > 4. CP | dnode(F) | inode(DF)
> > > -> If f2fs_iget fails, then goto next.
> > >
> > > 5. CP | dnode(F) | inode(x)
> > > -> If f2fs_iget fails, then goto next. But, this is an impossible
> > > scenario.
> > > Drop this dnode(F).
> > >
> > > Indeed, there were some missing scenarios.
> > >
> > > So, how about this patch?
> > >
> > > From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
> > > From: Jaegeuk Kim
> > > Date: Wed, 10 Sep 2014 00:16:34 -0700
> > > Subject: [PATCH] f2fs: fix roll-forward missing scenarios
> > >
> > > We can summarize the roll forward recovery scenarios as follows.
> > >
> > > [Term] F: fsync_mark, D: dentry_mark
> > >
> > > 1. inode(x) | CP | inode(x) | dnode(F)
> > > -> Update the latest inode(x).
> > >
> > > 2. inode(x) | CP | inode(F) | dnode(F)
> > > -> No problem.
> > >
> > > 3. inode(x) | CP | dnode(F) | inode(x)
> > >
RE: [f2fs-dev] [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
> -Original Message-
> From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
> Sent: Thursday, September 11, 2014 1:37 PM
> To: huang ying
> Cc: linux-f2fs-de...@lists.sourceforge.net; LKML; Huang Ying
> Subject: Re: [f2fs-dev] [PATCH] f2fs: Fix recover when nid of non-inode dnode
> < nid of inode
>
> On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> > On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim wrote:
> >
> > > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > > Hi Huang,
> > > > > > >
> > > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > > > > > > > For fsync, if the nid of a non-inode dnode < nid of inode and
> > > > > > > > the
> > > > > > > > inode is not checkpointed. The non-inode dnode may be written
> > > before
> > > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
> > > > > > > > recovery fail.
> > > > > > > >
> > > > > > > > Usually, inode will be allocated before non-inode dnode, so the
> > > nid
> > > > > of
> > > > > > > > inode < nid of non-inode dnode. But it is possible for the
> > > reverse.
> > > > > > > > For example, because of alloc_nid_failed.
> > > > > > > >
> > > > > > > > This is fixed via ignoring non-inode dnode before inode dnode in
> > > > > > > > find_fsync_dnodes.
> > > > > > > >
> > > > > > > > The patch was tested via allocating nid reversely via a
> > > > > > > > debugging
> > > > > > > > patch, that is, from big number to small number.
> > > > > > > >
> > > > > > > > Signed-off-by: Huang, Ying
> > > > > > > > ---
> > > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
> > > > > > > > if (IS_INODE(page) && is_dent_dnode(page))
> > > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > > FI_INC_LINK);
> > > > > > > > - } else {
> > > > > > > > - if (IS_INODE(page) && is_dent_dnode(page)) {
> > > > > > >
> > > > > > > If this is not inode block, we should add this inode to recover
> > > > > > > its
> > > > > data blocks.
> > > > > >
> > > > > > Is it possible that there is only non-inode dnode but no inode when
> > > > > > find_fsync_dnodes checking dnodes? Per my understanding, any
> > > changes to
> > > > > > file will cause inode page dirty (for example, mtime changed), so
> > > that
> > > > > > we will write inode block. Is it right? If so, the solution in
> > > > > > this
> > > > > > patch should work too.
> > > > >
> > > > > Your description says that f2fs_iget will fail, which causes the
> > > recovery
> > > > > fail.
> > > > > So, I thought it would be better to handle the f2fs_iget failure
> > > directly.
> > > > >
> > > >
> > > > Yes. That is another way to fix the issue.
> > > >
> > > >
> > > > > In addition, we cannot guarantee the write order of dnode and inode.
> > > > > For exmaple,
> > > > > 1. the inode is written by flusher or kswapd, then,
> > > > > 2. f2fs_sync_file writes its dnode.
> > > > >
> > > > > In that case, we can get only non-inode dnode in the node cha
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Thu, Sep 11, 2014 at 08:25:17PM +0800, Huang Ying wrote:
On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim jaeg...@kernel.org
wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode and
the
inode is not checkpointed. The non-inode dnode may be written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause
the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so
the
nid
of
inode nid of non-inode dnode. But it is possible for the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode
in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a
debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) is_dent_dnode(page)) {
If this is not inode block, we should add this inode to recover
its
data blocks.
Is it possible that there is only non-inode dnode but no inode
when
find_fsync_dnodes checking dnodes? Per my understanding, any
changes to
file will cause inode page dirty (for example, mtime changed), so
that
we will write inode block. Is it right? If so, the solution in
this
patch should work too.
Your description says that f2fs_iget will fail, which causes the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node chain,
since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all scenarios.
If
the inode is checkpointed, the file can be recovered, although the
inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep 2014 00:16:34 -0700
Subject: [PATCH] f2fs: fix roll-forward missing scenarios
We can summarize the roll forward recovery scenarios as follows.
[Term] F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Update the latest inode(x).
2. inode(x) | CP | inode(F) | dnode(F)
- No problem.
3. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
I think this is possible. If f2fs_sync_file runs concurrently with
writeback. f2fs_sync_file written dnode(F), then writeback written
inode(x).
If the inode(x) was written, f2fs_sync_file will do write the inode again
with
fsync_mark. So, dnode(F)
RE: [f2fs-dev] [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
-Original Message-
From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
Sent: Thursday, September 11, 2014 1:37 PM
To: huang ying
Cc: linux-f2fs-de...@lists.sourceforge.net; LKML; Huang Ying
Subject: Re: [f2fs-dev] [PATCH] f2fs: Fix recover when nid of non-inode dnode
nid of inode
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim jaeg...@kernel.org wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode and
the
inode is not checkpointed. The non-inode dnode may be written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so the
nid
of
inode nid of non-inode dnode. But it is possible for the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a
debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) is_dent_dnode(page)) {
If this is not inode block, we should add this inode to recover
its
data blocks.
Is it possible that there is only non-inode dnode but no inode when
find_fsync_dnodes checking dnodes? Per my understanding, any
changes to
file will cause inode page dirty (for example, mtime changed), so
that
we will write inode block. Is it right? If so, the solution in
this
patch should work too.
Your description says that f2fs_iget will fail, which causes the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node chain, since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all scenarios.
If
the inode is checkpointed, the file can be recovered, although the inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep 2014 00:16:34 -0700
Subject: [PATCH] f2fs: fix roll-forward missing scenarios
We can summarize the roll forward recovery scenarios as follows.
[Term] F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Update the latest inode(x).
2. inode(x) | CP | inode(F) | dnode(F)
- No problem.
3. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
I think this is possible. If f2fs_sync_file runs concurrently with
writeback. f2fs_sync_file written dnode(F), then writeback written inode(x).
If the inode(x) was written, f2fs_sync_file will do write the inode again with
fsync_mark. So, dnode(F) | inode(x) | inode(F) should
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Wed, 2014-09-10 at 22:37 -0700, Jaegeuk Kim wrote:
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim jaeg...@kernel.org wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode and
the
inode is not checkpointed. The non-inode dnode may be written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so the
nid
of
inode nid of non-inode dnode. But it is possible for the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a
debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) is_dent_dnode(page)) {
If this is not inode block, we should add this inode to recover
its
data blocks.
Is it possible that there is only non-inode dnode but no inode when
find_fsync_dnodes checking dnodes? Per my understanding, any
changes to
file will cause inode page dirty (for example, mtime changed), so
that
we will write inode block. Is it right? If so, the solution in
this
patch should work too.
Your description says that f2fs_iget will fail, which causes the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node chain, since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all scenarios.
If
the inode is checkpointed, the file can be recovered, although the inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep 2014 00:16:34 -0700
Subject: [PATCH] f2fs: fix roll-forward missing scenarios
We can summarize the roll forward recovery scenarios as follows.
[Term] F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Update the latest inode(x).
2. inode(x) | CP | inode(F) | dnode(F)
- No problem.
3. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
I think this is possible. If f2fs_sync_file runs concurrently with
writeback. f2fs_sync_file written dnode(F), then writeback written inode(x).
If the inode(x) was written, f2fs_sync_file will do write the inode again with
fsync_mark. So, dnode(F) | inode(x) | inode(F) should be shown.
In f2fs_sync_file,
...
while (!sync_node_pages(sbi, ino, wbc)) {
if (fsync_mark_done(sbi, ino))
goto out;
mark_inode_dirty_sync(inode);
ret =
Re: [f2fs-dev] [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Thu, 2014-09-11 at 18:47 +0800, Chao Yu wrote:
-Original Message-
From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
Sent: Thursday, September 11, 2014 1:37 PM
To: huang ying
Cc: linux-f2fs-de...@lists.sourceforge.net; LKML; Huang Ying
Subject: Re: [f2fs-dev] [PATCH] f2fs: Fix recover when nid of non-inode
dnode nid of inode
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim jaeg...@kernel.org
wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode and
the
inode is not checkpointed. The non-inode dnode may be written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause
the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so
the
nid
of
inode nid of non-inode dnode. But it is possible for the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode
in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a
debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) is_dent_dnode(page)) {
If this is not inode block, we should add this inode to recover
its
data blocks.
Is it possible that there is only non-inode dnode but no inode
when
find_fsync_dnodes checking dnodes? Per my understanding, any
changes to
file will cause inode page dirty (for example, mtime changed), so
that
we will write inode block. Is it right? If so, the solution in
this
patch should work too.
Your description says that f2fs_iget will fail, which causes the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node chain,
since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all scenarios.
If
the inode is checkpointed, the file can be recovered, although the
inode
information may be not up to date. But if the inode is not
checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep 2014 00:16:34 -0700
Subject: [PATCH] f2fs: fix roll-forward missing scenarios
We can summarize the roll forward recovery scenarios as follows.
[Term] F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Update the latest inode(x).
2. inode(x) | CP | inode(F) | dnode(F)
- No problem.
3. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
I think this is possible
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
> On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim wrote:
>
> > On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> > > On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim wrote:
> > >
> > > > Hi,
> > > >
> > > > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > > > Hi Huang,
> > > > > >
> > > > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > > > > > > For fsync, if the nid of a non-inode dnode < nid of inode and the
> > > > > > > inode is not checkpointed. The non-inode dnode may be written
> > before
> > > > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
> > > > > > > recovery fail.
> > > > > > >
> > > > > > > Usually, inode will be allocated before non-inode dnode, so the
> > nid
> > > > of
> > > > > > > inode < nid of non-inode dnode. But it is possible for the
> > reverse.
> > > > > > > For example, because of alloc_nid_failed.
> > > > > > >
> > > > > > > This is fixed via ignoring non-inode dnode before inode dnode in
> > > > > > > find_fsync_dnodes.
> > > > > > >
> > > > > > > The patch was tested via allocating nid reversely via a debugging
> > > > > > > patch, that is, from big number to small number.
> > > > > > >
> > > > > > > Signed-off-by: Huang, Ying
> > > > > > > ---
> > > > > > > fs/f2fs/recovery.c |7 ---
> > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > --- a/fs/f2fs/recovery.c
> > > > > > > +++ b/fs/f2fs/recovery.c
> > > > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
> > > > > > > if (IS_INODE(page) && is_dent_dnode(page))
> > > > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > > > FI_INC_LINK);
> > > > > > > - } else {
> > > > > > > - if (IS_INODE(page) && is_dent_dnode(page)) {
> > > > > >
> > > > > > If this is not inode block, we should add this inode to recover its
> > > > data blocks.
> > > > >
> > > > > Is it possible that there is only non-inode dnode but no inode when
> > > > > find_fsync_dnodes checking dnodes? Per my understanding, any
> > changes to
> > > > > file will cause inode page dirty (for example, mtime changed), so
> > that
> > > > > we will write inode block. Is it right? If so, the solution in this
> > > > > patch should work too.
> > > >
> > > > Your description says that f2fs_iget will fail, which causes the
> > recovery
> > > > fail.
> > > > So, I thought it would be better to handle the f2fs_iget failure
> > directly.
> > > >
> > >
> > > Yes. That is another way to fix the issue.
> > >
> > >
> > > > In addition, we cannot guarantee the write order of dnode and inode.
> > > > For exmaple,
> > > > 1. the inode is written by flusher or kswapd, then,
> > > > 2. f2fs_sync_file writes its dnode.
> > > >
> > > > In that case, we can get only non-inode dnode in the node chain, since
> > the
> > > > inode
> > > > has not fsync_mark.
> > > >
> > >
> > > I think your solution is better here, but does not fix all scenarios. If
> > > the inode is checkpointed, the file can be recovered, although the inode
> > > information may be not up to date. But if the inode is not checkpointed,
> > > f2fs_iget will fail too and recover will fail.
> >
> > Ok, let me consider your scenarios.
> >
> > Term: F: fsync_mark, D: dentry_mark
> >
> > 1. inode(x) | CP | inode(x) | dnode(F)
> > -> Lose the latest inode(x). Need to fix.
> >
> > 2. inode(x) | CP | dnode(F) | inode(x)
> > -> Impossible, but recover latest dnode(F)
> >
> > 3. CP | inode(x) | dnode(F)
> > -> Need to write inode(DF) in f2fs_sync_file.
> >
> > 4. CP | dnode(F) | inode(DF)
> > -> If f2fs_iget fails, then goto next.
> >
> > 5. CP | dnode(F) | inode(x)
> > -> If f2fs_iget fails, then goto next. But, this is an impossible
> > scenario.
> > Drop this dnode(F).
> >
> > Indeed, there were some missing scenarios.
> >
> > So, how about this patch?
> >
> > From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
> > From: Jaegeuk Kim
> > Date: Wed, 10 Sep 2014 00:16:34 -0700
> > Subject: [PATCH] f2fs: fix roll-forward missing scenarios
> >
> > We can summarize the roll forward recovery scenarios as follows.
> >
> > [Term] F: fsync_mark, D: dentry_mark
> >
> > 1. inode(x) | CP | inode(x) | dnode(F)
> > -> Update the latest inode(x).
> >
> > 2. inode(x) | CP | inode(F) | dnode(F)
> > -> No problem.
> >
> > 3. inode(x) | CP | dnode(F) | inode(x)
> > -> Impossible, but recover latest dnode(F)
> >
>
> I think this is possible. If f2fs_sync_file runs concurrently with
> writeback. f2fs_sync_file written dnode(F), then writeback written inode(x).
If the inode(x) was written, f2fs_sync_file will do write the inode again with
fsync_mark. So, dnode(F) | inode(x) | inode(F) should be shown.
In f2fs_sync_file,
...
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
> On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim wrote:
>
> > Hi,
> >
> > On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> > > On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > > > Hi Huang,
> > > >
> > > > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > > > > For fsync, if the nid of a non-inode dnode < nid of inode and the
> > > > > inode is not checkpointed. The non-inode dnode may be written before
> > > > > inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
> > > > > recovery fail.
> > > > >
> > > > > Usually, inode will be allocated before non-inode dnode, so the nid
> > of
> > > > > inode < nid of non-inode dnode. But it is possible for the reverse.
> > > > > For example, because of alloc_nid_failed.
> > > > >
> > > > > This is fixed via ignoring non-inode dnode before inode dnode in
> > > > > find_fsync_dnodes.
> > > > >
> > > > > The patch was tested via allocating nid reversely via a debugging
> > > > > patch, that is, from big number to small number.
> > > > >
> > > > > Signed-off-by: Huang, Ying
> > > > > ---
> > > > > fs/f2fs/recovery.c |7 ---
> > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > >
> > > > > --- a/fs/f2fs/recovery.c
> > > > > +++ b/fs/f2fs/recovery.c
> > > > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
> > > > > if (IS_INODE(page) && is_dent_dnode(page))
> > > > > set_inode_flag(F2FS_I(entry->inode),
> > > > > FI_INC_LINK);
> > > > > - } else {
> > > > > - if (IS_INODE(page) && is_dent_dnode(page)) {
> > > >
> > > > If this is not inode block, we should add this inode to recover its
> > data blocks.
> > >
> > > Is it possible that there is only non-inode dnode but no inode when
> > > find_fsync_dnodes checking dnodes? Per my understanding, any changes to
> > > file will cause inode page dirty (for example, mtime changed), so that
> > > we will write inode block. Is it right? If so, the solution in this
> > > patch should work too.
> >
> > Your description says that f2fs_iget will fail, which causes the recovery
> > fail.
> > So, I thought it would be better to handle the f2fs_iget failure directly.
> >
>
> Yes. That is another way to fix the issue.
>
>
> > In addition, we cannot guarantee the write order of dnode and inode.
> > For exmaple,
> > 1. the inode is written by flusher or kswapd, then,
> > 2. f2fs_sync_file writes its dnode.
> >
> > In that case, we can get only non-inode dnode in the node chain, since the
> > inode
> > has not fsync_mark.
> >
>
> I think your solution is better here, but does not fix all scenarios. If
> the inode is checkpointed, the file can be recovered, although the inode
> information may be not up to date. But if the inode is not checkpointed,
> f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
-> Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
-> Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
-> Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
-> If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
-> If f2fs_iget fails, then goto next. But, this is an impossible scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
>From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim
Date: Wed, 10 Sep 2014 00:16:34 -0700
Subject: [PATCH] f2fs: fix roll-forward missing scenarios
We can summarize the roll forward recovery scenarios as follows.
[Term] F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
-> Update the latest inode(x).
2. inode(x) | CP | inode(F) | dnode(F)
-> No problem.
3. inode(x) | CP | dnode(F) | inode(x)
-> Impossible, but recover latest dnode(F)
4. inode(x) | CP | dnode(F) | inode(F)
-> No problem.
5. CP | inode(x) | dnode(F)
-> Impossible. Write inode(DF) with dnode(F) by f2fs_sync_file.
6. CP | inode(DF) | dnode(F)
-> No problem.
7. CP | dnode(F) | inode(DF)
-> If f2fs_iget fails, then goto next to find inode(DF).
8. CP | dnode(F) | inode(x)
-> If f2fs_iget fails, then goto next. But, this is an impossible scenario.
Drop this dnode(F).
So, this patch adds some missing points such as #1, #5, #7, and #8.
Signed-off-by: Jaegeuk Kim
---
fs/f2fs/file.c | 10
fs/f2fs/recovery.c | 70 +-
2 files changed, 69 insertions(+), 11 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 5cde363..2660af2 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -207,6 +207,16 @@ int f2fs_sync_file(struct file *file, loff_t start, loff_t
end, int datasync)
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim jaeg...@kernel.org wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode and the
inode is not checkpointed. The non-inode dnode may be written before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so the nid
of
inode nid of non-inode dnode. But it is possible for the reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) is_dent_dnode(page)) {
If this is not inode block, we should add this inode to recover its
data blocks.
Is it possible that there is only non-inode dnode but no inode when
find_fsync_dnodes checking dnodes? Per my understanding, any changes to
file will cause inode page dirty (for example, mtime changed), so that
we will write inode block. Is it right? If so, the solution in this
patch should work too.
Your description says that f2fs_iget will fail, which causes the recovery
fail.
So, I thought it would be better to handle the f2fs_iget failure directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node chain, since the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all scenarios. If
the inode is checkpointed, the file can be recovered, although the inode
information may be not up to date. But if the inode is not checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep 2014 00:16:34 -0700
Subject: [PATCH] f2fs: fix roll-forward missing scenarios
We can summarize the roll forward recovery scenarios as follows.
[Term] F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Update the latest inode(x).
2. inode(x) | CP | inode(F) | dnode(F)
- No problem.
3. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
4. inode(x) | CP | dnode(F) | inode(F)
- No problem.
5. CP | inode(x) | dnode(F)
- Impossible. Write inode(DF) with dnode(F) by f2fs_sync_file.
6. CP | inode(DF) | dnode(F)
- No problem.
7. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next to find inode(DF).
8. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible scenario.
Drop this dnode(F).
So, this patch adds some missing points such as #1, #5, #7, and #8.
Signed-off-by: Jaegeuk Kim jaeg...@kernel.org
---
fs/f2fs/file.c | 10
fs/f2fs/recovery.c | 70 +-
2 files changed, 69 insertions(+), 11 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 5cde363..2660af2 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -207,6 +207,16 @@ int f2fs_sync_file(struct file *file, loff_t start, loff_t
end, int datasync)
up_write(fi-i_sem);
}
} else {
+ /*
+* CP | inode(x) | dnode(F)
+* We need to remain inode(DF) for roll-forward
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Wed, Sep 10, 2014 at 07:08:32PM +0800, huang ying wrote:
On Wed, Sep 10, 2014 at 3:21 PM, Jaegeuk Kim jaeg...@kernel.org wrote:
On Tue, Sep 09, 2014 at 07:31:49PM +0800, huang ying wrote:
On Tue, Sep 9, 2014 at 3:09 PM, Jaegeuk Kim jaeg...@kernel.org wrote:
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode and the
inode is not checkpointed. The non-inode dnode may be written
before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so the
nid
of
inode nid of non-inode dnode. But it is possible for the
reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) is_dent_dnode(page)) {
If this is not inode block, we should add this inode to recover its
data blocks.
Is it possible that there is only non-inode dnode but no inode when
find_fsync_dnodes checking dnodes? Per my understanding, any
changes to
file will cause inode page dirty (for example, mtime changed), so
that
we will write inode block. Is it right? If so, the solution in this
patch should work too.
Your description says that f2fs_iget will fail, which causes the
recovery
fail.
So, I thought it would be better to handle the f2fs_iget failure
directly.
Yes. That is another way to fix the issue.
In addition, we cannot guarantee the write order of dnode and inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node chain, since
the
inode
has not fsync_mark.
I think your solution is better here, but does not fix all scenarios. If
the inode is checkpointed, the file can be recovered, although the inode
information may be not up to date. But if the inode is not checkpointed,
f2fs_iget will fail too and recover will fail.
Ok, let me consider your scenarios.
Term: F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Lose the latest inode(x). Need to fix.
2. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
3. CP | inode(x) | dnode(F)
- Need to write inode(DF) in f2fs_sync_file.
4. CP | dnode(F) | inode(DF)
- If f2fs_iget fails, then goto next.
5. CP | dnode(F) | inode(x)
- If f2fs_iget fails, then goto next. But, this is an impossible
scenario.
Drop this dnode(F).
Indeed, there were some missing scenarios.
So, how about this patch?
From 552dc68c5f07a335d7b55c197bab531efb135521 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim jaeg...@kernel.org
Date: Wed, 10 Sep 2014 00:16:34 -0700
Subject: [PATCH] f2fs: fix roll-forward missing scenarios
We can summarize the roll forward recovery scenarios as follows.
[Term] F: fsync_mark, D: dentry_mark
1. inode(x) | CP | inode(x) | dnode(F)
- Update the latest inode(x).
2. inode(x) | CP | inode(F) | dnode(F)
- No problem.
3. inode(x) | CP | dnode(F) | inode(x)
- Impossible, but recover latest dnode(F)
I think this is possible. If f2fs_sync_file runs concurrently with
writeback. f2fs_sync_file written dnode(F), then writeback written inode(x).
If the inode(x) was written, f2fs_sync_file will do write the inode again with
fsync_mark. So, dnode(F) | inode(x) | inode(F) should be shown.
In f2fs_sync_file,
...
while (!sync_node_pages(sbi, ino, wbc)) {
if (fsync_mark_done(sbi, ino))
goto out;
mark_inode_dirty_sync(inode);
ret = f2fs_write_inode(inode, NULL);
if (ret)
goto out;
}
...
4. inode(x) | CP | dnode(F) | inode(F)
- No problem.
5. CP | inode(x) | dnode(F)
- Impossible. Write
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
> On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> > Hi Huang,
> >
> > On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > > For fsync, if the nid of a non-inode dnode < nid of inode and the
> > > inode is not checkpointed. The non-inode dnode may be written before
> > > inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
> > > recovery fail.
> > >
> > > Usually, inode will be allocated before non-inode dnode, so the nid of
> > > inode < nid of non-inode dnode. But it is possible for the reverse.
> > > For example, because of alloc_nid_failed.
> > >
> > > This is fixed via ignoring non-inode dnode before inode dnode in
> > > find_fsync_dnodes.
> > >
> > > The patch was tested via allocating nid reversely via a debugging
> > > patch, that is, from big number to small number.
> > >
> > > Signed-off-by: Huang, Ying
> > > ---
> > > fs/f2fs/recovery.c |7 ---
> > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > --- a/fs/f2fs/recovery.c
> > > +++ b/fs/f2fs/recovery.c
> > > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
> > > if (IS_INODE(page) && is_dent_dnode(page))
> > > set_inode_flag(F2FS_I(entry->inode),
> > > FI_INC_LINK);
> > > - } else {
> > > - if (IS_INODE(page) && is_dent_dnode(page)) {
> >
> > If this is not inode block, we should add this inode to recover its data
> > blocks.
>
> Is it possible that there is only non-inode dnode but no inode when
> find_fsync_dnodes checking dnodes? Per my understanding, any changes to
> file will cause inode page dirty (for example, mtime changed), so that
> we will write inode block. Is it right? If so, the solution in this
> patch should work too.
Your description says that f2fs_iget will fail, which causes the recovery fail.
So, I thought it would be better to handle the f2fs_iget failure directly.
In addition, we cannot guarantee the write order of dnode and inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node chain, since the inode
has not fsync_mark.
Thanks,
>
> Best Regards,
> Huang, Ying
>
> > Rather than this tweak, if iget is failed, we'd better go to next instead of
> > break.
> > Can you test that?
> >
> > > + } else if (IS_INODE(page)) {
> > > + if (is_dent_dnode(page)) {
> > > err = recover_inode_page(sbi, page);
> > > if (err)
> > > break;
> > > @@ -193,7 +193,8 @@ static int find_fsync_dnodes(struct f2fs
> > > break;
> > > }
> > > list_add_tail(>list, head);
> > > - }
> > > + } else
> > > + goto next;
> > > entry->blkaddr = blkaddr;
> > >
> > > err = recover_inode(entry->inode, page);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
Hi,
On Tue, Sep 09, 2014 at 01:39:30PM +0800, Huang Ying wrote:
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode and the
inode is not checkpointed. The non-inode dnode may be written before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so the nid of
inode nid of non-inode dnode. But it is possible for the reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) is_dent_dnode(page)) {
If this is not inode block, we should add this inode to recover its data
blocks.
Is it possible that there is only non-inode dnode but no inode when
find_fsync_dnodes checking dnodes? Per my understanding, any changes to
file will cause inode page dirty (for example, mtime changed), so that
we will write inode block. Is it right? If so, the solution in this
patch should work too.
Your description says that f2fs_iget will fail, which causes the recovery fail.
So, I thought it would be better to handle the f2fs_iget failure directly.
In addition, we cannot guarantee the write order of dnode and inode.
For exmaple,
1. the inode is written by flusher or kswapd, then,
2. f2fs_sync_file writes its dnode.
In that case, we can get only non-inode dnode in the node chain, since the inode
has not fsync_mark.
Thanks,
Best Regards,
Huang, Ying
Rather than this tweak, if iget is failed, we'd better go to next instead of
break.
Can you test that?
+ } else if (IS_INODE(page)) {
+ if (is_dent_dnode(page)) {
err = recover_inode_page(sbi, page);
if (err)
break;
@@ -193,7 +193,8 @@ static int find_fsync_dnodes(struct f2fs
break;
}
list_add_tail(entry-list, head);
- }
+ } else
+ goto next;
entry-blkaddr = blkaddr;
err = recover_inode(entry-inode, page);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
> Hi Huang,
>
> On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> > For fsync, if the nid of a non-inode dnode < nid of inode and the
> > inode is not checkpointed. The non-inode dnode may be written before
> > inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
> > recovery fail.
> >
> > Usually, inode will be allocated before non-inode dnode, so the nid of
> > inode < nid of non-inode dnode. But it is possible for the reverse.
> > For example, because of alloc_nid_failed.
> >
> > This is fixed via ignoring non-inode dnode before inode dnode in
> > find_fsync_dnodes.
> >
> > The patch was tested via allocating nid reversely via a debugging
> > patch, that is, from big number to small number.
> >
> > Signed-off-by: Huang, Ying
> > ---
> > fs/f2fs/recovery.c |7 ---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > --- a/fs/f2fs/recovery.c
> > +++ b/fs/f2fs/recovery.c
> > @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
> > if (IS_INODE(page) && is_dent_dnode(page))
> > set_inode_flag(F2FS_I(entry->inode),
> > FI_INC_LINK);
> > - } else {
> > - if (IS_INODE(page) && is_dent_dnode(page)) {
>
> If this is not inode block, we should add this inode to recover its data
> blocks.
Is it possible that there is only non-inode dnode but no inode when
find_fsync_dnodes checking dnodes? Per my understanding, any changes to
file will cause inode page dirty (for example, mtime changed), so that
we will write inode block. Is it right? If so, the solution in this
patch should work too.
Best Regards,
Huang, Ying
> Rather than this tweak, if iget is failed, we'd better go to next instead of
> break.
> Can you test that?
>
> > + } else if (IS_INODE(page)) {
> > + if (is_dent_dnode(page)) {
> > err = recover_inode_page(sbi, page);
> > if (err)
> > break;
> > @@ -193,7 +193,8 @@ static int find_fsync_dnodes(struct f2fs
> > break;
> > }
> > list_add_tail(>list, head);
> > - }
> > + } else
> > + goto next;
> > entry->blkaddr = blkaddr;
> >
> > err = recover_inode(entry->inode, page);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
> For fsync, if the nid of a non-inode dnode < nid of inode and the
> inode is not checkpointed. The non-inode dnode may be written before
> inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
> recovery fail.
>
> Usually, inode will be allocated before non-inode dnode, so the nid of
> inode < nid of non-inode dnode. But it is possible for the reverse.
> For example, because of alloc_nid_failed.
>
> This is fixed via ignoring non-inode dnode before inode dnode in
> find_fsync_dnodes.
>
> The patch was tested via allocating nid reversely via a debugging
> patch, that is, from big number to small number.
>
> Signed-off-by: Huang, Ying
> ---
> fs/f2fs/recovery.c |7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
> if (IS_INODE(page) && is_dent_dnode(page))
> set_inode_flag(F2FS_I(entry->inode),
> FI_INC_LINK);
> - } else {
> - if (IS_INODE(page) && is_dent_dnode(page)) {
If this is not inode block, we should add this inode to recover its data blocks.
Rather than this tweak, if iget is failed, we'd better go to next instead of
break.
Can you test that?
> + } else if (IS_INODE(page)) {
> + if (is_dent_dnode(page)) {
> err = recover_inode_page(sbi, page);
> if (err)
> break;
> @@ -193,7 +193,8 @@ static int find_fsync_dnodes(struct f2fs
> break;
> }
> list_add_tail(>list, head);
> - }
> + } else
> + goto next;
> entry->blkaddr = blkaddr;
>
> err = recover_inode(entry->inode, page);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] f2fs: Fix recover when nid of non-inode dnode < nid of inode
For fsync, if the nid of a non-inode dnode < nid of inode and the
inode is not checkpointed. The non-inode dnode may be written before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so the nid of
inode < nid of non-inode dnode. But it is possible for the reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) && is_dent_dnode(page))
set_inode_flag(F2FS_I(entry->inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) && is_dent_dnode(page)) {
+ } else if (IS_INODE(page)) {
+ if (is_dent_dnode(page)) {
err = recover_inode_page(sbi, page);
if (err)
break;
@@ -193,7 +193,8 @@ static int find_fsync_dnodes(struct f2fs
break;
}
list_add_tail(>list, head);
- }
+ } else
+ goto next;
entry->blkaddr = blkaddr;
err = recover_inode(entry->inode, page);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
For fsync, if the nid of a non-inode dnode nid of inode and the
inode is not checkpointed. The non-inode dnode may be written before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so the nid of
inode nid of non-inode dnode. But it is possible for the reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) is_dent_dnode(page)) {
+ } else if (IS_INODE(page)) {
+ if (is_dent_dnode(page)) {
err = recover_inode_page(sbi, page);
if (err)
break;
@@ -193,7 +193,8 @@ static int find_fsync_dnodes(struct f2fs
break;
}
list_add_tail(entry-list, head);
- }
+ } else
+ goto next;
entry-blkaddr = blkaddr;
err = recover_inode(entry-inode, page);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode and the
inode is not checkpointed. The non-inode dnode may be written before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so the nid of
inode nid of non-inode dnode. But it is possible for the reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) is_dent_dnode(page)) {
If this is not inode block, we should add this inode to recover its data blocks.
Rather than this tweak, if iget is failed, we'd better go to next instead of
break.
Can you test that?
+ } else if (IS_INODE(page)) {
+ if (is_dent_dnode(page)) {
err = recover_inode_page(sbi, page);
if (err)
break;
@@ -193,7 +193,8 @@ static int find_fsync_dnodes(struct f2fs
break;
}
list_add_tail(entry-list, head);
- }
+ } else
+ goto next;
entry-blkaddr = blkaddr;
err = recover_inode(entry-inode, page);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: Fix recover when nid of non-inode dnode nid of inode
On Mon, 2014-09-08 at 22:23 -0700, Jaegeuk Kim wrote:
Hi Huang,
On Mon, Sep 08, 2014 at 07:38:26PM +0800, Huang Ying wrote:
For fsync, if the nid of a non-inode dnode nid of inode and the
inode is not checkpointed. The non-inode dnode may be written before
inode. So in find_fsync_dnodes, f2fs_iget will fail, cause the
recovery fail.
Usually, inode will be allocated before non-inode dnode, so the nid of
inode nid of non-inode dnode. But it is possible for the reverse.
For example, because of alloc_nid_failed.
This is fixed via ignoring non-inode dnode before inode dnode in
find_fsync_dnodes.
The patch was tested via allocating nid reversely via a debugging
patch, that is, from big number to small number.
Signed-off-by: Huang, Ying ying.hu...@intel.com
---
fs/f2fs/recovery.c |7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -172,8 +172,8 @@ static int find_fsync_dnodes(struct f2fs
if (IS_INODE(page) is_dent_dnode(page))
set_inode_flag(F2FS_I(entry-inode),
FI_INC_LINK);
- } else {
- if (IS_INODE(page) is_dent_dnode(page)) {
If this is not inode block, we should add this inode to recover its data
blocks.
Is it possible that there is only non-inode dnode but no inode when
find_fsync_dnodes checking dnodes? Per my understanding, any changes to
file will cause inode page dirty (for example, mtime changed), so that
we will write inode block. Is it right? If so, the solution in this
patch should work too.
Best Regards,
Huang, Ying
Rather than this tweak, if iget is failed, we'd better go to next instead of
break.
Can you test that?
+ } else if (IS_INODE(page)) {
+ if (is_dent_dnode(page)) {
err = recover_inode_page(sbi, page);
if (err)
break;
@@ -193,7 +193,8 @@ static int find_fsync_dnodes(struct f2fs
break;
}
list_add_tail(entry-list, head);
- }
+ } else
+ goto next;
entry-blkaddr = blkaddr;
err = recover_inode(entry-inode, page);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
