Re: raid5: direct IO and md_wakeup_thread

[NeilBrown <neilb@suse.de>]

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

On Fri, 5 Sep 2014 17:52:53 +0000 Markus Stockhausen
<stockhausen@collogia.de> 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

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