[Ocfs2-devel] [PATCH] Track negative dentries v2

[Ocfs2-devel] [PATCH] Track negative dentries v2

[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 <rgoldwyn@suse.de>
---
diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
index b4957c7..6db61a3 100644
--- a/fs/ocfs2/dcache.c
+++ b/fs/ocfs2/dcache.c
@@ -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);
 }

diff --git a/fs/ocfs2/dcache.h b/fs/ocfs2/dcache.h
index f5dd178..b79eff7 100644
--- a/fs/ocfs2/dcache.h
+++ b/fs/ocfs2/dcache.h
@@ -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 */
diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index 39eb16a..bf79dcf 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -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;

diff --git a/fs/ocfs2/inode.h b/fs/ocfs2/inode.h
index 9f5f5fc..34bad2c 100644
--- a/fs/ocfs2/inode.h
+++ b/fs/ocfs2/inode.h
@@ -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;
 };
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index f171b51..e89bce7 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -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 --

-- 
Goldwyn

[Ocfs2-devel] [PATCH] Track negative dentries v2

[Mark Fasheh]

Hi Goldwyn,

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

On Wed, Jun 23, 2010 at 08:45:10PM -0500, Goldwyn Rodrigues wrote:
> 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 <rgoldwyn@suse.de>
> ---
> diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
> index b4957c7..6db61a3 100644
> --- a/fs/ocfs2/dcache.c
> +++ b/fs/ocfs2/dcache.c
> @@ -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();

You should turn those two lines into:
	BUG_ON(dentry->d_inode);


> +	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. */

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.


> +	/* 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
	 */


>  	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;
> +		}

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.


> +		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);

Why the call to ocfs2_dentry_attach_gen() here?


>  	iput(inode);
>  }
> 
> diff --git a/fs/ocfs2/dcache.h b/fs/ocfs2/dcache.h
> index f5dd178..b79eff7 100644
> --- a/fs/ocfs2/dcache.h
> +++ b/fs/ocfs2/dcache.h
> @@ -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 */
> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
> index 39eb16a..bf79dcf 100644
> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -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;
> 
> diff --git a/fs/ocfs2/inode.h b/fs/ocfs2/inode.h
> index 9f5f5fc..34bad2c 100644
> --- a/fs/ocfs2/inode.h
> +++ b/fs/ocfs2/inode.h
> @@ -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

[Ocfs2-devel] [PATCH] Track negative dentries v2

[Goldwyn Rodrigues]

Thanks Mark for reviewing.

On Thu, Jun 24, 2010 at 6:03 PM, Mark Fasheh <mfasheh@suse.com> wrote:
> Hi Goldwyn,
>
> ? ? ? ?The patch looks pretty good so far - thanks for taking on this task!
> My comments are inlined.
>
> On Wed, Jun 23, 2010 at 08:45:10PM -0500, Goldwyn Rodrigues wrote:
>> 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 <rgoldwyn@suse.de>
>> ---
>> diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
>> index b4957c7..6db61a3 100644
>> --- a/fs/ocfs2/dcache.c
>> +++ b/fs/ocfs2/dcache.c
>> @@ -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();
>
> You should turn those two lines into:
> ? ? ? ?BUG_ON(dentry->d_inode);
>

Yes. Incorporated.

>
>> + ? ? 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. */
>
> 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.
>
>
>> + ? ? /* 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
> ? ? ? ? */
>
>

Incorporated.

>> ? ? ? 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;
>> + ? ? ? ? ? ? }
>
> 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.


>
>> + ? ? ? ? ? ? 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);
>
> 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.

>
>> ? ? ? iput(inode);
>> ?}
>>
>> diff --git a/fs/ocfs2/dcache.h b/fs/ocfs2/dcache.h
>> index f5dd178..b79eff7 100644
>> --- a/fs/ocfs2/dcache.h
>> +++ b/fs/ocfs2/dcache.h
>> @@ -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 */
>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>> index 39eb16a..bf79dcf 100644
>> --- a/fs/ocfs2/dlmglue.c
>> +++ b/fs/ocfs2/dlmglue.c
>> @@ -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;
>>
>> diff --git a/fs/ocfs2/inode.h b/fs/ocfs2/inode.h
>> index 9f5f5fc..34bad2c 100644
>> --- a/fs/ocfs2/inode.h
>> +++ b/fs/ocfs2/inode.h
>> @@ -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.

> 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,


-- 
Goldwyn