Track negative dentries v2

PATCH New Goldwyn Rodrigues 2 participants 3 messages Jun 24, 2010 – Jun 25, 2010 OCFS2

Share & tools

Goldwyn Rodrigues

author

2010-06-24 01:45 UTC #0 lore raw

PATCH 1/1 Track negative dentries v2

Track negative dentries by recording the generation number of the parent directory in d_fsdata. The generation number for the parent directory is recorded in the inode_info, which increments every time the lock on the directory is dropped.

If the generation number of the parent directory and the negative dentry matches, there is no need to perform the revalidate, else a revalidate is forced. This improves performance in situations where nodes look for the same non-existent file multiple times.

Thanks Mark for explaining the DLM sequence.

Signed-off-by Goldwyn Rodrigues <[email protected]>

---

fs/ocfs2/dcache.c +32 -4

index b4957c7..6db61a3 100644
@@ -40,6 +40,16 @@
 #include "inode.h"
 #include "super.h"
 
+/* Attach a generation number to a negative dentry */
+void ocfs2_dentry_attach_gen(struct dentry *dentry)
+{
+	unsigned long gen = OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
+	/* Specifically for negative dentries */
+	if (dentry->d_inode)
+		BUG();
+	dentry->d_fsdata = (void *)gen;
+}
+
 
 static int ocfs2_dentry_revalidate(struct dentry *dentry,
 				   struct nameidata *nd)

@@ -51,11 +61,22 @@ static int ocfs2_dentry_revalidate(struct dentry *dentry,
 	mlog_entry("(0x%p, '%.*s')\n", dentry,
 		   dentry->d_name.len, dentry->d_name.name);
 
-	/* Never trust a negative dentry - force a new lookup. */
+	/* For a negative dentry -
+	   check the generation number of the parent and compare with the
+	   one stored in the inode.
+	   */
 	if (inode == NULL) {
-		mlog(0, "negative dentry: %.*s\n", dentry->d_name.len,
-		     dentry->d_name.name);
-		goto bail;
+		unsigned long gen = (unsigned long) dentry->d_fsdata;
+		unsigned long pgen =
+			OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
+		mlog(0, "negative dentry: %.*s parent gen: %lu "
+			"dentry gen: %lu\n",
+			dentry->d_name.len, dentry->d_name.name, pgen, gen);
+		if (gen != pgen) {
+			dentry->d_fsdata = (void *) pgen;
+			goto bail;
+		}
+		goto valid;
 	}
 
 	BUG_ON(!osb);

@@ -96,6 +117,7 @@ static int ocfs2_dentry_revalidate(struct dentry *dentry,
 		goto bail;
 	}
 
+valid:
 	ret = 1;
 
 bail:

@@ -227,6 +249,12 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
 	if (!inode)
 		return 0;
 
+	if (!dentry->d_inode && dentry->d_fsdata) {
+		/* Converting a negative dentry to positive
+		   Clear dentry->d_fsdata */
+		dentry->d_fsdata = dl = NULL;
+	}
+
 	if (dl) {
 		mlog_bug_on_msg(dl->dl_parent_blkno != parent_blkno,
 				" \"%.*s\": old parent: %llu, new: %llu\n",

@@ -451,6 +479,7 @@ static void ocfs2_dentry_iput(struct dentry

*dentry, struct inode *inode)
ocfs2_dentry_lock_put(OCFS2_SB(dentry->d_sb), dl);

out:
+ ocfs2_dentry_attach_gen(dentry);
iput(inode);
}

fs/ocfs2/dcache.h +0 -0

index f5dd178..b79eff7 100644
@@ -64,5 +64,6 @@ void ocfs2_dentry_move(struct dentry *dentry, struct

dentry *target,
struct inode *old_dir, struct inode *new_dir);

extern spinlock_t dentry_attach_lock;
+void ocfs2_dentry_attach_gen(struct dentry *dentry);

#endif /* OCFS2_DCACHE_H */

fs/ocfs2/dlmglue.c +0 -0

index 39eb16a..bf79dcf 100644
@@ -3635,10 +3635,18 @@ static int ocfs2_data_convert_worker(struct

ocfs2_lock_res *lockres,
{
struct inode *inode;
struct address_space *mapping;
+ struct ocfs2_inode_info *oi;

        	inode = ocfs2_lock_res_inode(lockres);

mapping = inode->i_mapping;

+ if (S_ISDIR(inode->i_mode)) {
+ oi = OCFS2_I(inode);
+ oi->ip_generation++;
+ mlog(0, "generation: %u\n", oi->ip_generation);
+ goto out;
+ }
+
if (!S_ISREG(inode->i_mode))
goto out;

fs/ocfs2/inode.h +1 -0

index 9f5f5fc..34bad2c 100644
@@ -70,6 +70,7 @@ struct ocfs2_inode_info
 	/* Only valid if the inode is the dir. */
 	u32				ip_last_used_slot;
 	u64				ip_last_used_group;
+	u32				ip_generation; /*for -ve dentry */
 
 	struct ocfs2_alloc_reservation	ip_la_data_resv;
 };

fs/ocfs2/namei.c +2 -1

index f171b51..e89bce7 100644
@@ -171,7 +171,8 @@ bail_add:
 			ret = ERR_PTR(status);
 			goto bail_unlock;
 		}
-	}
+	} else
+		ocfs2_dentry_attach_gen(dentry);
 
 bail_unlock:
 	/* Don't drop the cluster lock until *after* the d_add --

Signature · 1 line

Goldwyn

Mark Fasheh

2010-06-24 23:03 UTC #1 lore raw

PATCH 1/1 Track negative dentries v2

Hi Goldwyn,

The patch looks pretty good so far - thanks for taking on this task!
My comments are inlined.

Quoted 20 lines · Goldwyn Rodrigues

Track negative dentries by recording the generation number of the parent directory in d_fsdata. The generation number for the parent directory is recorded in the inode_info, which increments every time the lock on the directory is dropped.
If the generation number of the parent directory and the negative dentry matches, there is no need to perform the revalidate, else a revalidate is forced. This improves performance in situations where nodes look for the same non-existent file multiple times.
Thanks Mark for explaining the DLM sequence.
Signed-off-by Goldwyn Rodrigues <[email protected]>
---
fs/ocfs2/dcache.c +0 -0
index b4957c7..6db61a3 100644
@@ -40,6 +40,16 @@
 #include "inode.h"

fs/ocfs2/dcache.c line 50

Quoted 10 lines

fs/ocfs2/dcache.c +7 -0

 #include "super.h"
 
+/* Attach a generation number to a negative dentry */
+void ocfs2_dentry_attach_gen(struct dentry *dentry)
+{
+	unsigned long gen = OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
+	/* Specifically for negative dentries */
 
+	if (dentry->d_inode)
+		BUG();

You should turn those two lines into:

	BUG_ON(dentry->d_inode);

fs/ocfs2/dcache.c static int ocfs2_dentry_revalidate(struct dentry *dentry, · line 54

Quoted 11 lines

fs/ocfs2/dcache.c +3 -0

+	dentry->d_fsdata = (void *)gen;
+}
+
 
 static int ocfs2_dentry_revalidate(struct dentry *dentry,

fs/ocfs2/dcache.c +0 -1

 				   struct nameidata *nd)

@@ -51,11 +61,22 @@ static int ocfs2_dentry_revalidate(struct dentry *dentry,
 	mlog_entry("(0x%p, '%.*s')\n", dentry,
 		   dentry->d_name.len, dentry->d_name.name);
 
-	/* Never trust a negative dentry - force a new lookup. */

Btw, it occurs to me that we could also optimize away local mounts in a seperate patch by always trusting the dentry passed in that case.

fs/ocfs2/dcache.c static int ocfs2_dentry_revalidate(struct dentry *dentry, · line 67

Quoted 4 lines

fs/ocfs2/dcache.c +4 -0

+	/* For a negative dentry -
+	   check the generation number of the parent and compare with the
+	   one stored in the inode.
+	   */

The preferred style for long (multi-line) comments is:

	/*
	 * line one
	 * line two
	 */

Quoted 4 lines

fs/ocfs2/dcache.c +0 -3

 	if (inode == NULL) {
-		mlog(0, "negative dentry: %.*s\n", dentry->d_name.len,
-		     dentry->d_name.name);
-		goto bail;

fs/ocfs2/dcache.c static int ocfs2_dentry_revalidate(struct dentry *dentry, · line 78

Quoted 10 lines

fs/ocfs2/dcache.c +10 -0

+		unsigned long gen = (unsigned long) dentry->d_fsdata;
+		unsigned long pgen =
+			OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
+		mlog(0, "negative dentry: %.*s parent gen: %lu "
+			"dentry gen: %lu\n",
+			dentry->d_name.len, dentry->d_name.name, pgen, gen);
+		if (gen != pgen) {
+			dentry->d_fsdata = (void *) pgen;
+			goto bail;
+		}

What are you setting the dentry generation here? We should only be doing that when under cluster lock when we are guaranteed that the dentry is negative.

Quoted 26 lines

fs/ocfs2/dcache.c +8 -0

+		goto valid;
 	}
 
 	BUG_ON(!osb);

@@ -96,6 +117,7 @@ static int ocfs2_dentry_revalidate(struct dentry *dentry,
 		goto bail;
 	}
 
+valid:
 	ret = 1;
 
 bail:

@@ -227,6 +249,12 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
 	if (!inode)
 		return 0;
 
+	if (!dentry->d_inode && dentry->d_fsdata) {
+		/* Converting a negative dentry to positive
+		   Clear dentry->d_fsdata */
+		dentry->d_fsdata = dl = NULL;
+	}
+
 	if (dl) {
 		mlog_bug_on_msg(dl->dl_parent_blkno != parent_blkno,
 				" \"%.*s\": old parent: %llu, new: %llu\n",

fs/ocfs2/dcache.c static void ocfs2_dentry_iput(struct dentry · line 483

Quoted 5 lines

fs/ocfs2/dcache.c +1 -0

@@ -451,6 +479,7 @@ static void ocfs2_dentry_iput(struct dentry
 *dentry, struct inode *inode)
 	ocfs2_dentry_lock_put(OCFS2_SB(dentry->d_sb), dl);
 
 out:
+	ocfs2_dentry_attach_gen(dentry);

Why the call to ocfs2_dentry_attach_gen() here?

Quoted 44 lines

fs/ocfs2/dcache.c +0 -0

 	iput(inode);
 }

fs/ocfs2/dcache.h +1 -0

index f5dd178..b79eff7 100644
@@ -64,5 +64,6 @@ void ocfs2_dentry_move(struct dentry *dentry, struct
 dentry *target,
 		       struct inode *old_dir, struct inode *new_dir);
 
 extern spinlock_t dentry_attach_lock;
+void ocfs2_dentry_attach_gen(struct dentry *dentry);

#endif /* OCFS2_DCACHE_H */

fs/ocfs2/dlmglue.c +8 -0

index 39eb16a..bf79dcf 100644
@@ -3635,10 +3635,18 @@ static int ocfs2_data_convert_worker(struct
 ocfs2_lock_res *lockres,
 {
 	struct inode *inode;
 	struct address_space *mapping;
+	struct ocfs2_inode_info *oi;
 
        	inode = ocfs2_lock_res_inode(lockres);
 	mapping = inode->i_mapping;
 
+	if (S_ISDIR(inode->i_mode)) {
+		oi = OCFS2_I(inode);
+		oi->ip_generation++;
+		mlog(0, "generation: %u\n", oi->ip_generation);
+		goto out;
+	}
+
 	if (!S_ISREG(inode->i_mode))
 		goto out;

fs/ocfs2/inode.h +0 -0

index 9f5f5fc..34bad2c 100644

fs/ocfs2/inode.h struct ocfs2_inode_info · line 73

Quoted 4 lines

fs/ocfs2/inode.h +1 -0

@@ -70,6 +70,7 @@ struct ocfs2_inode_info
 	/* Only valid if the inode is the dir. */
 	u32				ip_last_used_slot;
 	u64				ip_last_used_group;
+	u32				ip_generation; /*for -ve dentry */

Comment doesn't make sense :) Also, can you pick something we're less
likely to confuse with i_generation? Maybe ip_dir_lock_gen?

Also, it should be initialized to a nonzero value in when we're initializing the directory inode under cluster lock in ocfs2_read_locked_inode().

	--Mark

--
Mark Fasheh

Goldwyn Rodrigues

author

2010-06-25 16:13 UTC #2 lore raw

PATCH 1/1 Track negative dentries v2

Thanks Mark for reviewing.

Quoted 34 lines · Mark Fasheh

Hi Goldwyn,
? ? ? ?The patch looks pretty good so far - thanks for taking on this task! My comments are inlined.
Quoted 27 lines · Goldwyn Rodrigues
Track negative dentries by recording the generation number of the parent directory in d_fsdata. The generation number for the parent directory is recorded in the inode_info, which increments every time the lock on the directory is dropped.
If the generation number of the parent directory and the negative dentry matches, there is no need to perform the revalidate, else a revalidate is forced. This improves performance in situations where nodes look for the same non-existent file multiple times.
Thanks Mark for explaining the DLM sequence.
Signed-off-by Goldwyn Rodrigues <[email protected]>
---
fs/ocfs2/dcache.c +0 -0
index b4957c7..6db61a3 100644
@@ -40,6 +40,16 @@
?#include "inode.h"
?#include "super.h"
fs/ocfs2/dcache.c +5 -0
+/* Attach a generation number to a negative dentry */
+void ocfs2_dentry_attach_gen(struct dentry *dentry)
+{
+ ? ? unsigned long gen = OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
+ ? ? /* Specifically for negative dentries */

Quoted 6 lines

Quoted 2 lines
+ ? ? if (dentry->d_inode)
+ ? ? ? ? ? ? BUG();
You should turn those two lines into:
? ? ? ?BUG_ON(dentry->d_inode);

Yes. Incorporated.

Quoted 22 lines

fs/ocfs2/dcache.c static int ocfs2_dentry_revalidate(struct dentry *dentry, · line 54
Quoted 12 lines
+ ? ? dentry->d_fsdata = (void *)gen;
fs/ocfs2/dcache.c +2 -0
+}
+
?static int ocfs2_dentry_revalidate(struct dentry *dentry, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct nameidata *nd)
fs/ocfs2/dcache.c +0 -1
@@ -51,11 +61,22 @@ static int ocfs2_dentry_revalidate(struct dentry *dentry,
 ? ? ? mlog_entry("(0x%p, '%.*s')\n", dentry,
 ? ? ? ? ? ? ? ? ?dentry->d_name.len, dentry->d_name.name);
 
- ? ? /* Never trust a negative dentry - force a new lookup. */
Btw, it occurs to me that we could also optimize away local mounts in a seperate patch by always trusting the dentry passed in that case.
fs/ocfs2/dcache.c static int ocfs2_dentry_revalidate(struct dentry *dentry, · line 67
Quoted 4 lines
fs/ocfs2/dcache.c +4 -0
+ ? ? /* For a negative dentry -
+ ? ? ? ?check the generation number of the parent and compare with the
+ ? ? ? ?one stored in the inode.
+ ? ? ? ?*/

Quoted 8 lines

The preferred style for long (multi-line) comments is:
? ? ? ?/*
? ? ? ? * line one
? ? ? ? * line two
? ? ? ? */

Incorporated.

Quoted 15 lines

Quoted 4 lines

fs/ocfs2/dcache.c +0 -3

 ? ? ? if (inode == NULL) {
- ? ? ? ? ? ? mlog(0, "negative dentry: %.*s\n", dentry->d_name.len,
- ? ? ? ? ? ? ? ? ?dentry->d_name.name);
- ? ? ? ? ? ? goto bail;

fs/ocfs2/dcache.c static int ocfs2_dentry_revalidate(struct dentry *dentry, · line 78

Quoted 10 lines

fs/ocfs2/dcache.c +10 -0

+ ? ? ? ? ? ? unsigned long gen = (unsigned long) dentry->d_fsdata;
+ ? ? ? ? ? ? unsigned long pgen =
+ ? ? ? ? ? ? ? ? ? ? OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
+ ? ? ? ? ? ? mlog(0, "negative dentry: %.*s parent gen: %lu "
+ ? ? ? ? ? ? ? ? ? ? "dentry gen: %lu\n",
+ ? ? ? ? ? ? ? ? ? ? dentry->d_name.len, dentry->d_name.name, pgen, gen);
+ ? ? ? ? ? ? if (gen != pgen) {
+ ? ? ? ? ? ? ? ? ? ? dentry->d_fsdata = (void *) pgen;
+ ? ? ? ? ? ? ? ? ? ? goto bail;
+ ? ? ? ? ? ? }

Quoted 4 lines

What are you setting the dentry generation here? We should only be doing that when under cluster lock when we are guaranteed that the dentry is negative.

Hm, I was thinking in terms to avoid the validation on the next lookup. However, I did not consider the failure path of the revalidation/lookup/locking operations. Will remove this.

Quoted 30 lines

fs/ocfs2/dcache.c +2 -0

+ ? ? ? ? ? ? goto valid;
 ? ? ? }
 
 ? ? ? BUG_ON(!osb);

@@ -96,6 +117,7 @@ static int ocfs2_dentry_revalidate(struct dentry *dentry,
 ? ? ? ? ? ? ? goto bail;
 ? ? ? }
 
+valid:
 ? ? ? ret = 1;

?bail:

fs/ocfs2/dcache.c +6 -0

@@ -227,6 +249,12 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
 ? ? ? if (!inode)
 ? ? ? ? ? ? ? return 0;
 
+ ? ? if (!dentry->d_inode && dentry->d_fsdata) {
+ ? ? ? ? ? ? /* Converting a negative dentry to positive
+ ? ? ? ? ? ? ? ?Clear dentry->d_fsdata */
+ ? ? ? ? ? ? dentry->d_fsdata = dl = NULL;
+ ? ? }
+
 ? ? ? if (dl) {
 ? ? ? ? ? ? ? mlog_bug_on_msg(dl->dl_parent_blkno != parent_blkno,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? " \"%.*s\": old parent: %llu, new: %llu\n",

@@ -451,6 +479,7 @@ static void ocfs2_dentry_iput(struct dentry
 *dentry, struct inode *inode)
 ? ? ? ocfs2_dentry_lock_put(OCFS2_SB(dentry->d_sb), dl);

Quoted 5 lines

Quoted 2 lines
?out:
+ ? ? ocfs2_dentry_attach_gen(dentry);
Why the call to ocfs2_dentry_attach_gen() here?

This is called when a positive dentry is converted into negative. However, this would trigger the BUG_ON in ocfs2_dentry_attach_gen(). Will find a better way to do this, perhaps NULL'ing the d_inode after iput.

Quoted 40 lines

? ? ? iput(inode);
?}

fs/ocfs2/dcache.h +1 -0

index f5dd178..b79eff7 100644
@@ -64,5 +64,6 @@ void ocfs2_dentry_move(struct dentry *dentry, struct
 dentry *target,
 ? ? ? ? ? ? ? ? ? ? ?struct inode *old_dir, struct inode *new_dir);
 
 ?extern spinlock_t dentry_attach_lock;
+void ocfs2_dentry_attach_gen(struct dentry *dentry);

?#endif /* OCFS2_DCACHE_H */

fs/ocfs2/dlmglue.c +8 -0

index 39eb16a..bf79dcf 100644
@@ -3635,10 +3635,18 @@ static int ocfs2_data_convert_worker(struct
 ocfs2_lock_res *lockres,
 ?{
 ? ? ? struct inode *inode;
 ? ? ? struct address_space *mapping;
+ ? ? struct ocfs2_inode_info *oi;
 
 ? ? ? ? ? ? ? inode = ocfs2_lock_res_inode(lockres);
 ? ? ? mapping = inode->i_mapping;
 
+ ? ? if (S_ISDIR(inode->i_mode)) {
+ ? ? ? ? ? ? oi = OCFS2_I(inode);
+ ? ? ? ? ? ? oi->ip_generation++;
+ ? ? ? ? ? ? mlog(0, "generation: %u\n", oi->ip_generation);
+ ? ? ? ? ? ? goto out;
+ ? ? }
+
 ? ? ? if (!S_ISREG(inode->i_mode))
 ? ? ? ? ? ? ? goto out;

Quoted 13 lines

Quoted 9 lines
fs/ocfs2/inode.h +0 -0
index 9f5f5fc..34bad2c 100644
@@ -70,6 +70,7 @@ struct ocfs2_inode_info
? ? ? /* Only valid if the inode is the dir. */
? ? ? u32 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ip_last_used_slot;
? ? ? u64 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ip_last_used_group;
+ ? ? u32 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ip_generation; /*for -ve dentry */
Comment doesn't make sense ?:) ? Also, can you pick something we're less likely to confuse with i_generation? Maybe ip_dir_lock_gen?

Ok.

Quoted 2 lines

Also, it should be initialized to a nonzero value in when we're initializing the directory inode under cluster lock in ocfs2_read_locked_inode().

Yes, you're right. Both may start from zero, making it avoid the revalidation when it shouldn't.

Thanks,

Signature · 1 line

Goldwyn