* [PATCH v2] ceph: properly handle statfs on multifs setups
@ 2021-11-02 20:45 Jeff Layton
2021-11-03 6:56 ` Xiubo Li
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Layton @ 2021-11-02 20:45 UTC (permalink / raw)
To: ceph-devel, idryomov; +Cc: Sachin Prabhu
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] ceph: properly handle statfs on multifs setups
2021-11-02 20:45 [PATCH v2] ceph: properly handle statfs on multifs setups Jeff Layton
@ 2021-11-03 6:56 ` Xiubo Li
2021-11-03 10:24 ` Jeff Layton
0 siblings, 1 reply; 4+ messages in thread
From: Xiubo Li @ 2021-11-03 6:56 UTC (permalink / raw)
To: Jeff Layton, ceph-devel, idryomov; +Cc: Sachin Prabhu
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] ceph: properly handle statfs on multifs setups
2021-11-03 6:56 ` Xiubo Li
@ 2021-11-03 10:24 ` Jeff Layton
2021-11-04 5:35 ` Xiubo Li
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Layton @ 2021-11-03 10:24 UTC (permalink / raw)
To: Xiubo Li, ceph-devel, idryomov; +Cc: Sachin Prabhu
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] ceph: properly handle statfs on multifs setups
2021-11-03 10:24 ` Jeff Layton
@ 2021-11-04 5:35 ` Xiubo Li
0 siblings, 0 replies; 4+ messages in thread
From: Xiubo Li @ 2021-11-04 5:35 UTC (permalink / raw)
To: Jeff Layton, ceph-devel, idryomov; +Cc: Sachin Prabhu
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-04 5:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-02 20:45 [PATCH v2] ceph: properly handle statfs on multifs setups Jeff Layton
2021-11-03 6:56 ` Xiubo Li
2021-11-03 10:24 ` Jeff Layton
2021-11-04 5:35 ` Xiubo Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.