On Mon, 2 Jul 2012 10:59:50 +0800 Shaohua Li <shli@xxxxxxxxxx> wrote:
> > From 04b7dd7d0ad4a21622cad7c10821f914a8d9ccd3 Mon Sep 17 00:00:00 2001
> > From: NeilBrown <neilb@xxxxxxx>
> > Date: Mon, 2 Jul 2012 12:14:49 +1000
> > Subject: [PATCH] md/plug: disable preempt when reported a plug is present.
> >
> > As 'schedule' will unplug a queue, a plug added by
> > mddev_check_plugged is only valid until the next schedule().
> > So call preempt_disable before installing the plug, and require the
> > called to call preempt_enable once the value has been used.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 1369c9d..63ea6d6 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -512,6 +512,10 @@ static void plugger_unplug(struct blk_plug_cb *cb)
> >
> > /* Check that an unplug wakeup will come shortly.
> > * If not, wakeup the md thread immediately
> > + * Note that the structure returned is only value until
> > + * the next schedule(), so preemption is disabled when it
> > + * is not NULL, and must be re-enabled after the value
> > + * has been used.
> > */
> > struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
> > md_unplug_func_t unplug, size_t size)
> > @@ -522,6 +526,7 @@ struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
> > if (!plug)
> > return NULL;
> >
> > + preempt_disable();
> > list_for_each_entry(mdcb, &plug->cb_list, cb.list) {
> > if (mdcb->cb.callback == plugger_unplug &&
> > mdcb->mddev == mddev) {
> > @@ -533,6 +538,7 @@ struct md_plug_cb *mddev_check_plugged(struct mddev *mddev,
> > return mdcb;
> > }
> > }
> > + preempt_enable();
>
> preempt doesn't do unplug, only yield(schedule) does, so I don't like this,
> just redoing mddev_check_plugged before checking the return value is fine to
> me.
<digs through code...>
Ahh, I see. preempt calls _schedule(), not schedule(), and schedule() does
the unplug before calling _schedule().
So you are right - I'll remove that extra patch.
Thanks.
> > - if (do_sync || !bitmap || !plugged)
> > + if (do_sync ||
> > + !mddev_check_plugged(mddev, NULL, 0))
> > md_wakeup_thread(mddev->thread);
>
> Do we really bother to recheck here? just a wakeup.
>
This isn't a re-check. This is the one-and-only check.
In the current code each request gets queued for raid1d and the all get
processed next time the thread wakes up. So we would prefer to create the
plug and do the wakeup later. But if creating the plug fails, we do the
wakeup immediately.
Once we are queueing the requests in the plug, this will change.
NeilBrown
Attachment:
signature.asc
Description: PGP signature
