[PATCH] btrfs: Use refcount_t instead of atomic_t for mmap_count

[PATCH] btrfs: Use refcount_t instead of atomic_t for mmap_count

[Bo Liu]

Use an API that resembles more the actual use of mmap_count.
Found by cocci:
fs/btrfs/bio.c:153:5-24: WARNING: atomic_dec_and_test variation before object free at line 155

Signed-off-by: Bo Liu <liubo03@inspur.com>
---
 fs/btrfs/bio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index a3ee9a976f6f..353c61936cd6 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -23,7 +23,7 @@ static mempool_t btrfs_failed_bio_pool;
 struct btrfs_failed_bio {
 	struct btrfs_bio *bbio;
 	int num_copies;
-	atomic_t repair_count;
+	refcount_t repair_count;
 };
 
 /* Is this a data path I/O that needs storage layer checksum and repair? */
@@ -150,7 +150,7 @@ static int prev_repair_mirror(struct btrfs_failed_bio *fbio, int cur_mirror)
 
 static void btrfs_repair_done(struct btrfs_failed_bio *fbio)
 {
-	if (atomic_dec_and_test(&fbio->repair_count)) {
+	if (refcount_dec_and_test(&fbio->repair_count)) {
 		btrfs_bio_end_io(fbio->bbio, fbio->bbio->bio.bi_status);
 		mempool_free(fbio, &btrfs_failed_bio_pool);
 	}
@@ -235,10 +235,10 @@ static struct btrfs_failed_bio *repair_one_sector(struct btrfs_bio *failed_bbio,
 		fbio = mempool_alloc(&btrfs_failed_bio_pool, GFP_NOFS);
 		fbio->bbio = failed_bbio;
 		fbio->num_copies = num_copies;
-		atomic_set(&fbio->repair_count, 1);
+		refcount_set(&fbio->repair_count, 1);
 	}
 
-	atomic_inc(&fbio->repair_count);
+	refcount_inc(&fbio->repair_count);
 
 	repair_bio = bio_alloc_bioset(NULL, 1, REQ_OP_READ, GFP_NOFS,
 				      &btrfs_repair_bioset);
-- 
2.31.1

Re: [PATCH] btrfs: Use refcount_t instead of atomic_t for mmap_count

[Qu Wenruo]

在 2025/4/29 16:50, Bo Liu 写道:
> Use an API that resembles more the actual use of mmap_count.
> Found by cocci:
> fs/btrfs/bio.c:153:5-24: WARNING: atomic_dec_and_test variation before object free at line 155

Please explain this better, I didn't see anything wrong about the 
decreasing the atomic to zero then freeing it.

Thanks,
Qu>
> Signed-off-by: Bo Liu <liubo03@inspur.com>
> ---
>   fs/btrfs/bio.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index a3ee9a976f6f..353c61936cd6 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -23,7 +23,7 @@ static mempool_t btrfs_failed_bio_pool;
>   struct btrfs_failed_bio {
>   	struct btrfs_bio *bbio;
>   	int num_copies;
> -	atomic_t repair_count;
> +	refcount_t repair_count;
>   };
>   
>   /* Is this a data path I/O that needs storage layer checksum and repair? */
> @@ -150,7 +150,7 @@ static int prev_repair_mirror(struct btrfs_failed_bio *fbio, int cur_mirror)
>   
>   static void btrfs_repair_done(struct btrfs_failed_bio *fbio)
>   {
> -	if (atomic_dec_and_test(&fbio->repair_count)) {
> +	if (refcount_dec_and_test(&fbio->repair_count)) {
>   		btrfs_bio_end_io(fbio->bbio, fbio->bbio->bio.bi_status);
>   		mempool_free(fbio, &btrfs_failed_bio_pool);
>   	}
> @@ -235,10 +235,10 @@ static struct btrfs_failed_bio *repair_one_sector(struct btrfs_bio *failed_bbio,
>   		fbio = mempool_alloc(&btrfs_failed_bio_pool, GFP_NOFS);
>   		fbio->bbio = failed_bbio;
>   		fbio->num_copies = num_copies;
> -		atomic_set(&fbio->repair_count, 1);
> +		refcount_set(&fbio->repair_count, 1);
>   	}
>   
> -	atomic_inc(&fbio->repair_count);
> +	refcount_inc(&fbio->repair_count);
>   
>   	repair_bio = bio_alloc_bioset(NULL, 1, REQ_OP_READ, GFP_NOFS,
>   				      &btrfs_repair_bioset);

Re: [PATCH] btrfs: Use refcount_t instead of atomic_t for mmap_count

[David Sterba]

On Tue, Apr 29, 2025 at 05:12:27PM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/4/29 16:50, Bo Liu 写道:
> > Use an API that resembles more the actual use of mmap_count.
> > Found by cocci:
> > fs/btrfs/bio.c:153:5-24: WARNING: atomic_dec_and_test variation before object free at line 155
> 
> Please explain this better, I didn't see anything wrong about the 
> decreasing the atomic to zero then freeing it.

Yeah, we'd need better explanation for that.  The refcount can catch
underflow, which I'm not expecting to happen for the repair bio as it's
set up, sent and the endio is done. We might assert that the
repair_count is 0 right before freeing the bios, but otherwise the
repair_count does not match a refcount semantics.