[PATCH] ceph: fix writeback_count leak in write_folio_nounlock()

Wentao Liang posted 1 patch 1 week, 4 days ago
There is a newer version of this series
fs/ceph/addr.c | 3 +++
1 file changed, 3 insertions(+)
[PATCH] ceph: fix writeback_count leak in write_folio_nounlock()
Posted by Wentao Liang 1 week, 4 days ago
write_folio_nounlock() increments fsc->writeback_count to track
in-flight writeback operations. On several error paths where the
function returns early (folio lookup failure, snapshot context
allocation failure, and writepages submission failure), the function
returns without calling atomic_long_dec() to decrement the counter.

Each leaked increment keeps the counter above zero, which can prevent
the filesystem from cleanly unmounting or suspending writes.

Add atomic_long_dec() calls on all error paths that currently return
without decrementing the counter.

Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 fs/ceph/addr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 0a86f672cc09..a606378649c3 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -790,6 +790,7 @@ static int write_folio_nounlock(struct folio *folio,
 				    ceph_wbc.truncate_size, true);
 	if (IS_ERR(req)) {
 		folio_redirty_for_writepage(wbc, folio);
+		atomic_long_dec(&fsc->writeback_count);
 		return PTR_ERR(req);
 	}
 
@@ -809,6 +810,7 @@ static int write_folio_nounlock(struct folio *folio,
 			folio_redirty_for_writepage(wbc, folio);
 			folio_end_writeback(folio);
 			ceph_osdc_put_request(req);
+			atomic_long_dec(&fsc->writeback_count);
 			return PTR_ERR(bounce_page);
 		}
 	}
@@ -847,6 +849,7 @@ static int write_folio_nounlock(struct folio *folio,
 			      ceph_vinop(inode), folio);
 			folio_redirty_for_writepage(wbc, folio);
 			folio_end_writeback(folio);
+			atomic_long_dec_return(&fsc->writeback_count);
 			return err;
 		}
 		if (err == -EBLOCKLISTED)
-- 
2.34.1
Re: [PATCH] ceph: fix writeback_count leak in write_folio_nounlock()
Posted by Viacheslav Dubeyko 1 week, 3 days ago
On Thu, 2026-05-28 at 03:54 +0000, Wentao Liang wrote:
> write_folio_nounlock() increments fsc->writeback_count to track
> in-flight writeback operations. On several error paths where the
> function returns early (folio lookup failure, snapshot context
> allocation failure, and writepages submission failure), the function
> returns without calling atomic_long_dec() to decrement the counter.
> 
> Each leaked increment keeps the counter above zero, which can prevent
> the filesystem from cleanly unmounting or suspending writes.
> 
> Add atomic_long_dec() calls on all error paths that currently return
> without decrementing the counter.
> 
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  fs/ceph/addr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 0a86f672cc09..a606378649c3 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -790,6 +790,7 @@ static int write_folio_nounlock(struct folio *folio,
>  				    ceph_wbc.truncate_size, true);
>  	if (IS_ERR(req)) {
>  		folio_redirty_for_writepage(wbc, folio);
> +		atomic_long_dec(&fsc->writeback_count);

Good catch. But I believe that we need to use this pattern in all three cases:

  if (atomic_long_dec_return(&fsc->writeback_count) <
      CONGESTION_OFF_THRESH(fsc->mount_options->congestion_kb))
      fsc->write_congested = false;

>  		return PTR_ERR(req);
>  	}
>  
> @@ -809,6 +810,7 @@ static int write_folio_nounlock(struct folio *folio,
>  			folio_redirty_for_writepage(wbc, folio);
>  			folio_end_writeback(folio);
>  			ceph_osdc_put_request(req);
> +			atomic_long_dec(&fsc->writeback_count);

Ditto.

>  			return PTR_ERR(bounce_page);
>  		}
>  	}
> @@ -847,6 +849,7 @@ static int write_folio_nounlock(struct folio *folio,
>  			      ceph_vinop(inode), folio);
>  			folio_redirty_for_writepage(wbc, folio);
>  			folio_end_writeback(folio);
> +			atomic_long_dec_return(&fsc->writeback_count);

Ditto. Especially, it is not clear why atomic_long_dec_return() was used instead
of atomic_long_dec() if we don't follow the above-mentioned pattern.

Thanks,
Slava.

>  			return err;
>  		}
>  		if (err == -EBLOCKLISTED)