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