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

[Peter Zijlstra <peterz@infradead.org>]

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.