Li Dongyang
2010-Apr-22 08:11 UTC
[Ocfs2-devel] [PATCH 3/3] ocfs2: use OCFS2_INODE_SKIP_ORPHAN_DIR in ocfs2_mknod error path
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. + */ + if ((status < 0) && inode) { + OCFS2_I(inode)->ip_flags |= OCFS2_INODE_SKIP_ORPHAN_DIR; + clear_nlink(inode); + iput(inode); + } + mlog_exit(status); return status; -- 1.6.4.2
Mark Fasheh
2010-Apr-23 19:59 UTC
[Ocfs2-devel] [PATCH 3/3] ocfs2: use OCFS2_INODE_SKIP_ORPHAN_DIR in ocfs2_mknod error path
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? 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) -- 1.6.4.2