ocfs2: make direntry invalid when deleting it

PATCH New Wengang Wang 4 participants 5 messages Jul 12, 2011 – Aug 22, 2011 OCFS2

Share & tools

Wengang Wang

author

2011-07-12 08:43 UTC #0 lore raw

PATCH 1/1 ocfs2: make direntry invalid when deleting it

When we deleting a direntry from a directory, if it's the first in a block we invalid it by setting inode to 0; otherwise, we merge the deleted one to the prior and contiguous direntry. And we don't truncate directories.

There is a problem for the later case since inode is not set to 0. This problem happens when the caller passes a file position as parameter to ocfs2_dir_foreach_blk(). If the position happens to point to a stale(not the first, deleted in betweens of ocfs2_dir_foreach_blk()s) direntry, we are not able to recognize its staleness. So that we treat it as a live one wrongly.

The fix is to set inode to 0 in both cases indicating the direntry is stale. This won't introduce additional IOs.

Signed-off-by Wengang Wang <[email protected]>

---
 fs/ocfs2/dir.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

fs/ocfs2/dir.c +1 -2

index 8582e3f..3302088 100644
@@ -1184,8 +1184,7 @@ static int __ocfs2_delete_entry(handle_t *handle, struct inode *dir,
 			if (pde)
 				le16_add_cpu(&pde->rec_len,
 						le16_to_cpu(de->rec_len));
-			else
-				de->inode = 0;
+			de->inode = 0;
 			dir->i_version++;
 			ocfs2_journal_dirty(handle, bh);
 			goto bail;

Signature · 1 line

1.7.5.2

Sunil Mushran

2011-07-12 15:31 UTC #1 lore raw

PATCH 1/1 ocfs2: make direntry invalid when deleting it

Acked-by Sunil Mushran <[email protected]>

Quoted 32 lines · Wengang Wang

When we deleting a direntry from a directory, if it's the first in a block we invalid it by setting inode to 0; otherwise, we merge the deleted one to the prior and contiguous direntry. And we don't truncate directories.
There is a problem for the later case since inode is not set to 0. This problem happens when the caller passes a file position as parameter to ocfs2_dir_foreach_blk(). If the position happens to point to a stale(not the first, deleted in betweens of ocfs2_dir_foreach_blk()s) direntry, we are not able to recognize its staleness. So that we treat it as a live one wrongly.
The fix is to set inode to 0 in both cases indicating the direntry is stale. This won't introduce additional IOs.
Signed-off-by Wengang Wang<[email protected]>
---
  fs/ocfs2/dir.c |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)
fs/ocfs2/dir.c +1 -2
index 8582e3f..3302088 100644
@@ -1184,8 +1184,7 @@ static int __ocfs2_delete_entry(handle_t *handle, struct inode *dir,
  			if (pde)
  				le16_add_cpu(&pde->rec_len,
  						le16_to_cpu(de->rec_len));
-			else
-				de->inode = 0;
+			de->inode = 0;
  			dir->i_version++;
  			ocfs2_journal_dirty(handle, bh);
  			goto bail;

Tao Ma

2011-07-13 02:04 UTC #2 lore raw

PATCH 1/1 ocfs2: make direntry invalid when deleting it

In reply to Wengang Wang

Hi wengang,

Quoted 9 lines · Wengang Wang

When we deleting a direntry from a directory, if it's the first in a block we invalid it by setting inode to 0; otherwise, we merge the deleted one to the prior and contiguous direntry. And we don't truncate directories.
There is a problem for the later case since inode is not set to 0. This problem happens when the caller passes a file position as parameter to ocfs2_dir_foreach_blk(). If the position happens to point to a stale(not the first, deleted in betweens of ocfs2_dir_foreach_blk()s) direntry, we are not able to recognize its staleness. So that we treat it as a live one wrongly.

looks fine to me.
But do you have any test cases that expose this bug or is it just inspired by code review?

Thnaks
Tao

Quoted 23 lines

The fix is to set inode to 0 in both cases indicating the direntry is stale. This won't introduce additional IOs.

Signed-off-by Wengang Wang <[email protected]>

---
 fs/ocfs2/dir.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

fs/ocfs2/dir.c +1 -2

index 8582e3f..3302088 100644
@@ -1184,8 +1184,7 @@ static int __ocfs2_delete_entry(handle_t *handle, struct inode *dir,
 			if (pde)
 				le16_add_cpu(&pde->rec_len,
 						le16_to_cpu(de->rec_len));
-			else
-				de->inode = 0;
+			de->inode = 0;
 			dir->i_version++;
 			ocfs2_journal_dirty(handle, bh);
 			goto bail;

Wengang Wang

author

2011-07-13 02:52 UTC #3 lore raw

PATCH 1/1 ocfs2: make direntry invalid when deleting it

Quoted 6 lines · Tao Ma

Hi wengang,
Quoted 4 lines · Wengang Wang
When we deleting a direntry from a directory, if it's the first in a block we invalid it by setting inode to 0; otherwise, we merge the deleted one to the prior and contiguous direntry. And we don't truncate directories.

Quoted 8 lines

Quoted 5 lines
There is a problem for the later case since inode is not set to 0. This problem happens when the caller passes a file position as parameter to ocfs2_dir_foreach_blk(). If the position happens to point to a stale(not the first, deleted in betweens of ocfs2_dir_foreach_blk()s) direntry, we are not able to recognize its staleness. So that we treat it as a live one wrongly.
looks fine to me.
But do you have any test cases that expose this bug or is it just inspired by code review?

Hi Tao,
Actually I hit the problem while making patch for orphan scan(thougth not proved to be this one exactly). It is the result of our(Srini and me) analysis. I am going to make some test on this today.
For readdir path, I think it's not hard to reproduce :)

thanks,
wengang.

Quoted 27 lines

Thnaks
Tao

Quoted 23 lines

The fix is to set inode to 0 in both cases indicating the direntry is stale. This won't introduce additional IOs.

Signed-off-by Wengang Wang <[email protected]>

---
 fs/ocfs2/dir.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

fs/ocfs2/dir.c +1 -2

index 8582e3f..3302088 100644
@@ -1184,8 +1184,7 @@ static int __ocfs2_delete_entry(handle_t *handle, struct inode *dir,
 			if (pde)
 				le16_add_cpu(&pde->rec_len,
 						le16_to_cpu(de->rec_len));
-			else
-				de->inode = 0;
+			de->inode = 0;
 			dir->i_version++;
 			ocfs2_journal_dirty(handle, bh);
 			goto bail;

Joel Becker

maintainer

2011-08-22 04:29 UTC #4 lore raw

PATCH 1/1 ocfs2: make direntry invalid when deleting it

In reply to Wengang Wang

This patch is now in the fixes branch of ocfs2.git.

Joel

Quoted 40 lines · Wengang Wang

When we deleting a direntry from a directory, if it's the first in a block we invalid it by setting inode to 0; otherwise, we merge the deleted one to the prior and contiguous direntry. And we don't truncate directories.
There is a problem for the later case since inode is not set to 0. This problem happens when the caller passes a file position as parameter to ocfs2_dir_foreach_blk(). If the position happens to point to a stale(not the first, deleted in betweens of ocfs2_dir_foreach_blk()s) direntry, we are not able to recognize its staleness. So that we treat it as a live one wrongly.
The fix is to set inode to 0 in both cases indicating the direntry is stale. This won't introduce additional IOs.
Signed-off-by Wengang Wang <[email protected]>
---
 fs/ocfs2/dir.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
fs/ocfs2/dir.c +1 -2
index 8582e3f..3302088 100644
@@ -1184,8 +1184,7 @@ static int __ocfs2_delete_entry(handle_t *handle, struct inode *dir,
 			if (pde)
 				le16_add_cpu(&pde->rec_len,
 						le16_to_cpu(de->rec_len));
-			else
-				de->inode = 0;
+			de->inode = 0;
 			dir->i_version++;
 			ocfs2_journal_dirty(handle, bh);
 			goto bail;
Signature · 7 lines
1.7.5.2


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel

Signature · 6 lines

"What no boss of a programmer can ever understand is that a programmer
 is working when he's staring out of the window"
	- With apologies to Burton Rascoe

			http://www.jlbec.org/
			jlbec at evilplan.org