[PATCH] block: ensure the memory order between bi_private and bi_status

[PATCH] block: ensure the memory order between bi_private and bi_status

[Hou Tao]

When running stress test on null_blk under linux-4.19.y, the following
warning is reported:

  percpu_ref_switch_to_atomic_rcu: percpu ref (css_release) <= 0 (-3) after switching to atomic

The cause is that css_put() is invoked twice on the same bio as shown below:

CPU 1:                         CPU 2:

// IO completion kworker       // IO submit thread
                               __blkdev_direct_IO_simple
                                 submit_bio

bio_endio
  bio_uninit(bio)
    css_put(bi_css)
    bi_css = NULL
                               set_current_state(TASK_UNINTERRUPTIBLE)
  bio->bi_end_io
    blkdev_bio_end_io_simple
      bio->bi_private = NULL
                               // bi_private is NULL
                               READ_ONCE(bio->bi_private)
        wake_up_process
          smp_mb__after_spinlock

                               bio_unint(bio)
                                 // read bi_css as no-NULL
                                 // so call css_put() again
                                 css_put(bi_css)

Because there is no memory barriers between the reading and the writing of
bi_private and bi_css, so reading bi_private as NULL can not guarantee
bi_css will also be NULL on weak-memory model host (e.g, ARM64).

For the latest kernel source, css_put() has been removed from bio_unint(),
but the memory-order problem still exists, because the order between
bio->bi_private and {bi_status|bi_blkg} is also assumed in
__blkdev_direct_IO_simple(). It is reproducible that
__blkdev_direct_IO_simple() may read bi_status as 0 event if
bi_status is set as an errno in req_bio_endio().

In __blkdev_direct_IO(), the memory order between dio->waiter and
dio->bio.bi_status is not guaranteed neither. Until now it is unable to
reproduce it, maybe because dio->waiter and dio->bio.bi_status are
in the same cache-line. But it is better to add guarantee for memory
order.

Fixing it by using smp_load_acquire() & smp_store_release() to guarantee
the order between {bio->bi_private|dio->waiter} and {bi_status|bi_blkg}.

Fixes: 189ce2b9dcc3 ("block: fast-path for small and simple direct I/O requests")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 fs/block_dev.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index eb34f5c357cf..a602c6315b0b 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -224,7 +224,11 @@ static void blkdev_bio_end_io_simple(struct bio *bio)
 {
 	struct task_struct *waiter = bio->bi_private;
 
-	WRITE_ONCE(bio->bi_private, NULL);
+	/*
+	 * Paired with smp_load_acquire in __blkdev_direct_IO_simple()
+	 * to ensure the order between bi_private and bi_xxx
+	 */
+	smp_store_release(&bio->bi_private, NULL);
 	blk_wake_io_task(waiter);
 }
 
@@ -283,7 +287,8 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
 	qc = submit_bio(&bio);
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
-		if (!READ_ONCE(bio.bi_private))
+		/* Refer to comments in blkdev_bio_end_io_simple() */
+		if (!smp_load_acquire(&bio.bi_private))
 			break;
 		if (!(iocb->ki_flags & IOCB_HIPRI) ||
 		    !blk_poll(bdev_get_queue(bdev), qc, true))
@@ -353,7 +358,12 @@ static void blkdev_bio_end_io(struct bio *bio)
 		} else {
 			struct task_struct *waiter = dio->waiter;
 
-			WRITE_ONCE(dio->waiter, NULL);
+			/*
+			 * Paired with smp_load_acquire() in
+			 * __blkdev_direct_IO() to ensure the order between
+			 * dio->waiter and bio->bi_xxx
+			 */
+			smp_store_release(&dio->waiter, NULL);
 			blk_wake_io_task(waiter);
 		}
 	}
@@ -478,7 +488,8 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
-		if (!READ_ONCE(dio->waiter))
+		/* Refer to comments in blkdev_bio_end_io */
+		if (!smp_load_acquire(&dio->waiter))
 			break;
 
 		if (!(iocb->ki_flags & IOCB_HIPRI) ||
-- 
2.29.2

Re: [PATCH] block: ensure the memory order between bi_private and bi_status

[Hou Tao]

ping ?

On 7/1/2021 7:35 PM, Hou Tao wrote:
> When running stress test on null_blk under linux-4.19.y, the following
> warning is reported:
>
>   percpu_ref_switch_to_atomic_rcu: percpu ref (css_release) <= 0 (-3) after switching to atomic
>
> The cause is that css_put() is invoked twice on the same bio as shown below:
>
> CPU 1:                         CPU 2:
>
> // IO completion kworker       // IO submit thread
>                                __blkdev_direct_IO_simple
>                                  submit_bio
>
> bio_endio
>   bio_uninit(bio)
>     css_put(bi_css)
>     bi_css = NULL
>                                set_current_state(TASK_UNINTERRUPTIBLE)
>   bio->bi_end_io
>     blkdev_bio_end_io_simple
>       bio->bi_private = NULL
>                                // bi_private is NULL
>                                READ_ONCE(bio->bi_private)
>         wake_up_process
>           smp_mb__after_spinlock
>
>                                bio_unint(bio)
>                                  // read bi_css as no-NULL
>                                  // so call css_put() again
>                                  css_put(bi_css)
>
> Because there is no memory barriers between the reading and the writing of
> bi_private and bi_css, so reading bi_private as NULL can not guarantee
> bi_css will also be NULL on weak-memory model host (e.g, ARM64).
>
> For the latest kernel source, css_put() has been removed from bio_unint(),
> but the memory-order problem still exists, because the order between
> bio->bi_private and {bi_status|bi_blkg} is also assumed in
> __blkdev_direct_IO_simple(). It is reproducible that
> __blkdev_direct_IO_simple() may read bi_status as 0 event if
> bi_status is set as an errno in req_bio_endio().
>
> In __blkdev_direct_IO(), the memory order between dio->waiter and
> dio->bio.bi_status is not guaranteed neither. Until now it is unable to
> reproduce it, maybe because dio->waiter and dio->bio.bi_status are
> in the same cache-line. But it is better to add guarantee for memory
> order.
>
> Fixing it by using smp_load_acquire() & smp_store_release() to guarantee
> the order between {bio->bi_private|dio->waiter} and {bi_status|bi_blkg}.
>
> Fixes: 189ce2b9dcc3 ("block: fast-path for small and simple direct I/O requests")
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  fs/block_dev.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index eb34f5c357cf..a602c6315b0b 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -224,7 +224,11 @@ static void blkdev_bio_end_io_simple(struct bio *bio)
>  {
>  	struct task_struct *waiter = bio->bi_private;
>  
> -	WRITE_ONCE(bio->bi_private, NULL);
> +	/*
> +	 * Paired with smp_load_acquire in __blkdev_direct_IO_simple()
> +	 * to ensure the order between bi_private and bi_xxx
> +	 */
> +	smp_store_release(&bio->bi_private, NULL);
>  	blk_wake_io_task(waiter);
>  }
>  
> @@ -283,7 +287,8 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
>  	qc = submit_bio(&bio);
>  	for (;;) {
>  		set_current_state(TASK_UNINTERRUPTIBLE);
> -		if (!READ_ONCE(bio.bi_private))
> +		/* Refer to comments in blkdev_bio_end_io_simple() */
> +		if (!smp_load_acquire(&bio.bi_private))
>  			break;
>  		if (!(iocb->ki_flags & IOCB_HIPRI) ||
>  		    !blk_poll(bdev_get_queue(bdev), qc, true))
> @@ -353,7 +358,12 @@ static void blkdev_bio_end_io(struct bio *bio)
>  		} else {
>  			struct task_struct *waiter = dio->waiter;
>  
> -			WRITE_ONCE(dio->waiter, NULL);
> +			/*
> +			 * Paired with smp_load_acquire() in
> +			 * __blkdev_direct_IO() to ensure the order between
> +			 * dio->waiter and bio->bi_xxx
> +			 */
> +			smp_store_release(&dio->waiter, NULL);
>  			blk_wake_io_task(waiter);
>  		}
>  	}
> @@ -478,7 +488,8 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
>  
>  	for (;;) {
>  		set_current_state(TASK_UNINTERRUPTIBLE);
> -		if (!READ_ONCE(dio->waiter))
> +		/* Refer to comments in blkdev_bio_end_io */
> +		if (!smp_load_acquire(&dio->waiter))
>  			break;
>  
>  		if (!(iocb->ki_flags & IOCB_HIPRI) ||

Re: [PATCH] block: ensure the memory order between bi_private and bi_status

[Hou Tao]

ping ?

On 7/7/2021 2:29 PM, Hou Tao wrote:
> ping ?
>
> On 7/1/2021 7:35 PM, Hou Tao wrote:
>> When running stress test on null_blk under linux-4.19.y, the following
>> warning is reported:
>>
>>   percpu_ref_switch_to_atomic_rcu: percpu ref (css_release) <= 0 (-3) after switching to atomic
>>
>> The cause is that css_put() is invoked twice on the same bio as shown below:
>>
>> CPU 1:                         CPU 2:
>>
>> // IO completion kworker       // IO submit thread
>>                                __blkdev_direct_IO_simple
>>                                  submit_bio
>>
>> bio_endio
>>   bio_uninit(bio)
>>     css_put(bi_css)
>>     bi_css = NULL
>>                                set_current_state(TASK_UNINTERRUPTIBLE)
>>   bio->bi_end_io
>>     blkdev_bio_end_io_simple
>>       bio->bi_private = NULL
>>                                // bi_private is NULL
>>                                READ_ONCE(bio->bi_private)
>>         wake_up_process
>>           smp_mb__after_spinlock
>>
>>                                bio_unint(bio)
>>                                  // read bi_css as no-NULL
>>                                  // so call css_put() again
>>                                  css_put(bi_css)
>>
>> Because there is no memory barriers between the reading and the writing of
>> bi_private and bi_css, so reading bi_private as NULL can not guarantee
>> bi_css will also be NULL on weak-memory model host (e.g, ARM64).
>>
>> For the latest kernel source, css_put() has been removed from bio_unint(),
>> but the memory-order problem still exists, because the order between
>> bio->bi_private and {bi_status|bi_blkg} is also assumed in
>> __blkdev_direct_IO_simple(). It is reproducible that
>> __blkdev_direct_IO_simple() may read bi_status as 0 event if
>> bi_status is set as an errno in req_bio_endio().
>>
>> In __blkdev_direct_IO(), the memory order between dio->waiter and
>> dio->bio.bi_status is not guaranteed neither. Until now it is unable to
>> reproduce it, maybe because dio->waiter and dio->bio.bi_status are
>> in the same cache-line. But it is better to add guarantee for memory
>> order.
>>
>> Fixing it by using smp_load_acquire() & smp_store_release() to guarantee
>> the order between {bio->bi_private|dio->waiter} and {bi_status|bi_blkg}.
>>
>> Fixes: 189ce2b9dcc3 ("block: fast-path for small and simple direct I/O requests")
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>  fs/block_dev.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>> index eb34f5c357cf..a602c6315b0b 100644
>> --- a/fs/block_dev.c
>> +++ b/fs/block_dev.c
>> @@ -224,7 +224,11 @@ static void blkdev_bio_end_io_simple(struct bio *bio)
>>  {
>>  	struct task_struct *waiter = bio->bi_private;
>>  
>> -	WRITE_ONCE(bio->bi_private, NULL);
>> +	/*
>> +	 * Paired with smp_load_acquire in __blkdev_direct_IO_simple()
>> +	 * to ensure the order between bi_private and bi_xxx
>> +	 */
>> +	smp_store_release(&bio->bi_private, NULL);
>>  	blk_wake_io_task(waiter);
>>  }
>>  
>> @@ -283,7 +287,8 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
>>  	qc = submit_bio(&bio);
>>  	for (;;) {
>>  		set_current_state(TASK_UNINTERRUPTIBLE);
>> -		if (!READ_ONCE(bio.bi_private))
>> +		/* Refer to comments in blkdev_bio_end_io_simple() */
>> +		if (!smp_load_acquire(&bio.bi_private))
>>  			break;
>>  		if (!(iocb->ki_flags & IOCB_HIPRI) ||
>>  		    !blk_poll(bdev_get_queue(bdev), qc, true))
>> @@ -353,7 +358,12 @@ static void blkdev_bio_end_io(struct bio *bio)
>>  		} else {
>>  			struct task_struct *waiter = dio->waiter;
>>  
>> -			WRITE_ONCE(dio->waiter, NULL);
>> +			/*
>> +			 * Paired with smp_load_acquire() in
>> +			 * __blkdev_direct_IO() to ensure the order between
>> +			 * dio->waiter and bio->bi_xxx
>> +			 */
>> +			smp_store_release(&dio->waiter, NULL);
>>  			blk_wake_io_task(waiter);
>>  		}
>>  	}
>> @@ -478,7 +488,8 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
>>  
>>  	for (;;) {
>>  		set_current_state(TASK_UNINTERRUPTIBLE);
>> -		if (!READ_ONCE(dio->waiter))
>> +		/* Refer to comments in blkdev_bio_end_io */
>> +		if (!smp_load_acquire(&dio->waiter))
>>  			break;
>>  
>>  		if (!(iocb->ki_flags & IOCB_HIPRI) ||
> .

Re: [PATCH] block: ensure the memory order between bi_private and bi_status

[Christoph Hellwig]

On Thu, Jul 01, 2021 at 07:35:37PM +0800, Hou Tao wrote:
> When running stress test on null_blk under linux-4.19.y, the following
> warning is reported:
> 
>   percpu_ref_switch_to_atomic_rcu: percpu ref (css_release) <= 0 (-3) after switching to atomic
> 
> The cause is that css_put() is invoked twice on the same bio as shown below:
> 
> CPU 1:                         CPU 2:
> 
> // IO completion kworker       // IO submit thread
>                                __blkdev_direct_IO_simple
>                                  submit_bio
> 
> bio_endio
>   bio_uninit(bio)
>     css_put(bi_css)
>     bi_css = NULL
>                                set_current_state(TASK_UNINTERRUPTIBLE)
>   bio->bi_end_io
>     blkdev_bio_end_io_simple
>       bio->bi_private = NULL
>                                // bi_private is NULL
>                                READ_ONCE(bio->bi_private)
>         wake_up_process
>           smp_mb__after_spinlock
> 
>                                bio_unint(bio)
>                                  // read bi_css as no-NULL
>                                  // so call css_put() again
>                                  css_put(bi_css)
> 
> Because there is no memory barriers between the reading and the writing of
> bi_private and bi_css, so reading bi_private as NULL can not guarantee
> bi_css will also be NULL on weak-memory model host (e.g, ARM64).
> 
> For the latest kernel source, css_put() has been removed from bio_unint(),
> but the memory-order problem still exists, because the order between
> bio->bi_private and {bi_status|bi_blkg} is also assumed in
> __blkdev_direct_IO_simple(). It is reproducible that
> __blkdev_direct_IO_simple() may read bi_status as 0 event if
> bi_status is set as an errno in req_bio_endio().
> 
> In __blkdev_direct_IO(), the memory order between dio->waiter and
> dio->bio.bi_status is not guaranteed neither. Until now it is unable to
> reproduce it, maybe because dio->waiter and dio->bio.bi_status are
> in the same cache-line. But it is better to add guarantee for memory
> order.
> 
> Fixing it by using smp_load_acquire() & smp_store_release() to guarantee
> the order between {bio->bi_private|dio->waiter} and {bi_status|bi_blkg}.
> 
> Fixes: 189ce2b9dcc3 ("block: fast-path for small and simple direct I/O requests")

This obviously does not look broken, but smp_load_acquire /
smp_store_release is way beyond my paygrade.  Adding some CCs.

> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  fs/block_dev.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index eb34f5c357cf..a602c6315b0b 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -224,7 +224,11 @@ static void blkdev_bio_end_io_simple(struct bio *bio)
>  {
>  	struct task_struct *waiter = bio->bi_private;
>  
> -	WRITE_ONCE(bio->bi_private, NULL);
> +	/*
> +	 * Paired with smp_load_acquire in __blkdev_direct_IO_simple()
> +	 * to ensure the order between bi_private and bi_xxx
> +	 */
> +	smp_store_release(&bio->bi_private, NULL);
>  	blk_wake_io_task(waiter);
>  }
>  
> @@ -283,7 +287,8 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
>  	qc = submit_bio(&bio);
>  	for (;;) {
>  		set_current_state(TASK_UNINTERRUPTIBLE);
> -		if (!READ_ONCE(bio.bi_private))
> +		/* Refer to comments in blkdev_bio_end_io_simple() */
> +		if (!smp_load_acquire(&bio.bi_private))
>  			break;
>  		if (!(iocb->ki_flags & IOCB_HIPRI) ||
>  		    !blk_poll(bdev_get_queue(bdev), qc, true))
> @@ -353,7 +358,12 @@ static void blkdev_bio_end_io(struct bio *bio)
>  		} else {
>  			struct task_struct *waiter = dio->waiter;
>  
> -			WRITE_ONCE(dio->waiter, NULL);
> +			/*
> +			 * Paired with smp_load_acquire() in
> +			 * __blkdev_direct_IO() to ensure the order between
> +			 * dio->waiter and bio->bi_xxx
> +			 */
> +			smp_store_release(&dio->waiter, NULL);
>  			blk_wake_io_task(waiter);
>  		}
>  	}
> @@ -478,7 +488,8 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
>  
>  	for (;;) {
>  		set_current_state(TASK_UNINTERRUPTIBLE);
> -		if (!READ_ONCE(dio->waiter))
> +		/* Refer to comments in blkdev_bio_end_io */
> +		if (!smp_load_acquire(&dio->waiter))
>  			break;
>  
>  		if (!(iocb->ki_flags & IOCB_HIPRI) ||
> -- 
> 2.29.2
---end quoted text---

Re: [PATCH] block: ensure the memory order between bi_private and bi_status

[Peter Zijlstra]

On Thu, Jul 15, 2021 at 09:01:48AM +0200, Christoph Hellwig wrote:
> On Thu, Jul 01, 2021 at 07:35:37PM +0800, Hou Tao wrote:
> > When running stress test on null_blk under linux-4.19.y, the following
> > warning is reported:
> > 
> >   percpu_ref_switch_to_atomic_rcu: percpu ref (css_release) <= 0 (-3) after switching to atomic
> > 
> > The cause is that css_put() is invoked twice on the same bio as shown below:
> > 
> > CPU 1:                         CPU 2:
> > 
> > // IO completion kworker       // IO submit thread
> >                                __blkdev_direct_IO_simple
> >                                  submit_bio
> > 
> > bio_endio
> >   bio_uninit(bio)
> >     css_put(bi_css)
> >     bi_css = NULL
> >                                set_current_state(TASK_UNINTERRUPTIBLE)
> >   bio->bi_end_io
> >     blkdev_bio_end_io_simple
> >       bio->bi_private = NULL
> >                                // bi_private is NULL
> >                                READ_ONCE(bio->bi_private)
> >         wake_up_process
> >           smp_mb__after_spinlock
> > 
> >                                bio_unint(bio)
> >                                  // read bi_css as no-NULL
> >                                  // so call css_put() again
> >                                  css_put(bi_css)
> > 
> > Because there is no memory barriers between the reading and the writing of
> > bi_private and bi_css, so reading bi_private as NULL can not guarantee
> > bi_css will also be NULL on weak-memory model host (e.g, ARM64).
> > 
> > For the latest kernel source, css_put() has been removed from bio_unint(),
> > but the memory-order problem still exists, because the order between
> > bio->bi_private and {bi_status|bi_blkg} is also assumed in
> > __blkdev_direct_IO_simple(). It is reproducible that
> > __blkdev_direct_IO_simple() may read bi_status as 0 event if
> > bi_status is set as an errno in req_bio_endio().
> > 
> > In __blkdev_direct_IO(), the memory order between dio->waiter and
> > dio->bio.bi_status is not guaranteed neither. Until now it is unable to
> > reproduce it, maybe because dio->waiter and dio->bio.bi_status are
> > in the same cache-line. But it is better to add guarantee for memory
> > order.

Cachelines don't guarantee anything, you can get partial forwards.

> > Fixing it by using smp_load_acquire() & smp_store_release() to guarantee
> > the order between {bio->bi_private|dio->waiter} and {bi_status|bi_blkg}.
> > 
> > Fixes: 189ce2b9dcc3 ("block: fast-path for small and simple direct I/O requests")
> 
> This obviously does not look broken, but smp_load_acquire /
> smp_store_release is way beyond my paygrade.  Adding some CCs.

This block stuff is a bit beyond me, lets see if we can make sense of
it.

> > Signed-off-by: Hou Tao <houtao1@huawei.com>
> > ---
> >  fs/block_dev.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/block_dev.c b/fs/block_dev.c
> > index eb34f5c357cf..a602c6315b0b 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -224,7 +224,11 @@ static void blkdev_bio_end_io_simple(struct bio *bio)
> >  {
> >  	struct task_struct *waiter = bio->bi_private;
> >  
> > -	WRITE_ONCE(bio->bi_private, NULL);
> > +	/*
> > +	 * Paired with smp_load_acquire in __blkdev_direct_IO_simple()
> > +	 * to ensure the order between bi_private and bi_xxx
> > +	 */

This comment doesn't help me; where are the other stores? Presumably
somewhere before this is called, but how does one go about finding them?

The Changelog seems to suggest you only care about bi_css, not bi_xxx in
general. In specific you can only care about stores that happen before
this; is all of bi_xxx written before here? If not, you have to be more
specific.

Also, this being a clear, this very much isn't the typical publish
pattern.

On top of all that, smp_wmb() would be sufficient here and would be
cheaper on some platforms (ARM comes to mind).

> > +	smp_store_release(&bio->bi_private, NULL);
> >  	blk_wake_io_task(waiter);
> >  }
> >  
> > @@ -283,7 +287,8 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
> >  	qc = submit_bio(&bio);
> >  	for (;;) {
> >  		set_current_state(TASK_UNINTERRUPTIBLE);
> > -		if (!READ_ONCE(bio.bi_private))
> > +		/* Refer to comments in blkdev_bio_end_io_simple() */
> > +		if (!smp_load_acquire(&bio.bi_private))
> >  			break;
> >  		if (!(iocb->ki_flags & IOCB_HIPRI) ||
> >  		    !blk_poll(bdev_get_queue(bdev), qc, true))

That comment there doesn't help me find any relevant later loads and is
thus again inadequate.

Here the purpose seems to be to ensure the bi_css load happens after the
bi_private load, and this again is cheaper done using smp_rmb().

Also, the implication seems to be -- but is not spelled out anywhere --
that if bi_private is !NULL, it is stable.

> > @@ -353,7 +358,12 @@ static void blkdev_bio_end_io(struct bio *bio)
> >  		} else {
> >  			struct task_struct *waiter = dio->waiter;
> >  
> > -			WRITE_ONCE(dio->waiter, NULL);
> > +			/*
> > +			 * Paired with smp_load_acquire() in
> > +			 * __blkdev_direct_IO() to ensure the order between
> > +			 * dio->waiter and bio->bi_xxx
> > +			 */
> > +			smp_store_release(&dio->waiter, NULL);
> >  			blk_wake_io_task(waiter);
> >  		}
> >  	}
> > @@ -478,7 +488,8 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
> >  
> >  	for (;;) {
> >  		set_current_state(TASK_UNINTERRUPTIBLE);
> > -		if (!READ_ONCE(dio->waiter))
> > +		/* Refer to comments in blkdev_bio_end_io */
> > +		if (!smp_load_acquire(&dio->waiter))
> >  			break;
> >  
> >  		if (!(iocb->ki_flags & IOCB_HIPRI) ||

Idem for these...

Re: [PATCH] block: ensure the memory order between bi_private and bi_status

[Hou Tao]

Hi Peter,

On 7/15/2021 4:13 PM, Peter Zijlstra wrote:
> On Thu, Jul 15, 2021 at 09:01:48AM +0200, Christoph Hellwig wrote:
>> On Thu, Jul 01, 2021 at 07:35:37PM +0800, Hou Tao wrote:
>>> When running stress test on null_blk under linux-4.19.y, the following
>>> warning is reported:
>>>
>>>   percpu_ref_switch_to_atomic_rcu: percpu ref (css_release) <= 0 (-3) after switching to atomic
>>>
snip
>>> In __blkdev_direct_IO(), the memory order between dio->waiter and
>>> dio->bio.bi_status is not guaranteed neither. Until now it is unable to
>>> reproduce it, maybe because dio->waiter and dio->bio.bi_status are
>>> in the same cache-line. But it is better to add guarantee for memory
>>> order.
> Cachelines don't guarantee anything, you can get partial forwards.

Could you please point me to any reference ? I can not google

any memory order things by using "partial forwards".

>
>>> Fixing it by using smp_load_acquire() & smp_store_release() to guarantee
>>> the order between {bio->bi_private|dio->waiter} and {bi_status|bi_blkg}.
>>>
>>> Fixes: 189ce2b9dcc3 ("block: fast-path for small and simple direct I/O requests")
>> This obviously does not look broken, but smp_load_acquire /
>> smp_store_release is way beyond my paygrade.  Adding some CCs.
> This block stuff is a bit beyond me, lets see if we can make sense of
> it.
>
>>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>>> ---
>>>  fs/block_dev.c | 19 +++++++++++++++----
>>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>>> index eb34f5c357cf..a602c6315b0b 100644
>>> --- a/fs/block_dev.c
>>> +++ b/fs/block_dev.c
>>> @@ -224,7 +224,11 @@ static void blkdev_bio_end_io_simple(struct bio *bio)
>>>  {
>>>  	struct task_struct *waiter = bio->bi_private;
>>>  
>>> -	WRITE_ONCE(bio->bi_private, NULL);
>>> +	/*
>>> +	 * Paired with smp_load_acquire in __blkdev_direct_IO_simple()
>>> +	 * to ensure the order between bi_private and bi_xxx
>>> +	 */
> This comment doesn't help me; where are the other stores? Presumably
> somewhere before this is called, but how does one go about finding them?

Yes, the change log is vague and it will be corrected. The other stores

happen in req_bio_endio() and its callees when the request completes.

> The Changelog seems to suggest you only care about bi_css, not bi_xxx in
> general. In specific you can only care about stores that happen before
> this; is all of bi_xxx written before here? If not, you have to be more
> specific.

Actually we care about all bi_xxx which are written in req_bio_endio, and all writes

to bi_xxx happen before blkdev_bio_end_io_simple(). Here I just try to

use bi_status as one example.

> Also, this being a clear, this very much isn't the typical publish
> pattern.
>
> On top of all that, smp_wmb() would be sufficient here and would be
> cheaper on some platforms (ARM comes to mind).
Thanks for your knowledge, I will use smp_wmb() instead of smp_store_release().
>
>>> +	smp_store_release(&bio->bi_private, NULL);
>>>  	blk_wake_io_task(waiter);
>>>  }
>>>  
>>> @@ -283,7 +287,8 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
>>>  	qc = submit_bio(&bio);
>>>  	for (;;) {
>>>  		set_current_state(TASK_UNINTERRUPTIBLE);
>>> -		if (!READ_ONCE(bio.bi_private))
>>> +		/* Refer to comments in blkdev_bio_end_io_simple() */
>>> +		if (!smp_load_acquire(&bio.bi_private))
>>>  			break;
>>>  		if (!(iocb->ki_flags & IOCB_HIPRI) ||
>>>  		    !blk_poll(bdev_get_queue(bdev), qc, true))
> That comment there doesn't help me find any relevant later loads and is
> thus again inadequate.
>
> Here the purpose seems to be to ensure the bi_css load happens after the
> bi_private load, and this again is cheaper done using smp_rmb().
Yes and thanks again.
>
> Also, the implication seems to be -- but is not spelled out anywhere --
> that if bi_private is !NULL, it is stable.

What is the meaning of "it is stable" ? Do you mean if bi_private is NULL,

the values of bi_xxx should be ensured ?

>>> @@ -353,7 +358,12 @@ static void blkdev_bio_end_io(struct bio *bio)
>>>  		} else {
>>>  			struct task_struct *waiter = dio->waiter;
>>>  
>>> -			WRITE_ONCE(dio->waiter, NULL);
>>> +			/*
>>> +			 * Paired with smp_load_acquire() in
>>> +			 * __blkdev_direct_IO() to ensure the order between
>>> +			 * dio->waiter and bio->bi_xxx
>>> +			 */
>>> +			smp_store_release(&dio->waiter, NULL);
>>>  			blk_wake_io_task(waiter);
>>>  		}
>>>  	}
>>> @@ -478,7 +488,8 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
>>>  
>>>  	for (;;) {
>>>  		set_current_state(TASK_UNINTERRUPTIBLE);
>>> -		if (!READ_ONCE(dio->waiter))
>>> +		/* Refer to comments in blkdev_bio_end_io */
>>> +		if (!smp_load_acquire(&dio->waiter))
>>>  			break;
>>>  
>>>  		if (!(iocb->ki_flags & IOCB_HIPRI) ||
> Idem for these...
> .

Thanks

Regards,

Tao

Re: [PATCH] block: ensure the memory order between bi_private and bi_status

[Peter Zijlstra]

On Fri, Jul 16, 2021 at 05:02:33PM +0800, Hou Tao wrote:

> > Cachelines don't guarantee anything, you can get partial forwards.
> 
> Could you please point me to any reference ? I can not google
> 
> any memory order things by using "partial forwards".

I'm not sure I have references, but there are CPUs that can do, for
example, store forwarding at a granularity below cachelines (ie at
register size).

In such a case a CPU might observe the stored value before it is
committed to memory.


> >>> @@ -224,7 +224,11 @@ static void blkdev_bio_end_io_simple(struct bio *bio)
> >>>  {
> >>>  	struct task_struct *waiter = bio->bi_private;
> >>>  
> >>> -	WRITE_ONCE(bio->bi_private, NULL);
> >>> +	/*
> >>> +	 * Paired with smp_load_acquire in __blkdev_direct_IO_simple()
> >>> +	 * to ensure the order between bi_private and bi_xxx
> >>> +	 */
> > This comment doesn't help me; where are the other stores? Presumably
> > somewhere before this is called, but how does one go about finding them?
> 
> Yes, the change log is vague and it will be corrected. The other stores
> 
> happen in req_bio_endio() and its callees when the request completes.

Aaah, right. So initially I was wondering if it would make sense to put
the barrier there, but having looked at this a little longer, this
really seems to be about these two DIO methods.

> > The Changelog seems to suggest you only care about bi_css, not bi_xxx in
> > general. In specific you can only care about stores that happen before
> > this; is all of bi_xxx written before here? If not, you have to be more
> > specific.
> 
> Actually we care about all bi_xxx which are written in req_bio_endio,
> and all writes to bi_xxx happen before blkdev_bio_end_io_simple().
> Here I just try to use bi_status as one example.

I see req_bio_endio() change bi_status, bi_flags and bi_iter, but afaict
there's more bi_ fields.

> >>> @@ -283,7 +287,8 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
> >>>  	qc = submit_bio(&bio);
> >>>  	for (;;) {
> >>>  		set_current_state(TASK_UNINTERRUPTIBLE);
> >>> -		if (!READ_ONCE(bio.bi_private))
> >>> +		/* Refer to comments in blkdev_bio_end_io_simple() */
> >>> +		if (!smp_load_acquire(&bio.bi_private))
> >>>  			break;
> >>>  		if (!(iocb->ki_flags & IOCB_HIPRI) ||
> >>>  		    !blk_poll(bdev_get_queue(bdev), qc, true))
> > That comment there doesn't help me find any relevant later loads and is
> > thus again inadequate.
> >
> > Here the purpose seems to be to ensure the bi_css load happens after the
> > bi_private load, and this again is cheaper done using smp_rmb().
> Yes and thanks again.
> >
> > Also, the implication seems to be -- but is not spelled out anywhere --
> > that if bi_private is !NULL, it is stable.
> 
> What is the meaning of "it is stable" ? Do you mean if bi_private is NULL,
> the values of bi_xxx should be ensured ?

With stable I mean that if it is !NULL the value is always the same.

I've read more code and this is indeed the case, specifically, here
bi_private seems to be 'current' and will only be changed to NULL.

Re: [PATCH] block: ensure the memory order between bi_private and bi_status

[Paul E. McKenney]

On Fri, Jul 16, 2021 at 12:19:29PM +0200, Peter Zijlstra wrote:
> On Fri, Jul 16, 2021 at 05:02:33PM +0800, Hou Tao wrote:
> 
> > > Cachelines don't guarantee anything, you can get partial forwards.
> > 
> > Could you please point me to any reference ? I can not google
> > 
> > any memory order things by using "partial forwards".
> 
> I'm not sure I have references, but there are CPUs that can do, for
> example, store forwarding at a granularity below cachelines (ie at
> register size).
> 
> In such a case a CPU might observe the stored value before it is
> committed to memory.

There have been examples of systems with multiple hardware threads
per core, but where the hardware threads share a store buffer.
In this case, the other threads in the core might see a store before
it is committed to a cache line.

As you might imagine, the hardware implementation of memory barriers
in such a system is tricky.  But not impossible.

							Thanx, Paul

Re: [PATCH] block: ensure the memory order between bi_private and bi_status

[Paul E. McKenney]

On Thu, Jul 15, 2021 at 09:01:48AM +0200, Christoph Hellwig wrote:
> On Thu, Jul 01, 2021 at 07:35:37PM +0800, Hou Tao wrote:

[ . . . ]

> > Fixing it by using smp_load_acquire() & smp_store_release() to guarantee
> > the order between {bio->bi_private|dio->waiter} and {bi_status|bi_blkg}.
> > 
> > Fixes: 189ce2b9dcc3 ("block: fast-path for small and simple direct I/O requests")
> 
> This obviously does not look broken, but smp_load_acquire /
> smp_store_release is way beyond my paygrade.  Adding some CCs.

Huh.

I think that it was back in 2006 when I first told Linus that my goal was
to make memory ordering routine.  I clearly have not yet achieved that
goal, even given a lot of help from a lot of people over a lot of years.

Oh well, what is life without an ongoing challenge?  ;-)

							Thanx, Paul