diff mbox series

ceph: fix duplicate increment of opened_inodes metric

Message ID 20211122142212.1621-1-sehuww@mail.scut.edu.cn (mailing list archive)
State New, archived
Headers show
Series ceph: fix duplicate increment of opened_inodes metric | expand

Commit Message

胡玮文 Nov. 22, 2021, 2:22 p.m. UTC
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(-)

Comments

Jeff Layton Nov. 22, 2021, 5:39 p.m. UTC | #1
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.
Xiubo Li Nov. 23, 2021, 1:10 a.m. UTC | #2
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>
Jeff Layton Nov. 23, 2021, 11:34 a.m. UTC | #3
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!
diff mbox series

Patch

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