* [Ocfs2-devel] [PATCH] Track negative dentries v2 @ 2010-06-24 1:45 Goldwyn Rodrigues 2010-06-24 23:03 ` Mark Fasheh 0 siblings, 1 reply; 3+ messages in thread From: Goldwyn Rodrigues @ 2010-06-24 1:45 UTC (permalink / raw) To: ocfs2-devel 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [Ocfs2-devel] [PATCH] Track negative dentries v2 2010-06-24 1:45 [Ocfs2-devel] [PATCH] Track negative dentries v2 Goldwyn Rodrigues @ 2010-06-24 23:03 ` Mark Fasheh 2010-06-25 16:13 ` Goldwyn Rodrigues 0 siblings, 1 reply; 3+ messages in thread From: Mark Fasheh @ 2010-06-24 23:03 UTC (permalink / raw) To: ocfs2-devel 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [Ocfs2-devel] [PATCH] Track negative dentries v2 2010-06-24 23:03 ` Mark Fasheh @ 2010-06-25 16:13 ` Goldwyn Rodrigues 0 siblings, 0 replies; 3+ messages in thread From: Goldwyn Rodrigues @ 2010-06-25 16:13 UTC (permalink / raw) To: ocfs2-devel 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-06-25 16:13 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-06-24 1:45 [Ocfs2-devel] [PATCH] Track negative dentries v2 Goldwyn Rodrigues 2010-06-24 23:03 ` Mark Fasheh 2010-06-25 16:13 ` Goldwyn Rodrigues
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.