All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] f2fs: remove false-positive bug_on
@ 2017-05-26 23:59 ` Jaegeuk Kim
  0 siblings, 0 replies; 8+ messages in thread
From: Jaegeuk Kim @ 2017-05-26 23:59 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim

If we got failure from both of create and evict_inode, we can hit this wrong
bug_on.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index e53c784ab11e..5673b0bd83b5 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
 		alloc_nid_failed(sbi, inode->i_ino);
 		clear_inode_flag(inode, FI_FREE_NID);
 	}
-	f2fs_bug_on(sbi, err &&
-		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
 out_clear:
 	fscrypt_put_encryption_info(inode, NULL);
 	clear_inode(inode);
-- 
2.13.0.rc1.294.g07d810a77f-goog

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH] f2fs: remove false-positive bug_on
@ 2017-05-26 23:59 ` Jaegeuk Kim
  0 siblings, 0 replies; 8+ messages in thread
From: Jaegeuk Kim @ 2017-05-26 23:59 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim

If we got failure from both of create and evict_inode, we can hit this wrong
bug_on.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index e53c784ab11e..5673b0bd83b5 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
 		alloc_nid_failed(sbi, inode->i_ino);
 		clear_inode_flag(inode, FI_FREE_NID);
 	}
-	f2fs_bug_on(sbi, err &&
-		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
 out_clear:
 	fscrypt_put_encryption_info(inode, NULL);
 	clear_inode(inode);
-- 
2.13.0.rc1.294.g07d810a77f-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: remove false-positive bug_on
  2017-05-26 23:59 ` Jaegeuk Kim
  (?)
@ 2017-05-31 13:24 ` Chao Yu
  2017-06-01  2:12     ` Jaegeuk Kim
  -1 siblings, 1 reply; 8+ messages in thread
From: Chao Yu @ 2017-05-31 13:24 UTC (permalink / raw)
  To: Jaegeuk Kim, linux-kernel, linux-fsdevel, linux-f2fs-devel

Hi Jaegeuk,

On 2017/5/27 7:59, Jaegeuk Kim wrote:
> If we got failure from both of create and evict_inode, we can hit this wrong
> bug_on.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/inode.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index e53c784ab11e..5673b0bd83b5 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
>  		alloc_nid_failed(sbi, inode->i_ino);
>  		clear_inode_flag(inode, FI_FREE_NID);
>  	}
> -	f2fs_bug_on(sbi, err &&
> -		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));

We expect that we can keep the inode in orphan list in
handle_failed_inode path when inode page have been persisted, so that if
there is anything error in evice_inode, we can have another chance to
release inode resource during next mount.

Here we need to check this case, additionally, if we failed to add the
inode into orphan list in handle_failed_inode, we must have set
SBI_NEED_FSCK in cp pack, so we need check the case too.

So we can change the code to:

f2fs_bug_on(err && err != -ENOENT &&
		(!exist_written_data(sbi, inode->i_ino, ORPHAN_INO) ||
		!is_sbi_flag_set(sbi, SBI_NEED_FSCK));

How do you think?

Thanks,

>  out_clear:
>  	fscrypt_put_encryption_info(inode, NULL);
>  	clear_inode(inode);
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: remove false-positive bug_on
  2017-05-31 13:24 ` [f2fs-dev] " Chao Yu
@ 2017-06-01  2:12     ` Jaegeuk Kim
  0 siblings, 0 replies; 8+ messages in thread
From: Jaegeuk Kim @ 2017-06-01  2:12 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel

On 05/31, Chao Yu wrote:
> Hi Jaegeuk,
> 
> On 2017/5/27 7:59, Jaegeuk Kim wrote:
> > If we got failure from both of create and evict_inode, we can hit this wrong
> > bug_on.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/inode.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index e53c784ab11e..5673b0bd83b5 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
> >  		alloc_nid_failed(sbi, inode->i_ino);
> >  		clear_inode_flag(inode, FI_FREE_NID);
> >  	}
> > -	f2fs_bug_on(sbi, err &&
> > -		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
> 
> We expect that we can keep the inode in orphan list in
> handle_failed_inode path when inode page have been persisted, so that if
> there is anything error in evice_inode, we can have another chance to
> release inode resource during next mount.
> 
> Here we need to check this case, additionally, if we failed to add the
> inode into orphan list in handle_failed_inode, we must have set
> SBI_NEED_FSCK in cp pack, so we need check the case too.
> 
> So we can change the code to:
> 
> f2fs_bug_on(err && err != -ENOENT &&
> 		(!exist_written_data(sbi, inode->i_ino, ORPHAN_INO) ||
> 		!is_sbi_flag_set(sbi, SBI_NEED_FSCK));

Yup, I'll try this. ;)

Thanks,

> 
> How do you think?
> 
> Thanks,
> 
> >  out_clear:
> >  	fscrypt_put_encryption_info(inode, NULL);
> >  	clear_inode(inode);
> > 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: remove false-positive bug_on
@ 2017-06-01  2:12     ` Jaegeuk Kim
  0 siblings, 0 replies; 8+ messages in thread
From: Jaegeuk Kim @ 2017-06-01  2:12 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel

On 05/31, Chao Yu wrote:
> Hi Jaegeuk,
> 
> On 2017/5/27 7:59, Jaegeuk Kim wrote:
> > If we got failure from both of create and evict_inode, we can hit this wrong
> > bug_on.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/inode.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index e53c784ab11e..5673b0bd83b5 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
> >  		alloc_nid_failed(sbi, inode->i_ino);
> >  		clear_inode_flag(inode, FI_FREE_NID);
> >  	}
> > -	f2fs_bug_on(sbi, err &&
> > -		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
> 
> We expect that we can keep the inode in orphan list in
> handle_failed_inode path when inode page have been persisted, so that if
> there is anything error in evice_inode, we can have another chance to
> release inode resource during next mount.
> 
> Here we need to check this case, additionally, if we failed to add the
> inode into orphan list in handle_failed_inode, we must have set
> SBI_NEED_FSCK in cp pack, so we need check the case too.
> 
> So we can change the code to:
> 
> f2fs_bug_on(err && err != -ENOENT &&
> 		(!exist_written_data(sbi, inode->i_ino, ORPHAN_INO) ||
> 		!is_sbi_flag_set(sbi, SBI_NEED_FSCK));

Yup, I'll try this. ;)

Thanks,

> 
> How do you think?
> 
> Thanks,
> 
> >  out_clear:
> >  	fscrypt_put_encryption_info(inode, NULL);
> >  	clear_inode(inode);
> > 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: remove false-positive bug_on
  2017-06-01  2:12     ` Jaegeuk Kim
@ 2017-06-01  2:56       ` Chao Yu
  -1 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2017-06-01  2:56 UTC (permalink / raw)
  To: Jaegeuk Kim, Chao Yu; +Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel

On 2017/6/1 10:12, Jaegeuk Kim wrote:
> On 05/31, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2017/5/27 7:59, Jaegeuk Kim wrote:
>>> If we got failure from both of create and evict_inode, we can hit this wrong
>>> bug_on.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>  fs/f2fs/inode.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index e53c784ab11e..5673b0bd83b5 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
>>>  		alloc_nid_failed(sbi, inode->i_ino);
>>>  		clear_inode_flag(inode, FI_FREE_NID);
>>>  	}
>>> -	f2fs_bug_on(sbi, err &&
>>> -		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
>>
>> We expect that we can keep the inode in orphan list in
>> handle_failed_inode path when inode page have been persisted, so that if
>> there is anything error in evice_inode, we can have another chance to
>> release inode resource during next mount.
>>
>> Here we need to check this case, additionally, if we failed to add the
>> inode into orphan list in handle_failed_inode, we must have set
>> SBI_NEED_FSCK in cp pack, so we need check the case too.
>>
>> So we can change the code to:
>>
>> f2fs_bug_on(err && err != -ENOENT &&
>> 		(!exist_written_data(sbi, inode->i_ino, ORPHAN_INO) ||
>> 		!is_sbi_flag_set(sbi, SBI_NEED_FSCK));

Oh, looks like it needs to use '&&' in between exist_written_data and
is_sbi_flag_set. Could you fix this?

Thanks,

> 
> Yup, I'll try this. ;)
> 
> Thanks,
> 
>>
>> How do you think?
>>
>> Thanks,
>>
>>>  out_clear:
>>>  	fscrypt_put_encryption_info(inode, NULL);
>>>  	clear_inode(inode);
>>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: remove false-positive bug_on
@ 2017-06-01  2:56       ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2017-06-01  2:56 UTC (permalink / raw)
  To: Jaegeuk Kim, Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel

On 2017/6/1 10:12, Jaegeuk Kim wrote:
> On 05/31, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2017/5/27 7:59, Jaegeuk Kim wrote:
>>> If we got failure from both of create and evict_inode, we can hit this wrong
>>> bug_on.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>  fs/f2fs/inode.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index e53c784ab11e..5673b0bd83b5 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -426,8 +426,6 @@ void f2fs_evict_inode(struct inode *inode)
>>>  		alloc_nid_failed(sbi, inode->i_ino);
>>>  		clear_inode_flag(inode, FI_FREE_NID);
>>>  	}
>>> -	f2fs_bug_on(sbi, err &&
>>> -		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
>>
>> We expect that we can keep the inode in orphan list in
>> handle_failed_inode path when inode page have been persisted, so that if
>> there is anything error in evice_inode, we can have another chance to
>> release inode resource during next mount.
>>
>> Here we need to check this case, additionally, if we failed to add the
>> inode into orphan list in handle_failed_inode, we must have set
>> SBI_NEED_FSCK in cp pack, so we need check the case too.
>>
>> So we can change the code to:
>>
>> f2fs_bug_on(err && err != -ENOENT &&
>> 		(!exist_written_data(sbi, inode->i_ino, ORPHAN_INO) ||
>> 		!is_sbi_flag_set(sbi, SBI_NEED_FSCK));

Oh, looks like it needs to use '&&' in between exist_written_data and
is_sbi_flag_set. Could you fix this?

Thanks,

> 
> Yup, I'll try this. ;)
> 
> Thanks,
> 
>>
>> How do you think?
>>
>> Thanks,
>>
>>>  out_clear:
>>>  	fscrypt_put_encryption_info(inode, NULL);
>>>  	clear_inode(inode);
>>>
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: remove false-positive bug_on
  2017-06-01  2:56       ` Chao Yu
  (?)
@ 2017-06-01 22:47       ` Jaegeuk Kim
  -1 siblings, 0 replies; 8+ messages in thread
From: Jaegeuk Kim @ 2017-06-01 22:47 UTC (permalink / raw)
  To: Chao Yu; +Cc: Chao Yu, linux-kernel, linux-fsdevel, linux-f2fs-devel

For example,

f2fs_create
 - new_node_page is failed
 - handle_failed_inode
  - skip to add it into orphan list, since ni.blk_addr == NULL_ADDR
   : set_inode_flag(inode, FI_FREE_NID)

f2fs_evict_inode
 - EIO due to fault injection
 - f2fs_bug_on() is triggered

So, we don't need to call f2fs_bug_on in this case.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/inode.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index e53c784ab11e..868d71436ebc 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -425,9 +425,10 @@ void f2fs_evict_inode(struct inode *inode)
 	if (is_inode_flag_set(inode, FI_FREE_NID)) {
 		alloc_nid_failed(sbi, inode->i_ino);
 		clear_inode_flag(inode, FI_FREE_NID);
+	} else {
+		f2fs_bug_on(sbi, err &&
+			!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
 	}
-	f2fs_bug_on(sbi, err &&
-		!exist_written_data(sbi, inode->i_ino, ORPHAN_INO));
 out_clear:
 	fscrypt_put_encryption_info(inode, NULL);
 	clear_inode(inode);
-- 
2.13.0.rc1.294.g07d810a77f-goog

^ permalink raw reply related	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-01 22:47 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-26 23:59 [PATCH] f2fs: remove false-positive bug_on Jaegeuk Kim
2017-05-26 23:59 ` Jaegeuk Kim
2017-05-31 13:24 ` [f2fs-dev] " Chao Yu
2017-06-01  2:12   ` Jaegeuk Kim
2017-06-01  2:12     ` Jaegeuk Kim
2017-06-01  2:56     ` [f2fs-dev] " Chao Yu
2017-06-01  2:56       ` Chao Yu
2017-06-01 22:47       ` [f2fs-dev] [PATCH v2] " Jaegeuk Kim

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.