| Message ID | 20210315180717.266155-2-jlayton@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ceph: snapdir dentry handling fixes | expand |
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; > }
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>
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; >>> } >>
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; }
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(-)