| Message ID | 20170526235923.31058-1-jaegeuk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
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); > >
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); >>> > > . >
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);
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(-)