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

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.

 
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.

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?

Thanks,
NeilBrown

void do_release_stripe(struct r5conf *conf, struct stripe_head *sh,
                              struct list_head *temp_inactive_list, int mstwake)
{
        BUG_ON(!list_empty(&sh->lru));
        BUG_ON(atomic_read(&conf->active_stripes)==0);
        if (test_bit(STRIPE_HANDLE, &sh->state)) {
                if (test_bit(STRIPE_DELAYED, &sh->state) &&
                    !test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) {
                        list_add_tail(&sh->lru, &conf->delayed_list);
                        if (atomic_read(&conf->preread_active_stripes)
                            < IO_THRESHOLD)
                                md_wakeup_thread(conf->mddev->thread);
                } else if (test_bit(STRIPE_BIT_DELAY, &sh->state) &&
                        ...
                }
                md_wakeup_thread(conf->mddev->thread);
                ...

Thanks in advance.

Markus