[PATCH v2] ceph: properly handle statfs on multifs setups

[PATCH v2] ceph: properly handle statfs on multifs setups

[Jeff Layton]

ceph_statfs currently stuffs the cluster fsid into the f_fsid field.
This was fine when we only had a single filesystem per cluster, but now
that we have multiples we need to use something that will vary between
them.

Change ceph_statfs to xor each 32-bit chunk of the fsid (aka cluster id)
into the lower bits of the statfs->f_fsid. Change the lower bits to hold
the fscid (filesystem ID within the cluster).

That should give us a value that is guaranteed to be unique between
filesystems within a cluster, and should minimize the chance of
collisions between mounts of different clusters.

URL: https://tracker.ceph.com/issues/52812
Reported-by: Sachin Prabhu <sprabhu@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/super.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

While looking at making an equivalent change to the userland libraries,
it occurred to me that the earlier patch's method for computing this
was overly-complex. This makes it a bit simpler, and avoids the
intermediate step of setting up a u64.

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 9bb88423417e..e7b839aa08f6 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -52,8 +52,7 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct ceph_fs_client *fsc = ceph_inode_to_client(d_inode(dentry));
 	struct ceph_mon_client *monc = &fsc->client->monc;
 	struct ceph_statfs st;
-	u64 fsid;
-	int err;
+	int i, err;
 	u64 data_pool;
 
 	if (fsc->mdsc->mdsmap->m_num_data_pg_pools == 1) {
@@ -99,12 +98,14 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_namelen = NAME_MAX;
 
 	/* Must convert the fsid, for consistent values across arches */
+	buf->f_fsid.val[0] = 0;
 	mutex_lock(&monc->mutex);
-	fsid = le64_to_cpu(*(__le64 *)(&monc->monmap->fsid)) ^
-	       le64_to_cpu(*((__le64 *)&monc->monmap->fsid + 1));
+	for (i = 0; i < 4; ++i)
+		buf->f_fsid.val[0] ^= le32_to_cpu(((__le32 *)&monc->monmap->fsid)[i]);
 	mutex_unlock(&monc->mutex);
 
-	buf->f_fsid = u64_to_fsid(fsid);
+	/* fold the fs_cluster_id into the upper bits */
+	buf->f_fsid.val[1] = monc->fs_cluster_id;
 
 	return 0;
 }
-- 
2.31.1

Re: [PATCH v2] ceph: properly handle statfs on multifs setups

[Xiubo Li]

On 11/3/21 4:45 AM, Jeff Layton wrote:
> ceph_statfs currently stuffs the cluster fsid into the f_fsid field.
> This was fine when we only had a single filesystem per cluster, but now
> that we have multiples we need to use something that will vary between
> them.
>
> Change ceph_statfs to xor each 32-bit chunk of the fsid (aka cluster id)
> into the lower bits of the statfs->f_fsid. Change the lower bits to hold
> the fscid (filesystem ID within the cluster).
>
> That should give us a value that is guaranteed to be unique between
> filesystems within a cluster, and should minimize the chance of
> collisions between mounts of different clusters.
>
> URL: https://tracker.ceph.com/issues/52812
> Reported-by: Sachin Prabhu <sprabhu@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/ceph/super.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> While looking at making an equivalent change to the userland libraries,
> it occurred to me that the earlier patch's method for computing this
> was overly-complex. This makes it a bit simpler, and avoids the
> intermediate step of setting up a u64.
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 9bb88423417e..e7b839aa08f6 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -52,8 +52,7 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
>   	struct ceph_fs_client *fsc = ceph_inode_to_client(d_inode(dentry));
>   	struct ceph_mon_client *monc = &fsc->client->monc;
>   	struct ceph_statfs st;
> -	u64 fsid;
> -	int err;
> +	int i, err;
>   	u64 data_pool;
>   
>   	if (fsc->mdsc->mdsmap->m_num_data_pg_pools == 1) {
> @@ -99,12 +98,14 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
>   	buf->f_namelen = NAME_MAX;
>   
>   	/* Must convert the fsid, for consistent values across arches */
> +	buf->f_fsid.val[0] = 0;
>   	mutex_lock(&monc->mutex);
> -	fsid = le64_to_cpu(*(__le64 *)(&monc->monmap->fsid)) ^
> -	       le64_to_cpu(*((__le64 *)&monc->monmap->fsid + 1));
> +	for (i = 0; i < 4; ++i)
> +		buf->f_fsid.val[0] ^= le32_to_cpu(((__le32 *)&monc->monmap->fsid)[i]);
>   	mutex_unlock(&monc->mutex);
>   
> -	buf->f_fsid = u64_to_fsid(fsid);
> +	/* fold the fs_cluster_id into the upper bits */
> +	buf->f_fsid.val[1] = monc->fs_cluster_id;
>   
>   	return 0;
>   }

This version looks better.

Reviewed-by: Xiubo Li <xiubli@redhat.com>

Re: [PATCH v2] ceph: properly handle statfs on multifs setups

[Jeff Layton]

On Wed, 2021-11-03 at 14:56 +0800, Xiubo Li wrote:
> On 11/3/21 4:45 AM, Jeff Layton wrote:
> > ceph_statfs currently stuffs the cluster fsid into the f_fsid field.
> > This was fine when we only had a single filesystem per cluster, but now
> > that we have multiples we need to use something that will vary between
> > them.
> > 
> > Change ceph_statfs to xor each 32-bit chunk of the fsid (aka cluster id)
> > into the lower bits of the statfs->f_fsid. Change the lower bits to hold
> > the fscid (filesystem ID within the cluster).
> > 
> > That should give us a value that is guaranteed to be unique between
> > filesystems within a cluster, and should minimize the chance of
> > collisions between mounts of different clusters.
> > 
> > URL: https://tracker.ceph.com/issues/52812
> > Reported-by: Sachin Prabhu <sprabhu@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >   fs/ceph/super.c | 11 ++++++-----
> >   1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > While looking at making an equivalent change to the userland libraries,
> > it occurred to me that the earlier patch's method for computing this
> > was overly-complex. This makes it a bit simpler, and avoids the
> > intermediate step of setting up a u64.
> > 
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 9bb88423417e..e7b839aa08f6 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -52,8 +52,7 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
> >   	struct ceph_fs_client *fsc = ceph_inode_to_client(d_inode(dentry));
> >   	struct ceph_mon_client *monc = &fsc->client->monc;
> >   	struct ceph_statfs st;
> > -	u64 fsid;
> > -	int err;
> > +	int i, err;
> >   	u64 data_pool;
> >   
> >   	if (fsc->mdsc->mdsmap->m_num_data_pg_pools == 1) {
> > @@ -99,12 +98,14 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
> >   	buf->f_namelen = NAME_MAX;
> >   
> >   	/* Must convert the fsid, for consistent values across arches */
> > +	buf->f_fsid.val[0] = 0;
> >   	mutex_lock(&monc->mutex);
> > -	fsid = le64_to_cpu(*(__le64 *)(&monc->monmap->fsid)) ^
> > -	       le64_to_cpu(*((__le64 *)&monc->monmap->fsid + 1));
> > +	for (i = 0; i < 4; ++i)
> > +		buf->f_fsid.val[0] ^= le32_to_cpu(((__le32 *)&monc->monmap->fsid)[i]);
> >   	mutex_unlock(&monc->mutex);
> >   
> > -	buf->f_fsid = u64_to_fsid(fsid);
> > +	/* fold the fs_cluster_id into the upper bits */
> > +	buf->f_fsid.val[1] = monc->fs_cluster_id;
> >   
> >   	return 0;
> >   }
> 
> This version looks better.
> 
> Reviewed-by: Xiubo Li <xiubli@redhat.com>
> 
> 

Thanks. I think I'm going to make one more small change in there and
express the loop conditional as:

    i < sizeof(monc->monmap->fsid) / sizeof(__le32)

That should work out to be '4', but should be safer in case the size of
fsid ever changes. I'm not going to bother to re-post for that though.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] ceph: properly handle statfs on multifs setups

[Xiubo Li]

On 11/3/21 6:24 PM, Jeff Layton wrote:
> On Wed, 2021-11-03 at 14:56 +0800, Xiubo Li wrote:
>> On 11/3/21 4:45 AM, Jeff Layton wrote:
>>> ceph_statfs currently stuffs the cluster fsid into the f_fsid field.
>>> This was fine when we only had a single filesystem per cluster, but now
>>> that we have multiples we need to use something that will vary between
>>> them.
>>>
>>> Change ceph_statfs to xor each 32-bit chunk of the fsid (aka cluster id)
>>> into the lower bits of the statfs->f_fsid. Change the lower bits to hold
>>> the fscid (filesystem ID within the cluster).
>>>
>>> That should give us a value that is guaranteed to be unique between
>>> filesystems within a cluster, and should minimize the chance of
>>> collisions between mounts of different clusters.
>>>
>>> URL: https://tracker.ceph.com/issues/52812
>>> Reported-by: Sachin Prabhu <sprabhu@redhat.com>
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>    fs/ceph/super.c | 11 ++++++-----
>>>    1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> While looking at making an equivalent change to the userland libraries,
>>> it occurred to me that the earlier patch's method for computing this
>>> was overly-complex. This makes it a bit simpler, and avoids the
>>> intermediate step of setting up a u64.
>>>
>>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>>> index 9bb88423417e..e7b839aa08f6 100644
>>> --- a/fs/ceph/super.c
>>> +++ b/fs/ceph/super.c
>>> @@ -52,8 +52,7 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
>>>    	struct ceph_fs_client *fsc = ceph_inode_to_client(d_inode(dentry));
>>>    	struct ceph_mon_client *monc = &fsc->client->monc;
>>>    	struct ceph_statfs st;
>>> -	u64 fsid;
>>> -	int err;
>>> +	int i, err;
>>>    	u64 data_pool;
>>>    
>>>    	if (fsc->mdsc->mdsmap->m_num_data_pg_pools == 1) {
>>> @@ -99,12 +98,14 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
>>>    	buf->f_namelen = NAME_MAX;
>>>    
>>>    	/* Must convert the fsid, for consistent values across arches */
>>> +	buf->f_fsid.val[0] = 0;
>>>    	mutex_lock(&monc->mutex);
>>> -	fsid = le64_to_cpu(*(__le64 *)(&monc->monmap->fsid)) ^
>>> -	       le64_to_cpu(*((__le64 *)&monc->monmap->fsid + 1));
>>> +	for (i = 0; i < 4; ++i)
>>> +		buf->f_fsid.val[0] ^= le32_to_cpu(((__le32 *)&monc->monmap->fsid)[i]);
>>>    	mutex_unlock(&monc->mutex);
>>>    
>>> -	buf->f_fsid = u64_to_fsid(fsid);
>>> +	/* fold the fs_cluster_id into the upper bits */
>>> +	buf->f_fsid.val[1] = monc->fs_cluster_id;
>>>    
>>>    	return 0;
>>>    }
>> This version looks better.
>>
>> Reviewed-by: Xiubo Li <xiubli@redhat.com>
>>
>>
> Thanks. I think I'm going to make one more small change in there and
> express the loop conditional as:
>
>      i < sizeof(monc->monmap->fsid) / sizeof(__le32)
>
> That should work out to be '4', but should be safer in case the size of
> fsid ever changes. I'm not going to bother to re-post for that though.
Yeah, make sense.