[Ocfs2-devel] [PATCH 3/3] ocfs2: use OCFS2_INODE_SKIP_ORPHAN_DIR in ocfs2_mknod error path

[Li Dongyang]

On Monday 26 April 2010 17:59:04 Li Dongyang wrote:
> On Saturday 24 April 2010 03:59:00 Mark Fasheh wrote:
> > On Thu, Apr 22, 2010 at 04:11:29PM +0800, Li Dongyang wrote:
> > > mark the inode with flag OCFS2_INODE_SKIP_ORPHAN_DIR in ocfs2_mknod,
> > > so we can kill the inode in case of error.
> > >
> > > Signed-off-by: Li Dongyang <lidongyang at novell.com>
> > > ---
> > >  fs/ocfs2/namei.c |   16 +++++++++++-----
> > >  1 files changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> > > index 4ce4416..f989823 100644
> > > --- a/fs/ocfs2/namei.c
> > > +++ b/fs/ocfs2/namei.c
> > > @@ -447,11 +447,6 @@ leave:
> > >
> > >  	ocfs2_free_dir_lookup_result(&lookup);
> > >
> > > -	if ((status < 0) && inode) {
> > > -		clear_nlink(inode);
> > > -		iput(inode);
> > > -	}
> > > -
> > >  	if (inode_ac)
> > >  		ocfs2_free_alloc_context(inode_ac);
> > >
> > > @@ -461,6 +456,17 @@ leave:
> > >  	if (meta_ac)
> > >  		ocfs2_free_alloc_context(meta_ac);
> > >
> > > +	/*
> > > +	We should call iput after the i_mutex of the bitmap
> > > +	been unlocked in ocfs2_free_alloc_context, or the ocfs2_delete_inode
> > > +	will mutex_lock again.
> > > +	*/
> >
> > Comment style here is wrong. It should look like:
> >
> > 	/*
> > 	 * We should call iput after the i_mutex of the bitmap been
> > 	 * unlocked in ocfs2_free_alloc_context, or the
> > 	 * ocfs2_delete_inode will mutex_lock again.
> > 	 */
> >
> > > +	if ((status < 0) && inode) {
> > > +		OCFS2_I(inode)->ip_flags |= OCFS2_INODE_SKIP_ORPHAN_DIR;
> > > +		clear_nlink(inode);
> > > +		iput(inode);
> > > +	}
> >
> > So, I realized that this doesn't fix all the issues we have.
> >
> > If you look at symlink and mknod, you'll see that we can get here _after_
> > the inode name has been added to the directory. Removing the inode then
> > would leave a dangling directory reference. The following patch tries to
> >  fix the issue. Can you please confirm that it works?
> 
> Thanks for pointing out this and fixing the comment style and the
>  description, I've tested with your patch and it works for me.
> Br,
> Li Dongyang
> 
> > If you want to pull the whole set of patches (with some cleanups by me),
> >  you can get it at:
> >
> > git pull
> >  git://git.kernel.org/pub/scm/linux/kernel/git/mfasheh/ocfs2-mark.git
> >  skip_delete_inode ---Mark
> >
> > --
> > Mark Fasheh
> >
> > From: Mark Fasheh <mfasheh at suse.com>
> >
> > ocfs2: Add directory entry later in ocfs2_symlink() and ocfs2_mknod()
> >
> > If we get a failure during creation of an inode we'll allow the orphan
> > code to remove the inode, which is correct. However, we need to ensure
> > that we don't get any errors after the call to ocfs2_add_entry(),
> > otherwise we could leave a dangling directory reference. The solution is
> > simple - in both cases, all I had to do was move
> > ocfs2_dentry_attach_lock() above the ocfs2_add_entry() call.
> >
> > Signed-off-by: Mark Fasheh <mfasheh at suse.com>
> > ---
> >  fs/ocfs2/namei.c |   40 +++++++++++++++++++++++++---------------
> >  1 files changed, 25 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> > index 8ff035e..4cbb18f 100644
> > --- a/fs/ocfs2/namei.c
> > +++ b/fs/ocfs2/namei.c
> > @@ -408,23 +408,28 @@ static int ocfs2_mknod(struct inode *dir,
> >  		}
> >  	}
> >
> > -	status = ocfs2_add_entry(handle, dentry, inode,
> > -				 OCFS2_I(inode)->ip_blkno, parent_fe_bh,
> > -				 &lookup);
> > -	if (status < 0) {
> > +	/*
> > +	 * Do this before adding the entry to the directory. We add
> > +	 * also set d_op after success so that ->d_iput() will cleanup
> > +	 * the dentry lock even if ocfs2_add_entry() fails below.
> > +	 */
> > +	status = ocfs2_dentry_attach_lock(dentry, inode,
> > +					  OCFS2_I(dir)->ip_blkno);
> > +	if (status) {
> >  		mlog_errno(status);
> >  		goto leave;
> >  	}
> > +	dentry->d_op = &ocfs2_dentry_ops;
> >
> > -	status = ocfs2_dentry_attach_lock(dentry, inode,
> > -					  OCFS2_I(dir)->ip_blkno);
> > -	if (status) {
> > +	status = ocfs2_add_entry(handle, dentry, inode,
> > +				 OCFS2_I(inode)->ip_blkno, parent_fe_bh,
> > +				 &lookup);
> > +	if (status < 0) {
> >  		mlog_errno(status);
> >  		goto leave;
> >  	}
> >
> >  	insert_inode_hash(inode);
> > -	dentry->d_op = &ocfs2_dentry_ops;
> >  	d_instantiate(dentry, inode);
> >  	status = 0;
> >  leave:
> > @@ -1777,22 +1782,27 @@ static int ocfs2_symlink(struct inode *dir,
> >  		}
> >  	}
> >
> > -	status = ocfs2_add_entry(handle, dentry, inode,
> > -				 le64_to_cpu(fe->i_blkno), parent_fe_bh,
> > -				 &lookup);
> > -	if (status < 0) {
> > +	/*
> > +	 * Do this before adding the entry to the directory. We add
> > +	 * also set d_op after success so that ->d_iput() will cleanup
> > +	 * the dentry lock even if ocfs2_add_entry() fails below.
> > +	 */
> > +	status = ocfs2_dentry_attach_lock(dentry, inode,
> > OCFS2_I(dir)->ip_blkno); +	if (status) {
> >  		mlog_errno(status);
> >  		goto bail;
> >  	}
> > +	dentry->d_op = &ocfs2_dentry_ops;
> >
> > -	status = ocfs2_dentry_attach_lock(dentry, inode,
> > OCFS2_I(dir)->ip_blkno); -	if (status) {
> > +	status = ocfs2_add_entry(handle, dentry, inode,
> > +				 le64_to_cpu(fe->i_blkno), parent_fe_bh,
> > +				 &lookup);
> > +	if (status < 0) {
> >  		mlog_errno(status);
> >  		goto bail;
> >  	}
> >
> >  	insert_inode_hash(inode);
> > -	dentry->d_op = &ocfs2_dentry_ops;
> >  	d_instantiate(dentry, inode);
> >  bail:
> >  	if (status < 0 && did_quota)
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 
forgot to add tested-by
I've kept fsstress running on my box around 50hrs with these 4 
OCFS2_SKIP_OPRHAN_DIR patches, 3 from me and 1 from Mark, no problems so far.

Tested-by: Li Dongyang <lidongyang at novell.com>

Br,
Li Dongyang