* [PATCH 0/3] Ceph fixes for 3.2 final
@ 2011-12-29 2:05 Sage Weil
2011-12-29 2:05 ` [PATCH 1/3] ceph: take inode lock when finding an inode alias Sage Weil
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Sage Weil @ 2011-12-29 2:05 UTC (permalink / raw)
To: ceph-devel; +Cc: Sage Weil
This is a final set of fixes for 3.2. The main issue is with the
d_find_any_alias() code, which used to rely on the caller holding i_lock
(this is no longer the case). The last patch adds a mount option to
enable/disable use of the dcache for negative lookups and readdir on
directories the client has completely cached; there are some bugs
lurking in that code so it is disabled for now.
I'll be sending this to Linus tomorrow, unless anyone see any problems!
sage
Alex Elder (2):
ceph: take inode lock when finding an inode alias
ceph: take a reference to the dentry in d_find_any_alias()
Sage Weil (1):
ceph: enable/disable dentry complete flags via mount option
fs/ceph/dir.c | 50 +++++++++++++++++++++++++++++++++++++++++---------
fs/ceph/super.c | 14 ++++++++++++++
fs/ceph/super.h | 1 +
3 files changed, 56 insertions(+), 9 deletions(-)
--
1.7.2.5
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] ceph: take inode lock when finding an inode alias 2011-12-29 2:05 [PATCH 0/3] Ceph fixes for 3.2 final Sage Weil @ 2011-12-29 2:05 ` Sage Weil 2011-12-29 9:10 ` Christoph Hellwig 2011-12-29 2:05 ` [PATCH 2/3] ceph: take a reference to the dentry in d_find_any_alias() Sage Weil 2011-12-29 2:05 ` [PATCH 3/3] ceph: enable/disable dentry complete flags via mount option Sage Weil 2 siblings, 1 reply; 10+ messages in thread From: Sage Weil @ 2011-12-29 2:05 UTC (permalink / raw) To: ceph-devel; +Cc: Alex Elder From: Alex Elder <elder@dreamhost.com> In the ceph_dir_*_complete() functions, a call to __d_find_any_alias() is used to get a dentry for a inode. Previously this was done under the inode's i_lock, but recently this change converted things to use the ceph inode's i_ceph_lock instead: be655596 ceph: use i_ceph_lock instead of i_lock Finding an inode alias operates (only) on the Linux inode, so we really do need to take the Linux lock for this. Since i_lock is ordered inside i_ceph_lock, we can safely do this for all these ceph cases. For now, just copy the d_find_any_alias() function from "fs/dcache.c" and use that instead. Signed-off-by: Alex Elder <elder@dreamhost.com> --- fs/ceph/dir.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-) diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index 3eeb976..e58b0d1 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1104,9 +1104,21 @@ static struct dentry * __d_find_any_alias(struct inode *inode) return alias; } +/* The following code copied from "fs/dcache.c" */ +static struct dentry * d_find_any_alias(struct inode *inode) +{ + struct dentry *de; + + spin_lock(&inode->i_lock); + de = __d_find_any_alias(inode); + spin_unlock(&inode->i_lock); + return de; +} +/* End of code copied from "fs/dcache.c" */ + void ceph_dir_set_complete(struct inode *inode) { - struct dentry *dentry = __d_find_any_alias(inode); + struct dentry *dentry = d_find_any_alias(inode); if (dentry && ceph_dentry(dentry)) { dout(" marking %p (%p) complete\n", inode, dentry); @@ -1116,7 +1128,7 @@ void ceph_dir_set_complete(struct inode *inode) void ceph_dir_clear_complete(struct inode *inode) { - struct dentry *dentry = __d_find_any_alias(inode); + struct dentry *dentry = d_find_any_alias(inode); if (dentry && ceph_dentry(dentry)) { dout(" marking %p (%p) NOT complete\n", inode, dentry); @@ -1126,7 +1138,7 @@ void ceph_dir_clear_complete(struct inode *inode) bool ceph_dir_test_complete(struct inode *inode) { - struct dentry *dentry = __d_find_any_alias(inode); + struct dentry *dentry = d_find_any_alias(inode); if (dentry && ceph_dentry(dentry)) return test_bit(CEPH_D_COMPLETE, &ceph_dentry(dentry)->flags); -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ceph: take inode lock when finding an inode alias 2011-12-29 2:05 ` [PATCH 1/3] ceph: take inode lock when finding an inode alias Sage Weil @ 2011-12-29 9:10 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2011-12-29 9:10 UTC (permalink / raw) To: Sage Weil; +Cc: ceph-devel, Alex Elder On Wed, Dec 28, 2011 at 06:05:13PM -0800, Sage Weil wrote: > +/* The following code copied from "fs/dcache.c" */ > +static struct dentry * d_find_any_alias(struct inode *inode) > +{ > + struct dentry *de; > + > + spin_lock(&inode->i_lock); > + de = __d_find_any_alias(inode); > + spin_unlock(&inode->i_lock); > + return de; > +} > +/* End of code copied from "fs/dcache.c" */ I would be much happier about just exporting d_find_any_alias. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] ceph: take a reference to the dentry in d_find_any_alias() 2011-12-29 2:05 [PATCH 0/3] Ceph fixes for 3.2 final Sage Weil 2011-12-29 2:05 ` [PATCH 1/3] ceph: take inode lock when finding an inode alias Sage Weil @ 2011-12-29 2:05 ` Sage Weil 2011-12-29 9:11 ` Christoph Hellwig 2011-12-29 2:05 ` [PATCH 3/3] ceph: enable/disable dentry complete flags via mount option Sage Weil 2 siblings, 1 reply; 10+ messages in thread From: Sage Weil @ 2011-12-29 2:05 UTC (permalink / raw) To: ceph-devel; +Cc: Alex Elder From: Alex Elder <elder@dreamhost.com> The ceph code duplicates __d_find_any_alias(), but it currently does not take a reference to the returned dentry as it should. Replace the ceph implementation with an exact copy of what's found in "fs/dcache.c", and update the callers so they drop their reference when they're done with it. Unfortunately this requires the wholesale copy of the functions that implement __dget(). It would be much nicer to just export d_find_any_alias() from "fs/dcache.c" instead. Signed-off-by: Alex Elder <elder@dreamhost.com> --- fs/ceph/dir.c | 31 +++++++++++++++++++++++++------ 1 files changed, 25 insertions(+), 6 deletions(-) diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index e58b0d1..caddb7d 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1091,9 +1091,20 @@ static int ceph_snapdir_d_revalidate(struct dentry *dentry, return 1; } -/* - * Set/clear/test dir complete flag on the dir's dentry. - */ +/* The following code copied from "fs/dcache.c" */ +/* This must be called with d_lock held */ +static inline void __dget_dlock(struct dentry *dentry) +{ + dentry->d_count++; +} + +static inline void __dget(struct dentry *dentry) +{ + spin_lock(&dentry->d_lock); + __dget_dlock(dentry); + spin_unlock(&dentry->d_lock); +} + static struct dentry * __d_find_any_alias(struct inode *inode) { struct dentry *alias; @@ -1101,10 +1112,10 @@ static struct dentry * __d_find_any_alias(struct inode *inode) if (list_empty(&inode->i_dentry)) return NULL; alias = list_first_entry(&inode->i_dentry, struct dentry, d_alias); + __dget(alias); return alias; } -/* The following code copied from "fs/dcache.c" */ static struct dentry * d_find_any_alias(struct inode *inode) { struct dentry *de; @@ -1116,6 +1127,9 @@ static struct dentry * d_find_any_alias(struct inode *inode) } /* End of code copied from "fs/dcache.c" */ +/* + * Set/clear/test dir complete flag on the dir's dentry. + */ void ceph_dir_set_complete(struct inode *inode) { struct dentry *dentry = d_find_any_alias(inode); @@ -1124,6 +1138,7 @@ void ceph_dir_set_complete(struct inode *inode) dout(" marking %p (%p) complete\n", inode, dentry); set_bit(CEPH_D_COMPLETE, &ceph_dentry(dentry)->flags); } + dput(dentry); } void ceph_dir_clear_complete(struct inode *inode) @@ -1134,15 +1149,19 @@ void ceph_dir_clear_complete(struct inode *inode) dout(" marking %p (%p) NOT complete\n", inode, dentry); clear_bit(CEPH_D_COMPLETE, &ceph_dentry(dentry)->flags); } + dput(dentry); } bool ceph_dir_test_complete(struct inode *inode) { struct dentry *dentry = d_find_any_alias(inode); + bool ret = false; if (dentry && ceph_dentry(dentry)) - return test_bit(CEPH_D_COMPLETE, &ceph_dentry(dentry)->flags); - return false; + ret = test_bit(CEPH_D_COMPLETE, &ceph_dentry(dentry)->flags); + dput(dentry); + + return ret; } /* -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ceph: take a reference to the dentry in d_find_any_alias() 2011-12-29 2:05 ` [PATCH 2/3] ceph: take a reference to the dentry in d_find_any_alias() Sage Weil @ 2011-12-29 9:11 ` Christoph Hellwig 2011-12-29 14:34 ` Alex Elder 0 siblings, 1 reply; 10+ messages in thread From: Christoph Hellwig @ 2011-12-29 9:11 UTC (permalink / raw) To: Sage Weil; +Cc: ceph-devel, Alex Elder On Wed, Dec 28, 2011 at 06:05:14PM -0800, Sage Weil wrote: > From: Alex Elder <elder@dreamhost.com> > > The ceph code duplicates __d_find_any_alias(), but it currently > does not take a reference to the returned dentry as it should. > Replace the ceph implementation with an exact copy of what's > found in "fs/dcache.c", and update the callers so they drop > their reference when they're done with it. > > Unfortunately this requires the wholesale copy of the functions > that implement __dget(). It would be much nicer to just export > d_find_any_alias() from "fs/dcache.c" instead. Just exporting it would indeed be much better. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ceph: take a reference to the dentry in d_find_any_alias() 2011-12-29 9:11 ` Christoph Hellwig @ 2011-12-29 14:34 ` Alex Elder 2011-12-29 14:54 ` Christoph Hellwig 0 siblings, 1 reply; 10+ messages in thread From: Alex Elder @ 2011-12-29 14:34 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Sage Weil, ceph-devel On Thu, 2011-12-29 at 04:11 -0500, Christoph Hellwig wrote: > On Wed, Dec 28, 2011 at 06:05:14PM -0800, Sage Weil wrote: > > From: Alex Elder <elder@dreamhost.com> > > > > The ceph code duplicates __d_find_any_alias(), but it currently > > does not take a reference to the returned dentry as it should. > > Replace the ceph implementation with an exact copy of what's > > found in "fs/dcache.c", and update the callers so they drop > > their reference when they're done with it. > > > > Unfortunately this requires the wholesale copy of the functions > > that implement __dget(). It would be much nicer to just export > > d_find_any_alias() from "fs/dcache.c" instead. > > Just exporting it would indeed be much better. Yes, that's the plan. We were originally hoping to get this into 3.2 and I figured it was too late in the cycle to be suggesting a (albeit pretty harmless) change to the dcache code. I will respin the patches. I'll wait an hour or two to see what Sage plans to do with these. -Alex ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ceph: take a reference to the dentry in d_find_any_alias() 2011-12-29 14:34 ` Alex Elder @ 2011-12-29 14:54 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2011-12-29 14:54 UTC (permalink / raw) To: Alex Elder; +Cc: Christoph Hellwig, Sage Weil, ceph-devel On Thu, Dec 29, 2011 at 08:34:38AM -0600, Alex Elder wrote: > Yes, that's the plan. We were originally hoping to get > this into 3.2 and I figured it was too late in the cycle > to be suggesting a (albeit pretty harmless) change to > the dcache code. > > I will respin the patches. I'll wait an hour or two > to see what Sage plans to do with these. I don't think getting the export in should be a problem. Cc Al to make sure, but IMHO this is still easily doable for 3.2. It's a much smaller change than duplicating all that code, too. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] ceph: enable/disable dentry complete flags via mount option 2011-12-29 2:05 [PATCH 0/3] Ceph fixes for 3.2 final Sage Weil 2011-12-29 2:05 ` [PATCH 1/3] ceph: take inode lock when finding an inode alias Sage Weil 2011-12-29 2:05 ` [PATCH 2/3] ceph: take a reference to the dentry in d_find_any_alias() Sage Weil @ 2011-12-29 2:05 ` Sage Weil 2011-12-29 9:11 ` Christoph Hellwig 2 siblings, 1 reply; 10+ messages in thread From: Sage Weil @ 2011-12-29 2:05 UTC (permalink / raw) To: ceph-devel; +Cc: Sage Weil Enable/disable use of the dentry dir 'complete' flag via a mount option. Signed-off-by: Sage Weil <sage@newdream.net> --- fs/ceph/dir.c | 3 ++- fs/ceph/super.c | 14 ++++++++++++++ fs/ceph/super.h | 1 + 3 files changed, 17 insertions(+), 1 deletions(-) diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index caddb7d..35db035 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1134,7 +1134,8 @@ void ceph_dir_set_complete(struct inode *inode) { struct dentry *dentry = d_find_any_alias(inode); - if (dentry && ceph_dentry(dentry)) { + if (dentry && ceph_dentry(dentry) && + ceph_test_mount_opt(ceph_sb_to_client(dentry->d_sb), USEDCACHE)) { dout(" marking %p (%p) complete\n", inode, dentry); set_bit(CEPH_D_COMPLETE, &ceph_dentry(dentry)->flags); } diff --git a/fs/ceph/super.c b/fs/ceph/super.c index b48f15f..3f4e5ad 100644 --- a/fs/ceph/super.c +++ b/fs/ceph/super.c @@ -131,6 +131,8 @@ enum { Opt_rbytes, Opt_norbytes, Opt_noasyncreaddir, + Opt_usedcache, + Opt_nousedcache, Opt_ino32, }; @@ -152,6 +154,8 @@ static match_table_t fsopt_tokens = { {Opt_rbytes, "rbytes"}, {Opt_norbytes, "norbytes"}, {Opt_noasyncreaddir, "noasyncreaddir"}, + {Opt_usedcache, "usedcache"}, + {Opt_nousedcache, "nousedcache"}, {Opt_ino32, "ino32"}, {-1, NULL} }; @@ -231,6 +235,12 @@ static int parse_fsopt_token(char *c, void *private) case Opt_noasyncreaddir: fsopt->flags |= CEPH_MOUNT_OPT_NOASYNCREADDIR; break; + case Opt_usedcache: + fsopt->flags |= CEPH_MOUNT_OPT_USEDCACHE; + break; + case Opt_nousedcache: + fsopt->flags &= ~CEPH_MOUNT_OPT_USEDCACHE; + break; case Opt_ino32: fsopt->flags |= CEPH_MOUNT_OPT_INO32; break; @@ -377,6 +387,10 @@ static int ceph_show_options(struct seq_file *m, struct vfsmount *mnt) seq_puts(m, ",norbytes"); if (fsopt->flags & CEPH_MOUNT_OPT_NOASYNCREADDIR) seq_puts(m, ",noasyncreaddir"); + if (fsopt->flags & CEPH_MOUNT_OPT_USEDCACHE) + seq_puts(m, ",usedcache"); + else + seq_puts(m, ",nousedcache"); if (fsopt->wsize) seq_printf(m, ",wsize=%d", fsopt->wsize); diff --git a/fs/ceph/super.h b/fs/ceph/super.h index edcbf37..9270d9d 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -28,6 +28,7 @@ #define CEPH_MOUNT_OPT_RBYTES (1<<5) /* dir st_bytes = rbytes */ #define CEPH_MOUNT_OPT_NOASYNCREADDIR (1<<7) /* no dcache readdir */ #define CEPH_MOUNT_OPT_INO32 (1<<8) /* 32 bit inos */ +#define CEPH_MOUNT_OPT_USEDCACHE (1<<9) #define CEPH_MOUNT_OPT_DEFAULT (CEPH_MOUNT_OPT_RBYTES) -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] ceph: enable/disable dentry complete flags via mount option 2011-12-29 2:05 ` [PATCH 3/3] ceph: enable/disable dentry complete flags via mount option Sage Weil @ 2011-12-29 9:11 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2011-12-29 9:11 UTC (permalink / raw) To: Sage Weil; +Cc: ceph-devel On Wed, Dec 28, 2011 at 06:05:15PM -0800, Sage Weil wrote: > Enable/disable use of the dentry dir 'complete' flag via a mount option. Please add documentation when a user would specify this option. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] ceph: take inode lock when finding an inode alias
@ 2011-12-22 20:13 Sage Weil
0 siblings, 0 replies; 10+ messages in thread
From: Sage Weil @ 2011-12-22 20:13 UTC (permalink / raw)
To: ceph-devel; +Cc: Alex Elder
From: Alex Elder <elder@dreamhost.com>
In the ceph_dir_*_complete() functions, a call to
__d_find_any_alias() is used to get a dentry for a inode.
Previously this was done under the inode's i_lock, but
recently this change converted things to use the ceph
inode's i_ceph_lock instead:
be655596 ceph: use i_ceph_lock instead of i_lock
Finding an inode alias operates (only) on the Linux inode, so
we really do need to take the Linux lock for this. Since i_lock
is ordered inside i_ceph_lock, we can safely do this for all
these ceph cases.
For now, just copy the d_find_any_alias() function from
"fs/dcache.c" and use that instead.
Signed-off-by: Alex Elder <elder@dreamhost.com>
---
fs/ceph/dir.c | 18 +++++++++++++++---
1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 3eeb976..e58b0d1 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1104,9 +1104,21 @@ static struct dentry * __d_find_any_alias(struct inode *inode)
return alias;
}
+/* The following code copied from "fs/dcache.c" */
+static struct dentry * d_find_any_alias(struct inode *inode)
+{
+ struct dentry *de;
+
+ spin_lock(&inode->i_lock);
+ de = __d_find_any_alias(inode);
+ spin_unlock(&inode->i_lock);
+ return de;
+}
+/* End of code copied from "fs/dcache.c" */
+
void ceph_dir_set_complete(struct inode *inode)
{
- struct dentry *dentry = __d_find_any_alias(inode);
+ struct dentry *dentry = d_find_any_alias(inode);
if (dentry && ceph_dentry(dentry)) {
dout(" marking %p (%p) complete\n", inode, dentry);
@@ -1116,7 +1128,7 @@ void ceph_dir_set_complete(struct inode *inode)
void ceph_dir_clear_complete(struct inode *inode)
{
- struct dentry *dentry = __d_find_any_alias(inode);
+ struct dentry *dentry = d_find_any_alias(inode);
if (dentry && ceph_dentry(dentry)) {
dout(" marking %p (%p) NOT complete\n", inode, dentry);
@@ -1126,7 +1138,7 @@ void ceph_dir_clear_complete(struct inode *inode)
bool ceph_dir_test_complete(struct inode *inode)
{
- struct dentry *dentry = __d_find_any_alias(inode);
+ struct dentry *dentry = d_find_any_alias(inode);
if (dentry && ceph_dentry(dentry))
return test_bit(CEPH_D_COMPLETE, &ceph_dentry(dentry)->flags);
--
1.7.5.4
^ permalink raw reply related [flat|nested] 10+ messages in threadend of thread, other threads:[~2011-12-29 14:54 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-12-29 2:05 [PATCH 0/3] Ceph fixes for 3.2 final Sage Weil 2011-12-29 2:05 ` [PATCH 1/3] ceph: take inode lock when finding an inode alias Sage Weil 2011-12-29 9:10 ` Christoph Hellwig 2011-12-29 2:05 ` [PATCH 2/3] ceph: take a reference to the dentry in d_find_any_alias() Sage Weil 2011-12-29 9:11 ` Christoph Hellwig 2011-12-29 14:34 ` Alex Elder 2011-12-29 14:54 ` Christoph Hellwig 2011-12-29 2:05 ` [PATCH 3/3] ceph: enable/disable dentry complete flags via mount option Sage Weil 2011-12-29 9:11 ` Christoph Hellwig -- strict thread matches above, loose matches on Subject: below -- 2011-12-22 20:13 [PATCH 1/3] ceph: take inode lock when finding an inode alias Sage Weil
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.