* [PATCH 0/2] ceph: snapdir dentry handling fixes
@ 2021-03-15 18:07 Jeff Layton
2021-03-15 18:07 ` [PATCH 1/2] ceph: don't clobber i_snap_caps on non-I_NEW inode Jeff Layton
2021-03-15 18:07 ` [PATCH 2/2] ceph: don't use d_add in ceph_handle_snapdir Jeff Layton
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Layton @ 2021-03-15 18:07 UTC (permalink / raw)
To: ceph-devel, idryomov; +Cc: linux-fsdevel, viro
These patches fix a couple of problems that Al noticed around our
handling of snapdir dentries.
I'm going to feed these into our testing branch soon, along with
the ceph fixes (and helper patch) that Al posted in this series:
https://lore.kernel.org/linux-fsdevel/YExBLBEbJRXDV19F@zeniv-ca.linux.org.uk/
It makes for a bit of a messy testing branch for now, unfortunately,
with the fscache changes in there as well, but hopefully we can
merge some of these fixes soon and get the branch back into a
simpler state.
Jeff Layton (2):
ceph: don't clobber i_snap_caps on non-I_NEW inode
ceph: don't use d_add in ceph_handle_snapdir
fs/ceph/dir.c | 30 ++++++++++++++++++------------
fs/ceph/file.c | 6 ++++--
fs/ceph/inode.c | 7 ++++---
fs/ceph/super.h | 2 +-
4 files changed, 27 insertions(+), 18 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] ceph: don't clobber i_snap_caps on non-I_NEW inode 2021-03-15 18:07 [PATCH 0/2] ceph: snapdir dentry handling fixes Jeff Layton @ 2021-03-15 18:07 ` Jeff Layton 2021-03-19 5:03 ` Xiubo Li 2021-03-15 18:07 ` [PATCH 2/2] ceph: don't use d_add in ceph_handle_snapdir Jeff Layton 1 sibling, 1 reply; 6+ messages in thread From: Jeff Layton @ 2021-03-15 18:07 UTC (permalink / raw) To: ceph-devel, idryomov; +Cc: linux-fsdevel, viro We want the snapdir to mirror the non-snapped directory's attributes for most things, but i_snap_caps represents the caps granted on the snapshot directory by the MDS itself. A misbehaving MDS could issue different caps for the snapdir and we lose them here. Only reset i_snap_caps when the inode is I_NEW. Reported-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/inode.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 26dc7a296f6b..fc7f4bf63306 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -101,12 +101,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) inode->i_atime = parent->i_atime; inode->i_op = &ceph_snapdir_iops; inode->i_fop = &ceph_snapdir_fops; - ci->i_snap_caps = CEPH_CAP_PIN; /* so we can open */ - ci->i_rbytes = 0; ci->i_btime = ceph_inode(parent)->i_btime; + ci->i_rbytes = 0; - if (inode->i_state & I_NEW) + if (inode->i_state & I_NEW) { + ci->i_snap_caps = CEPH_CAP_PIN; /* so we can open */ unlock_new_inode(inode); + } return inode; } -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ceph: don't clobber i_snap_caps on non-I_NEW inode 2021-03-15 18:07 ` [PATCH 1/2] ceph: don't clobber i_snap_caps on non-I_NEW inode Jeff Layton @ 2021-03-19 5:03 ` Xiubo Li 2021-03-19 12:51 ` Jeff Layton 0 siblings, 1 reply; 6+ messages in thread From: Xiubo Li @ 2021-03-19 5:03 UTC (permalink / raw) To: Jeff Layton, ceph-devel, idryomov; +Cc: linux-fsdevel, viro On 2021/3/16 2:07, Jeff Layton wrote: > We want the snapdir to mirror the non-snapped directory's attributes for > most things, but i_snap_caps represents the caps granted on the snapshot > directory by the MDS itself. A misbehaving MDS could issue different > caps for the snapdir and we lose them here. > > Only reset i_snap_caps when the inode is I_NEW. > > Reported-by: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/inode.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 26dc7a296f6b..fc7f4bf63306 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -101,12 +101,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) > inode->i_atime = parent->i_atime; > inode->i_op = &ceph_snapdir_iops; > inode->i_fop = &ceph_snapdir_fops; > - ci->i_snap_caps = CEPH_CAP_PIN; /* so we can open */ > - ci->i_rbytes = 0; > ci->i_btime = ceph_inode(parent)->i_btime; > + ci->i_rbytes = 0; > Hi Jeff, BTW, why we need to set other members here if the i_state is not I_NEW ? Are they necessary ? > - if (inode->i_state & I_NEW) > + if (inode->i_state & I_NEW) { > + ci->i_snap_caps = CEPH_CAP_PIN; /* so we can open */ > unlock_new_inode(inode); > + } > > return inode; > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ceph: don't clobber i_snap_caps on non-I_NEW inode 2021-03-19 5:03 ` Xiubo Li @ 2021-03-19 12:51 ` Jeff Layton 2021-03-19 13:42 ` Xiubo Li 0 siblings, 1 reply; 6+ messages in thread From: Jeff Layton @ 2021-03-19 12:51 UTC (permalink / raw) To: Xiubo Li, ceph-devel, idryomov; +Cc: linux-fsdevel, viro On Fri, 2021-03-19 at 13:03 +0800, Xiubo Li wrote: > On 2021/3/16 2:07, Jeff Layton wrote: > > We want the snapdir to mirror the non-snapped directory's attributes for > > most things, but i_snap_caps represents the caps granted on the snapshot > > directory by the MDS itself. A misbehaving MDS could issue different > > caps for the snapdir and we lose them here. > > > > Only reset i_snap_caps when the inode is I_NEW. > > > > Reported-by: Al Viro <viro@zeniv.linux.org.uk> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/ceph/inode.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > > index 26dc7a296f6b..fc7f4bf63306 100644 > > --- a/fs/ceph/inode.c > > +++ b/fs/ceph/inode.c > > @@ -101,12 +101,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) > > inode->i_atime = parent->i_atime; > > inode->i_op = &ceph_snapdir_iops; > > inode->i_fop = &ceph_snapdir_fops; > > - ci->i_snap_caps = CEPH_CAP_PIN; /* so we can open */ > > - ci->i_rbytes = 0; > > ci->i_btime = ceph_inode(parent)->i_btime; > > + ci->i_rbytes = 0; > > > > > > > > > > Hi Jeff, > > BTW, why we need to set other members here if the i_state is not I_NEW ? > > Are they necessary ? > I think so, at least for most of them. IIUC, we want the .snap directory's metadata to mirror that of the parent directory, so we want stuff like the ownership and mtime to track changes in the parent. I can move the setting of i_op and i_fop into the if block though. Those should never change anyway, though setting them is harmless here since we're checking to make sure the type is correct above. I'll go ahead and do that, but I won't bother re-posting the v2 patch since it's a trivial change. > > - if (inode->i_state & I_NEW) > > + if (inode->i_state & I_NEW) { > > + ci->i_snap_caps = CEPH_CAP_PIN; /* so we can open */ > > unlock_new_inode(inode); > > + } > > > > > > > > > > return inode; > > } > > -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ceph: don't clobber i_snap_caps on non-I_NEW inode 2021-03-19 12:51 ` Jeff Layton @ 2021-03-19 13:42 ` Xiubo Li 0 siblings, 0 replies; 6+ messages in thread From: Xiubo Li @ 2021-03-19 13:42 UTC (permalink / raw) To: Jeff Layton, ceph-devel, idryomov; +Cc: linux-fsdevel, viro On 2021/3/19 20:51, Jeff Layton wrote: > On Fri, 2021-03-19 at 13:03 +0800, Xiubo Li wrote: >> On 2021/3/16 2:07, Jeff Layton wrote: >>> We want the snapdir to mirror the non-snapped directory's attributes for >>> most things, but i_snap_caps represents the caps granted on the snapshot >>> directory by the MDS itself. A misbehaving MDS could issue different >>> caps for the snapdir and we lose them here. >>> >>> Only reset i_snap_caps when the inode is I_NEW. >>> >>> Reported-by: Al Viro <viro@zeniv.linux.org.uk> >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> fs/ceph/inode.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >>> index 26dc7a296f6b..fc7f4bf63306 100644 >>> --- a/fs/ceph/inode.c >>> +++ b/fs/ceph/inode.c >>> @@ -101,12 +101,13 @@ struct inode *ceph_get_snapdir(struct inode *parent) >>> inode->i_atime = parent->i_atime; >>> inode->i_op = &ceph_snapdir_iops; >>> inode->i_fop = &ceph_snapdir_fops; >>> - ci->i_snap_caps = CEPH_CAP_PIN; /* so we can open */ >>> - ci->i_rbytes = 0; >>> ci->i_btime = ceph_inode(parent)->i_btime; >>> + ci->i_rbytes = 0; >>> >>> >>> >>> >> Hi Jeff, >> >> BTW, why we need to set other members here if the i_state is not I_NEW ? >> >> Are they necessary ? >> > I think so, at least for most of them. > > IIUC, we want the .snap directory's metadata to mirror that of the > parent directory, so we want stuff like the ownership and mtime to track > changes in the parent. Okay. > I can move the setting of i_op and i_fop into the if block though. Those > should never change anyway, though setting them is harmless here since > we're checking to make sure the type is correct above. > > I'll go ahead and do that, but I won't bother re-posting the v2 patch > since it's a trivial change. Yeah, make sense :-) Thanks > >>> - if (inode->i_state & I_NEW) >>> + if (inode->i_state & I_NEW) { >>> + ci->i_snap_caps = CEPH_CAP_PIN; /* so we can open */ >>> unlock_new_inode(inode); >>> + } >>> >>> >>> >>> >>> return inode; >>> } >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] ceph: don't use d_add in ceph_handle_snapdir 2021-03-15 18:07 [PATCH 0/2] ceph: snapdir dentry handling fixes Jeff Layton 2021-03-15 18:07 ` [PATCH 1/2] ceph: don't clobber i_snap_caps on non-I_NEW inode Jeff Layton @ 2021-03-15 18:07 ` Jeff Layton 1 sibling, 0 replies; 6+ messages in thread From: Jeff Layton @ 2021-03-15 18:07 UTC (permalink / raw) To: ceph-devel, idryomov; +Cc: linux-fsdevel, viro It's possible ceph_get_snapdir could end up finding a (disconnected) inode that already exists in the cache. Change the prototype for ceph_handle_snapdir to return a dentry pointer and have it use d_splice_alias so we don't end up with an aliased dentry in the cache. Reported-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/dir.c | 30 ++++++++++++++++++------------ fs/ceph/file.c | 6 ++++-- fs/ceph/super.h | 2 +- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index 113f669d71dd..4b871b7017a0 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -667,8 +667,8 @@ static loff_t ceph_dir_llseek(struct file *file, loff_t offset, int whence) /* * Handle lookups for the hidden .snap directory. */ -int ceph_handle_snapdir(struct ceph_mds_request *req, - struct dentry *dentry, int err) +struct dentry *ceph_handle_snapdir(struct ceph_mds_request *req, + struct dentry *dentry, int err) { struct ceph_fs_client *fsc = ceph_sb_to_client(dentry->d_sb); struct inode *parent = d_inode(dentry->d_parent); /* we hold i_mutex */ @@ -676,18 +676,19 @@ int ceph_handle_snapdir(struct ceph_mds_request *req, /* .snap dir? */ if (err == -ENOENT && ceph_snap(parent) == CEPH_NOSNAP && - strcmp(dentry->d_name.name, - fsc->mount_options->snapdir_name) == 0) { + strcmp(dentry->d_name.name, fsc->mount_options->snapdir_name) == 0) { + struct dentry *res; struct inode *inode = ceph_get_snapdir(parent); + if (IS_ERR(inode)) - return PTR_ERR(inode); - dout("ENOENT on snapdir %p '%pd', linking to snapdir %p\n", - dentry, dentry, inode); - BUG_ON(!d_unhashed(dentry)); - d_add(dentry, inode); - err = 0; + return ERR_CAST(inode); + res = d_splice_alias(inode, dentry); + dout("ENOENT on snapdir %p '%pd', linking to snapdir %p. Spliced dentry %p\n", + dentry, dentry, inode, res); + if (res) + dentry = res; } - return err; + return dentry; } /* @@ -743,6 +744,7 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry, struct ceph_fs_client *fsc = ceph_sb_to_client(dir->i_sb); struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(dir->i_sb); struct ceph_mds_request *req; + struct dentry *res; int op; int mask; int err; @@ -793,7 +795,11 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry, req->r_parent = dir; set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags); err = ceph_mdsc_do_request(mdsc, NULL, req); - err = ceph_handle_snapdir(req, dentry, err); + res = ceph_handle_snapdir(req, dentry, err); + if (IS_ERR(res)) + err = PTR_ERR(res); + else + dentry = res; dentry = ceph_finish_lookup(req, dentry, err); ceph_mdsc_put_request(req); /* will dput(dentry) */ dout("lookup result=%p\n", dentry); diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 209535d5b8d3..847beb9f43dd 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -739,9 +739,11 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry, err = ceph_mdsc_do_request(mdsc, (flags & (O_CREAT|O_TRUNC)) ? dir : NULL, req); - err = ceph_handle_snapdir(req, dentry, err); - if (err) + dentry = ceph_handle_snapdir(req, dentry, err); + if (IS_ERR(dentry)) { + err = PTR_ERR(dentry); goto out_req; + } if ((flags & O_CREAT) && !req->r_reply_info.head->is_dentry) err = ceph_handle_notrace_create(dir, dentry); diff --git a/fs/ceph/super.h b/fs/ceph/super.h index 188565d806b2..07a3fb52ae30 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -1193,7 +1193,7 @@ extern const struct dentry_operations ceph_dentry_ops; extern loff_t ceph_make_fpos(unsigned high, unsigned off, bool hash_order); extern int ceph_handle_notrace_create(struct inode *dir, struct dentry *dentry); -extern int ceph_handle_snapdir(struct ceph_mds_request *req, +extern struct dentry *ceph_handle_snapdir(struct ceph_mds_request *req, struct dentry *dentry, int err); extern struct dentry *ceph_finish_lookup(struct ceph_mds_request *req, struct dentry *dentry, int err); -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-19 13:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-15 18:07 [PATCH 0/2] ceph: snapdir dentry handling fixes Jeff Layton 2021-03-15 18:07 ` [PATCH 1/2] ceph: don't clobber i_snap_caps on non-I_NEW inode Jeff Layton 2021-03-19 5:03 ` Xiubo Li 2021-03-19 12:51 ` Jeff Layton 2021-03-19 13:42 ` Xiubo Li 2021-03-15 18:07 ` [PATCH 2/2] ceph: don't use d_add in ceph_handle_snapdir Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).