FLUSH/FUA documentation & code discrepancy

FLUSH/FUA documentation & code discrepancy

[Philipp Reisner]

Hi,

I think commit 1e87901e18 was wrong. Starting with that commit the REQ_FLUSH 
and REQ_FUA bits get stripped away if the queue does not advertise REQ_FLUSH
or REQ_FUA support.

But the REQ_FLUSH bit is also tested for when not merging requests
(blk_queue_bio()) or when it comes to the elevator (blk_flush_plug_list()).

So, since this patch the elevator reorders write requests on queues that 
do not have REQ_FLUSH or REQ_FUA set.

While on queues that have REQ_FLUSH or REQ_FUA set, the elevator does
not reorder writes across FLUSHes.

The Documentation/block/writeback_cache_control.txt file says:
--snip--
Implementation details for filesystems
--------------------------------------

Filesystems can simply set the REQ_FLUSH and REQ_FUA bits and do not have to
worry if the underlying devices need any explicit cache flushing and how
the Forced Unit Access is implemented.  The REQ_FLUSH and REQ_FUA flags
may both be set on a single bio.
--snap--

I have the impression every file system lets IO drain, and issues a
flush afterwards with the blkdev_issue_flush() function. BTW that
function turns into a non-obvious no-op as soon as the queue does not
have the REQ_FUA or REQ_FLUSH bits set. It does not look like it is
a no-op by intention.

The file systems seem to be all fine, only in DRBD we have a mode were
we depend on REQ_FUA/REQ_FLUSH requests being real boundaries for reordering
of writes. This is broken since the mentioned commit as we recently found out.

I suggest that either this commit gets reverted, or the documentation
is updated. I am ready to prepare such a patch, but I need directions
how it should be fixed.

Best regards,
 Phil

Re: FLUSH/FUA documentation & code discrepancy

[Tejun Heo]

Hello,

On Tue, Sep 04, 2012 at 02:32:01PM +0200, Philipp Reisner wrote:
> I think commit 1e87901e18 was wrong. Starting with that commit the REQ_FLUSH 
> and REQ_FUA bits get stripped away if the queue does not advertise REQ_FLUSH
> or REQ_FUA support.
> 
> But the REQ_FLUSH bit is also tested for when not merging requests
> (blk_queue_bio()) or when it comes to the elevator (blk_flush_plug_list()).
> 
> So, since this patch the elevator reorders write requests on queues that 
> do not have REQ_FLUSH or REQ_FUA set.
> 
> While on queues that have REQ_FLUSH or REQ_FUA set, the elevator does
> not reorder writes across FLUSHes.

Currently, FLUSH/FUA doesn't enforce any ordering requirement.  File
systems are responsible for draining all writes which have to happen
before and not issue further writes which should come after.  The use
of SOFTBARRIER there is mostly historical.  I *suspect* we should be
able to drop that as RQ_NOMERGE_FLAGS already contain FLUSH and FUA.
Not really looked through it tho.

> I have the impression every file system lets IO drain, and issues a
> flush afterwards with the blkdev_issue_flush() function. BTW that
> function turns into a non-obvious no-op as soon as the queue does not
> have the REQ_FUA or REQ_FLUSH bits set. It does not look like it is
> a no-op by intention.

So, yeah, it is no-op by intention.  We probably should make the
documentation clearer.

Thanks.

-- 
tejun

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Philipp Reisner]

> Currently, FLUSH/FUA doesn't enforce any ordering requirement.  File
> systems are responsible for draining all writes which have to happen
> before and not issue further writes which should come after.

Ok. That is a clear statement. So we will do it that way.

The "Currently" in you statement, suggests that there might be something
more mighty in the future. Is that true?

We are looking for a method that allows us to submit some writes, then
an IO-barrier, and then further writes. 

Best,
 Phil

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Tejun Heo]

On Wed, Sep 05, 2012 at 10:44:55AM +0200, Philipp Reisner wrote:
> > Currently, FLUSH/FUA doesn't enforce any ordering requirement.  File
> > systems are responsible for draining all writes which have to happen
> > before and not issue further writes which should come after.
> 
> Ok. That is a clear statement. So we will do it that way.
> 
> The "Currently" in you statement, suggests that there might be something
> more mighty in the future. Is that true?

Heh, I was more thinking about the past.  We used to have barrier
support with much stricter ordering.  I don't think we're gonna change
the ordering requirement in any foreseeable future.

Thanks.

-- 
tejun

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Lars Ellenberg]

On Wed, Sep 05, 2012 at 01:49:15AM -0700, Tejun Heo wrote:
> On Wed, Sep 05, 2012 at 10:44:55AM +0200, Philipp Reisner wrote:
> > > Currently, FLUSH/FUA doesn't enforce any ordering requirement.  File
> > > systems are responsible for draining all writes which have to happen
> > > before and not issue further writes which should come after.
> > 
> > Ok. That is a clear statement. So we will do it that way.
> > 
> > The "Currently" in you statement, suggests that there might be something
> > more mighty in the future. Is that true?
> 
> Heh, I was more thinking about the past.  We used to have barrier
> support with much stricter ordering.  I don't think we're gonna change
> the ordering requirement in any foreseeable future.

So reiterating the situation:

If I'd submit a non-empty bio with FLUSH/FUA set,
on a queue that does support flush, we get to
	blk_queue_bio()
		if (bio->bi_rw & (REQ_FLUSH | REQ_FUA)) {
			spin_lock_irq(q->queue_lock);
			where = ELEVATOR_INSERT_FLUSH;
			goto get_rq;

This bio ends up *not* being merged or reordered by the elevator.
(and, by means of flush/fua not by the hardware, either, obviously)


If the queue does not support it, flags are stripped away in
generic_make_request_checks(), and we will not take that branch
in blk_queue_bio(), but enter the normal elevator code path,
attempting a merge, or doing ELEVATOR_INSERT_SORT.

This same bio, happening to be submitted on a different IO stack,
now *is* being reordered in the elevator already,
even before being sent to the hardware.




If we somehow can express at submit_bio time that we would like
this bio, once it reaches the elevator, to not be reordered,
regardless of whether or not FLUSH is supported respectively required
by the IO stack in use, that would be better than now, IMO.
In fact, for our particular use case it would even be good enough.


Could we "just" strip these flags a "little bit later"?
Or set some other indicator when stripping them?


Thanks,

	Lars

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Tejun Heo]

Hello,

On Wed, Sep 05, 2012 at 12:07:24PM +0200, Lars Ellenberg wrote:
> So reiterating the situation:
> 
> If I'd submit a non-empty bio with FLUSH/FUA set,
> on a queue that does support flush, we get to
> 	blk_queue_bio()
> 		if (bio->bi_rw & (REQ_FLUSH | REQ_FUA)) {
> 			spin_lock_irq(q->queue_lock);
> 			where = ELEVATOR_INSERT_FLUSH;
> 			goto get_rq;
> 
> This bio ends up *not* being merged or reordered by the elevator.
> (and, by means of flush/fua not by the hardware, either, obviously)
> 
> If the queue does not support it, flags are stripped away in
> generic_make_request_checks(), and we will not take that branch
> in blk_queue_bio(), but enter the normal elevator code path,
> attempting a merge, or doing ELEVATOR_INSERT_SORT.

which is an implementation detail.

> This same bio, happening to be submitted on a different IO stack,
> now *is* being reordered in the elevator already,
> even before being sent to the hardware.

and this is perfectly fine.

I really don't see what problem you're trying to solve here.  The
ordering requirement is weak.  Certain implementation path uses
stronger requirement for convenience / historical reasons.  If any
change makes sense, it's relaxing the unnecessarily strict ordering if
possible.

What actual problem are you seeing?

Thanks.

-- 
tejun

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Lars Ellenberg]

On Thu, Sep 06, 2012 at 02:29:52PM -0700, Tejun Heo wrote:
> Hello,
> 
> On Wed, Sep 05, 2012 at 12:07:24PM +0200, Lars Ellenberg wrote:
> > So reiterating the situation:
> > 
> > If I'd submit a non-empty bio with FLUSH/FUA set,
> > on a queue that does support flush, we get to
> > 	blk_queue_bio()
> > 		if (bio->bi_rw & (REQ_FLUSH | REQ_FUA)) {
> > 			spin_lock_irq(q->queue_lock);
> > 			where = ELEVATOR_INSERT_FLUSH;
> > 			goto get_rq;
> > 
> > This bio ends up *not* being merged or reordered by the elevator.
> > (and, by means of flush/fua not by the hardware, either, obviously)
> > 
> > If the queue does not support it, flags are stripped away in
> > generic_make_request_checks(), and we will not take that branch
> > in blk_queue_bio(), but enter the normal elevator code path,
> > attempting a merge, or doing ELEVATOR_INSERT_SORT.
> 
> which is an implementation detail.
> 
> > This same bio, happening to be submitted on a different IO stack,
> > now *is* being reordered in the elevator already,
> > even before being sent to the hardware.
> 
> and this is perfectly fine.

Absolutely.

It was just "unexpected" ;-)

BTW, out of curiosity, if it is fine to take the normal elevator path in
the case the flags are stripped, why would it not be ok to take that
same normal path in the case the flags are not stripped?

I'd assume it is not about ordering, but only about merging, as the flush
machinery seems to rely on requests with single bios attached?

> I really don't see what problem you're trying to solve here.  The
> ordering requirement is weak.  Certain implementation path uses
> stronger requirement for convenience / historical reasons.  If any
> change makes sense, it's relaxing the unnecessarily strict ordering if
> possible.

Ok.

> What actual problem are you seeing?

We have a kernel thread that is receiving data blocks,
and some "boundary" information (in the sense that between such
boundaries, we have a reorder domain, where requests may reorder freely,
but no requests may be reordered across such boundaries).

This same thread submits the assembled bios.

With the old, stronger, BIO_RW_BARRIER implementation,
if it was supported, we could just submit the first bio of a reorder
domain (plus some special cases) with that flag,
and could keep receiving -> assembling -> submitting.

Now, we assumed that with FLUSH/FUA, we can do the same.
And we could, as long as it is supported through the whole stack.

But if it is not supported at some level in the stack, we must first drain.

And since it is all "transparent", we just cannot determine
if the whole stack does or does not support it.

So we have to drain always.

We did not realize that. 
In certain cases, where we submitted in the right order, and even
indicated what we thought would amount to at least a "soft barrier"
(reorder boundary) for the elevator, we ended up with data corruption
because the elevator never sees these indicators, and reorders.

Fine, our mistake/misunderstanding of the drain requirement.
That's fixed now, we do always drain
(unless specifically configured not to, where the admin takes the blame
if that does not work on his stack).


To always drain is also a performance hit, as we would rather keep
receiving data and assembling bios and submitting them.

We can possibly work around that by introducing an additional submitter thread,
or at least our own list where we queue assembled bios until the lower
level device queue drains.

But we'd rather have the elevator see the FLUSH/FUA,
and treat them as at least a soft barrier/reorder boundary.

I may be wrong here, but all the necessary bits for this seem to be in
place already, if the information would even reach the elevator in one
way or other, and not be completely stripped away early.


What would you rather see, the elevator recognizing reorder boundaries?
Or additional higher level queueing and extra thread/work queue/whatever?

Both are fine with me, I'm just asking for an opinion.


Thanks,
	Lars

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Tejun Heo]

Hello, Lars.

On Fri, Sep 07, 2012 at 10:42:21AM +0200, Lars Ellenberg wrote:
> We have a kernel thread that is receiving data blocks,
> and some "boundary" information (in the sense that between such
> boundaries, we have a reorder domain, where requests may reorder freely,
> but no requests may be reordered across such boundaries).

What purpose does this boundary serve?  Why is it necessary?  Which
driver is this?

> This same thread submits the assembled bios.
> 
> With the old, stronger, BIO_RW_BARRIER implementation,
> if it was supported, we could just submit the first bio of a reorder
> domain (plus some special cases) with that flag,
> and could keep receiving -> assembling -> submitting.

Yes, but the actual request processing would continue to stall as
block layer would have been draining requests continuously.

> Now, we assumed that with FLUSH/FUA, we can do the same.
> And we could, as long as it is supported through the whole stack.
> 
> But if it is not supported at some level in the stack, we must first drain.
> 
> And since it is all "transparent", we just cannot determine
> if the whole stack does or does not support it.
> 
> So we have to drain always.

The driver was hitching on BARRIER for draining.  As that's gone now,
if you want the same behavior, the driver would need to drain itself.

> We did not realize that. 
> In certain cases, where we submitted in the right order, and even
> indicated what we thought would amount to at least a "soft barrier"
> (reorder boundary) for the elevator, we ended up with data corruption
> because the elevator never sees these indicators, and reorders.
> 
> Fine, our mistake/misunderstanding of the drain requirement.
> That's fixed now, we do always drain
> (unless specifically configured not to, where the admin takes the blame
> if that does not work on his stack).
> 
> 
> To always drain is also a performance hit, as we would rather keep
> receiving data and assembling bios and submitting them.

Is the performance hit measureable?  Block BARRIER support had some
optimizations but it still had to constantly drain all the same.

> We can possibly work around that by introducing an additional submitter thread,
> or at least our own list where we queue assembled bios until the lower
> level device queue drains.
> 
> But we'd rather have the elevator see the FLUSH/FUA,
> and treat them as at least a soft barrier/reorder boundary.
> 
> I may be wrong here, but all the necessary bits for this seem to be in
> place already, if the information would even reach the elevator in one
> way or other, and not be completely stripped away early.
> 
> What would you rather see, the elevator recognizing reorder boundaries?
> Or additional higher level queueing and extra thread/work queue/whatever?
> 
> Both are fine with me, I'm just asking for an opinion.

First of all, using FLUSH/FUA for such purpose is an error-prone
abuse.  You're trying to exploit an implementation detail which may
change at any time.  I think what you want is to be able to specify
REQ_SOFTBARRIER on bio submission, which shouldn't be too hard but I'm
still lost why this is necessary.  Can you please explain it a bit
more?

Thanks.

-- 
tejun

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Tejun Heo]

Hello, again.

cc'ing Kent and Vivek.  The original thread is at

  http://thread.gmane.org/gmane.linux.network.drbd.devel/2130

On Mon, Sep 10, 2012 at 03:54:42PM -0700, Tejun Heo wrote:
> > We can possibly work around that by introducing an additional submitter thread,
> > or at least our own list where we queue assembled bios until the lower
> > level device queue drains.
> > 
> > But we'd rather have the elevator see the FLUSH/FUA,
> > and treat them as at least a soft barrier/reorder boundary.
> > 
> > I may be wrong here, but all the necessary bits for this seem to be in
> > place already, if the information would even reach the elevator in one
> > way or other, and not be completely stripped away early.
> > 
> > What would you rather see, the elevator recognizing reorder boundaries?
> > Or additional higher level queueing and extra thread/work queue/whatever?
> > 
> > Both are fine with me, I'm just asking for an opinion.
> 
> First of all, using FLUSH/FUA for such purpose is an error-prone
> abuse.  You're trying to exploit an implementation detail which may
> change at any time.  I think what you want is to be able to specify
> REQ_SOFTBARRIER on bio submission, which shouldn't be too hard but I'm
> still lost why this is necessary.  Can you please explain it a bit
> more?

The problem with exposing REQ_SOFTBARRIER at bio submission is that it
would require block layer not to reorder bios while passing through
stacked adrivers until it reaches a rq-based driver.  I *suspect* this
has been true until now but Kent's pending patch to fix possible
deadlock issue breaks that.

  http://thread.gmane.org/gmane.linux.kernel.bcache.devel/1017/focus=1356250

As for what the resolution should be, urgh... I don't know. :(

Thanks.

-- 
tejun

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Kent Overstreet]

On Mon, Sep 10, 2012 at 04:06:54PM -0700, Tejun Heo wrote:
> Hello, again.
> 
> cc'ing Kent and Vivek.  The original thread is at
> 
>   http://thread.gmane.org/gmane.linux.network.drbd.devel/2130
> 
> On Mon, Sep 10, 2012 at 03:54:42PM -0700, Tejun Heo wrote:
> > > We can possibly work around that by introducing an additional submitter thread,
> > > or at least our own list where we queue assembled bios until the lower
> > > level device queue drains.
> > > 
> > > But we'd rather have the elevator see the FLUSH/FUA,
> > > and treat them as at least a soft barrier/reorder boundary.
> > > 
> > > I may be wrong here, but all the necessary bits for this seem to be in
> > > place already, if the information would even reach the elevator in one
> > > way or other, and not be completely stripped away early.
> > > 
> > > What would you rather see, the elevator recognizing reorder boundaries?
> > > Or additional higher level queueing and extra thread/work queue/whatever?
> > > 
> > > Both are fine with me, I'm just asking for an opinion.
> > 
> > First of all, using FLUSH/FUA for such purpose is an error-prone
> > abuse.  You're trying to exploit an implementation detail which may
> > change at any time.  I think what you want is to be able to specify
> > REQ_SOFTBARRIER on bio submission, which shouldn't be too hard but I'm
> > still lost why this is necessary.  Can you please explain it a bit
> > more?
> 
> The problem with exposing REQ_SOFTBARRIER at bio submission is that it
> would require block layer not to reorder bios while passing through
> stacked adrivers until it reaches a rq-based driver.  I *suspect* this
> has been true until now but Kent's pending patch to fix possible
> deadlock issue breaks that.
> 
>   http://thread.gmane.org/gmane.linux.kernel.bcache.devel/1017/focus=1356250
> 
> As for what the resolution should be, urgh... I don't know. :(

No, that ordering has definitely not been preserved before - any
stacking driver would need either a giant global lock around its
make_request fn and everything else, or it'd need to serialize all bios
with some kind of linked list or something until they got to the
underlying devices.

And if that wasn't bad enough performance wise, getting the ordering
semantics correct when you've got multiple underlying devices...
*shudder*

I thought these kinds of ordering requirements were what we were getting
away from when old style barriers went away in 2.6.38.

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Kent Overstreet]

cc'ing Neil

On Mon, Sep 10, 2012 at 04:06:54PM -0700, Tejun Heo wrote:
> Hello, again.
> 
> cc'ing Kent and Vivek.  The original thread is at
> 
>   http://thread.gmane.org/gmane.linux.network.drbd.devel/2130
> 
> On Mon, Sep 10, 2012 at 03:54:42PM -0700, Tejun Heo wrote:
> > > We can possibly work around that by introducing an additional submitter thread,
> > > or at least our own list where we queue assembled bios until the lower
> > > level device queue drains.
> > > 
> > > But we'd rather have the elevator see the FLUSH/FUA,
> > > and treat them as at least a soft barrier/reorder boundary.
> > > 
> > > I may be wrong here, but all the necessary bits for this seem to be in
> > > place already, if the information would even reach the elevator in one
> > > way or other, and not be completely stripped away early.
> > > 
> > > What would you rather see, the elevator recognizing reorder boundaries?
> > > Or additional higher level queueing and extra thread/work queue/whatever?
> > > 
> > > Both are fine with me, I'm just asking for an opinion.
> > 
> > First of all, using FLUSH/FUA for such purpose is an error-prone
> > abuse.  You're trying to exploit an implementation detail which may
> > change at any time.  I think what you want is to be able to specify
> > REQ_SOFTBARRIER on bio submission, which shouldn't be too hard but I'm
> > still lost why this is necessary.  Can you please explain it a bit
> > more?
> 
> The problem with exposing REQ_SOFTBARRIER at bio submission is that it
> would require block layer not to reorder bios while passing through
> stacked adrivers until it reaches a rq-based driver.  I *suspect* this
> has been true until now but Kent's pending patch to fix possible
> deadlock issue breaks that.

Yeah, you might be right about that. I think Neil Brown would know
better than I if this ordering was ever explicitly broken.

But I don't think anything else is relying on that kind of ordering any
more.

>   http://thread.gmane.org/gmane.linux.kernel.bcache.devel/1017/focus=1356250
> 
> As for what the resolution should be, urgh... I don't know. :(

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2931 bytes --]

On Mon, 10 Sep 2012 16:31:59 -0700 Kent Overstreet <koverstreet@google.com>
wrote:

> cc'ing Neil
> 
> On Mon, Sep 10, 2012 at 04:06:54PM -0700, Tejun Heo wrote:
> > Hello, again.
> > 
> > cc'ing Kent and Vivek.  The original thread is at
> > 
> >   http://thread.gmane.org/gmane.linux.network.drbd.devel/2130
> > 
> > On Mon, Sep 10, 2012 at 03:54:42PM -0700, Tejun Heo wrote:
> > > > We can possibly work around that by introducing an additional submitter thread,
> > > > or at least our own list where we queue assembled bios until the lower
> > > > level device queue drains.
> > > > 
> > > > But we'd rather have the elevator see the FLUSH/FUA,
> > > > and treat them as at least a soft barrier/reorder boundary.
> > > > 
> > > > I may be wrong here, but all the necessary bits for this seem to be in
> > > > place already, if the information would even reach the elevator in one
> > > > way or other, and not be completely stripped away early.
> > > > 
> > > > What would you rather see, the elevator recognizing reorder boundaries?
> > > > Or additional higher level queueing and extra thread/work queue/whatever?
> > > > 
> > > > Both are fine with me, I'm just asking for an opinion.
> > > 
> > > First of all, using FLUSH/FUA for such purpose is an error-prone
> > > abuse.  You're trying to exploit an implementation detail which may
> > > change at any time.  I think what you want is to be able to specify
> > > REQ_SOFTBARRIER on bio submission, which shouldn't be too hard but I'm
> > > still lost why this is necessary.  Can you please explain it a bit
> > > more?
> > 
> > The problem with exposing REQ_SOFTBARRIER at bio submission is that it
> > would require block layer not to reorder bios while passing through
> > stacked adrivers until it reaches a rq-based driver.  I *suspect* this
> > has been true until now but Kent's pending patch to fix possible
> > deadlock issue breaks that.
> 
> Yeah, you might be right about that. I think Neil Brown would know
> better than I if this ordering was ever explicitly broken.

While don't deliberately re-order bios just for the fun of it, there is no
real effort to keep the bios "in order" (assuming that even means something
in a multi-processor environment).  The old barrier code did impose ordering
but thankfully we don't have that any more.

RAID5 can certainly re-order write bios as it tries to assemble complete
stripes.  RAID1/4/5/6/10 can certainly re-order things if there is an error
that needs to be handled.

The only way to ensure one request completes before another is to not submit
the other until the one has completed.

NeilBrown


> 
> But I don't think anything else is relying on that kind of ordering any
> more.
> 
> >   http://thread.gmane.org/gmane.linux.kernel.bcache.devel/1017/focus=1356250
> > 
> > As for what the resolution should be, urgh... I don't know. :(


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Lars Ellenberg]

On Tue, Sep 11, 2012 at 03:58:20PM +1000, NeilBrown wrote:
> On Mon, 10 Sep 2012 16:31:59 -0700 Kent Overstreet <koverstreet@google.com>
> wrote:
> 
> > cc'ing Neil
> > 
> > On Mon, Sep 10, 2012 at 04:06:54PM -0700, Tejun Heo wrote:
> > > Hello, again.
> > > 
> > > cc'ing Kent and Vivek.  The original thread is at
> > > 
> > >   http://thread.gmane.org/gmane.linux.network.drbd.devel/2130
> > > 
> > > On Mon, Sep 10, 2012 at 03:54:42PM -0700, Tejun Heo wrote:
> > > > > We can possibly work around that by introducing an additional submitter thread,
> > > > > or at least our own list where we queue assembled bios until the lower
> > > > > level device queue drains.
> > > > > 
> > > > > But we'd rather have the elevator see the FLUSH/FUA,
> > > > > and treat them as at least a soft barrier/reorder boundary.
> > > > > 
> > > > > I may be wrong here, but all the necessary bits for this seem to be in
> > > > > place already, if the information would even reach the elevator in one
> > > > > way or other, and not be completely stripped away early.
> > > > > 
> > > > > What would you rather see, the elevator recognizing reorder boundaries?
> > > > > Or additional higher level queueing and extra thread/work queue/whatever?
> > > > > 
> > > > > Both are fine with me, I'm just asking for an opinion.
> > > > 
> > > > First of all, using FLUSH/FUA for such purpose is an error-prone
> > > > abuse.  You're trying to exploit an implementation detail which may
> > > > change at any time.  I think what you want is to be able to specify
> > > > REQ_SOFTBARRIER on bio submission, which shouldn't be too hard but I'm
> > > > still lost why this is necessary.  Can you please explain it a bit
> > > > more?
> > > 
> > > The problem with exposing REQ_SOFTBARRIER at bio submission is that it
> > > would require block layer not to reorder bios while passing through
> > > stacked adrivers until it reaches a rq-based driver.  I *suspect* this
> > > has been true until now but Kent's pending patch to fix possible
> > > deadlock issue breaks that.
> > 
> > Yeah, you might be right about that. I think Neil Brown would know
> > better than I if this ordering was ever explicitly broken.
> 
> While don't deliberately re-order bios just for the fun of it, there is no
> real effort to keep the bios "in order" (assuming that even means something
> in a multi-processor environment).  The old barrier code did impose ordering
> but thankfully we don't have that any more.
> 
> RAID5 can certainly re-order write bios as it tries to assemble complete
> stripes.  RAID1/4/5/6/10 can certainly re-order things if there is an error
> that needs to be handled.
> 
> The only way to ensure one request completes before another is to not submit
> the other until the one has completed.

That settles it, then.
Thank you for the clear wording.

Should this be in Documentation/block/writeback_cache_control.txt,
even though I realize that that is about hardware,
and not the stack above it?

Or do we have a better place to document write ordering requirements?

"To enforce write-after-write dependencies, you *have* to drain the
queue (do we have a generic interface available for that?),
or at least wait for the completion of all the requests you
(potentially) depend upon, before even submitting the dependent request.

Additionally, to avoid integrity issues due to volatile caches,
you need to use FLUSH/FUA as appropriate."

(please rephrase as you see fit, no native speaker here ...)


	Lars

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Vivek Goyal]

On Tue, Sep 11, 2012 at 10:25:01AM +0200, Lars Ellenberg wrote:

[..]
> "To enforce write-after-write dependencies, you *have* to drain the
> queue (do we have a generic interface available for that?),
> or at least wait for the completion of all the requests you
> (potentially) depend upon, before even submitting the dependent request.

I think you need to wait for all previous IOs (you are dependent on) to finish.

Thanks
Vivek

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Tejun Heo]

Hello,

On Tue, Sep 11, 2012 at 10:25:01AM +0200, Lars Ellenberg wrote:
> Or do we have a better place to document write ordering requirements?
> 
> "To enforce write-after-write dependencies, you *have* to drain the
> queue (do we have a generic interface available for that?),
> or at least wait for the completion of all the requests you
> (potentially) depend upon, before even submitting the dependent request.
> 
> Additionally, to avoid integrity issues due to volatile caches,
> you need to use FLUSH/FUA as appropriate."

I think it would be best to note the lack of any ordering guarantee in
the comment of bio_submit()?  Any one interested in submitting a
patch?

Thanks.

-- 
tejun

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Joseph Glanville]

On 13 September 2012 04:58, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Tue, Sep 11, 2012 at 10:25:01AM +0200, Lars Ellenberg wrote:
>> Or do we have a better place to document write ordering requirements?
>>
>> "To enforce write-after-write dependencies, you *have* to drain the
>> queue (do we have a generic interface available for that?),
>> or at least wait for the completion of all the requests you
>> (potentially) depend upon, before even submitting the dependent request.
>>
>> Additionally, to avoid integrity issues due to volatile caches,
>> you need to use FLUSH/FUA as appropriate."
>
> I think it would be best to note the lack of any ordering guarantee in
> the comment of bio_submit()?  Any one interested in submitting a
> patch?
>
> Thanks.
>
> --
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Something along the lines of this?

diff --git a/block/blk-core.c b/block/blk-core.c
index 4b4dbdf..68b5671 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1809,6 +1809,9 @@ EXPORT_SYMBOL(generic_make_request);
  * uses that function to do most of the work. Both are fairly rough
  * interfaces; @bio must be presetup and ready for I/O.
  *
+ * Ordering of requests is not guaranteed, callers should drain the queue
+ * and issue a flush before submission of any dependent bios.
+ *
  */
 void submit_bio(int rw, struct bio *bio)
 {

-- 
CTO | Orion Virtualisation Solutions | www.orionvm.com.au
Phone: 1300 56 99 52 | Mobile: 0428 754 846

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Tejun Heo]

On Thu, Sep 13, 2012 at 09:12:25AM +1000, Joseph Glanville wrote:
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 4b4dbdf..68b5671 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1809,6 +1809,9 @@ EXPORT_SYMBOL(generic_make_request);
>   * uses that function to do most of the work. Both are fairly rough
>   * interfaces; @bio must be presetup and ready for I/O.
>   *
> + * Ordering of requests is not guaranteed, callers should drain the queue
> + * and issue a flush before submission of any dependent bios.

I think using "requests" here is confusing given that struct request
means something else in the block layer.  Also, the caller doesn't
have to drain the whole queue but just the ones involved in the
dependency and flush doesn't really matter here although mentioning
that flush too doesn't have any ordering guarantee would be nice.
Also, updating submit_bio() comment too would be nice - something
simple about lack of ordering and then reference to
generic_make_request().

Thanks.

-- 
tejun

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Joseph Glanville]

On 13 September 2012 09:20, Tejun Heo <tj@kernel.org> wrote:
> On Thu, Sep 13, 2012 at 09:12:25AM +1000, Joseph Glanville wrote:
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 4b4dbdf..68b5671 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1809,6 +1809,9 @@ EXPORT_SYMBOL(generic_make_request);
>>   * uses that function to do most of the work. Both are fairly rough
>>   * interfaces; @bio must be presetup and ready for I/O.
>>   *
>> + * Ordering of requests is not guaranteed, callers should drain the queue
>> + * and issue a flush before submission of any dependent bios.
>
> I think using "requests" here is confusing given that struct request
> means something else in the block layer.  Also, the caller doesn't

Agreed, my bad.

> have to drain the whole queue but just the ones involved in the
> dependency and flush doesn't really matter here although mentioning
> that flush too doesn't have any ordering guarantee would be nice.

Will do.
Would you prefer it being added to the top of the file describing the
semantics or the blkdev_issue_flush function itself?

> Also, updating submit_bio() comment too would be nice - something
> simple about lack of ordering and then reference to
> generic_make_request().

Will do.

>
> Thanks.
>
> --
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
CTO | Orion Virtualisation Solutions | www.orionvm.com.au
Phone: 1300 56 99 52 | Mobile: 0428 754 846

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Joseph Glanville]

On 13 September 2012 09:53, Joseph Glanville
<joseph.glanville@orionvm.com.au> wrote:
> On 13 September 2012 09:20, Tejun Heo <tj@kernel.org> wrote:
>> On Thu, Sep 13, 2012 at 09:12:25AM +1000, Joseph Glanville wrote:
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index 4b4dbdf..68b5671 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -1809,6 +1809,9 @@ EXPORT_SYMBOL(generic_make_request);
>>>   * uses that function to do most of the work. Both are fairly rough
>>>   * interfaces; @bio must be presetup and ready for I/O.
>>>   *
>>> + * Ordering of requests is not guaranteed, callers should drain the queue
>>> + * and issue a flush before submission of any dependent bios.
>>
>> I think using "requests" here is confusing given that struct request
>> means something else in the block layer.  Also, the caller doesn't
>
> Agreed, my bad.
>
>> have to drain the whole queue but just the ones involved in the
>> dependency and flush doesn't really matter here although mentioning
>> that flush too doesn't have any ordering guarantee would be nice.
>
> Will do.
> Would you prefer it being added to the top of the file describing the
> semantics or the blkdev_issue_flush function itself?
>
>> Also, updating submit_bio() comment too would be nice - something
>> simple about lack of ordering and then reference to
>> generic_make_request().
>
> Will do.
>
>>
>> Thanks.
>>
>> --
>> tejun
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>
> --
> CTO | Orion Virtualisation Solutions | www.orionvm.com.au
> Phone: 1300 56 99 52 | Mobile: 0428 754 846

diff --git a/block/blk-core.c b/block/blk-core.c
index 4b4dbdf..b3559d8 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1749,6 +1749,11 @@ end_io:
  * bio happens to be merged with someone else, and may resubmit the bio to
  * a lower device by calling into generic_make_request recursively, which
  * means the bio should NOT be touched after the call to ->make_request_fn.
+ *
+ * Ordering is not guaranteed, callers of generic_make_request()
+ * that require ordering should ensure dependencies are first drained
+ * before submission of dependent bios.
+ *
  */
 void generic_make_request(struct bio *bio)
 {
@@ -1808,6 +1813,7 @@ EXPORT_SYMBOL(generic_make_request);
  * submit_bio() is very similar in purpose to generic_make_request(), and
  * uses that function to do most of the work. Both are fairly rough
  * interfaces; @bio must be presetup and ready for I/O.
+ * Note: submit_bio() doesn't ensure ordering, see generic_make_request()
  *
  */
 void submit_bio(int rw, struct bio *bio)

-- 
CTO | Orion Virtualisation Solutions | www.orionvm.com.au
Phone: 1300 56 99 52 | Mobile: 0428 754 846

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Joseph Glanville]

Hi Tejun, list.

While reviewing some of the documentation in Documentation/block I
found there is still out of date references to the old barrier code.
Should this also be removed?
I am not sufficiently confident of my understanding of the block layer
to do a through review/update of the Documentation/block stuff but my
untrained eye suggests maybe that such a undertaking is probably abit
overdue.


-- 
CTO | Orion Virtualisation Solutions | www.orionvm.com.au
Phone: 1300 56 99 52 | Mobile: 0428 754 846

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Tejun Heo]

Hello, Joseph.

On Thu, Sep 13, 2012 at 01:10:13PM +1000, Joseph Glanville wrote:
> While reviewing some of the documentation in Documentation/block I
> found there is still out of date references to the old barrier code.
> Should this also be removed?
> I am not sufficiently confident of my understanding of the block layer
> to do a through review/update of the Documentation/block stuff but my
> untrained eye suggests maybe that such a undertaking is probably abit
> overdue.

Please be my guest.  I'll be happy to review it.

Thanks.

-- 
tejun

Re: [Drbd-dev] FLUSH/FUA documentation & code discrepancy

[Vivek Goyal]

On Mon, Sep 10, 2012 at 04:06:54PM -0700, Tejun Heo wrote:
> Hello, again.
> 
> cc'ing Kent and Vivek.  The original thread is at
> 
>   http://thread.gmane.org/gmane.linux.network.drbd.devel/2130
> 
> On Mon, Sep 10, 2012 at 03:54:42PM -0700, Tejun Heo wrote:
> > > We can possibly work around that by introducing an additional submitter thread,
> > > or at least our own list where we queue assembled bios until the lower
> > > level device queue drains.
> > > 
> > > But we'd rather have the elevator see the FLUSH/FUA,
> > > and treat them as at least a soft barrier/reorder boundary.
> > > 
> > > I may be wrong here, but all the necessary bits for this seem to be in
> > > place already, if the information would even reach the elevator in one
> > > way or other, and not be completely stripped away early.
> > > 
> > > What would you rather see, the elevator recognizing reorder boundaries?
> > > Or additional higher level queueing and extra thread/work queue/whatever?
> > > 
> > > Both are fine with me, I'm just asking for an opinion.
> > 
> > First of all, using FLUSH/FUA for such purpose is an error-prone
> > abuse.  You're trying to exploit an implementation detail which may
> > change at any time.  I think what you want is to be able to specify
> > REQ_SOFTBARRIER on bio submission, which shouldn't be too hard but I'm
> > still lost why this is necessary.  Can you please explain it a bit
> > more?
> 
> The problem with exposing REQ_SOFTBARRIER at bio submission is that it
> would require block layer not to reorder bios while passing through
> stacked adrivers until it reaches a rq-based driver.  I *suspect* this
> has been true until now but Kent's pending patch to fix possible
> deadlock issue breaks that.
> 
>   http://thread.gmane.org/gmane.linux.kernel.bcache.devel/1017/focus=1356250
> 
> As for what the resolution should be, urgh... I don't know. :(

I think we should not go back to providing any kind of ordering interface
or gurantees. IIRC, Jens had mentioned that older ordering requirements
were problematic while implemeting multiqueue stuff he is working on. He
was glad that ordering requirements were gone.

Also if somebody is implementing caching target as stacked device, then
it can very well reorder the bios. It can submit bios to underlying device
it does not want to cache and hold back some of the bios it wants to
cache.

REQ_FLUSH/REQ_FUA never provided any ordering gurantees. Just that file
system needs to wait for previous IOs to finish and then submit REQ_FLUSH/
REQ_FUA to implement any kind of ordering. I guess any driver in the
stack needs to do the same if they need some kind of ordering behavior.

Thanks
Vivek