[Ocfs2-devel] [PATCH] ocfs2: alloc orphaned inode in ocfs2_symlink

[Ocfs2-devel] [PATCH] ocfs2: alloc orphaned inode in ocfs2_symlink

[Li Dongyang]

alloc the inode orphaned in ocfs2_symlink so we can delete the inode
in the iput if we meet errors with the inode, e.g. ocfs2_add_inode_data.
or ocfs2_query_inode_wipe will complain the inode is not orphaned on disk.


Signed-off-by: Li Dongyang <lidongyang@novell.com>
---
 fs/ocfs2/namei.c |   60 +++++++++++++++++++++++++++++------------------------
 1 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index f010b22..daf1fdf 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -94,6 +94,8 @@ static int ocfs2_create_symlink_data(struct ocfs2_super 
*osb,
 				     struct inode *inode,
 				     const char *symname);
 
+static int ocfs2_blkno_stringify(u64 blkno, char *name);
+
 /* An orphan dir name is an 8 byte value, printed as a hex string */
 #define OCFS2_ORPHAN_NAMELEN ((int)(2 * sizeof(u64)))
 
@@ -1579,6 +1581,7 @@ static int ocfs2_symlink(struct inode *dir,
 	u64 newsize;
 	struct ocfs2_super *osb = NULL;
 	struct inode *inode = NULL;
+	struct inode *orphan_dir = NULL;
 	struct super_block *sb;
 	struct buffer_head *new_fe_bh = NULL;
 	struct buffer_head *parent_fe_bh = NULL;
@@ -1594,7 +1597,9 @@ static int ocfs2_symlink(struct inode *dir,
 		.enable = 1,
 	};
 	int did_quota = 0, did_quota_inode = 0;
+	char orphan_name[OCFS2_ORPHAN_NAMELEN + 1];
 	struct ocfs2_dir_lookup_result lookup = { NULL, };
+	struct ocfs2_dir_lookup_result orphan_insert = { NULL, };
 
 	mlog_entry("(0x%p, 0x%p, symname='%s' actual='%.*s')\n", dir,
 		   dentry, symname, dentry->d_name.len, dentry->d_name.name);
@@ -1621,14 +1626,9 @@ static int ocfs2_symlink(struct inode *dir,
 		goto bail;
 	}
 
-	status = ocfs2_check_dir_for_entry(dir, dentry->d_name.name,
-					   dentry->d_name.len);
-	if (status)
-		goto bail;
-
-	status = ocfs2_prepare_dir_for_insert(osb, dir, parent_fe_bh,
-					      dentry->d_name.name,
-					      dentry->d_name.len, &lookup);
+	status = ocfs2_prepare_orphan_dir(osb, &orphan_dir,
+					  osb->root_blkno,
+					  orphan_name, &orphan_insert);
 	if (status < 0) {
 		mlog_errno(status);
 		goto bail;
@@ -1709,7 +1709,20 @@ static int ocfs2_symlink(struct inode *dir,
 		goto bail;
 	}
 
+	status = ocfs2_blkno_stringify(OCFS2_I(inode)->ip_blkno, orphan_name);
+	if (status < 0) {
+		mlog_errno(status);
+		goto bail;
+	}
+
 	fe = (struct ocfs2_dinode *) new_fe_bh->b_data;
+	status = ocfs2_orphan_add(osb, handle, inode, fe, orphan_name,
+				  &orphan_insert, orphan_dir);
+	if (status < 0) {
+		mlog_errno(status);
+		goto bail;
+	}
+
 	inode->i_rdev = 0;
 	newsize = l - 1;
 	if (l > ocfs2_fast_symlink_chars(sb)) {
@@ -1768,24 +1781,6 @@ static int ocfs2_symlink(struct inode *dir,
 			goto bail;
 		}
 	}
-
-	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;
-	}
-
-	status = ocfs2_dentry_attach_lock(dentry, inode, OCFS2_I(dir)->ip_blkno);
-	if (status) {
-		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)
 		vfs_dq_free_space_nodirty(inode,
@@ -1802,13 +1797,24 @@ bail:
 	kfree(si.name);
 	kfree(si.value);
 	ocfs2_free_dir_lookup_result(&lookup);
+	ocfs2_free_dir_lookup_result(&orphan_insert);
 	if (inode_ac)
 		ocfs2_free_alloc_context(inode_ac);
 	if (data_ac)
 		ocfs2_free_alloc_context(data_ac);
 	if (xattr_ac)
 		ocfs2_free_alloc_context(xattr_ac);
-	if ((status < 0) && inode) {
+	if (orphan_dir) {
+		ocfs2_inode_unlock(orphan_dir, 1);
+		mutex_unlock(&orphan_dir->i_mutex);
+		iput(orphan_dir);
+	}
+	if (inode && !status) {
+		status = ocfs2_mv_orphaned_inode_to_new(dir, inode, dentry);
+		if (status)
+			mlog_errno(status);
+	}
+	if (inode && status < 0) {
 		clear_nlink(inode);
 		iput(inode);
 	}
-- 
1.7.0.3

[Ocfs2-devel] [PATCH] ocfs2: alloc orphaned inode in ocfs2_symlink

[Joel Becker]

On Thu, Apr 08, 2010 at 09:13:08AM +0800, Li Dongyang wrote:
> alloc the inode orphaned in ocfs2_symlink so we can delete the inode
> in the iput if we meet errors with the inode, e.g. ocfs2_add_inode_data.
> or ocfs2_query_inode_wipe will complain the inode is not orphaned on disk.

	Huh.  At first I was sure this was ok, as the iput is sanely
killing the inode.  But you're right, the wipe code expects it to be
orphaned.
	Do you have a testcase for this?

Joel

-- 

Life's Little Instruction Book #497

	"Go down swinging."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH] ocfs2: alloc orphaned inode in ocfs2_symlink

[Mark Fasheh]

Hi Li,

On Thu, Apr 08, 2010 at 09:13:08AM +0800, Li Dongyang wrote:
> alloc the inode orphaned in ocfs2_symlink so we can delete the inode
> in the iput if we meet errors with the inode, e.g. ocfs2_add_inode_data.
> or ocfs2_query_inode_wipe will complain the inode is not orphaned on disk.

Thanks for the patch. Your solution is too heavy for such a special case -
we can't lock the orphan dir and orphan / de-orphan an inode every time a
symlink is made.

The good news is that I believe there's an easier way to handle this. You
can add a flag to ocfs2_inode_info->ip_flags (see fs/ocfs2/inode.h). I would
call this flag something like OCFS2_INODE_SKIP_ORPHAN_DIR. If the allocation
fails, we could mark the inode with that flag. From a quick look at the
code, the only places that need additional checking for the flag would be:
ocfs2_query_inode_wipe() and ocfs2_wipe_inode().


Ultimately, I think the best solution is for us to delete the inode
"in-place". We actually already have all the locks and journal credits we
need at that point. It's a bit tricky though because we have to be careful
what state the inode gets set in, which is why for a short-term fix, the
above solution is ok.
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH] ocfs2: alloc orphaned inode in ocfs2_symlink

[Li Dongyang]

On Friday 09 April 2010 05:12:22 Joel Becker wrote:
> On Thu, Apr 08, 2010 at 09:13:08AM +0800, Li Dongyang wrote:
> > alloc the inode orphaned in ocfs2_symlink so we can delete the inode
> > in the iput if we meet errors with the inode, e.g. ocfs2_add_inode_data.
> > or ocfs2_query_inode_wipe will complain the inode is not orphaned on
> > disk.
> 
> 	Huh.  At first I was sure this was ok, as the iput is sanely
> killing the inode.  But you're right, the wipe code expects it to be
> orphaned.
> 	Do you have a testcase for this?
> 
> Joel
> 
I found it while trying to reproduce the bug fixed by the patch ocfs2: avoid 
direct write if we fall back to buffered.
In my case, just run fsstress on a single node, and when the ocfs2 partition 
is out of space, ocfs2_add_inode_data will return -ENOSPC and then will 
trigger the error in ocfs2_query_inode_wipe

[Ocfs2-devel] [PATCH] ocfs2: alloc orphaned inode in ocfs2_symlink

[Li Dongyang]

On Friday 09 April 2010 07:10:22 Mark Fasheh wrote:
> Hi Li,
> 
> On Thu, Apr 08, 2010 at 09:13:08AM +0800, Li Dongyang wrote:
> > alloc the inode orphaned in ocfs2_symlink so we can delete the inode
> > in the iput if we meet errors with the inode, e.g. ocfs2_add_inode_data.
> > or ocfs2_query_inode_wipe will complain the inode is not orphaned on
> > disk.
> 
> Thanks for the patch. Your solution is too heavy for such a special case -
> we can't lock the orphan dir and orphan / de-orphan an inode every time a
> symlink is made.
> 
Hi, Mark,
Thanks for reviewing, Jan Kara and I also think this is expensive.
> The good news is that I believe there's an easier way to handle this. You
> can add a flag to ocfs2_inode_info->ip_flags (see fs/ocfs2/inode.h). I
>  would call this flag something like OCFS2_INODE_SKIP_ORPHAN_DIR. If the
>  allocation fails, we could mark the inode with that flag. From a quick
>  look at the code, the only places that need additional checking for the
>  flag would be: ocfs2_query_inode_wipe() and ocfs2_wipe_inode().
> 
Looking into that.
> 
> Ultimately, I think the best solution is for us to delete the inode
> "in-place". We actually already have all the locks and journal credits we
> need at that point. It's a bit tricky though because we have to be careful
> what state the inode gets set in, which is why for a short-term fix, the
> above solution is ok.
Glad to know this way, maybe I'll post an updated patch.
> 	--Mark
> 
> --
> Mark Fasheh
>