[v2,2/2] btrfs: simplify waiting for encoded read endios

Commit Message

Johannes Thumshirn Nov. 12, 2024, 1:53 p.m. UTC

From: Johannes Thumshirn <[e-mail]>

Simplify the I/O completion path for encoded reads by using a
completion instead of a wait_queue.

Furthermore skip taking an extra reference that is instantly
dropped anyways and convert btrfs_encoded_read_private::pending into a
refcount_t filed instead of atomic_t.

Freeing of the private data is now handled at a common place in
btrfs_encoded_read_regular_fill_pages() and if btrfs_encoded_read_endio()
is freeing the data in case it has an io_uring context associated, the
btrfs_bio's private filed is NULLed to avoid a double free of the private
data.

Suggested-by: Damien Le Moal <[e-mail]>
Signed-off-by: Johannes Thumshirn <[e-mail]>
---
 fs/btrfs/inode.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

Comments

Filipe Manana Nov. 12, 2024, 2:57 p.m. UTC | #1

On Tue, Nov 12, 2024 at 1:54 PM Johannes Thumshirn <[e-mail]> wrote:
>
> From: Johannes Thumshirn <[e-mail]>
>
> Simplify the I/O completion path for encoded reads by using a
> completion instead of a wait_queue.
>
> Furthermore skip taking an extra reference that is instantly
> dropped anyways and convert btrfs_encoded_read_private::pending into a
> refcount_t filed instead of atomic_t.
>
> Freeing of the private data is now handled at a common place in
> btrfs_encoded_read_regular_fill_pages() and if btrfs_encoded_read_endio()
> is freeing the data in case it has an io_uring context associated, the
> btrfs_bio's private filed is NULLed to avoid a double free of the private
> data.
>
> Suggested-by: Damien Le Moal <[e-mail]>
> Signed-off-by: Johannes Thumshirn <[e-mail]>
> ---
>  fs/btrfs/inode.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index cb8b23a3e56b..3093905364e5 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9068,9 +9068,9 @@ static ssize_t btrfs_encoded_read_inline(
>  }
>
>  struct btrfs_encoded_read_private {
> -       wait_queue_head_t wait;
> +       struct completion done;
>         void *uring_ctx;
> -       atomic_t pending;
> +       refcount_t refs;
>         blk_status_t status;
>  };
>
> @@ -9090,14 +9090,15 @@ static void btrfs_encoded_read_endio(struct btrfs_bio *bbio)
>                 WRITE_ONCE(priv->status, bbio->bio.bi_status);
>         }
>         bio_put(&bbio->bio);
> -       if (atomic_dec_and_test(&priv->pending)) {
> +       if (refcount_dec_and_test(&priv->refs)) {
>                 int err = blk_status_to_errno(READ_ONCE(priv->status));
>
>                 if (priv->uring_ctx) {
>                         btrfs_uring_read_extent_endio(priv->uring_ctx, err);
> +                       bbio->private = NULL;

Isn't this racy?

We decremented priv->refs to 0, the task at
btrfs_encoded_read_regular_fill_pages() sees it as 0 and sees
bbio->private still as non-NULL, does a kfree() on it and then we do
it here again.

I.e. we should set bbio->private to NULL before decrementing, and
possibly some barriers.

Thanks.


>                         kfree(priv);
>                 } else {
> -                       wake_up(&priv->wait);
> +                       complete(&priv->done);
>                 }
>         }
>  }
> @@ -9116,8 +9117,8 @@ int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
>         if (!priv)
>                 return -ENOMEM;
>
> -       init_waitqueue_head(&priv->wait);
> -       atomic_set(&priv->pending, 1);
> +       init_completion(&priv->done);
> +       refcount_set(&priv->refs, 1);
>         priv->status = 0;
>         priv->uring_ctx = uring_ctx;
>
> @@ -9130,7 +9131,7 @@ int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
>                 size_t bytes = min_t(u64, disk_io_size, PAGE_SIZE);
>
>                 if (bio_add_page(&bbio->bio, pages[i], bytes, 0) < bytes) {
> -                       atomic_inc(&priv->pending);
> +                       refcount_inc(&priv->refs);
>                         btrfs_submit_bbio(bbio, 0);
>
>                         bbio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, fs_info,
> @@ -9145,26 +9146,26 @@ int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
>                 disk_io_size -= bytes;
>         } while (disk_io_size);
>
> -       atomic_inc(&priv->pending);
>         btrfs_submit_bbio(bbio, 0);
>
>         if (uring_ctx) {
> -               if (atomic_dec_return(&priv->pending) == 0) {
> +               if (bbio->private && refcount_read(&priv->refs) == 0) {
>                         ret = blk_status_to_errno(READ_ONCE(priv->status));
>                         btrfs_uring_read_extent_endio(uring_ctx, ret);
> -                       kfree(priv);
> -                       return ret;
> +                       goto out;
>                 }
>
>                 return -EIOCBQUEUED;
>         } else {
> -               if (atomic_dec_return(&priv->pending) != 0)
> -                       io_wait_event(priv->wait, !atomic_read(&priv->pending));
> +               wait_for_completion_io(&priv->done);
>                 /* See btrfs_encoded_read_endio() for ordering. */
>                 ret = blk_status_to_errno(READ_ONCE(priv->status));
> -               kfree(priv);
> -               return ret;
>         }
> +
> +out:
> +       kfree(priv);
> +       return ret;
> +
>  }
>
>  ssize_t btrfs_encoded_read_regular(struct kiocb *iocb, struct iov_iter *iter,
> --
> 2.43.0
>
>

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index cb8b23a3e56b..3093905364e5 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9068,9 +9068,9 @@  static ssize_t btrfs_encoded_read_inline(
 }
 
 struct btrfs_encoded_read_private {
-	wait_queue_head_t wait;
+	struct completion done;
 	void *uring_ctx;
-	atomic_t pending;
+	refcount_t refs;
 	blk_status_t status;
 };
 
@@ -9090,14 +9090,15 @@  static void btrfs_encoded_read_endio(struct btrfs_bio *bbio)
 		WRITE_ONCE(priv->status, bbio->bio.bi_status);
 	}
 	bio_put(&bbio->bio);
-	if (atomic_dec_and_test(&priv->pending)) {
+	if (refcount_dec_and_test(&priv->refs)) {
 		int err = blk_status_to_errno(READ_ONCE(priv->status));
 
 		if (priv->uring_ctx) {
 			btrfs_uring_read_extent_endio(priv->uring_ctx, err);
+			bbio->private = NULL;
 			kfree(priv);
 		} else {
-			wake_up(&priv->wait);
+			complete(&priv->done);
 		}
 	}
 }
@@ -9116,8 +9117,8 @@  int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
 	if (!priv)
 		return -ENOMEM;
 
-	init_waitqueue_head(&priv->wait);
-	atomic_set(&priv->pending, 1);
+	init_completion(&priv->done);
+	refcount_set(&priv->refs, 1);
 	priv->status = 0;
 	priv->uring_ctx = uring_ctx;
 
@@ -9130,7 +9131,7 @@  int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
 		size_t bytes = min_t(u64, disk_io_size, PAGE_SIZE);
 
 		if (bio_add_page(&bbio->bio, pages[i], bytes, 0) < bytes) {
-			atomic_inc(&priv->pending);
+			refcount_inc(&priv->refs);
 			btrfs_submit_bbio(bbio, 0);
 
 			bbio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, fs_info,
@@ -9145,26 +9146,26 @@  int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode,
 		disk_io_size -= bytes;
 	} while (disk_io_size);
 
-	atomic_inc(&priv->pending);
 	btrfs_submit_bbio(bbio, 0);
 
 	if (uring_ctx) {
-		if (atomic_dec_return(&priv->pending) == 0) {
+		if (bbio->private && refcount_read(&priv->refs) == 0) {
 			ret = blk_status_to_errno(READ_ONCE(priv->status));
 			btrfs_uring_read_extent_endio(uring_ctx, ret);
-			kfree(priv);
-			return ret;
+			goto out;
 		}
 
 		return -EIOCBQUEUED;
 	} else {
-		if (atomic_dec_return(&priv->pending) != 0)
-			io_wait_event(priv->wait, !atomic_read(&priv->pending));
+		wait_for_completion_io(&priv->done);
 		/* See btrfs_encoded_read_endio() for ordering. */
 		ret = blk_status_to_errno(READ_ONCE(priv->status));
-		kfree(priv);
-		return ret;
 	}
+
+out:
+	kfree(priv);
+	return ret;
+
 }
 
 ssize_t btrfs_encoded_read_regular(struct kiocb *iocb, struct iov_iter *iter,