Bug 725816

Also requesting perf group test additional workloads

Barry

So -156 performs well and -158 regresses ...

Barry

Please include the hardware description for red-eye, and let us know what the configuration of the storage is (is lvm in the picture? how many luns, etc).

Thanks!

Also, please attach dmesg output so we can see what your storage advertises.

Created attachment 515528 [details]
dmesg output from redeye boot of -158 kernel

Additionally in the log is the mount information associated with the AIM run.

The storage is a dual ported Qlogic HBA connected through a switch to an HP EVA 4100 array (56 146GB SCSI drives 4 enclosures).

0d:00.0 Fibre Channel: QLogic Corp. ISP2432-based 4Gb Fibre Channel to PCI Express HBA (rev 02)

56 36GB RAID0 (virtualized) LUNS are presented to the OS.  4 way multipath is set up with alias names.  56 file systems are built directly on the multi-path devices.  LVM is not in the picture.

tuned enterprise-storage is enabled but the script plays with ioelevators to test the performance difference between cfg and deadline.  The test script also tests the difference mounting with/without barriers.

barry

Created attachment 515529 [details]
multipath -ll output

multipath -ll output

if 156 is fine and 158 regresses, it could be related to flush merge and other scsi patches that went into .157.

As per dmesg, write cache seems to be disabled on raid luns. If that's the case then flush should not even be sent to device. So may be it is not flush merge but something else in the patch series.

I verified that flushes are not sent down to the storage using blktrace.

(In reply to comment #12)
> I verified that flushes are not sent down to the storage using blktrace.

bmarson is using mpath (request-based DM).. DM advertises FLUSH/FUA support via its queue regardless of whether underlying device queues do (e.g. SCSI doesn't set FLUSH and FUA unless device advertises WCE).  So it could be that request-based DM is seeing additional flush overhead even though the underlying SCSI layer is throwing the FLUSH/FUA on the floor.

I'll cook up a patch to disable FLUSH/FUA on the DM device's request_queue if all the underlying devices' queues have them disabled.  Will then get bmarson a brew built -169 based kernel.

Created attachment 515815 [details]
conditionally advertise FLUSH|FUA support based on device(s) in DM table

(In reply to comment #13)
> I'll cook up a patch to disable FLUSH/FUA on the DM device's request_queue if
> all the underlying devices' queues have them disabled.

Here is the patch, it needs performance testing.  Even if it helps a lot, by avoiding block and DM processing of flushes that go on to be dropped by SCSI anyway, we need to better understand why this AIM7 workload regressed.

DM has always advertised FLUSH|FUA and we still had good results (as comment#0's "Expected results" show).  So what is it about the flush-merge that upsets things?

> Will then get bmarson a brew built -169 based kernel.

And the associated scratch brew build:
http://download.devel.redhat.com/brewroot/scratch/msnitzer/task_3524470/

(private since url cannot be resolved externally)

git bisecting has yielded the following culprit.

[root@redeye rhel6]# git bisect bad
7eb9ebc74df0d98e23adaf7a4edee37bfe220165 is the first bad commit
commit 7eb9ebc74df0d98e23adaf7a4edee37bfe220165
Author: Mike Snitzer <snitzer>
Date:   Thu Apr 21 21:04:47 2011 -0400

    [block] reimplement FLUSH/FUA to support merge
   
    Message-id: <1303419893-9728-22-git-send-email-snitzer>
    Patchwork-id: 35586
    O-Subject: [RHEL6.2 PATCH 21/27] block: reimplement FLUSH/FUA to support merge
    Bugzilla: 672988
    RH-Acked-by: Jeff Moyer <jmoyer>
   
    From: Tejun Heo <tj>
   
    BZ 672988
   
    Upstream commit ae1b1539622fb46e51b4d13b3f9e5f4c713f86ae
    Author: Tejun Heo <tj>
    Date:   Tue Jan 25 12:43:54 2011 +0100
   
        block: reimplement FLUSH/FUA to support merge
   
        The current FLUSH/FUA support has evolved from the implementation
        which had to perform queue draining.  As such, sequencing is done
        queue-wide one flush request after another.  However, with the
        draining requirement gone, there's no reason to keep the queue-wide
        sequential approach.
   
        This patch reimplements FLUSH/FUA support such that each FLUSH/FUA
        request is sequenced individually.  The actual FLUSH execution is
        double buffered and whenever a request wants to execute one for either
        PRE or POSTFLUSH, it queues on the pending queue.  Once certain
        conditions are met, a flush request is issued and on its completion
        all pending requests proceed to the next sequence.
   
        This allows arbitrary merging of different type of flushes.  How they
        are merged can be primarily controlled and tuned by adjusting the
        above said 'conditions' used to determine when to issue the next
        flush.
   
        This is inspired by Darrick's patches to merge multiple zero-data
        flushes which helps workloads with highly concurrent fsync requests.
   
        * As flush requests are never put on the IO scheduler, request fields
          used for flush share space with rq->rb_node.  rq->completion_data is
          moved out of the union.  This increases the request size by one
          pointer.
   
          As rq->elevator_private* are used only by the iosched too, it is
          possible to reduce the request size further.  However, to do that,
          we need to modify request allocation path such that iosched data is
          not allocated for flush requests.
   
        * FLUSH/FUA processing happens on insertion now instead of dispatch.
   
        - Comments updated as per Vivek and Mike.
   
        Signed-off-by: Tejun Heo <tj>
        Cc: "Darrick J. Wong" <djwong.com>
        Cc: Shaohua Li <shli>
        Cc: Christoph Hellwig <hch>
        Cc: Vivek Goyal <vgoyal>
        Cc: Mike Snitzer <snitzer>
        Signed-off-by: Jens Axboe <jaxboe>
   
    Also includes changes from the following merge commit:
   
    Upstream commit e83a46bbb1d4c03defd733a64b727632a40059ad
    Author: Tejun Heo <tj>
   
        [Resolves conflicts] caused by commit 255bb490c8 (block: blk-flush shouldn't
        call directly into q->request_fn() __blk_run_queue()) in for-linus
        crashing with FLUSH reimplementation in for-2.6.39/core.  The conflict
        isn't trivial but the resolution is straight-forward.
   
        * __blk_run_queue() calls in flush_end_io() and flush_data_end_io()
          should be called with @force_kblockd set to %true.
   
        * elv_insert() in blk_kick_flush() should use
          %ELEVATOR_INSERT_REQUEUE.
   
        Both changes are to avoid invoking ->request_fn() directly from
        request completion path and closely match the changes in the commit
        255bb490c8.
   
        Signed-off-by: Tejun Heo <tj>
   
    Signed-off-by: Aristeu Rozanski <arozansk>

:040000 040000 1c9dfd2baddce2f167692ff5ea1c7367daaa2fa2 838d25e65cdcda13fec3ca2d7e58f876d3ee8dc3 M    block
:040000 040000 d1a2038e99cc3f8a08d3c5bde37254e2c3b04f49 d7ec56c5f70e35717d912b744a1074496d53e613 M    include

----

Testing Mike's kernel  in comment 14 next
Barry

(Cc'ing Darrick Wong (IBM) as he and IBM have a vested interest in flush-merge in RHEL6.2 -- from bug#672988)

Darrick, AIM7 to an DM mpath device (who underlying SCSI devices do _not_ advertise WCE) shows significant regression now with the block core's "flush-merge" code.  DM has always advertised REQ_FLUSH|REQ_FUA flush_flags regardless of whether the underlying devices also do.

The patch attached via comment#14 seems to resolve this regression for Barry and he'll be sharing results shortly.

Really my remaining concern (also conveyed in comment#14) is that flush-merge is clearly more costly in this odd case of DM forcing flush processing only to have the flushes dropped by SCSI.

My question: do you still have confidence in the flush-merge code not having performance regressions when devices report that flushes are actually needed?

If so then we can just fixup DM to not force these extra throwaway flushes and put this issue behind us.

Here's the results using 2.6.32-171.el6.bz725816.x86_64

ext4_barrierOFF_cfq_aim7.txt

Tasks    jobs/min  jti  jobs/min/task      real       cpu
    1      360.82  100       360.8184     16.13      1.55   
  101    28956.65   97       286.6995     20.30     74.67   
  201    43912.16   95       218.4685     26.64    150.68   
  301    52512.59   95       174.4604     33.36    228.28   
  401    58727.23   94       146.4519     39.74    306.66   
  501    64781.60   93       129.3046     45.01    382.21   
  601    68050.97   93       113.2296     51.40    460.87   
  701    71967.19   92       102.6636     56.69    537.97   
  911    77742.23   91        85.3372     68.20    702.01   
 1000    79205.23   91        79.2052     73.48    774.95   

ext4_barrierON_cfq_aim7.txt

Tasks    jobs/min  jti  jobs/min/task      real       cpu
    1      361.04  100       361.0422     16.12      1.64   
  101    28688.14   97       284.0410     20.49     74.70   
  201    43961.67   96       218.7148     26.61    151.65   
  301    52575.63   94       174.6699     33.32    228.43   
  401    58875.38   94       146.8214     39.64    306.81   
  501    64781.60   93       129.3046     45.01    383.33   
  601    67774.07   93       112.7688     51.61    461.04   
  701    71663.80   92       102.2308     56.93    539.26   
  911    76718.56   91        84.2136     69.11    704.35   
 1000    78457.81   91        78.4578     74.18    776.51   


Successful run.
Barry

so issue is indeed with flush merge on device mapper target. Interesting. I think we should now try to run some simple workload on a single lun on the same system and try to root cause the issue by looking at blktrace. Are we doing any sort of idling, waiting for a bunch of flushes to gather before dispatch and that is hurting?

Barry mentioned that iozone shows the problem, so that would indeed be a nice simple reproducer.  I've asked for that data to be posted here.

(In reply to comment #16)

> My question: do you still have confidence in the flush-merge code not having
> performance regressions when devices report that flushes are actually needed?
> 
> If so then we can just fixup DM to not force these extra throwaway flushes and
> put this issue behind us.

Since the dm device advertises flush/fua support to the block layer, any IO request with the flush or fua bits set will be forced through the flush merge state machine, which means that the requests stall while dm wastes time handing out throwaway flush requests.

So yes, the patch should fix the situation.  I vaguely remember proposing a patch that would have made dm smarter about checking its underlying devices for flush/fua capability, but I think it fell by the wayside.

This request was evaluated by Red Hat Product Management for inclusion
in a Red Hat Enterprise Linux maintenance release. Product Management has 
requested further review of this request by Red Hat Engineering, for potential
inclusion in a Red Hat Enterprise Linux Update release for currently deployed 
products. This request is not yet committed for inclusion in an Update release.

Created attachment 516166 [details]
iozone performance on -170 kernel with ext4 (incache + fsync) comparing barriers enabled/disabled

I had tim wilkinson run iozone with/without barriers to an ext4 file system.  The incache run which measures time including a final fsync call, is the only one which regressed with barriers on.  Im investigating to see if there is any -131 comparison data.

small reads and especially smaller re-reads and random reads are quite sensitive.  

Not sure what is going on here.

The system is an AMD white box with fiber channel HBA to an HP MSA2000 with a multi spindle RAID 0 LUN presented.  No multipath, no lvm.

Barry

(In reply to comment #20)
> (In reply to comment #16)
> 
> > My question: do you still have confidence in the flush-merge code not having
> > performance regressions when devices report that flushes are actually needed?
> > 
> > If so then we can just fixup DM to not force these extra throwaway flushes and
> > put this issue behind us.
> 
> Since the dm device advertises flush/fua support to the block layer, any IO
> request with the flush or fua bits set will be forced through the flush merge
> state machine, which means that the requests stall while dm wastes time handing
> out throwaway flush requests.
> 
> So yes, the patch should fix the situation.  I vaguely remember proposing a
> patch that would have made dm smarter about checking its underlying devices for
> flush/fua capability, but I think it fell by the wayside.

But that does not explain that why are we seeing it now with flush merge patches. Same should have been the situation before merge patches where we would wait on
every flush/fua request to finish. In fact with flush merge it should have become little bit better as number of request we wait on should come down if some merging of flush is happening.

Created attachment 516204 [details]
fix the regression introduced by 7eb9ebc7

blk_insert_flush has the following code:

        unsigned int policy = blk_flush_policy(fflags, rq);
...
        /*
         * 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;
        }

Notice the assumption that REQ_FSEQ_DATA can be set without REQ_FSEQ_PRE or POSTFLUSH.  In reality, this can't happen given the way blk_flush_policy is written, since the REQ_FSEQ_DATA bit is only set if the queue flush_flags are non-zero.

If we look at the code prior to the above commit, you'll see this handled properly in blk_do_flush:

        /*
         * Special case.  If there's data but flush is not necessary,
         * the request can be issued directly.
         *
         * Flush w/o data should be able to be issued directly too but
         * currently some drivers assume that rq->bio contains
         * non-zero data if it isn't NULL and empty FLUSH requests
         * getting here usually have bio's without data.
         */
        if (blk_rq_sectors(rq) && !do_preflush && !do_postflush) {
                rq->cmd_flags &= ~REQ_FLUSH;
                if (!has_fua)
                        rq->cmd_flags &= ~REQ_FUA;
                return rq;
        }

I tested this patch successfully on an HP EVA that was showing the problem.  I believe this is the real cause of (and fix for) the regression.

(In reply to comment #24)
> Created attachment 516204 [details]
> fix the regression introduced by 7eb9ebc7

I take it all back.  ;-)  My test setup was flawed, this does not fix the problem.

The flush/merge patch changes the following:

 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;
                }

And it also gets rid of blk_do_flush, which used to look like this:

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, flush requests inserted via blk_insert_cloned_request would actually traverse through the flush machinery, and then get FLUSH|FUA stripped off in blk_do_flush.

With the new code, the request with FLUSH|FUA set does not go through the flush machinery at all, instead going straight to the scsi request fn with FLUSH|FUA set.  So, we see 20ms flush I/Os on the EVA (since I guess it does something with those flushes?).

Deciding where to fix this is up for discussion.  We could:

A) put checks in blk_insert_cloned_request, stripping whatever flags the queue doesn't support.  This would take care of any layered drivers, such as dm-mpath.

B) put checks back into __elv_next_request.  This would bring us closer to what the code previously did.  This, I believe, casts an even wider net, so may be more robust than A.

C) accept the patch Mike posted to not send down requests from dm-mpath and ignore everything else.

I posed the question upstream:
https://lkml.org/lkml/2011/8/2/259

Patch(es) available on kernel-2.6.32-189.el6

Have verified kernel-2.6.32-189.el6 has no regression with the AIM workload on redeye.

Barry

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHSA-2011-1530.html