| Message ID | 20130627145855.6ed028022d3a63fa97dcee13@linux-foundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2013/6/28 5:58, Andrew Morton wrote: > On Thu, 27 Jun 2013 11:05:40 +0800 Younger Liu <younger.liu@huawei.com> wrote: > >> While deleting a file with ocfs2_unlink(), there is a bug in this >> function. This bug will result in filesystem read-only. >> >> After calling ocfs2_orphan_add(), the file which will be deleted >> is added into orphan dir. If ocfs2_delete_entry() fails, >> the file still exists in the parent dir. >> And this scenario introduces a conflict of metadata. >> >> If a file is added into orphan dir, when we put inode of the file >> with iput(), the inode i_flags is setted (~OCFS2_VALID_FL) in >> ocfs2_remove_inode(), and then write back to disk. >> >> But as previously mentioned, the file still exists in the parent dir. >> On other nodes, the file can be still accessed. When first read the file >> with ocfs2_read_blocks() from disk, It will check and avalidate inode >> using ocfs2_validate_inode_block(). >> So File system will be readonly because the inode is invalid. >> In other words, the inode i_flags has been setted (~OCFS2_VALID_FL). >> >> ... >> >> --- a/fs/ocfs2/namei.c >> +++ b/fs/ocfs2/namei.c >> @@ -790,7 +790,7 @@ static int ocfs2_unlink(struct inode *dir, >> struct dentry *dentry) >> { >> int status; >> - int child_locked = 0; >> + int child_locked = 0, is_unlinkable = 0; > > Please note that the surrounding code was carful to use the > one-definition-per-line convention. That's a good convention - more > readable, less patch rejects during code evolution, leaves room for a > nice little comment. > > Also, type `bool' would have been appropraite here. > >> struct inode *inode = dentry->d_inode; >> struct inode *orphan_dir = NULL; >> struct ocfs2_super *osb = OCFS2_SB(dir->i_sb); >> @@ -873,6 +873,7 @@ static int ocfs2_unlink(struct inode *dir, >> mlog_errno(status); >> goto leave; >> } >> + is_unlinkable = 1; >> } >> >> handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb)); >> @@ -892,15 +893,6 @@ static int ocfs2_unlink(struct inode *dir, >> >> fe = (struct ocfs2_dinode *) fe_bh->b_data; >> >> - if (inode_is_unlinkable(inode)) { >> - status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name, >> - &orphan_insert, orphan_dir); >> - if (status < 0) { >> - mlog_errno(status); >> - goto leave; >> - } >> - } >> - >> /* delete the name from the parent dir */ >> status = ocfs2_delete_entry(handle, dir, &lookup); >> if (status < 0) { >> @@ -923,6 +915,14 @@ static int ocfs2_unlink(struct inode *dir, >> mlog_errno(status); >> if (S_ISDIR(inode->i_mode)) >> inc_nlink(dir); >> + goto leave; >> + } >> + >> + if (is_unlinkable) { >> + status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name, >> + &orphan_insert, orphan_dir); >> + if (status < 0) >> + mlog_errno(status); >> } > > This is yet another ocfs2 function which reports the same error two > times. ho hum. > Thanks for your review. > Please review: > > --- a/fs/ocfs2/namei.c~ocfs2-fix-readonly-issue-in-ocfs2_unlink-fix > +++ a/fs/ocfs2/namei.c > @@ -790,7 +790,8 @@ static int ocfs2_unlink(struct inode *di > struct dentry *dentry) > { > int status; > - int child_locked = 0, is_unlinkable = 0; > + int child_locked = 0; > + bool is_unlinkable = false; > struct inode *inode = dentry->d_inode; > struct inode *orphan_dir = NULL; > struct ocfs2_super *osb = OCFS2_SB(dir->i_sb); > @@ -873,7 +874,7 @@ static int ocfs2_unlink(struct inode *di > mlog_errno(status); > goto leave; > } > - is_unlinkable = 1; > + is_unlinkable = true; > } > > handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb)); > @@ -919,8 +920,8 @@ static int ocfs2_unlink(struct inode *di > } > > if (is_unlinkable) { > - status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name, > - &orphan_insert, orphan_dir); > + status = ocfs2_orphan_add(osb, handle, inode, fe_bh, > + orphan_name, &orphan_insert, orphan_dir); > if (status < 0) > mlog_errno(status); > } > _ > ... This patch looks fine to me. I also test it, and the result is fine. You can consider to add: Reviewed-by: Younger Liu <younger.liu@huawei.com>
--- a/fs/ocfs2/namei.c~ocfs2-fix-readonly-issue-in-ocfs2_unlink-fix +++ a/fs/ocfs2/namei.c @@ -790,7 +790,8 @@ static int ocfs2_unlink(struct inode *di struct dentry *dentry) { int status; - int child_locked = 0, is_unlinkable = 0; + int child_locked = 0; + bool is_unlinkable = false; struct inode *inode = dentry->d_inode; struct inode *orphan_dir = NULL; struct ocfs2_super *osb = OCFS2_SB(dir->i_sb); @@ -873,7 +874,7 @@ static int ocfs2_unlink(struct inode *di mlog_errno(status); goto leave; } - is_unlinkable = 1; + is_unlinkable = true; } handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb)); @@ -919,8 +920,8 @@ static int ocfs2_unlink(struct inode *di } if (is_unlinkable) { - status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name, - &orphan_insert, orphan_dir); + status = ocfs2_orphan_add(osb, handle, inode, fe_bh, + orphan_name, &orphan_insert, orphan_dir); if (status < 0) mlog_errno(status); }