[Ocfs2-devel] [PATCH] Track negative dentries

[Ocfs2-devel] [PATCH] Track negative dentries

[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..f29095b 100644
--- a/fs/ocfs2/dcache.c
+++ b/fs/ocfs2/dcache.c
@@ -40,6 +40,16 @@
 #include "inode.h"
 #include "super.h"

+void ocfs2_dentry_attach_gen(struct dentry *dentry)
+{
+	int *gen = (int *)kmalloc(sizeof(int), GFP_KERNEL);
+	*gen = OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
+	/* Generation numbers are 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,10 +61,21 @@ 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);
+		int *gen = (int *)dentry->d_fsdata;
+		int parent_gen =
+			OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
+		mlog(0, "negative dentry: %.*s parent gen: %u dentry gen: %u\n",
+				dentry->d_name.len, dentry->d_name.name,
+				parent_gen, *gen);
+		if (*gen == parent_gen)
+			ret = 1;
+		else
+			*gen = parent_gen;
 		goto bail;
 	}

@@ -227,6 +248,13 @@ 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 */
+		kfree(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,8 @@ static void ocfs2_dentry_iput(struct dentry
*dentry, struct inode *inode)
 	ocfs2_dentry_lock_put(OCFS2_SB(dentry->d_sb), dl);

 out:
+	/* Attach generation number to dentry */
+	ocfs2_dentry_attach_gen(dentry);
 	iput(inode);
 }

@@ -500,7 +530,15 @@ out_move:
 	d_move(dentry, target);
 }

+static void ocfs2_dentry_release(struct dentry *dentry)
+{
+	/* Free the generation number stored in negative dentry */
+	if (!dentry->d_inode && dentry->d_fsdata)
+		kfree(dentry->d_fsdata);
+}
+
 const struct dentry_operations ocfs2_dentry_ops = {
 	.d_revalidate		= ocfs2_dentry_revalidate,
 	.d_iput			= ocfs2_dentry_iput,
+	.d_release		= ocfs2_dentry_release,
 };
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..d5fb79b 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -2565,7 +2565,6 @@ void ocfs2_inode_unlock(struct inode *inode,
 	if (!ocfs2_is_hard_readonly(OCFS2_SB(inode->i_sb)) &&
 	    !ocfs2_mount_local(osb))
 		ocfs2_cluster_unlock(OCFS2_SB(inode->i_sb), lockres, level);
-
 	mlog_exit_void();
 }

@@ -3635,10 +3634,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..529729c 100644
--- a/fs/ocfs2/inode.h
+++ b/fs/ocfs2/inode.h
@@ -70,6 +70,8 @@ struct ocfs2_inode_info
 	/* Only valid if the inode is the dir. */
 	u32				ip_last_used_slot;
 	u64				ip_last_used_group;
+	/* Generation number for negative inodes */
+	u32				ip_generation;

 	struct ocfs2_alloc_reservation	ip_la_data_resv;
 };
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index f171b51..c06753a 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -172,6 +172,8 @@ bail_add:
 			goto bail_unlock;
 		}
 	}
+	else /* Attach generation number for negative dentry */
+		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

[Sunil Mushran]

Thanks for taking on this task.

One overall comment about comments. Instead of sprinkling
the same line everywhere, add the full description in one place.
Maybe atop ocfs2_attach_dentry_gen(). Describe it fully. And then
let the code speak for itself.

Also, do remember to run the patch thru scripts/checkpatch.pl.

On 06/18/2010 08:02 AM, 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..f29095b 100644
> --- a/fs/ocfs2/dcache.c
> +++ b/fs/ocfs2/dcache.c
> @@ -40,6 +40,16 @@
>   #include "inode.h"
>   #include "super.h"
>
> +void ocfs2_dentry_attach_gen(struct dentry *dentry)
> +{
> +	int *gen = (int *)kmalloc(sizeof(int), GFP_KERNEL);
> +	*gen = OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
> +	/* Generation numbers are specifically for negative dentries */
> +	if (dentry->d_inode)
> +		BUG();
> +	dentry->d_fsdata = (void *)gen;
> +}
> +
>    

kmalloc()s can fail. If this is just an int, why not just store it
directly in d_fsdata. Add appropriate casts.

Also, are you sure about the locking when reading the parent's
generation.

>   static int ocfs2_dentry_revalidate(struct dentry *dentry,
>   				   struct nameidata *nd)
> @@ -51,10 +61,21 @@ 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);
> +		int *gen = (int *)dentry->d_fsdata;
> +		int parent_gen =
> +			OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
> +		mlog(0, "negative dentry: %.*s parent gen: %u dentry gen: %u\n",
> +				dentry->d_name.len, dentry->d_name.name,
> +				parent_gen, *gen);
> +		if (*gen == parent_gen)
> +			ret = 1;
> +		else
> +			*gen = parent_gen;
>   		goto bail;
>   	}
>    

Code is hard to read. See one possible rewrite below. This rewrite
requires adding a "revalidated" label above ret = 1.

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




> @@ -227,6 +248,13 @@ 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 */
> +		kfree(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,8 @@ static void ocfs2_dentry_iput(struct dentry
> *dentry, struct inode *inode)
>   	ocfs2_dentry_lock_put(OCFS2_SB(dentry->d_sb), dl);
>
>   out:
> +	/* Attach generation number to dentry */
> +	ocfs2_dentry_attach_gen(dentry);
>   	iput(inode);
>   }
>    

Remove comment. Code is clear.

> @@ -500,7 +530,15 @@ out_move:
>   	d_move(dentry, target);
>   }
>
> +static void ocfs2_dentry_release(struct dentry *dentry)
> +{
> +	/* Free the generation number stored in negative dentry */
> +	if (!dentry->d_inode&&  dentry->d_fsdata)
> +		kfree(dentry->d_fsdata);
> +}
>    

Shouldn't you be setting d_fsdata to NULL.

> +
>   const struct dentry_operations ocfs2_dentry_ops = {
>   	.d_revalidate		= ocfs2_dentry_revalidate,
>   	.d_iput			= ocfs2_dentry_iput,
> +	.d_release		= ocfs2_dentry_release,
>   };
> 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..d5fb79b 100644
> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -2565,7 +2565,6 @@ void ocfs2_inode_unlock(struct inode *inode,
>   	if (!ocfs2_is_hard_readonly(OCFS2_SB(inode->i_sb))&&
>   	!ocfs2_mount_local(osb))
>   		ocfs2_cluster_unlock(OCFS2_SB(inode->i_sb), lockres, level);
> -
>   	mlog_exit_void();
>   }
>    

??

> @@ -3635,10 +3634,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..529729c 100644
> --- a/fs/ocfs2/inode.h
> +++ b/fs/ocfs2/inode.h
> @@ -70,6 +70,8 @@ struct ocfs2_inode_info
>   	/* Only valid if the inode is the dir. */
>   	u32				ip_last_used_slot;
>   	u64				ip_last_used_group;
> +	/* Generation number for negative inodes */
> +	u32				ip_generation;
>
>   	struct ocfs2_alloc_reservation	ip_la_data_resv;
>   };
>    

Move the comment to the right. The comment above holds valid
for this field too.

> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index f171b51..c06753a 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -172,6 +172,8 @@ bail_add:
>   			goto bail_unlock;
>   		}
>   	}
> +	else /* Attach generation number for negative dentry */
> +		ocfs2_dentry_attach_gen(dentry);
>    

Remove comment. Code is clear. Also, else needs to be in the same
line as }.

>   bail_unlock:
>   	/* Don't drop the cluster lock until *after* the d_add --
>
>    

[Ocfs2-devel] [PATCH] Track negative dentries

[Wengang Wang]

Hi Goldwyn,

Has you ever test the hit race?
Actually I also wrote the codes locally monthes ago. When I was testing it,
I found the dentry are different memory objects. For example, fileA is not
exist, we issue a command of 'ls -l /path/to/fileA', At the first run, set
parent ino to dentry. At the second run, the parent ino is not there. By
printing the address, I found the two dentries are different ones though
they are both for "fileA".
So I wonder if you tested it.

regards,
wengang.
On 10-06-18 10:02, 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..f29095b 100644
> --- a/fs/ocfs2/dcache.c
> +++ b/fs/ocfs2/dcache.c
> @@ -40,6 +40,16 @@
>  #include "inode.h"
>  #include "super.h"
> 
> +void ocfs2_dentry_attach_gen(struct dentry *dentry)
> +{
> +	int *gen = (int *)kmalloc(sizeof(int), GFP_KERNEL);
> +	*gen = OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
> +	/* Generation numbers are 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,10 +61,21 @@ 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);
> +		int *gen = (int *)dentry->d_fsdata;
> +		int parent_gen =
> +			OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
> +		mlog(0, "negative dentry: %.*s parent gen: %u dentry gen: %u\n",
> +				dentry->d_name.len, dentry->d_name.name,
> +				parent_gen, *gen);
> +		if (*gen == parent_gen)
> +			ret = 1;
> +		else
> +			*gen = parent_gen;
>  		goto bail;
>  	}
> 
> @@ -227,6 +248,13 @@ 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 */
> +		kfree(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,8 @@ static void ocfs2_dentry_iput(struct dentry
> *dentry, struct inode *inode)
>  	ocfs2_dentry_lock_put(OCFS2_SB(dentry->d_sb), dl);
> 
>  out:
> +	/* Attach generation number to dentry */
> +	ocfs2_dentry_attach_gen(dentry);
>  	iput(inode);
>  }
> 
> @@ -500,7 +530,15 @@ out_move:
>  	d_move(dentry, target);
>  }
> 
> +static void ocfs2_dentry_release(struct dentry *dentry)
> +{
> +	/* Free the generation number stored in negative dentry */
> +	if (!dentry->d_inode && dentry->d_fsdata)
> +		kfree(dentry->d_fsdata);
> +}
> +
>  const struct dentry_operations ocfs2_dentry_ops = {
>  	.d_revalidate		= ocfs2_dentry_revalidate,
>  	.d_iput			= ocfs2_dentry_iput,
> +	.d_release		= ocfs2_dentry_release,
>  };
> 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..d5fb79b 100644
> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -2565,7 +2565,6 @@ void ocfs2_inode_unlock(struct inode *inode,
>  	if (!ocfs2_is_hard_readonly(OCFS2_SB(inode->i_sb)) &&
>  	    !ocfs2_mount_local(osb))
>  		ocfs2_cluster_unlock(OCFS2_SB(inode->i_sb), lockres, level);
> -
>  	mlog_exit_void();
>  }
> 
> @@ -3635,10 +3634,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..529729c 100644
> --- a/fs/ocfs2/inode.h
> +++ b/fs/ocfs2/inode.h
> @@ -70,6 +70,8 @@ struct ocfs2_inode_info
>  	/* Only valid if the inode is the dir. */
>  	u32				ip_last_used_slot;
>  	u64				ip_last_used_group;
> +	/* Generation number for negative inodes */
> +	u32				ip_generation;
> 
>  	struct ocfs2_alloc_reservation	ip_la_data_resv;
>  };
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index f171b51..c06753a 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -172,6 +172,8 @@ bail_add:
>  			goto bail_unlock;
>  		}
>  	}
> +	else /* Attach generation number for negative dentry */
> +		ocfs2_dentry_attach_gen(dentry);
> 
>  bail_unlock:
>  	/* Don't drop the cluster lock until *after* the d_add --
> 
> -- 
> Goldwyn
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH] Track negative dentries

[Sunil Mushran]

On 06/20/2010 09:27 PM, Wengang Wang wrote:
> Hi Goldwyn,
>
> Has you ever test the hit race?
> Actually I also wrote the codes locally monthes ago. When I was testing it,
> I found the dentry are different memory objects. For example, fileA is not
> exist, we issue a command of 'ls -l /path/to/fileA', At the first run, set
> parent ino to dentry. At the second run, the parent ino is not there. By
> printing the address, I found the two dentries are different ones though
> they are both for "fileA".
> So I wonder if you tested it.
>
>    

I am trying to understand your issue.

Do you mean you encountered a case in which there were two
dentries for the same directory entry (/path/to/fileA) but one had
a correct parent inode and the other had no parent inode.

Note that a dentry points to a parent dentry that points to the inode.
So are you claiming that the two dentries pointed to two different parent
dentries one of which had the correct inode the other null inode?

Sunil

[Ocfs2-devel] [PATCH] Track negative dentries

[Wengang Wang]

On 10-06-21 10:08, Sunil Mushran wrote:
> On 06/20/2010 09:27 PM, Wengang Wang wrote:
> >Hi Goldwyn,
> >
> >Has you ever test the hit race?
> >Actually I also wrote the codes locally monthes ago. When I was testing it,
> >I found the dentry are different memory objects. For example, fileA is not
> >exist, we issue a command of 'ls -l /path/to/fileA', At the first run, set
> >parent ino to dentry. At the second run, the parent ino is not there. By
> >printing the address, I found the two dentries are different ones though
> >they are both for "fileA".
> >So I wonder if you tested it.
> >
> 
> I am trying to understand your issue.
> 
> Do you mean you encountered a case in which there were two
> dentries for the same directory entry (/path/to/fileA) but one had
> a correct parent inode and the other had no parent inode.
> 
> Note that a dentry points to a parent dentry that points to the inode.
> So are you claiming that the two dentries pointed to two different parent
> dentries one of which had the correct inode the other null inode?

Actually I meant two dentries in the two run of 'ls -l'.
At the first run, a dentry, dentry A, is created. Because fileA doesn't exist,
dentry->d_inode is NULL. Then a do_revalidate() is run,
ocfs2_dentry_revalidate() returns "not valid" since d_inode is NULL.
Thus the dentry A is unhashed from cache by d_invalidate().
At the second run of 'ls -l', since dentry A is unhashed, there is no dentry for
fileA exist in dentry hash, a new dentry, dentry B, is created. The new dentry B
don't have any info about parent ino.

I found that when testing my patch for the "track negative dentries".
Maybe I misunderstand something?

regards,
wengang.

[Ocfs2-devel] [PATCH] Track negative dentries

[Joel Becker]

On Tue, Jun 22, 2010 at 11:23:33AM +0800, Wengang Wang wrote:
> Actually I meant two dentries in the two run of 'ls -l'.
> At the first run, a dentry, dentry A, is created. Because fileA doesn't exist,
> dentry->d_inode is NULL. Then a do_revalidate() is run,
> ocfs2_dentry_revalidate() returns "not valid" since d_inode is NULL.
> Thus the dentry A is unhashed from cache by d_invalidate().
> At the second run of 'ls -l', since dentry A is unhashed, there is no dentry for
> fileA exist in dentry hash, a new dentry, dentry B, is created. The new dentry B
> don't have any info about parent ino.
> 
> I found that when testing my patch for the "track negative dentries".
> Maybe I misunderstand something?

	When that dentry gets linked into the tree, it will point to its
d_parent.  So the parent inode is dentryB->d_parent->d_inode.

Joel

-- 

"Too much walking shoes worn thin.
 Too much trippin' and my soul's worn thin.
 Time to catch a ride it leaves today
 Her name is what it means.
 Too much walking shoes worn thin."

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

[Ocfs2-devel] [PATCH] Track negative dentries

[Wengang Wang]

On 10-06-21 20:36, Joel Becker wrote:
> On Tue, Jun 22, 2010 at 11:23:33AM +0800, Wengang Wang wrote:
> > Actually I meant two dentries in the two run of 'ls -l'.
> > At the first run, a dentry, dentry A, is created. Because fileA doesn't exist,
> > dentry->d_inode is NULL. Then a do_revalidate() is run,
> > ocfs2_dentry_revalidate() returns "not valid" since d_inode is NULL.
> > Thus the dentry A is unhashed from cache by d_invalidate().
> > At the second run of 'ls -l', since dentry A is unhashed, there is no dentry for
> > fileA exist in dentry hash, a new dentry, dentry B, is created. The new dentry B
> > don't have any info about parent ino.
> > 
> > I found that when testing my patch for the "track negative dentries".
> > Maybe I misunderstand something?
> 
> 	When that dentry gets linked into the tree, it will point to its
> d_parent.  So the parent inode is dentryB->d_parent->d_inode.

Yes Joel, I know that link.
I meant there is no parent inode number stored on on dentryB it's self,
thus no way to compare it with dentryB->d_parent->d_inode.
We set the parent inode number info to dentryA(somewhere in d_fsdata).
But it's lost at the second 'ls -l' because dentryB is got instead of dentryA. 

regards,
wengang.

[Ocfs2-devel] [PATCH] Track negative dentries

[Sunil Mushran]

Are you referring to the dentry lock? We encode the parent inode in  
the lock. But the difference is that a dentry having that lock cannot  
be a negative dentry.

On Jun 21, 2010, at 10:22 PM, Wengang Wang <wen.gang.wang@oracle.com>  
wrote:

> On 10-06-21 20:36, Joel Becker wrote:
>> On Tue, Jun 22, 2010 at 11:23:33AM +0800, Wengang Wang wrote:
>>> Actually I meant two dentries in the two run of 'ls -l'.
>>> At the first run, a dentry, dentry A, is created. Because fileA  
>>> doesn't exist,
>>> dentry->d_inode is NULL. Then a do_revalidate() is run,
>>> ocfs2_dentry_revalidate() returns "not valid" since d_inode is NULL.
>>> Thus the dentry A is unhashed from cache by d_invalidate().
>>> At the second run of 'ls -l', since dentry A is unhashed, there is  
>>> no dentry for
>>> fileA exist in dentry hash, a new dentry, dentry B, is created.  
>>> The new dentry B
>>> don't have any info about parent ino.
>>>
>>> I found that when testing my patch for the "track negative  
>>> dentries".
>>> Maybe I misunderstand something?
>>
>>    When that dentry gets linked into the tree, it will point to its
>> d_parent.  So the parent inode is dentryB->d_parent->d_inode.
>
> Yes Joel, I know that link.
> I meant there is no parent inode number stored on on dentryB it's  
> self,
> thus no way to compare it with dentryB->d_parent->d_inode.
> We set the parent inode number info to dentryA(somewhere in d_fsdata).
> But it's lost at the second 'ls -l' because dentryB is got instead  
> of dentryA.
>
> regards,
> wengang.

[Ocfs2-devel] [PATCH] Track negative dentries

[Wengang Wang]

On 10-06-21 22:38, Sunil Mushran wrote:
> Are you referring to the dentry lock? We encode the parent inode in
> the lock. But the difference is that a dentry having that lock
> cannot be a negative dentry.

Yes, I know currently the dentry->d_fsdata is used for dentry lock.
In my patch, I make it a union of "dentry lock" and the "parent ino".
So that I can set the "parent ino" to a negative dentry.

But as I said, the dentry is unhashed because it's negative in
do_lookup(). So the fsdata, the parent ino, is lost. Thus when a second
such ls comes, we got a new dentry with fsdata being NULL. so we have to
hit disk(ocfs2_lookup).

regards,
wengang.

> 
> On Jun 21, 2010, at 10:22 PM, Wengang Wang
> <wen.gang.wang@oracle.com> wrote:
> 
> >On 10-06-21 20:36, Joel Becker wrote:
> >>On Tue, Jun 22, 2010 at 11:23:33AM +0800, Wengang Wang wrote:
> >>>Actually I meant two dentries in the two run of 'ls -l'.
> >>>At the first run, a dentry, dentry A, is created. Because
> >>>fileA doesn't exist,
> >>>dentry->d_inode is NULL. Then a do_revalidate() is run,
> >>>ocfs2_dentry_revalidate() returns "not valid" since d_inode is NULL.
> >>>Thus the dentry A is unhashed from cache by d_invalidate().
> >>>At the second run of 'ls -l', since dentry A is unhashed,
> >>>there is no dentry for
> >>>fileA exist in dentry hash, a new dentry, dentry B, is
> >>>created. The new dentry B
> >>>don't have any info about parent ino.
> >>>
> >>>I found that when testing my patch for the "track negative
> >>>dentries".
> >>>Maybe I misunderstand something?
> >>
> >>   When that dentry gets linked into the tree, it will point to its
> >>d_parent.  So the parent inode is dentryB->d_parent->d_inode.
> >
> >Yes Joel, I know that link.
> >I meant there is no parent inode number stored on on dentryB it's
> >self,
> >thus no way to compare it with dentryB->d_parent->d_inode.
> >We set the parent inode number info to dentryA(somewhere in d_fsdata).
> >But it's lost at the second 'ls -l' because dentryB is got instead
> >of dentryA.
> >
> >regards,
> >wengang.

[Ocfs2-devel] [PATCH] Track negative dentries

[Mark Fasheh]

On Tue, Jun 22, 2010 at 11:23:33AM +0800, Wengang Wang wrote:
> Actually I meant two dentries in the two run of 'ls -l'.
> At the first run, a dentry, dentry A, is created. Because fileA doesn't exist,
> dentry->d_inode is NULL.

Ok so you do a lookup on a name which doesn't exist, which results in a
negative dentry for fileA.

> Then a do_revalidate() is run, ocfs2_dentry_revalidate() returns "not
> valid" since d_inode is NULL. Thus the dentry A is unhashed from cache by
> d_invalidate().

Part of the patch is to change this behavior right here.

ocfs2_dentry_revalidate() is seeing the NULL inode and forcing the dentry to
be invalidated (thus requiring another lookup).


The patch needs to handle this by comparing sequence numbers with the parent
dentry and ONLY if they're different should it force the dentry to be
invalidated. Otherwise it can allow the dentry to stay hashed (and
negative).
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH] Track negative dentries

[Wengang Wang]

Thanks Mark!

The patch works fine.
I went to a wrong direction.

regards,
wengang.
On 10-06-22 09:53, Mark Fasheh wrote:
> On Tue, Jun 22, 2010 at 11:23:33AM +0800, Wengang Wang wrote:
> > Actually I meant two dentries in the two run of 'ls -l'.
> > At the first run, a dentry, dentry A, is created. Because fileA doesn't exist,
> > dentry->d_inode is NULL.
> 
> Ok so you do a lookup on a name which doesn't exist, which results in a
> negative dentry for fileA.
> 
> > Then a do_revalidate() is run, ocfs2_dentry_revalidate() returns "not
> > valid" since d_inode is NULL. Thus the dentry A is unhashed from cache by
> > d_invalidate().
> 
> Part of the patch is to change this behavior right here.
> 
> ocfs2_dentry_revalidate() is seeing the NULL inode and forcing the dentry to
> be invalidated (thus requiring another lookup).
> 
> 
> The patch needs to handle this by comparing sequence numbers with the parent
> dentry and ONLY if they're different should it force the dentry to be
> invalidated. Otherwise it can allow the dentry to stay hashed (and
> negative).
> 	--Mark
> 
> --
> Mark Fasheh

[Ocfs2-devel] [PATCH] Track negative dentries

[Goldwyn Rodrigues]

Hi,

Sorry for the delay in responding. I was swamped with other high priority tasks.

On Fri, Jun 18, 2010 at 11:06 AM, Sunil Mushran
<sunil.mushran@oracle.com> wrote:
> Thanks for taking on this task.
>
> One overall comment about comments. Instead of sprinkling
> the same line everywhere, add the full description in one place.
> Maybe atop ocfs2_attach_dentry_gen(). Describe it fully. And then
> let the code speak for itself.
>
> Also, do remember to run the patch thru scripts/checkpatch.pl.
>

Yes, I understand. Will make sure from next time.


> On 06/18/2010 08:02 AM, 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..f29095b 100644
>> --- a/fs/ocfs2/dcache.c
>> +++ b/fs/ocfs2/dcache.c
>> @@ -40,6 +40,16 @@
>> ?#include "inode.h"
>> ?#include "super.h"
>>
>> +void ocfs2_dentry_attach_gen(struct dentry *dentry)
>> +{
>> + ? ? ? int *gen = (int *)kmalloc(sizeof(int), GFP_KERNEL);
>> + ? ? ? *gen = OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
>> + ? ? ? /* Generation numbers are specifically for negative dentries */
>> + ? ? ? if (dentry->d_inode)
>> + ? ? ? ? ? ? ? BUG();
>> + ? ? ? dentry->d_fsdata = (void *)gen;
>> +}
>> +
>>
>
> kmalloc()s can fail. If this is just an int, why not just store it
> directly in d_fsdata. Add appropriate casts.

This will cause compilation warnings for either x86_64 or i686 systems
depending on the data type we choose. I will do this anyways.

>
> Also, are you sure about the locking when reading the parent's
> generation.
>

No, what lockings do you suspect might be required?


>> ?static int ocfs2_dentry_revalidate(struct dentry *dentry,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct nameidata *nd)
>> @@ -51,10 +61,21 @@ 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);
>> + ? ? ? ? ? ? ? int *gen = (int *)dentry->d_fsdata;
>> + ? ? ? ? ? ? ? int parent_gen =
>> + ? ? ? ? ? ? ? ? ? ? ? OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
>> + ? ? ? ? ? ? ? mlog(0, "negative dentry: %.*s parent gen: %u dentry gen:
>> %u\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dentry->d_name.len, dentry->d_name.name,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? parent_gen, *gen);
>> + ? ? ? ? ? ? ? if (*gen == parent_gen)
>> + ? ? ? ? ? ? ? ? ? ? ? ret = 1;
>> + ? ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? ? *gen = parent_gen;
>> ? ? ? ? ? ? ? ?goto bail;
>> ? ? ? ?}
>>
>
> Code is hard to read. See one possible rewrite below. This rewrite
> requires adding a "revalidated" label above ret = 1.
>
> + ? ? ? ? ? ? ? int *gen = (int *)dentry->d_fsdata;
> + ? ? ? ? ? ? ? int pgen =
> OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
> +
> + ? ? ? ? ? ? ? mlog(0, "negative dentry: %.*s parent gen: %u dentry gen:
> %u\n",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dentry->d_name.len, dentry->d_name.name,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? parent_gen, *gen);
> +
> + ? ? ? ? ? ? ? if (*gen != pgen) {
> + ? ? ? ? ? ? ? ? ? ? ? *gen = pgen;
> + ? ? ? ? ? ? ? ? ? ? ? goto bail;
> + ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? goto revalidated;
> ? ? ? ?}
>

Ok, understood.

<snipped>

-- 
Goldwyn

[Ocfs2-devel] [PATCH] Track negative dentries

[Goldwyn Rodrigues]

On Sun, Jun 20, 2010 at 11:27 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
> Hi Goldwyn,
>
> Has you ever test the hit race?

Yes, I tested it before posting.

> Actually I also wrote the codes locally monthes ago. When I was testing it,
> I found the dentry are different memory objects. For example, fileA is not
> exist, we issue a command of 'ls -l /path/to/fileA', At the first run, set
> parent ino to dentry. At the second run, the parent ino is not there. By
> printing the address, I found the two dentries are different ones though
> they are both for "fileA".

What you might be hitting is reuse of negative dentries. IOW,
conversion of negative dentries into positve dentries. I have handled
that. I did not printk pointers but did print generation numbers and
they looked fine. However, I did not encounter a case with a negative
dentry with a misplaced or a null d_parent.

-- 
Goldwyn

[Ocfs2-devel] [PATCH] Track negative dentries

[Sunil Mushran]

On 06/23/2010 11:25 AM, Goldwyn Rodrigues wrote:
>>> +void ocfs2_dentry_attach_gen(struct dentry *dentry)
>>> +{
>>> +       int *gen = (int *)kmalloc(sizeof(int), GFP_KERNEL);
>>> +       *gen = OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
>>> +       /* Generation numbers are specifically for negative dentries */
>>> +       if (dentry->d_inode)
>>> +               BUG();
>>> +       dentry->d_fsdata = (void *)gen;
>>> +}
>>> +
>>>
>>>        
>> kmalloc()s can fail. If this is just an int, why not just store it
>> directly in d_fsdata. Add appropriate casts.
>>      
> This will cause compilation warnings for either x86_64 or i686 systems
> depending on the data type we choose. I will do this anyways.
>    

See if you can silence the warning. My main problem here is that
we don't handle the error case. So if kmalloc() is seen as the best
solution, go ahead use it, but do handle the ENOMEM case.

>> Also, are you sure about the locking when reading the parent's
>> generation.
>>      
> No, what lockings do you suspect might be required?
>    

Can we race with the data convert worker. It is a increment only.
So should not matter. But I'll need to think about this more.

[Ocfs2-devel] [PATCH] Track negative dentries

[Goldwyn Rodrigues]

On Wed, Jun 23, 2010 at 2:05 PM, Sunil Mushran <sunil.mushran@oracle.com> wrote:
> On 06/23/2010 11:25 AM, Goldwyn Rodrigues wrote:
>>>>
>>>> +void ocfs2_dentry_attach_gen(struct dentry *dentry)
>>>> +{
>>>> + ? ? ? int *gen = (int *)kmalloc(sizeof(int), GFP_KERNEL);
>>>> + ? ? ? *gen = OCFS2_I(dentry->d_parent->d_inode)->ip_generation;
>>>> + ? ? ? /* Generation numbers are specifically for negative dentries */
>>>> + ? ? ? if (dentry->d_inode)
>>>> + ? ? ? ? ? ? ? BUG();
>>>> + ? ? ? dentry->d_fsdata = (void *)gen;
>>>> +}
>>>> +
>>>>
>>>>
>>>
>>> kmalloc()s can fail. If this is just an int, why not just store it
>>> directly in d_fsdata. Add appropriate casts.
>>>
>>
>> This will cause compilation warnings for either x86_64 or i686 systems
>> depending on the data type we choose. I will do this anyways.
>>
>
> See if you can silence the warning. My main problem here is that
> we don't handle the error case. So if kmalloc() is seen as the best
> solution, go ahead use it, but do handle the ENOMEM case.
>

Oh yes, changing the (function) variables to long should do it, since
long is the same size as pointer.
I will make the change and test it. Thanks.

-- 
Goldwyn

[Ocfs2-devel] [PATCH] Track negative dentries

[Joel Becker]

On Wed, Jun 23, 2010 at 03:27:50PM -0500, Goldwyn Rodrigues wrote:
> On Wed, Jun 23, 2010 at 2:05 PM, Sunil Mushran <sunil.mushran@oracle.com> wrote:
> >>> kmalloc()s can fail. If this is just an int, why not just store it
> >>> directly in d_fsdata. Add appropriate casts.
> >>>
> >>
> >> This will cause compilation warnings for either x86_64 or i686 systems
> >> depending on the data type we choose. I will do this anyways.
> >>
> >
> > See if you can silence the warning. My main problem here is that
> > we don't handle the error case. So if kmalloc() is seen as the best
> > solution, go ahead use it, but do handle the ENOMEM case.
> >
> 
> Oh yes, changing the (function) variables to long should do it, since
> long is the same size as pointer.
> I will make the change and test it. Thanks.

	You can use an int too.  Casting works.  That said, using an
unsigned long does not hurt.  Make sure you use unsigned long, as
pointers are unsigned longs.  You'll still need to cast to/from.

Joel

-- 

"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

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

[Ocfs2-devel] [PATCH] Track negative dentries

[Goldwyn Rodrigues]

On Wed, Jun 23, 2010 at 4:01 PM, Joel Becker <Joel.Becker@oracle.com> wrote:
> On Wed, Jun 23, 2010 at 03:27:50PM -0500, Goldwyn Rodrigues wrote:
>> On Wed, Jun 23, 2010 at 2:05 PM, Sunil Mushran <sunil.mushran@oracle.com> wrote:
>> >>> kmalloc()s can fail. If this is just an int, why not just store it
>> >>> directly in d_fsdata. Add appropriate casts.
>> >>>
>> >>
>> >> This will cause compilation warnings for either x86_64 or i686 systems
>> >> depending on the data type we choose. I will do this anyways.
>> >>
>> >
>> > See if you can silence the warning. My main problem here is that
>> > we don't handle the error case. So if kmalloc() is seen as the best
>> > solution, go ahead use it, but do handle the ENOMEM case.
>> >
>>
>> Oh yes, changing the (function) variables to long should do it, since
>> long is the same size as pointer.
>> I will make the change and test it. Thanks.
>
> ? ? ? ?You can use an int too. ?Casting works. ?That said, using an

Without warnings? this is on an x86_64 compile -

unsigned int gen = (unsigned int)dentry->d_fsdata;

/home/goldwyn/work/linux-2.6/fs/ocfs2/dcache.c: In function
?ocfs2_dentry_revalidate?:
/home/goldwyn/work/linux-2.6/fs/ocfs2/dcache.c:69: warning: cast from
pointer to integer of different size

Changing it to a different cast type might help, but it would create
more confusion.

> unsigned long does not hurt. ?Make sure you use unsigned long, as
> pointers are unsigned longs. ?You'll still need to cast to/from.

Yes, I am using unsigned long.

-- 
Goldwyn