Re: [PATCH 1/2] ceph: don't clobber i_snap_caps on non-I_NEW inode

[Jeff Layton <jlayton@kernel.org>]

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>