[patch] blk-flush: fix flush policy calculation

[patch] blk-flush: fix flush policy calculation

[Jeff Moyer]

Hi,

Reading through the code in blk-flush.c, it appears that there is an
oversight in the policy returned from blk_flush_policy:

        if (fflags & REQ_FLUSH) {
                if (rq->cmd_flags & REQ_FLUSH)
                        policy |= REQ_FSEQ_PREFLUSH;
                if (blk_rq_sectors(rq))
                        policy |= REQ_FSEQ_DATA;
                if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
                        policy |= REQ_FSEQ_POSTFLUSH;
        }
        return policy;

This means that REQ_FSEQ_DATA can only be set if the queue flush_flags
include FLUSH and/or FUA.  However, the short-circuit for not issuing
flushes when the device doesn't need/support them depends on
REQ_FSEQ_DATA being set while the other two bits are clear:

        /*
         * If there's data but flush is not necessary, the request can be
         * processed directly without going through flush machinery.  Queue
         * for normal execution.
         */
        if ((policy & REQ_FSEQ_DATA) &&
            !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
                list_add_tail(&rq->queuelist, &q->queue_head);
                return;
        }

Given the code as it stands, I don't think the body of this if statement
will ever be executed.  I've attached a fix for this below.  It seems
like this could be both a performance and a correctness issue, though
I've not run into any problems I can directly attribute to this (perhaps
due to file systems not issuing flushes when support is not advertised?).

Comments are appreciated.

Cheers,
Jeff

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

diff --git a/block/blk-flush.c b/block/blk-flush.c
index bb21e4c..3a06118 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -95,11 +95,11 @@ static unsigned int blk_flush_policy(unsigned int fflags, struct request *rq)
 {
 	unsigned int policy = 0;
 
+	if (blk_rq_sectors(rq))
+		policy |= REQ_FSEQ_DATA;
 	if (fflags & REQ_FLUSH) {
 		if (rq->cmd_flags & REQ_FLUSH)
 			policy |= REQ_FSEQ_PREFLUSH;
-		if (blk_rq_sectors(rq))
-			policy |= REQ_FSEQ_DATA;
 		if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
 			policy |= REQ_FSEQ_POSTFLUSH;
 	}

Re: [patch] blk-flush: fix flush policy calculation

[Shaohua Li]

2011/8/2 Jeff Moyer <jmoyer@redhat.com>:
> Hi,
>
> Reading through the code in blk-flush.c, it appears that there is an
> oversight in the policy returned from blk_flush_policy:
>
>        if (fflags & REQ_FLUSH) {
>                if (rq->cmd_flags & REQ_FLUSH)
>                        policy |= REQ_FSEQ_PREFLUSH;
>                if (blk_rq_sectors(rq))
>                        policy |= REQ_FSEQ_DATA;
>                if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
>                        policy |= REQ_FSEQ_POSTFLUSH;
>        }
>        return policy;
>
> This means that REQ_FSEQ_DATA can only be set if the queue flush_flags
> include FLUSH and/or FUA.  However, the short-circuit for not issuing
> flushes when the device doesn't need/support them depends on
> REQ_FSEQ_DATA being set while the other two bits are clear:
>
>        /*
>         * If there's data but flush is not necessary, the request can be
>         * processed directly without going through flush machinery.  Queue
>         * for normal execution.
>         */
>        if ((policy & REQ_FSEQ_DATA) &&
>            !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
>                list_add_tail(&rq->queuelist, &q->queue_head);
>                return;
>        }
>
> Given the code as it stands, I don't think the body of this if statement
> will ever be executed.  I've attached a fix for this below.  It seems
> like this could be both a performance and a correctness issue, though
> I've not run into any problems I can directly attribute to this (perhaps
> due to file systems not issuing flushes when support is not advertised?).
>
> Comments are appreciated.
>
> Cheers,
> Jeff
>
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
>
> diff --git a/block/blk-flush.c b/block/blk-flush.c
> index bb21e4c..3a06118 100644
> --- a/block/blk-flush.c
> +++ b/block/blk-flush.c
> @@ -95,11 +95,11 @@ static unsigned int blk_flush_policy(unsigned int fflags, struct request *rq)
>  {
>        unsigned int policy = 0;
>
> +       if (blk_rq_sectors(rq))
> +               policy |= REQ_FSEQ_DATA;
>        if (fflags & REQ_FLUSH) {
>                if (rq->cmd_flags & REQ_FLUSH)
>                        policy |= REQ_FSEQ_PREFLUSH;
> -               if (blk_rq_sectors(rq))
> -                       policy |= REQ_FSEQ_DATA;
>                if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
>                        policy |= REQ_FSEQ_POSTFLUSH;
>        }
> --
__generic_make_request always handles this:
if ((bio->bi_rw & (REQ_FLUSH | REQ_FUA)) && !q->flush_flags) {
                        bio->bi_rw &= ~(REQ_FLUSH | REQ_FUA);
                        if (!nr_sectors) {
                                err = 0;
                                goto end_io;
                        }
                }

Thanks,
Shaohua

Re: [patch] blk-flush: fix flush policy calculation

[Jeff Moyer]

Shaohua Li <shli@kernel.org> writes:

> 2011/8/2 Jeff Moyer <jmoyer@redhat.com>:
>> Hi,
>>
>> Reading through the code in blk-flush.c, it appears that there is an
>> oversight in the policy returned from blk_flush_policy:
>>
>>        if (fflags & REQ_FLUSH) {
>>                if (rq->cmd_flags & REQ_FLUSH)
>>                        policy |= REQ_FSEQ_PREFLUSH;
>>                if (blk_rq_sectors(rq))
>>                        policy |= REQ_FSEQ_DATA;
>>                if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
>>                        policy |= REQ_FSEQ_POSTFLUSH;
>>        }
>>        return policy;
>>
>> This means that REQ_FSEQ_DATA can only be set if the queue flush_flags
>> include FLUSH and/or FUA.  However, the short-circuit for not issuing
>> flushes when the device doesn't need/support them depends on
>> REQ_FSEQ_DATA being set while the other two bits are clear:
>>
>>        /*
>>         * If there's data but flush is not necessary, the request can be
>>         * processed directly without going through flush machinery.  Queue
>>         * for normal execution.
>>         */
>>        if ((policy & REQ_FSEQ_DATA) &&
>>            !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
>>                list_add_tail(&rq->queuelist, &q->queue_head);
>>                return;
>>        }
>>
>> Given the code as it stands, I don't think the body of this if statement
>> will ever be executed.  I've attached a fix for this below.  It seems
>> like this could be both a performance and a correctness issue, though
>> I've not run into any problems I can directly attribute to this (perhaps
>> due to file systems not issuing flushes when support is not advertised?).
>>
>> Comments are appreciated.
>>
>> Cheers,
>> Jeff
>>
>> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
>>
>> diff --git a/block/blk-flush.c b/block/blk-flush.c
>> index bb21e4c..3a06118 100644
>> --- a/block/blk-flush.c
>> +++ b/block/blk-flush.c
>> @@ -95,11 +95,11 @@ static unsigned int blk_flush_policy(unsigned int fflags, struct request *rq)
>>  {
>>        unsigned int policy = 0;
>>
>> +       if (blk_rq_sectors(rq))
>> +               policy |= REQ_FSEQ_DATA;
>>        if (fflags & REQ_FLUSH) {
>>                if (rq->cmd_flags & REQ_FLUSH)
>>                        policy |= REQ_FSEQ_PREFLUSH;
>> -               if (blk_rq_sectors(rq))
>> -                       policy |= REQ_FSEQ_DATA;
>>                if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
>>                        policy |= REQ_FSEQ_POSTFLUSH;
>>        }
>> --
> __generic_make_request always handles this:
> if ((bio->bi_rw & (REQ_FLUSH | REQ_FUA)) && !q->flush_flags) {
>                         bio->bi_rw &= ~(REQ_FLUSH | REQ_FUA);
>                         if (!nr_sectors) {
>                                 err = 0;
>                                 goto end_io;
>                         }
>                 }
>

dm-multipath exports flush and fua, even if underlying devices do not
support those flags (but this will change shortly).  It then issues I/O
using blk_insert_cloned_request, which bypasses generic_make_request.
Plus, the logic was clearly wrong so I think we should take the proposed
patch.

Cheers,
Jeff

Re: [patch] blk-flush: fix flush policy calculation

[Jeff Moyer]

OK, sorry for top-posting here, but I chased the problem down further.

Commit ae1b1539622fb46e51b4d13b3f9e5f4c713f86ae, block: reimplement
FLUSH/FUA to support merge, introduced a regression when running any
sort of fsyncing workload using dm-multipath and certain storage (in our
case, an HP EVA).  It turns out that dm-multipath always advertised
flush+fua support, and passed commands on down the stack, where they
used to get stripped off.  The above commit, unfortunately, changed that
behavior:

static inline struct request *__elv_next_request(struct request_queue *q)
{
        struct request *rq;

        while (1) {
-               while (!list_empty(&q->queue_head)) {
+               if (!list_empty(&q->queue_head)) {
                        rq = list_entry_rq(q->queue_head.next);
-                       if (!(rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) ||
-                           (rq->cmd_flags & REQ_FLUSH_SEQ))
-                               return rq;
-                       rq = blk_do_flush(q, rq);
-                       if (rq)
-                               return rq;
+                       return rq;
                }

Note that previously, a command would come in here, have
REQ_FLUSH|REQ_FUA set, and then get handed off to blk_do_flush:

struct request *blk_do_flush(struct request_queue *q, struct request *rq)
{
        unsigned int fflags = q->flush_flags; /* may change, cache it */
        bool has_flush = fflags & REQ_FLUSH, has_fua = fflags & REQ_FUA;
        bool do_preflush = has_flush && (rq->cmd_flags & REQ_FLUSH);
        bool do_postflush = has_flush && !has_fua && (rq->cmd_flags &
        REQ_FUA);
        unsigned skip = 0;
...
        if (blk_rq_sectors(rq) && !do_preflush && !do_postflush) {
                rq->cmd_flags &= ~REQ_FLUSH;
		if (!has_fua)
			rq->cmd_flags &= ~REQ_FUA;
	        return rq;
	}

So, the flush machinery was bypassed in such cases (q->flush_flags == 0
&& rq->cmd_flags & (REQ_FLUSH|REQ_FUA)).

Now, however, we don't get into the flush machinery at all (which is why
my initial patch didn't help this situation).  Instead,
__elv_next_request just hands a request with flush and fua bits set to
the scsi_request_fn, even though the underlying request_queue does not
support flush or fua.

So, where do we fix this?  We could just accept Mike's patch to not send
such requests down from dm-mpath, but that seems short-sighted.  We
could reinstate some checks in __elv_next_request.  Or, we could put the
checks into blk_insert_cloned_request.

Suggestions?

Cheers,
Jeff


Shaohua Li <shli@kernel.org> writes:

> 2011/8/2 Jeff Moyer <jmoyer@redhat.com>:
>> Hi,
>>
>> Reading through the code in blk-flush.c, it appears that there is an
>> oversight in the policy returned from blk_flush_policy:
>>
>>        if (fflags & REQ_FLUSH) {
>>                if (rq->cmd_flags & REQ_FLUSH)
>>                        policy |= REQ_FSEQ_PREFLUSH;
>>                if (blk_rq_sectors(rq))
>>                        policy |= REQ_FSEQ_DATA;
>>                if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
>>                        policy |= REQ_FSEQ_POSTFLUSH;
>>        }
>>        return policy;
>>
>> This means that REQ_FSEQ_DATA can only be set if the queue flush_flags
>> include FLUSH and/or FUA.  However, the short-circuit for not issuing
>> flushes when the device doesn't need/support them depends on
>> REQ_FSEQ_DATA being set while the other two bits are clear:
>>
>>        /*
>>         * If there's data but flush is not necessary, the request can be
>>         * processed directly without going through flush machinery.  Queue
>>         * for normal execution.
>>         */
>>        if ((policy & REQ_FSEQ_DATA) &&
>>            !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
>>                list_add_tail(&rq->queuelist, &q->queue_head);
>>                return;
>>        }
>>
>> Given the code as it stands, I don't think the body of this if statement
>> will ever be executed.  I've attached a fix for this below.  It seems
>> like this could be both a performance and a correctness issue, though
>> I've not run into any problems I can directly attribute to this (perhaps
>> due to file systems not issuing flushes when support is not advertised?).
>>
>> Comments are appreciated.
>>
>> Cheers,
>> Jeff
>>
>> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
>>
>> diff --git a/block/blk-flush.c b/block/blk-flush.c
>> index bb21e4c..3a06118 100644
>> --- a/block/blk-flush.c
>> +++ b/block/blk-flush.c
>> @@ -95,11 +95,11 @@ static unsigned int blk_flush_policy(unsigned int fflags, struct request *rq)
>>  {
>>        unsigned int policy = 0;
>>
>> +       if (blk_rq_sectors(rq))
>> +               policy |= REQ_FSEQ_DATA;
>>        if (fflags & REQ_FLUSH) {
>>                if (rq->cmd_flags & REQ_FLUSH)
>>                        policy |= REQ_FSEQ_PREFLUSH;
>> -               if (blk_rq_sectors(rq))
>> -                       policy |= REQ_FSEQ_DATA;
>>                if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
>>                        policy |= REQ_FSEQ_POSTFLUSH;
>>        }
>> --
> __generic_make_request always handles this:
> if ((bio->bi_rw & (REQ_FLUSH | REQ_FUA)) && !q->flush_flags) {
>                         bio->bi_rw &= ~(REQ_FLUSH | REQ_FUA);
>                         if (!nr_sectors) {
>                                 err = 0;
>                                 goto end_io;
>                         }
>                 }
>
> Thanks,
> Shaohua

Re: [patch] blk-flush: fix flush policy calculation

[Vivek Goyal]

On Tue, Aug 02, 2011 at 01:39:46PM -0400, Jeff Moyer wrote:
> OK, sorry for top-posting here, but I chased the problem down further.
> 
> Commit ae1b1539622fb46e51b4d13b3f9e5f4c713f86ae, block: reimplement
> FLUSH/FUA to support merge, introduced a regression when running any
> sort of fsyncing workload using dm-multipath and certain storage (in our
> case, an HP EVA).  It turns out that dm-multipath always advertised
> flush+fua support, and passed commands on down the stack, where they
> used to get stripped off.  The above commit, unfortunately, changed that
> behavior:
> 
> static inline struct request *__elv_next_request(struct request_queue *q)
> {
>         struct request *rq;
> 
>         while (1) {
> -               while (!list_empty(&q->queue_head)) {
> +               if (!list_empty(&q->queue_head)) {
>                         rq = list_entry_rq(q->queue_head.next);
> -                       if (!(rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) ||
> -                           (rq->cmd_flags & REQ_FLUSH_SEQ))
> -                               return rq;
> -                       rq = blk_do_flush(q, rq);
> -                       if (rq)
> -                               return rq;
> +                       return rq;
>                 }
> 
> Note that previously, a command would come in here, have
> REQ_FLUSH|REQ_FUA set, and then get handed off to blk_do_flush:
> 
> struct request *blk_do_flush(struct request_queue *q, struct request *rq)
> {
>         unsigned int fflags = q->flush_flags; /* may change, cache it */
>         bool has_flush = fflags & REQ_FLUSH, has_fua = fflags & REQ_FUA;
>         bool do_preflush = has_flush && (rq->cmd_flags & REQ_FLUSH);
>         bool do_postflush = has_flush && !has_fua && (rq->cmd_flags &
>         REQ_FUA);
>         unsigned skip = 0;
> ...
>         if (blk_rq_sectors(rq) && !do_preflush && !do_postflush) {
>                 rq->cmd_flags &= ~REQ_FLUSH;
> 		if (!has_fua)
> 			rq->cmd_flags &= ~REQ_FUA;
> 	        return rq;
> 	}
> 
> So, the flush machinery was bypassed in such cases (q->flush_flags == 0
> && rq->cmd_flags & (REQ_FLUSH|REQ_FUA)).
> 
> Now, however, we don't get into the flush machinery at all (which is why
> my initial patch didn't help this situation).  Instead,
> __elv_next_request just hands a request with flush and fua bits set to
> the scsi_request_fn, even though the underlying request_queue does not
> support flush or fua.
> 
> So, where do we fix this?  We could just accept Mike's patch to not send
> such requests down from dm-mpath, but that seems short-sighted.  We
> could reinstate some checks in __elv_next_request.  Or, we could put the
> checks into blk_insert_cloned_request.
> 
> Suggestions?

IMHO, we should fix it at multiple places.

- Your initial fix in blk_insert_flush makes sense. blk_insert_flush()
  is equivalent of blk_do_flush() so resetting REQ_FLUSH and REQ_FUA there
  makes sense to me. 

- Fixing blk_insert_cloned_request() also makes sense to me so that if
  a request is REQ_FLUSH or REQ_FUA set, we try to add it to underlying
  device using ELEVATOR_INSERT_FLUSH and not ELEVATOR_INSERT_BACK.

- Fixing dm-multipath makes sense too as what's the point in dispatching
  unnecessary flush/fua requests to underlying devices if underlying
  queue does not have FLUSH capability.

So I would say, fix it at all the places. :-)

I have one question though. What happens if we have an empty request
with REQ_FLUSH set and request queue does not support flush. Where
will we complete the IO for that request? I see that __generic_make_request()
takes care of that but we might have to take care of if it insert_cloned
path too.

Thanks
Vivek

Re: [patch] blk-flush: fix flush policy calculation

[Jeff Moyer]

Vivek Goyal <vgoyal@redhat.com> writes:

> On Tue, Aug 02, 2011 at 01:39:46PM -0400, Jeff Moyer wrote:
>> OK, sorry for top-posting here, but I chased the problem down further.
>> 
>> Commit ae1b1539622fb46e51b4d13b3f9e5f4c713f86ae, block: reimplement
>> FLUSH/FUA to support merge, introduced a regression when running any
>> sort of fsyncing workload using dm-multipath and certain storage (in our
>> case, an HP EVA).  It turns out that dm-multipath always advertised
>> flush+fua support, and passed commands on down the stack, where they
>> used to get stripped off.  The above commit, unfortunately, changed that
>> behavior:
>> 
>> static inline struct request *__elv_next_request(struct request_queue *q)
>> {
>>         struct request *rq;
>> 
>>         while (1) {
>> -               while (!list_empty(&q->queue_head)) {
>> +               if (!list_empty(&q->queue_head)) {
>>                         rq = list_entry_rq(q->queue_head.next);
>> -                       if (!(rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) ||
>> -                           (rq->cmd_flags & REQ_FLUSH_SEQ))
>> -                               return rq;
>> -                       rq = blk_do_flush(q, rq);
>> -                       if (rq)
>> -                               return rq;
>> +                       return rq;
>>                 }
>> 
>> Note that previously, a command would come in here, have
>> REQ_FLUSH|REQ_FUA set, and then get handed off to blk_do_flush:
>> 
>> struct request *blk_do_flush(struct request_queue *q, struct request *rq)
>> {
>>         unsigned int fflags = q->flush_flags; /* may change, cache it */
>>         bool has_flush = fflags & REQ_FLUSH, has_fua = fflags & REQ_FUA;
>>         bool do_preflush = has_flush && (rq->cmd_flags & REQ_FLUSH);
>>         bool do_postflush = has_flush && !has_fua && (rq->cmd_flags &
>>         REQ_FUA);
>>         unsigned skip = 0;
>> ...
>>         if (blk_rq_sectors(rq) && !do_preflush && !do_postflush) {
>>                 rq->cmd_flags &= ~REQ_FLUSH;
>> 		if (!has_fua)
>> 			rq->cmd_flags &= ~REQ_FUA;
>> 	        return rq;
>> 	}
>> 
>> So, the flush machinery was bypassed in such cases (q->flush_flags == 0
>> && rq->cmd_flags & (REQ_FLUSH|REQ_FUA)).
>> 
>> Now, however, we don't get into the flush machinery at all (which is why
>> my initial patch didn't help this situation).  Instead,
>> __elv_next_request just hands a request with flush and fua bits set to
>> the scsi_request_fn, even though the underlying request_queue does not
>> support flush or fua.
>> 
>> So, where do we fix this?  We could just accept Mike's patch to not send
>> such requests down from dm-mpath, but that seems short-sighted.  We
>> could reinstate some checks in __elv_next_request.  Or, we could put the
>> checks into blk_insert_cloned_request.
>> 
>> Suggestions?
>
> IMHO, we should fix it at multiple places.
>
> - Your initial fix in blk_insert_flush makes sense. blk_insert_flush()
>   is equivalent of blk_do_flush() so resetting REQ_FLUSH and REQ_FUA there
>   makes sense to me. 

Right, I still stand by that fix.  It was a thinko.

> - Fixing blk_insert_cloned_request() also makes sense to me so that if
>   a request is REQ_FLUSH or REQ_FUA set, we try to add it to underlying
>   device using ELEVATOR_INSERT_FLUSH and not ELEVATOR_INSERT_BACK.

Good point.

> - Fixing dm-multipath makes sense too as what's the point in dispatching
>   unnecessary flush/fua requests to underlying devices if underlying
>   queue does not have FLUSH capability.
>
> So I would say, fix it at all the places. :-)

You missed __elv_next_request.  :)

> I have one question though. What happens if we have an empty request
> with REQ_FLUSH set and request queue does not support flush. Where
> will we complete the IO for that request? I see that __generic_make_request()
> takes care of that but we might have to take care of if it insert_cloned
> path too.

In testing, I did this:

@@ -1817,6 +1817,14 @@ int blk_insert_cloned_request(struct
request_queue *q, struct request *rq)
                return -EIO;
 #endif
 
+       if ((rq->cmd_flags & (REQ_FLUSH|REQ_FUA)) && !q->flush_flags) {
+               rq->cmd_flags &= ~(REQ_FLUSH|REQ_FUA);
+               if (!blk_rq_bytes(rq)) {
+                       blk_end_request(rq, 0, 0);
+                       return 0;
+               }
+       }
+

Cheers,
Jeff

Re: [patch] blk-flush: fix flush policy calculation

[Mike Snitzer]

On Tue, Aug 2, 2011 at 2:17 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Aug 02, 2011 at 01:39:46PM -0400, Jeff Moyer wrote:

>> So, where do we fix this?  We could just accept Mike's patch to not send
>> such requests down from dm-mpath, but that seems short-sighted.  We
>> could reinstate some checks in __elv_next_request.  Or, we could put the
>> checks into blk_insert_cloned_request.
>>
>> Suggestions?
>
> IMHO, we should fix it at multiple places.
>
> - Your initial fix in blk_insert_flush makes sense. blk_insert_flush()
>  is equivalent of blk_do_flush() so resetting REQ_FLUSH and REQ_FUA there
>  makes sense to me.

Makes sense to me too.

> - Fixing blk_insert_cloned_request() also makes sense to me so that if
>  a request is REQ_FLUSH or REQ_FUA set, we try to add it to underlying
>  device using ELEVATOR_INSERT_FLUSH and not ELEVATOR_INSERT_BACK.

blk_insert_cloned_request is currently only used for request-based DM
(so just dm-mpath).
I don't see a problem with adding logic to it but in practice the DM
change to properly propagate underlying devices' flush_flags is going
to obviate the need (until the kernel grows another
blk_insert_cloned_request caller).

> - Fixing dm-multipath makes sense too as what's the point in dispatching
>  unnecessary flush/fua requests to underlying devices if underlying
>  queue does not have FLUSH capability.

To be clear: It isn't just a fix for dm-multipath.  It fixes all DM
devices (both bio-based and request-based).
That change is already destined for 3.1 via Alasdair's recent DM pull request.

Mike

Re: [patch] blk-flush: fix flush policy calculation

[Vivek Goyal]

On Tue, Aug 02, 2011 at 02:31:00PM -0400, Jeff Moyer wrote:
> Vivek Goyal <vgoyal@redhat.com> writes:
> 
> > On Tue, Aug 02, 2011 at 01:39:46PM -0400, Jeff Moyer wrote:
> >> OK, sorry for top-posting here, but I chased the problem down further.
> >> 
> >> Commit ae1b1539622fb46e51b4d13b3f9e5f4c713f86ae, block: reimplement
> >> FLUSH/FUA to support merge, introduced a regression when running any
> >> sort of fsyncing workload using dm-multipath and certain storage (in our
> >> case, an HP EVA).  It turns out that dm-multipath always advertised
> >> flush+fua support, and passed commands on down the stack, where they
> >> used to get stripped off.  The above commit, unfortunately, changed that
> >> behavior:
> >> 
> >> static inline struct request *__elv_next_request(struct request_queue *q)
> >> {
> >>         struct request *rq;
> >> 
> >>         while (1) {
> >> -               while (!list_empty(&q->queue_head)) {
> >> +               if (!list_empty(&q->queue_head)) {
> >>                         rq = list_entry_rq(q->queue_head.next);
> >> -                       if (!(rq->cmd_flags & (REQ_FLUSH | REQ_FUA)) ||
> >> -                           (rq->cmd_flags & REQ_FLUSH_SEQ))
> >> -                               return rq;
> >> -                       rq = blk_do_flush(q, rq);
> >> -                       if (rq)
> >> -                               return rq;
> >> +                       return rq;
> >>                 }
> >> 
> >> Note that previously, a command would come in here, have
> >> REQ_FLUSH|REQ_FUA set, and then get handed off to blk_do_flush:
> >> 
> >> struct request *blk_do_flush(struct request_queue *q, struct request *rq)
> >> {
> >>         unsigned int fflags = q->flush_flags; /* may change, cache it */
> >>         bool has_flush = fflags & REQ_FLUSH, has_fua = fflags & REQ_FUA;
> >>         bool do_preflush = has_flush && (rq->cmd_flags & REQ_FLUSH);
> >>         bool do_postflush = has_flush && !has_fua && (rq->cmd_flags &
> >>         REQ_FUA);
> >>         unsigned skip = 0;
> >> ...
> >>         if (blk_rq_sectors(rq) && !do_preflush && !do_postflush) {
> >>                 rq->cmd_flags &= ~REQ_FLUSH;
> >> 		if (!has_fua)
> >> 			rq->cmd_flags &= ~REQ_FUA;
> >> 	        return rq;
> >> 	}
> >> 
> >> So, the flush machinery was bypassed in such cases (q->flush_flags == 0
> >> && rq->cmd_flags & (REQ_FLUSH|REQ_FUA)).
> >> 
> >> Now, however, we don't get into the flush machinery at all (which is why
> >> my initial patch didn't help this situation).  Instead,
> >> __elv_next_request just hands a request with flush and fua bits set to
> >> the scsi_request_fn, even though the underlying request_queue does not
> >> support flush or fua.
> >> 
> >> So, where do we fix this?  We could just accept Mike's patch to not send
> >> such requests down from dm-mpath, but that seems short-sighted.  We
> >> could reinstate some checks in __elv_next_request.  Or, we could put the
> >> checks into blk_insert_cloned_request.
> >> 
> >> Suggestions?
> >
> > IMHO, we should fix it at multiple places.
> >
> > - Your initial fix in blk_insert_flush makes sense. blk_insert_flush()
> >   is equivalent of blk_do_flush() so resetting REQ_FLUSH and REQ_FUA there
> >   makes sense to me. 
> 
> Right, I still stand by that fix.  It was a thinko.
> 
> > - Fixing blk_insert_cloned_request() also makes sense to me so that if
> >   a request is REQ_FLUSH or REQ_FUA set, we try to add it to underlying
> >   device using ELEVATOR_INSERT_FLUSH and not ELEVATOR_INSERT_BACK.
> 
> Good point.
> 
> > - Fixing dm-multipath makes sense too as what's the point in dispatching
> >   unnecessary flush/fua requests to underlying devices if underlying
> >   queue does not have FLUSH capability.
> >
> > So I would say, fix it at all the places. :-)
> 
> You missed __elv_next_request.  :)

Actually we will take care of resetting FLUSH/FUA flag when request is
being queued on request queue (blk_insert_flush()). So I think there is
no need to fix __elv_next_request(). That's why I had skipped it. 

> 
> > I have one question though. What happens if we have an empty request
> > with REQ_FLUSH set and request queue does not support flush. Where
> > will we complete the IO for that request? I see that __generic_make_request()
> > takes care of that but we might have to take care of if it insert_cloned
> > path too.
> 
> In testing, I did this:
> 
> @@ -1817,6 +1817,14 @@ int blk_insert_cloned_request(struct
> request_queue *q, struct request *rq)
>                 return -EIO;
>  #endif
>  
> +       if ((rq->cmd_flags & (REQ_FLUSH|REQ_FUA)) && !q->flush_flags) {
> +               rq->cmd_flags &= ~(REQ_FLUSH|REQ_FUA);
> +               if (!blk_rq_bytes(rq)) {
> +                       blk_end_request(rq, 0, 0);
> +                       return 0;
> +               }
> +       }
> +

Will it make more sense to take care resetting flush/fua flags in
blk_insert_flush() and also the part which will end the request if
request is empty. And in cloned request we just take care of using
ELVATOR_FLUSH_INSERT. 

The reason being that it will make __elv_insert() path safe for all
the cases and insert_cloned_request is just one of the consumers.

But this is just a minor point. At the end of the day, I think both
the solutions will work.

Thanks
Vivek

Re: [patch] blk-flush: fix flush policy calculation

[Jeff Moyer]

Vivek Goyal <vgoyal@redhat.com> writes:

>> In testing, I did this:
>> 
>> @@ -1817,6 +1817,14 @@ int blk_insert_cloned_request(struct
>> request_queue *q, struct request *rq)
>>                 return -EIO;
>>  #endif
>>  
>> +       if ((rq->cmd_flags & (REQ_FLUSH|REQ_FUA)) && !q->flush_flags) {
>> +               rq->cmd_flags &= ~(REQ_FLUSH|REQ_FUA);
>> +               if (!blk_rq_bytes(rq)) {
>> +                       blk_end_request(rq, 0, 0);
>> +                       return 0;
>> +               }
>> +       }
>> +
>
> Will it make more sense to take care resetting flush/fua flags in
> blk_insert_flush()

Yes, that's much cleaner.

> and also the part which will end the request if request is empty.

Mmm-hmm.

I tried this, and it blew up in my face.  ;-)  Cloned requests from
device-mapper-land have rq->end_io filled in, which causes
blk_insert_flush to BUG here:

        BUG_ON(rq->end_io);

So, we can take the usual route of trying to squirrel away a pointer,
and reinstate that after the flush_data_end_io.  Or, perhaps someone has
a more clever idea, that might keep us from further expanding struct
request...?

Cheers,
Jeff

Re: [patch] blk-flush: fix flush policy calculation

[Shaohua Li]

2011/8/3 Jeff Moyer <jmoyer@redhat.com>:
> Vivek Goyal <vgoyal@redhat.com> writes:
>
>>> In testing, I did this:
>>>
>>> @@ -1817,6 +1817,14 @@ int blk_insert_cloned_request(struct
>>> request_queue *q, struct request *rq)
>>>                 return -EIO;
>>>  #endif
>>>
>>> +       if ((rq->cmd_flags & (REQ_FLUSH|REQ_FUA)) && !q->flush_flags) {
>>> +               rq->cmd_flags &= ~(REQ_FLUSH|REQ_FUA);
>>> +               if (!blk_rq_bytes(rq)) {
>>> +                       blk_end_request(rq, 0, 0);
>>> +                       return 0;
>>> +               }
>>> +       }
>>> +
>>
>> Will it make more sense to take care resetting flush/fua flags in
>> blk_insert_flush()
>
> Yes, that's much cleaner.
agreed. doing at blk_insert_flush makes a lot of sense, maybe we have
similar issue
like blk_insert_cloned_request in the future.

>> and also the part which will end the request if request is empty.
>
> Mmm-hmm.
>
> I tried this, and it blew up in my face.  ;-)  Cloned requests from
> device-mapper-land have rq->end_io filled in, which causes
> blk_insert_flush to BUG here:
>
>        BUG_ON(rq->end_io);
>
> So, we can take the usual route of trying to squirrel away a pointer,
> and reinstate that after the flush_data_end_io.  Or, perhaps someone has
> a more clever idea, that might keep us from further expanding struct
> request...?
we still have space at request->rb_node union. And since flush request
doesn't go to io scheduler, we can use the space.

Thanks,
Shaohua

Re: [patch] blk-flush: fix flush policy calculation

[Tejun Heo]

Hello,

On Mon, Aug 01, 2011 at 04:32:17PM -0400, Jeff Moyer wrote:
>         /*
>          * If there's data but flush is not necessary, the request can be
>          * processed directly without going through flush machinery.  Queue
>          * for normal execution.
>          */
>         if ((policy & REQ_FSEQ_DATA) &&
>             !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
>                 list_add_tail(&rq->queuelist, &q->queue_head);
>                 return;
>         }
> 
> Given the code as it stands, I don't think the body of this if statement
> will ever be executed.

It does if the device has a WB cache and filesystem sends down a FUA
(but no FLUSH) request.

> I've attached a fix for this below.  It seems
> like this could be both a performance and a correctness issue, though
> I've not run into any problems I can directly attribute to this (perhaps
> due to file systems not issuing flushes when support is not advertised?).
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Hmmm... yes, this can become a correctness issue if (and only if)
blk_queue_flush() is called to change q->flush_flags while requests
are in-flight; otherwise, requests wouldn't reach the function at all.
Also, I think it would be a generally good idea to always set
FSEQ_DATA if the request has data.

 Acked-by: Tejun Heo <tj@kernel.org>

but can you please update the description?

Thank you.

-- 
tejun

Re: [patch] blk-flush: fix flush policy calculation

[Tejun Heo]

Hello,

On Tue, Aug 02, 2011 at 01:39:46PM -0400, Jeff Moyer wrote:
> OK, sorry for top-posting here, but I chased the problem down further.
> 
> Commit ae1b1539622fb46e51b4d13b3f9e5f4c713f86ae, block: reimplement
> FLUSH/FUA to support merge, introduced a regression when running any
> sort of fsyncing workload using dm-multipath and certain storage (in our
> case, an HP EVA).  It turns out that dm-multipath always advertised
> flush+fua support, and passed commands on down the stack, where they
> used to get stripped off.  The above commit, unfortunately, changed that
> behavior:
...
> So, the flush machinery was bypassed in such cases (q->flush_flags == 0
> && rq->cmd_flags & (REQ_FLUSH|REQ_FUA)).
> 
> Now, however, we don't get into the flush machinery at all (which is why
> my initial patch didn't help this situation).  Instead,
> __elv_next_request just hands a request with flush and fua bits set to
> the scsi_request_fn, even though the underlying request_queue does not
> support flush or fua.
> 
> So, where do we fix this?  We could just accept Mike's patch to not send
> such requests down from dm-mpath, but that seems short-sighted.  We
> could reinstate some checks in __elv_next_request.  Or, we could put the
> checks into blk_insert_cloned_request.

Ah, okay, what changed there was where a request is passed into flush
machinery.  Before, it was while the request was being dispatched from
elevator to device.  After, it's de-composed when the request enters
elevator.  The bug is that there are paths which insert new requests
to elevator but didn't check for REQ_FLUSH|FUA.

I think it would be cleaner to add a wrapper around
__elv_add_request() which checks for REQ_FLUSH|FUA and enforce
REQ_INSERT_FLUSH if the request needs it.  Note that this should only
happen when a request enters the queue for the first time but not on
requeues - that was the reason why the decision wasn't made inside
__elv_add_request().

Thank you.

-- 
tejun

Re: [patch] blk-flush: fix flush policy calculation

[Jeff Moyer]

Tejun Heo <tj@kernel.org> writes:

> Hello,
>
> On Tue, Aug 02, 2011 at 01:39:46PM -0400, Jeff Moyer wrote:
>> OK, sorry for top-posting here, but I chased the problem down further.
>> 
>> Commit ae1b1539622fb46e51b4d13b3f9e5f4c713f86ae, block: reimplement
>> FLUSH/FUA to support merge, introduced a regression when running any
>> sort of fsyncing workload using dm-multipath and certain storage (in our
>> case, an HP EVA).  It turns out that dm-multipath always advertised
>> flush+fua support, and passed commands on down the stack, where they
>> used to get stripped off.  The above commit, unfortunately, changed that
>> behavior:
> ...
>> So, the flush machinery was bypassed in such cases (q->flush_flags == 0
>> && rq->cmd_flags & (REQ_FLUSH|REQ_FUA)).
>> 
>> Now, however, we don't get into the flush machinery at all (which is why
>> my initial patch didn't help this situation).  Instead,
>> __elv_next_request just hands a request with flush and fua bits set to
>> the scsi_request_fn, even though the underlying request_queue does not
>> support flush or fua.
>> 
>> So, where do we fix this?  We could just accept Mike's patch to not send
>> such requests down from dm-mpath, but that seems short-sighted.  We
>> could reinstate some checks in __elv_next_request.  Or, we could put the
>> checks into blk_insert_cloned_request.
>
> Ah, okay, what changed there was where a request is passed into flush
> machinery.  Before, it was while the request was being dispatched from
> elevator to device.  After, it's de-composed when the request enters
> elevator.  The bug is that there are paths which insert new requests
> to elevator but didn't check for REQ_FLUSH|FUA.
>
> I think it would be cleaner to add a wrapper around
> __elv_add_request() which checks for REQ_FLUSH|FUA and enforce
> REQ_INSERT_FLUSH if the request needs it.  Note that this should only
> happen when a request enters the queue for the first time but not on
> requeues - that was the reason why the decision wasn't made inside
> __elv_add_request().

OK, we can do a wrapper, but it probably wouldn't be too horrific to
just fix up blk_insert_cloned_request.

Now, the next issue is that flush requests issued from the dm target
down through the stack have no bio associated with them, so we blow up
on the BUG_ON(!req->bio || req->bio != req->biotail).

Cheers,
Jeff

Re: [patch] blk-flush: fix flush policy calculation

[Jeff Moyer]

Tejun Heo <tj@kernel.org> writes:

> Hello,
>
> On Tue, Aug 02, 2011 at 01:39:46PM -0400, Jeff Moyer wrote:
>> OK, sorry for top-posting here, but I chased the problem down further.
>> 
>> Commit ae1b1539622fb46e51b4d13b3f9e5f4c713f86ae, block: reimplement
>> FLUSH/FUA to support merge, introduced a regression when running any
>> sort of fsyncing workload using dm-multipath and certain storage (in our
>> case, an HP EVA).  It turns out that dm-multipath always advertised
>> flush+fua support, and passed commands on down the stack, where they
>> used to get stripped off.  The above commit, unfortunately, changed that
>> behavior:
> ...
>> So, the flush machinery was bypassed in such cases (q->flush_flags == 0
>> && rq->cmd_flags & (REQ_FLUSH|REQ_FUA)).
>> 
>> Now, however, we don't get into the flush machinery at all (which is why
>> my initial patch didn't help this situation).  Instead,
>> __elv_next_request just hands a request with flush and fua bits set to
>> the scsi_request_fn, even though the underlying request_queue does not
>> support flush or fua.
>> 
>> So, where do we fix this?  We could just accept Mike's patch to not send
>> such requests down from dm-mpath, but that seems short-sighted.  We
>> could reinstate some checks in __elv_next_request.  Or, we could put the
>> checks into blk_insert_cloned_request.
>
> Ah, okay, what changed there was where a request is passed into flush
> machinery.  Before, it was while the request was being dispatched from
> elevator to device.  After, it's de-composed when the request enters
> elevator.  The bug is that there are paths which insert new requests
> to elevator but didn't check for REQ_FLUSH|FUA.
>
> I think it would be cleaner to add a wrapper around
> __elv_add_request() which checks for REQ_FLUSH|FUA and enforce
> REQ_INSERT_FLUSH if the request needs it.  Note that this should only
> happen when a request enters the queue for the first time but not on
> requeues - that was the reason why the decision wasn't made inside
> __elv_add_request().

OK, we can do a wrapper, but it probably wouldn't be too horrific to
just fix up blk_insert_cloned_request.

Now, the next issue is that flush requests issued from the dm target
down through the stack have no bio associated with them, so we blow up
on the BUG_ON(!req->bio || req->bio != req->biotail).

Cheers,
Jeff

Re: [patch] blk-flush: fix flush policy calculation

[Vivek Goyal]

On Tue, Aug 02, 2011 at 03:46:49PM -0400, Jeff Moyer wrote:
> Vivek Goyal <vgoyal@redhat.com> writes:
> 
> >> In testing, I did this:
> >> 
> >> @@ -1817,6 +1817,14 @@ int blk_insert_cloned_request(struct
> >> request_queue *q, struct request *rq)
> >>                 return -EIO;
> >>  #endif
> >>  
> >> +       if ((rq->cmd_flags & (REQ_FLUSH|REQ_FUA)) && !q->flush_flags) {
> >> +               rq->cmd_flags &= ~(REQ_FLUSH|REQ_FUA);
> >> +               if (!blk_rq_bytes(rq)) {
> >> +                       blk_end_request(rq, 0, 0);
> >> +                       return 0;
> >> +               }
> >> +       }
> >> +
> >
> > Will it make more sense to take care resetting flush/fua flags in
> > blk_insert_flush()
> 
> Yes, that's much cleaner.
> 
> > and also the part which will end the request if request is empty.
> 
> Mmm-hmm.
> 
> I tried this, and it blew up in my face.  ;-)  Cloned requests from
> device-mapper-land have rq->end_io filled in, which causes
> blk_insert_flush to BUG here:
> 
>         BUG_ON(rq->end_io);

So how does it work when underlying device supports FLUSH/FUA? That means
that dm-multipath requets never go through flush machinery? So if a
queue does not support FUA but supports FLUSH, then FUA will not be
honored by the underlying driver and request can very well be in cache?

IOW, looks like even if component devices of a multipath device support
FLUSH, requests don't go through it and hence atleast FUA is broken? And
that would also mean that these don't benefit from flush merge logic.

I guess we need to sort out this req->end_io issue and if both caller
and flush machinery needs to make use of this pointer, then flush 
machinery should save it and restore on completion path.

Thanks
Vivek

Re: [patch] blk-flush: fix flush policy calculation

[Jeff Moyer]

Vivek Goyal <vgoyal@redhat.com> writes:

> On Tue, Aug 02, 2011 at 03:46:49PM -0400, Jeff Moyer wrote:
>> Vivek Goyal <vgoyal@redhat.com> writes:
>> 
>> >> In testing, I did this:
>> >> 
>> >> @@ -1817,6 +1817,14 @@ int blk_insert_cloned_request(struct
>> >> request_queue *q, struct request *rq)
>> >>                 return -EIO;
>> >>  #endif
>> >>  
>> >> +       if ((rq->cmd_flags & (REQ_FLUSH|REQ_FUA)) && !q->flush_flags) {
>> >> +               rq->cmd_flags &= ~(REQ_FLUSH|REQ_FUA);
>> >> +               if (!blk_rq_bytes(rq)) {
>> >> +                       blk_end_request(rq, 0, 0);
>> >> +                       return 0;
>> >> +               }
>> >> +       }
>> >> +
>> >
>> > Will it make more sense to take care resetting flush/fua flags in
>> > blk_insert_flush()
>> 
>> Yes, that's much cleaner.
>> 
>> > and also the part which will end the request if request is empty.
>> 
>> Mmm-hmm.
>> 
>> I tried this, and it blew up in my face.  ;-)  Cloned requests from
>> device-mapper-land have rq->end_io filled in, which causes
>> blk_insert_flush to BUG here:
>> 
>>         BUG_ON(rq->end_io);
>
> So how does it work when underlying device supports FLUSH/FUA? That means
> that dm-multipath requets never go through flush machinery? So if a

Yes, they do.  They go through it *above* dm-multipath, and dm-multipath
just hands the requests down to the device via whatever path it selects.

Cheers,
Jeff