[Ocfs2-devel] [PATCH 1/2] Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

[Tristan Ye]

Currently, some callers were missing to journal the dirty inode after
adding it to orphan dir.

Now we're going to journal such modifications within the ocfs2_orphan_add()
itself, It's safe to do so, though some existing caller may duplicate this,
and it makes the logic look more straightforward anyway.

Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
 fs/ocfs2/namei.c |   32 +++++++++++++++++++++++++++-----
 1 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 50fb26a..e501adf 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -84,7 +84,7 @@ static int ocfs2_prepare_orphan_dir(struct ocfs2_super *osb,
 static int ocfs2_orphan_add(struct ocfs2_super *osb,
 			    handle_t *handle,
 			    struct inode *inode,
-			    struct ocfs2_dinode *fe,
+			    struct buffer_head *fe_bh,
 			    char *name,
 			    struct ocfs2_dir_lookup_result *lookup,
 			    struct inode *orphan_dir_inode);
@@ -877,7 +877,7 @@ 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, orphan_name,
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
 					  &orphan_insert, orphan_dir);
 		if (status < 0) {
 			mlog_errno(status);
@@ -1295,7 +1295,7 @@ static int ocfs2_rename(struct inode *old_dir,
 		if (S_ISDIR(new_inode->i_mode) ||
 		    (ocfs2_read_links_count(newfe) == 1)) {
 			status = ocfs2_orphan_add(osb, handle, new_inode,
-						  newfe, orphan_name,
+						  newfe_bh, orphan_name,
 						  &orphan_insert, orphan_dir);
 			if (status < 0) {
 				mlog_errno(status);
@@ -1909,7 +1909,7 @@ leave:
 static int ocfs2_orphan_add(struct ocfs2_super *osb,
 			    handle_t *handle,
 			    struct inode *inode,
-			    struct ocfs2_dinode *fe,
+			    struct buffer_head *fe_bh,
 			    char *name,
 			    struct ocfs2_dir_lookup_result *lookup,
 			    struct inode *orphan_dir_inode)
@@ -1917,6 +1917,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	struct buffer_head *orphan_dir_bh = NULL;
 	int status = 0;
 	struct ocfs2_dinode *orphan_fe;
+	struct ocfs2_dinode *fe = (struct ocfs2_dinode *) fe_bh->b_data;
 
 	mlog_entry("(inode->i_ino = %lu)\n", inode->i_ino);
 
@@ -1957,6 +1958,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 		goto leave;
 	}
 
+	/* 
+	 * We're going to journal the change of i_flags and i_orphaned_slot.
+	 * It's safe anyway, though some callers may duplicate the journaling.
+	 * Journaling within the func just make the logic look more
+	 * straightforward.
+	 */
+	status = ocfs2_journal_access_di(handle,
+					 INODE_CACHE(inode),
+					 fe_bh,
+					 OCFS2_JOURNAL_ACCESS_WRITE);
+	if (status < 0) {
+		mlog_errno(status);
+		goto leave;
+	}
+
 	le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
 
 	/* Record which orphan dir our inode now resides
@@ -1964,6 +1980,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	 * dir to lock. */
 	fe->i_orphaned_slot = cpu_to_le16(osb->slot_num);
 
+	status = ocfs2_journal_dirty(handle, fe_bh);
+	if (status < 0) {
+		mlog_errno(status);
+		goto leave;
+	}
+
 	mlog(0, "Inode %llu orphaned in slot %d\n",
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno, osb->slot_num);
 
@@ -2125,7 +2147,7 @@ int ocfs2_create_inode_in_orphan(struct inode *dir,
 	}
 
 	di = (struct ocfs2_dinode *)new_di_bh->b_data;
-	status = ocfs2_orphan_add(osb, handle, inode, di, orphan_name,
+	status = ocfs2_orphan_add(osb, handle, inode, new_di_bh, orphan_name,
 				  &orphan_insert, orphan_dir);
 	if (status < 0) {
 		mlog_errno(status);
-- 
1.5.5

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

[Tristan Ye]

Currently, some callers were missing to journal the dirty inode after
adding it to orphan dir.

Now we're going to journal such modifications within the ocfs2_orphan_add()
itself, It's safe to do so, though some existing caller may duplicate this,
and it makes the logic look more straightforward anyway.

Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
 fs/ocfs2/namei.c |   32 +++++++++++++++++++++++++++-----
 1 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 50fb26a..cafb921 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -84,7 +84,7 @@ static int ocfs2_prepare_orphan_dir(struct ocfs2_super *osb,
 static int ocfs2_orphan_add(struct ocfs2_super *osb,
 			    handle_t *handle,
 			    struct inode *inode,
-			    struct ocfs2_dinode *fe,
+			    struct buffer_head *fe_bh,
 			    char *name,
 			    struct ocfs2_dir_lookup_result *lookup,
 			    struct inode *orphan_dir_inode);
@@ -877,7 +877,7 @@ 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, orphan_name,
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
 					  &orphan_insert, orphan_dir);
 		if (status < 0) {
 			mlog_errno(status);
@@ -1295,7 +1295,7 @@ static int ocfs2_rename(struct inode *old_dir,
 		if (S_ISDIR(new_inode->i_mode) ||
 		    (ocfs2_read_links_count(newfe) == 1)) {
 			status = ocfs2_orphan_add(osb, handle, new_inode,
-						  newfe, orphan_name,
+						  newfe_bh, orphan_name,
 						  &orphan_insert, orphan_dir);
 			if (status < 0) {
 				mlog_errno(status);
@@ -1909,7 +1909,7 @@ leave:
 static int ocfs2_orphan_add(struct ocfs2_super *osb,
 			    handle_t *handle,
 			    struct inode *inode,
-			    struct ocfs2_dinode *fe,
+			    struct buffer_head *fe_bh,
 			    char *name,
 			    struct ocfs2_dir_lookup_result *lookup,
 			    struct inode *orphan_dir_inode)
@@ -1917,6 +1917,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	struct buffer_head *orphan_dir_bh = NULL;
 	int status = 0;
 	struct ocfs2_dinode *orphan_fe;
+	struct ocfs2_dinode *fe = (struct ocfs2_dinode *) fe_bh->b_data;
 
 	mlog_entry("(inode->i_ino = %lu)\n", inode->i_ino);
 
@@ -1957,6 +1958,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 		goto leave;
 	}
 
+	/*
+	 * We're going to journal the change of i_flags and i_orphaned_slot.
+	 * It's safe anyway, though some callers may duplicate the journaling.
+	 * Journaling within the func just make the logic look more
+	 * straightforward.
+	 */
+	status = ocfs2_journal_access_di(handle,
+					 INODE_CACHE(inode),
+					 fe_bh,
+					 OCFS2_JOURNAL_ACCESS_WRITE);
+	if (status < 0) {
+		mlog_errno(status);
+		goto leave;
+	}
+
 	le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
 
 	/* Record which orphan dir our inode now resides
@@ -1964,6 +1980,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	 * dir to lock. */
 	fe->i_orphaned_slot = cpu_to_le16(osb->slot_num);
 
+	status = ocfs2_journal_dirty(handle, fe_bh);
+	if (status < 0) {
+		mlog_errno(status);
+		goto leave;
+	}
+
 	mlog(0, "Inode %llu orphaned in slot %d\n",
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno, osb->slot_num);
 
@@ -2125,7 +2147,7 @@ int ocfs2_create_inode_in_orphan(struct inode *dir,
 	}
 
 	di = (struct ocfs2_dinode *)new_di_bh->b_data;
-	status = ocfs2_orphan_add(osb, handle, inode, di, orphan_name,
+	status = ocfs2_orphan_add(osb, handle, inode, new_di_bh, orphan_name,
 				  &orphan_insert, orphan_dir);
 	if (status < 0) {
 		mlog_errno(status);
-- 
1.5.5

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Handle deletion of reflinked oprhan inodes correctly.

[Tristan Ye]

Current rule of orphan dir is that all inodes in the orphan dir
have ORPHANED_FL, otherwise we treated it as an ERROR. this rule
works well except for some rare cases of reflink operation:

http://oss.oracle.com/bugzilla/show_bug.cgi?id=1215

The problem is introduced by the essense of how reflink and our
orphan_scan thread were working:

 * Orphan_scan scan the orphan dir into a queue first, and run
   queue in a later time, we only hold the orphan_dir's lock
   during scanning.

 * Reflink create a oprhaned target in orphan_dir at the first
   step, and remove the targets and unset the flag at the third
   step, these two steps respectively hold the orphan_dir's lock
   themselves.

Based on above semantics, there is a possibility that a reflink
inode can be moved out of the orphan dir and have its ORPHANED_FL
cleared before the queue is run, which leads to a ERROR in
ocfs2_query_wipde_inode().

This patch helps to judge if a orphan inode to be wiped off, which
has NO ORPHANED_FL, is a legal alive reflinked target or not.

The patch also works for failed reflinked targets from a crash or
other failures during the reflink operation, they can be wiped off
as desired since these failed reflinked inodes always has ORPHANED_FL
set ondisk.

Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
 fs/ocfs2/inode.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index 88459bd..be27e72 100644
--- a/fs/ocfs2/inode.c
+++ b/fs/ocfs2/inode.c
@@ -891,6 +891,21 @@ static int ocfs2_query_inode_wipe(struct inode *inode,
 	/* Do some basic inode verification... */
 	di = (struct ocfs2_dinode *) di_bh->b_data;
 	if (!(di->i_flags & cpu_to_le32(OCFS2_ORPHANED_FL))) {
+		/*
+		 * Inodes in the orphan dir must have ORPHANED_FL.  The only
+		 * inodes that come back out of the orphan dir are reflink
+		 * targets. A reflink target may be moved out of the orphan
+		 * dir between the time we scan the directory and the time we
+		 * process it. This would lead to HAS_REFCOUNT_FL being set but
+		 * ORPHANED_FL not.
+		 */
+		if (di->i_dyn_features & cpu_to_le16(OCFS2_HAS_REFCOUNT_FL)) {
+			mlog(0, "Reflinked inode %llu is no longer orphaned.  "
+			     "it shouldn't be deleted\n",
+			     (unsigned long long)oi->ip_blkno);
+			goto bail;
+		}
+
 		/* for lack of a better error? */
 		status = -EEXIST;
 		mlog(ML_ERROR,
-- 
1.5.5

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

[Joel Becker]

On Mon, Mar 01, 2010 at 03:23:11PM +0800, Tristan Ye wrote:
> +	status = ocfs2_journal_dirty(handle, fe_bh);
> +	if (status < 0) {
> +		mlog_errno(status);
> +		goto leave;
> +	}

	Don't check the status of ocfs2_journal_dirty().  It can't fail.
Just call it as if it were void.

Joel

-- 

Life's Little Instruction Book #306

	"Take a nap on Sunday afternoons."

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

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

[Tristan Ye]

Currently, some callers were missing to journal the dirty inode after
adding it to orphan dir.

Now we're going to journal such modifications within the ocfs2_orphan_add()
itself, It's safe to do so, though some existing caller may duplicate this,
and it makes the logic look more straightforward anyway.

Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
 fs/ocfs2/namei.c |   28 +++++++++++++++++++++++-----
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 50fb26a..5ad79ec 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -84,7 +84,7 @@ static int ocfs2_prepare_orphan_dir(struct ocfs2_super *osb,
 static int ocfs2_orphan_add(struct ocfs2_super *osb,
 			    handle_t *handle,
 			    struct inode *inode,
-			    struct ocfs2_dinode *fe,
+			    struct buffer_head *fe_bh,
 			    char *name,
 			    struct ocfs2_dir_lookup_result *lookup,
 			    struct inode *orphan_dir_inode);
@@ -877,7 +877,7 @@ 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, orphan_name,
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
 					  &orphan_insert, orphan_dir);
 		if (status < 0) {
 			mlog_errno(status);
@@ -1295,7 +1295,7 @@ static int ocfs2_rename(struct inode *old_dir,
 		if (S_ISDIR(new_inode->i_mode) ||
 		    (ocfs2_read_links_count(newfe) == 1)) {
 			status = ocfs2_orphan_add(osb, handle, new_inode,
-						  newfe, orphan_name,
+						  newfe_bh, orphan_name,
 						  &orphan_insert, orphan_dir);
 			if (status < 0) {
 				mlog_errno(status);
@@ -1909,7 +1909,7 @@ leave:
 static int ocfs2_orphan_add(struct ocfs2_super *osb,
 			    handle_t *handle,
 			    struct inode *inode,
-			    struct ocfs2_dinode *fe,
+			    struct buffer_head *fe_bh,
 			    char *name,
 			    struct ocfs2_dir_lookup_result *lookup,
 			    struct inode *orphan_dir_inode)
@@ -1917,6 +1917,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	struct buffer_head *orphan_dir_bh = NULL;
 	int status = 0;
 	struct ocfs2_dinode *orphan_fe;
+	struct ocfs2_dinode *fe = (struct ocfs2_dinode *) fe_bh->b_data;
 
 	mlog_entry("(inode->i_ino = %lu)\n", inode->i_ino);
 
@@ -1957,6 +1958,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 		goto leave;
 	}
 
+	/*
+	 * We're going to journal the change of i_flags and i_orphaned_slot.
+	 * It's safe anyway, though some callers may duplicate the journaling.
+	 * Journaling within the func just make the logic look more
+	 * straightforward.
+	 */
+	status = ocfs2_journal_access_di(handle,
+					 INODE_CACHE(inode),
+					 fe_bh,
+					 OCFS2_JOURNAL_ACCESS_WRITE);
+	if (status < 0) {
+		mlog_errno(status);
+		goto leave;
+	}
+
 	le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
 
 	/* Record which orphan dir our inode now resides
@@ -1964,6 +1980,8 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	 * dir to lock. */
 	fe->i_orphaned_slot = cpu_to_le16(osb->slot_num);
 
+	ocfs2_journal_dirty(handle, fe_bh);
+
 	mlog(0, "Inode %llu orphaned in slot %d\n",
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno, osb->slot_num);
 
@@ -2125,7 +2143,7 @@ int ocfs2_create_inode_in_orphan(struct inode *dir,
 	}
 
 	di = (struct ocfs2_dinode *)new_di_bh->b_data;
-	status = ocfs2_orphan_add(osb, handle, inode, di, orphan_name,
+	status = ocfs2_orphan_add(osb, handle, inode, new_di_bh, orphan_name,
 				  &orphan_insert, orphan_dir);
 	if (status < 0) {
 		mlog_errno(status);
-- 
1.5.5

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

[Mark Fasheh]

On Fri, Mar 19, 2010 at 09:21:09AM +0800, Tristan Ye wrote:
> Currently, some callers were missing to journal the dirty inode after
> adding it to orphan dir.
> 
> Now we're going to journal such modifications within the ocfs2_orphan_add()
> itself, It's safe to do so, though some existing caller may duplicate this,
> and it makes the logic look more straightforward anyway.
> 
> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>

I would have liked to see if any of the callers could be cleaned up.
Otherwise though, this patch looks good and it fixes a bug :)

Acked-by: Mark Fasheh <mfasheh@suse.com>
	--Mark


--
Mark Fasheh

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

[Joel Becker]

On Fri, Mar 19, 2010 at 04:42:46PM -0700, Mark Fasheh wrote:
> On Fri, Mar 19, 2010 at 09:21:09AM +0800, Tristan Ye wrote:
> > Currently, some callers were missing to journal the dirty inode after
> > adding it to orphan dir.
> > 
> > Now we're going to journal such modifications within the ocfs2_orphan_add()
> > itself, It's safe to do so, though some existing caller may duplicate this,
> > and it makes the logic look more straightforward anyway.
> > 
> > Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
> 
> I would have liked to see if any of the callers could be cleaned up.
> Otherwise though, this patch looks good and it fixes a bug :)

	Let's call this the fix and expect the followup.

Joel

-- 

"Anything that is too stupid to be spoken is sung."  
        - Voltaire

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

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

[Joel Becker]

On Fri, Mar 19, 2010 at 09:21:09AM +0800, Tristan Ye wrote:
> Currently, some callers were missing to journal the dirty inode after
> adding it to orphan dir.
> 
> Now we're going to journal such modifications within the ocfs2_orphan_add()
> itself, It's safe to do so, though some existing caller may duplicate this,
> and it makes the logic look more straightforward anyway.
> 
> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>

	This patch has been added to the 'fixes' branch of ocfs2.git.
Can you go over the callers of ocfs2_orphan_add() to see if there is any
cleanup you can do?  Redundant dirty calls where nothing else is
dirtied, etc.  That cleanup will be a separate patch that I push to the
merge window when done.

Joel

-- 

 Joel's First Law:

	Nature abhors a GUI.

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

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

[tristan]

Joel Becker wrote:
> On Fri, Mar 19, 2010 at 09:21:09AM +0800, Tristan Ye wrote:
>   
>> Currently, some callers were missing to journal the dirty inode after
>> adding it to orphan dir.
>>
>> Now we're going to journal such modifications within the ocfs2_orphan_add()
>> itself, It's safe to do so, though some existing caller may duplicate this,
>> and it makes the logic look more straightforward anyway.
>>
>> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
>>     
>
> 	This patch has been added to the 'fixes' branch of ocfs2.git.
> Can you go over the callers of ocfs2_orphan_add() to see if there is any
> cleanup you can do?  Redundant dirty calls where nothing else is
> dirtied, etc.  That cleanup will be a separate patch that I push to the
> merge window when done.
>   

Sure, you're right, redundant dirty calls doesn't make any sense, and 
even makes the logic confused.

I'll handle this.

Thanks,
Tristan.

> Joel
>
>   

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

[tristan]

Joel Becker wrote:
> On Fri, Mar 19, 2010 at 09:21:09AM +0800, Tristan Ye wrote:
>> Currently, some callers were missing to journal the dirty inode after
>> adding it to orphan dir.
>>
>> Now we're going to journal such modifications within the ocfs2_orphan_add()
>> itself, It's safe to do so, though some existing caller may duplicate this,
>> and it makes the logic look more straightforward anyway.
>>
>> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
>
> 	This patch has been added to the 'fixes' branch of ocfs2.git.
> Can you go over the callers of ocfs2_orphan_add() to see if there is any
> cleanup you can do?  Redundant dirty calls where nothing else is
> dirtied, etc.  That cleanup will be a separate patch that I push to the
> merge window when done.

3 callers are calling ocfs2_orphan_add() for now:

1. ocfs2_unlink()

2. ocfs2_rename()

3. ocfs2_create_inode_in_orphan()


caller #1 and #2 are also doing dirty journals, they however are not 
redundant, just for i_links_count change on-disk.

while #3 caller is doing nothing dirty journal outside the 
ocfs2_oprhan_add(), it's fine.

As in, we're not going to do any cleanup in the callers.


Regards,
Tristan.


>
> Joel
>