[PATCH 0/2] ceph: snapdir dentry handling fixes

[PATCH 0/2] ceph: snapdir dentry handling fixes

[Jeff Layton]

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

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

[Jeff Layton]

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

[PATCH 2/2] ceph: don't use d_add in ceph_handle_snapdir

[Jeff Layton]

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

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

[Xiubo Li]

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;
>   }

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

[Jeff Layton]

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>

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

[Xiubo Li]

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;
>>>    }
>>