Von: NeilBrown [neilb@suse.de]
Gesendet: Montag, 8. September 2014 08:46
An: Markus Stockhausen
Cc: linux-raid@vger.kernel.org
Betreff: Re: raid5: direct IO and md_wakeup_thread

On Fri, 5 Sep 2014 17:52:53 +0000 Markus Stockhausen
[off-list ref] wrote:

quoted

Hi Neil,

I hope you are doing better after the flu. Maybe you find some time
to explain the following codings in raid5.c to me. At least one of
the patchs seems to be signed off by you. The RAID5 I'm using has
default settings.

1) Generally speaking of md_wakup_thread() calls. Does it make
any sense to call them the from within raid5d()? Or when being
implemented at locations that might be called from the
make_request() path too - for simplicity no further caller destinction
was programmed.

I doesn't make sense to all md_wakeup_thread() from somewhere that is *only*
called within raid5d().  However most of raid5d() is handle_stripe() and
handle_stripe() is called from other places.  So it could make sence to call
md_wakeup_thread from within handle_stripe().

make_request() very likely wants to call md_wakeup_thread().... though I'm
wondering if I understand that part of the question properly.

quoted

1) In case of a single direct I/O writer to the /dev/md0 device the
following coding will always fire the md_wakeup_thread(). That
is one reason for alternating raid5d loops one with data to
process and the next without. Is that corner case wanted?

Is this a problem?  We often do things just in case they are needed,
providing the cost isn't too great.

If there was a measurable cost it might make sense to

    clear_bit(THREAD_WAKEUP, &mddev->thread->flags);

in raid5d() at the top of the while loop in raid5d().  We would need to move
the md_check_recovery() call to the end of the function then.

quoted

static void handle_stripe(struct stripe_head *sh)
        ...
        ops_run_io(sh, &s);

        if (s.dec_preread_active) {
                /* We delay this until after ops_run_io so that if make_request
                 * is waiting on a flush, it won't continue until the writes
                 * have actually been submitted.
                 */
                atomic_dec(&conf->preread_active_stripes);
                if (atomic_read(&conf->preread_active_stripes) <
                    IO_THRESHOLD)
                        md_wakeup_thread(conf->mddev->thread); */
        }

2) The wakeup_thread() in the following preread path seems
to be unnecesary or will a double call wake the thread twice?

Unnecessary in some cases, necessary in others.  Tracking exactly when it is
necessary would be complex and error prone.

Are you just trying to understand the code, or is there something that you
think is a problem that could be fixed?

HI Neil,

thanks for the explanations. I tried to understand the reason for the
"bad" benchmark numbers during my direct IO write tests for my rmw
patch. I only choose it to avoid caching side effects. Even with 64 
threads there is still a large gap to the theoretical maximum. So I 
reduced the test to 1 writer and took some time to have a look how 
raid5.c handles sync/direct writes. See my other post with only 5MB/s
4K direct writes to ramdisk backed raid5 md. 

With that insight I started to compare it to raid1.c. After some 
iterations it came to my mind that the empty raid5d() runs between
two sync writes might give extra latency and so I looked for corners 
where md_wakeup_thread() might be unnecessary. The result are
the questions above.

I'll give the clear_bit(THREAD_WAKEUP,...) a try.

Markus