[PATCH v1 0/1] ceph: Fix log output race condition in osd client

[PATCH v1 0/1] ceph: Fix log output race condition in osd client

[Simon Buttgereit]

Hi Slava,
I have updated the code according to our previous discussion. The
refcount_read() call now occurs only once per debug output, and the
increment/decrement information is included directly in the log text.
I hope this addresses all our concerns.

Best Regards
Simon

Simon Buttgereit (1):
  ceph: Fix log output race condition in osd client

 net/ceph/osd_client.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

--
2.51.0

[PATCH v1 1/1] ceph: Fix log output race condition in osd client

[Simon Buttgereit]

OSD client logging has a problem in get_osd() and put_osd().
For one logging output refcount_read() is called twice. If recount
value changes between both calls logging output is not consistent.

This patch prints out only the resulting value and adds to the text
whether it was incremented or decremented.

Signed-off-by: Simon Buttgereit <simon.buttgereit@tu-ilmenau.de>
---
 net/ceph/osd_client.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 6664ea73ccf8..9cf91eed8020 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1280,8 +1280,7 @@ static struct ceph_osd *create_osd(struct ceph_osd_client *osdc, int onum)
 static struct ceph_osd *get_osd(struct ceph_osd *osd)
 {
 	if (refcount_inc_not_zero(&osd->o_ref)) {
-		dout("get_osd %p %d -> %d\n", osd, refcount_read(&osd->o_ref)-1,
-		     refcount_read(&osd->o_ref));
+		dout("get_osd %p; increment refcnt to %d\n", osd, refcount_read(&osd->o_ref));
 		return osd;
 	} else {
 		dout("get_osd %p FAIL\n", osd);
@@ -1291,8 +1290,7 @@ static struct ceph_osd *get_osd(struct ceph_osd *osd)
 
 static void put_osd(struct ceph_osd *osd)
 {
-	dout("put_osd %p %d -> %d\n", osd, refcount_read(&osd->o_ref),
-	     refcount_read(&osd->o_ref) - 1);
+	dout("put_osd %p; decrement refcnt to %d\n", osd, refcount_read(&osd->o_ref) - 1);
 	if (refcount_dec_and_test(&osd->o_ref)) {
 		osd_cleanup(osd);
 		kfree(osd);
-- 
2.51.0

Re: [PATCH v1 0/1] ceph: Fix log output race condition in osd client

[Viacheslav Dubeyko]

On Thu, 2025-09-25 at 09:57 +0200, Simon Buttgereit wrote:
> Hi Slava,
> I have updated the code according to our previous discussion. The
> refcount_read() call now occurs only once per debug output, and the
> increment/decrement information is included directly in the log text.
> I hope this addresses all our concerns.
> 

Usually, you don't need in cover letter for one patch only. :) And it's the
second version of the patch. ;)


Thanks,
Slava.

Re: [PATCH v1 1/1] ceph: Fix log output race condition in osd client

[Viacheslav Dubeyko]

On Thu, 2025-09-25 at 09:57 +0200, Simon Buttgereit wrote:
> OSD client logging has a problem in get_osd() and put_osd().
> For one logging output refcount_read() is called twice. If recount
> value changes between both calls logging output is not consistent.
> 
> This patch prints out only the resulting value and adds to the text
> whether it was incremented or decremented.
> 
> Signed-off-by: Simon Buttgereit <simon.buttgereit@tu-ilmenau.de>
> ---
>  net/ceph/osd_client.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 6664ea73ccf8..9cf91eed8020 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -1280,8 +1280,7 @@ static struct ceph_osd *create_osd(struct ceph_osd_client *osdc, int onum)
>  static struct ceph_osd *get_osd(struct ceph_osd *osd)
>  {
>  	if (refcount_inc_not_zero(&osd->o_ref)) {
> -		dout("get_osd %p %d -> %d\n", osd, refcount_read(&osd->o_ref)-1,
> -		     refcount_read(&osd->o_ref));
> +		dout("get_osd %p; increment refcnt to %d\n", osd, refcount_read(&osd->o_ref));
>  		return osd;
>  	} else {
>  		dout("get_osd %p FAIL\n", osd);
> @@ -1291,8 +1290,7 @@ static struct ceph_osd *get_osd(struct ceph_osd *osd)
>  
>  static void put_osd(struct ceph_osd *osd)
>  {
> -	dout("put_osd %p %d -> %d\n", osd, refcount_read(&osd->o_ref),
> -	     refcount_read(&osd->o_ref) - 1);
> +	dout("put_osd %p; decrement refcnt to %d\n", osd, refcount_read(&osd->o_ref) - 1);
>  	if (refcount_dec_and_test(&osd->o_ref)) {
>  		osd_cleanup(osd);
>  		kfree(osd);

Looks good.

Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.