| Message ID | 20140319211005.3A1EB5A41DB@corp2gmr1-2.hot.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 19, 2014 at 02:10:04PM -0700, Andrew Morton wrote: > From: jiangyiwen <jiangyiwen@huawei.com> > Subject: ocfs2: manually do the iput once ocfs2_add_entry failed in ocfs2_symlink and ocfs2_mknod > > When the call to ocfs2_add_entry() failed in ocfs2_symlink() and > ocfs2_mknod(), iput() will not be called during dput(dentry) because no > d_instantiate(), and this will lead to umount hung. This is probably the heaviest way to do this. If you're getting a hang on mount, there's something wrong with the cleanup code in fs/ocfs2/dcache.c. See the following comment just above ocfs2_add_entry() /* * 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; } status = ocfs2_add_entry(handle, dentry, inode, OCFS2_I(inode)->ip_blkno, parent_fe_bh, &lookup); if (status < 0) { mlog_errno(status); goto leave; } So if you hung during unmount on a dentry lock, I think we need to investigate that path of recovery first. If there's something that it simply can't handle, then the large block of code you copied into namei.c needs to go into it's own function (or be a function broken out of the dentry lock cleanup code). Thanks, --Mark -- Mark Fasheh
On 2014/4/2 3:45, Mark Fasheh wrote: > On Wed, Mar 19, 2014 at 02:10:04PM -0700, Andrew Morton wrote: >> From: jiangyiwen <jiangyiwen@huawei.com> >> Subject: ocfs2: manually do the iput once ocfs2_add_entry failed in ocfs2_symlink and ocfs2_mknod >> >> When the call to ocfs2_add_entry() failed in ocfs2_symlink() and >> ocfs2_mknod(), iput() will not be called during dput(dentry) because no >> d_instantiate(), and this will lead to umount hung. > > This is probably the heaviest way to do this. If you're getting a hang on > mount, there's something wrong with the cleanup code in fs/ocfs2/dcache.c. > See the following comment just above ocfs2_add_entry() > > /* > * 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; > } > > status = ocfs2_add_entry(handle, dentry, inode, > OCFS2_I(inode)->ip_blkno, parent_fe_bh, > &lookup); > if (status < 0) { > mlog_errno(status); > goto leave; > } Mark: I am sorry I reply you too late. The above comment was added in your patch "ocfs2: Add directory entry later in ocfs2_symlink() and ocfs2_mknod()". But there has two problems: 1. I think this comment has little effect, d_iput() will not be called during dput(dentry) because no d_instantiate(). So, I will manually do the iput. I think your suggestions very good, I will form a function to include the large block of code. 2. Why do this handle only in ocfs2_mknod and ocfs2_symlink, not in ocfs2_link and ocfs2_mv_orphaned_inode_to_new? Thanks, --jiangyiwen > > So if you hung during unmount on a dentry lock, I think we need to > investigate that path of recovery first. If there's something that it simply > can't handle, then the large block of code you copied into namei.c needs to > go into it's own function (or be a function broken out of the dentry lock > cleanup code). > > Thanks, > --Mark > > -- > Mark Fasheh > >
diff -puN fs/ocfs2/namei.c~ocfs2-manually-do-the-iput-once-ocfs2_add_entry-failed-in-ocfs2_symlink-and-ocfs2_mknod fs/ocfs2/namei.c --- a/fs/ocfs2/namei.c~ocfs2-manually-do-the-iput-once-ocfs2_add_entry-failed-in-ocfs2_symlink-and-ocfs2_mknod +++ a/fs/ocfs2/namei.c @@ -231,6 +231,7 @@ static int ocfs2_mknod(struct inode *dir sigset_t oldset; int did_block_signals = 0; struct posix_acl *default_acl = NULL, *acl = NULL; + struct ocfs2_dentry_lock *dl = NULL; trace_ocfs2_mknod(dir, dentry, dentry->d_name.len, dentry->d_name.name, (unsigned long long)OCFS2_I(dir)->ip_blkno, @@ -423,6 +424,8 @@ static int ocfs2_mknod(struct inode *dir goto leave; } + dl = dentry->d_fsdata; + status = ocfs2_add_entry(handle, dentry, inode, OCFS2_I(inode)->ip_blkno, parent_fe_bh, &lookup); @@ -470,6 +473,16 @@ leave: * ocfs2_delete_inode will mutex_lock again. */ if ((status < 0) && inode) { + if (dl) { + ocfs2_simple_drop_lockres(osb, &dl->dl_lockres); + ocfs2_lock_res_free(&dl->dl_lockres); + BUG_ON(dl->dl_count != 1); + spin_lock(&dentry_attach_lock); + dentry->d_fsdata = NULL; + spin_unlock(&dentry_attach_lock); + kfree(dl); + iput(inode); + } OCFS2_I(inode)->ip_flags |= OCFS2_INODE_SKIP_ORPHAN_DIR; clear_nlink(inode); iput(inode); @@ -1739,6 +1752,7 @@ static int ocfs2_symlink(struct inode *d struct ocfs2_dir_lookup_result lookup = { NULL, }; sigset_t oldset; int did_block_signals = 0; + struct ocfs2_dentry_lock *dl = NULL; trace_ocfs2_symlink_begin(dir, dentry, symname, dentry->d_name.len, dentry->d_name.name); @@ -1927,6 +1941,8 @@ static int ocfs2_symlink(struct inode *d goto bail; } + dl = dentry->d_fsdata; + status = ocfs2_add_entry(handle, dentry, inode, le64_to_cpu(fe->i_blkno), parent_fe_bh, &lookup); @@ -1962,6 +1978,16 @@ bail: if (xattr_ac) ocfs2_free_alloc_context(xattr_ac); if ((status < 0) && inode) { + if (dl) { + ocfs2_simple_drop_lockres(osb, &dl->dl_lockres); + ocfs2_lock_res_free(&dl->dl_lockres); + BUG_ON(dl->dl_count != 1); + spin_lock(&dentry_attach_lock); + dentry->d_fsdata = NULL; + spin_unlock(&dentry_attach_lock); + kfree(dl); + iput(inode); + } OCFS2_I(inode)->ip_flags |= OCFS2_INODE_SKIP_ORPHAN_DIR; clear_nlink(inode); iput(inode);