* Re: [PATCH] ceph: fix duplicate increment of opened_inodes metric
2021-11-22 14:22 [PATCH] ceph: fix duplicate increment of opened_inodes metric Hu Weiwen
@ 2021-11-22 17:39 ` Jeff Layton
2021-11-23 1:10 ` Xiubo Li
2021-11-23 11:34 ` Jeff Layton
2 siblings, 0 replies; 4+ messages in thread
From: Jeff Layton @ 2021-11-22 17:39 UTC (permalink / raw)
To: Hu Weiwen, ceph-devel; +Cc: Xiubo Li
On Mon, 2021-11-22 at 22:22 +0800, Hu Weiwen wrote:
> opened_inodes is incremented twice when the same inode is opened twice
> with O_RDONLY and O_WRONLY respectively.
>
> To reproduce, run this python script, then check the metrics:
>
> import os
> for _ in range(10000):
> fd_r = os.open('a', os.O_RDONLY)
> fd_w = os.open('a', os.O_WRONLY)
> os.close(fd_r)
> os.close(fd_w)
>
> Fixes: 1dd8d4708136 ("ceph: metrics for opened files, pinned caps and opened inodes")
> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> ---
> fs/ceph/caps.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index b9460b6fb76f..c447fa2e2d1f 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4350,7 +4350,7 @@ void ceph_get_fmode(struct ceph_inode_info *ci, int fmode, int count)
> {
> struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(ci->vfs_inode.i_sb);
> int bits = (fmode << 1) | 1;
> - bool is_opened = false;
> + bool already_opened = false;
> int i;
>
> if (count == 1)
> @@ -4358,19 +4358,19 @@ void ceph_get_fmode(struct ceph_inode_info *ci, int fmode, int count)
>
> spin_lock(&ci->i_ceph_lock);
> for (i = 0; i < CEPH_FILE_MODE_BITS; i++) {
> - if (bits & (1 << i))
> - ci->i_nr_by_mode[i] += count;
> -
> /*
> - * If any of the mode ref is larger than 1,
> + * If any of the mode ref is larger than 0,
> * that means it has been already opened by
> * others. Just skip checking the PIN ref.
> */
> - if (i && ci->i_nr_by_mode[i] > 1)
> - is_opened = true;
> + if (i && ci->i_nr_by_mode[i])
> + already_opened = true;
> +
> + if (bits & (1 << i))
> + ci->i_nr_by_mode[i] += count;
> }
>
> - if (!is_opened)
> + if (!already_opened)
> percpu_counter_inc(&mdsc->metric.opened_inodes);
> spin_unlock(&ci->i_ceph_lock);
> }
Thanks for the patch. I think it's correct, but I'd like Xiubo to
confirm before we merge this.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ceph: fix duplicate increment of opened_inodes metric
2021-11-22 14:22 [PATCH] ceph: fix duplicate increment of opened_inodes metric Hu Weiwen
2021-11-22 17:39 ` Jeff Layton
@ 2021-11-23 1:10 ` Xiubo Li
2021-11-23 11:34 ` Jeff Layton
2 siblings, 0 replies; 4+ messages in thread
From: Xiubo Li @ 2021-11-23 1:10 UTC (permalink / raw)
To: Hu Weiwen, ceph-devel
On 11/22/21 10:22 PM, Hu Weiwen wrote:
> opened_inodes is incremented twice when the same inode is opened twice
> with O_RDONLY and O_WRONLY respectively.
>
> To reproduce, run this python script, then check the metrics:
>
> import os
> for _ in range(10000):
> fd_r = os.open('a', os.O_RDONLY)
> fd_w = os.open('a', os.O_WRONLY)
> os.close(fd_r)
> os.close(fd_w)
>
> Fixes: 1dd8d4708136 ("ceph: metrics for opened files, pinned caps and opened inodes")
> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> ---
> fs/ceph/caps.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index b9460b6fb76f..c447fa2e2d1f 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4350,7 +4350,7 @@ void ceph_get_fmode(struct ceph_inode_info *ci, int fmode, int count)
> {
> struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(ci->vfs_inode.i_sb);
> int bits = (fmode << 1) | 1;
> - bool is_opened = false;
> + bool already_opened = false;
> int i;
>
> if (count == 1)
> @@ -4358,19 +4358,19 @@ void ceph_get_fmode(struct ceph_inode_info *ci, int fmode, int count)
>
> spin_lock(&ci->i_ceph_lock);
> for (i = 0; i < CEPH_FILE_MODE_BITS; i++) {
> - if (bits & (1 << i))
> - ci->i_nr_by_mode[i] += count;
> -
> /*
> - * If any of the mode ref is larger than 1,
> + * If any of the mode ref is larger than 0,
> * that means it has been already opened by
> * others. Just skip checking the PIN ref.
> */
> - if (i && ci->i_nr_by_mode[i] > 1)
> - is_opened = true;
> + if (i && ci->i_nr_by_mode[i])
> + already_opened = true;
> +
> + if (bits & (1 << i))
> + ci->i_nr_by_mode[i] += count;
> }
>
> - if (!is_opened)
> + if (!already_opened)
> percpu_counter_inc(&mdsc->metric.opened_inodes);
> spin_unlock(&ci->i_ceph_lock);
> }
LGTM.
Reviewed-by: Xiubo Li <xiubli@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ceph: fix duplicate increment of opened_inodes metric
2021-11-22 14:22 [PATCH] ceph: fix duplicate increment of opened_inodes metric Hu Weiwen
2021-11-22 17:39 ` Jeff Layton
2021-11-23 1:10 ` Xiubo Li
@ 2021-11-23 11:34 ` Jeff Layton
2 siblings, 0 replies; 4+ messages in thread
From: Jeff Layton @ 2021-11-23 11:34 UTC (permalink / raw)
To: Hu Weiwen, ceph-devel; +Cc: Xiubo Li
On Mon, 2021-11-22 at 22:22 +0800, Hu Weiwen wrote:
> opened_inodes is incremented twice when the same inode is opened twice
> with O_RDONLY and O_WRONLY respectively.
>
> To reproduce, run this python script, then check the metrics:
>
> import os
> for _ in range(10000):
> fd_r = os.open('a', os.O_RDONLY)
> fd_w = os.open('a', os.O_WRONLY)
> os.close(fd_r)
> os.close(fd_w)
>
> Fixes: 1dd8d4708136 ("ceph: metrics for opened files, pinned caps and opened inodes")
> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> ---
> fs/ceph/caps.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index b9460b6fb76f..c447fa2e2d1f 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4350,7 +4350,7 @@ void ceph_get_fmode(struct ceph_inode_info *ci, int fmode, int count)
> {
> struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(ci->vfs_inode.i_sb);
> int bits = (fmode << 1) | 1;
> - bool is_opened = false;
> + bool already_opened = false;
> int i;
>
> if (count == 1)
> @@ -4358,19 +4358,19 @@ void ceph_get_fmode(struct ceph_inode_info *ci, int fmode, int count)
>
> spin_lock(&ci->i_ceph_lock);
> for (i = 0; i < CEPH_FILE_MODE_BITS; i++) {
> - if (bits & (1 << i))
> - ci->i_nr_by_mode[i] += count;
> -
> /*
> - * If any of the mode ref is larger than 1,
> + * If any of the mode ref is larger than 0,
> * that means it has been already opened by
> * others. Just skip checking the PIN ref.
> */
> - if (i && ci->i_nr_by_mode[i] > 1)
> - is_opened = true;
> + if (i && ci->i_nr_by_mode[i])
> + already_opened = true;
> +
> + if (bits & (1 << i))
> + ci->i_nr_by_mode[i] += count;
> }
>
> - if (!is_opened)
> + if (!already_opened)
> percpu_counter_inc(&mdsc->metric.opened_inodes);
> spin_unlock(&ci->i_ceph_lock);
> }
Merged into the ceph-client testing branch. Thanks for the patch!
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 4+ messages in thread