Re: [PATCH 05/10] block: remove per-queue plugging

[[NOTE to dm-devel people - one of the patches here remove some
now-unused code from dm-raid.c plus a declaration from device-mapper.h ]]]


On Mon, 18 Apr 2011 10:10:18 +0200 Jens Axboe wrote:

> On 2011-04-18 09:25, NeilBrown wrote:

> >>> Therefore md cannot put anything useful on the list in 'struct blk_plug'.
> >
> > That is the heart of the problem.
>
> Hmm, I don't really see a way to avoid the list in that case. You really
> do need some way to queue items, a single callback or flag or pointer
> will not suffice.
>
> I've added the patch and removed the (now) useless ->unplugged_fn
> callback. I suggest you base your md changes on top of my for-linus
> branch and tell me when you are confident it looks good, then I'll pull
> in your MD changes and submit them later today.
>
> OK with you?
>

Yes, that's perfect. Thanks.

All of my plugging-related patches are now in a 'for-jens' branch:

The following changes since commit 99e22598e9a8e0a996d69c8c0f6b7027cb57720a:

block: drop queue lock before calling __blk_run_queue() for kblockd punt (2011-04-18 09:59:55 +0200)

are available in the git repository at:
git://neil.brown.name/md for-jens

NeilBrown (6):
md: use new plugging interface for RAID IO.
md/dm - remove remains of plug_fn callback.
md - remove old plugging code.
md: provide generic support for handling unplug callbacks.
md: incorporate new plugging into raid5.
md: fix up raid1/raid10 unplugging.

drivers/md/dm-raid.c | 8 ----
drivers/md/md.c | 87 +++++++++++++++++++++-------------------
drivers/md/md.h | 26 ++----------
drivers/md/raid1.c | 29 +++++++-------
drivers/md/raid10.c | 27 ++++++-------
drivers/md/raid5.c | 61 ++++++++++++----------------
drivers/md/raid5.h | 2 -
include/linux/device-mapper.h | 1 -
8 files changed, 103 insertions(+), 138 deletions(-)


Thanks,
NeilBrown

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] block: remove per-queue plugging

On 2011-04-18 10:33, NeilBrown wrote:
>
>
> [[NOTE to dm-devel people - one of the patches here remove some
> now-unused code from dm-raid.c plus a declaration from device-mapper.h ]]]
>
>
> On Mon, 18 Apr 2011 10:10:18 +0200 Jens Axboe wrote:
>
>> On 2011-04-18 09:25, NeilBrown wrote:
>
>>>>> Therefore md cannot put anything useful on the list in 'struct blk_plug'.
>>>
>>> That is the heart of the problem.
>>
>> Hmm, I don't really see a way to avoid the list in that case. You really
>> do need some way to queue items, a single callback or flag or pointer
>> will not suffice.
>>
>> I've added the patch and removed the (now) useless ->unplugged_fn
>> callback. I suggest you base your md changes on top of my for-linus
>> branch and tell me when you are confident it looks good, then I'll pull
>> in your MD changes and submit them later today.
>>
>> OK with you?
>>
>
> Yes, that's perfect. Thanks.
>
> All of my plugging-related patches are now in a 'for-jens' branch:
>
> The following changes since commit 99e22598e9a8e0a996d69c8c0f6b7027cb57720a:
>
> block: drop queue lock before calling __blk_run_queue() for kblockd punt (2011-04-18 09:59:55 +0200)
>
> are available in the git repository at:
> git://neil.brown.name/md for-jens
>
> NeilBrown (6):
> md: use new plugging interface for RAID IO.
> md/dm - remove remains of plug_fn callback.
> md - remove old plugging code.
> md: provide generic support for handling unplug callbacks.
> md: incorporate new plugging into raid5.
> md: fix up raid1/raid10 unplugging.
>
> drivers/md/dm-raid.c | 8 ----
> drivers/md/md.c | 87 +++++++++++++++++++++-------------------
> drivers/md/md.h | 26 ++----------
> drivers/md/raid1.c | 29 +++++++-------
> drivers/md/raid10.c | 27 ++++++-------
> drivers/md/raid5.c | 61 ++++++++++++----------------
> drivers/md/raid5.h | 2 -
> include/linux/device-mapper.h | 1 -
> 8 files changed, 103 insertions(+), 138 deletions(-)

Great, thanks a lot Neil! It's pulled in now, will send the request to
Linus today.

--
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] block: remove per-queue plugging

> NeilBrown (6):
> md: use new plugging interface for RAID IO.
> md/dm - remove remains of plug_fn callback.
> md - remove old plugging code.
> md: provide generic support for handling unplug callbacks.
> md: incorporate new plugging into raid5.
> md: fix up raid1/raid10 unplugging.

Looking over more of the unplugging left over, is there a reason to
keep the unplug_work bits in CFQ? They seem to rather counter the
current scheme (and it is the last user of kblockd outside of
blk-core.c)

Re: [PATCH 05/10] block: remove per-queue plugging

> md: provide generic support for handling unplug callbacks.

This looks like some horribly ugly code to me. The real fix is to do
the plugging in the block layers for bios instead of requests. The
effect should be about the same, except that merging will become a
little easier as all bios will be on the list now when calling into
__make_request or it's equivalent, and even better if we extent the
list sort callback to also sort by the start block it will actually
simplify the merge algorithm a lot as it only needs to do front merges
and no back merges for the on-stack merging.

In addition it should also allow for much more optimal queue_lock
roundtrips - we can keep it locked at the end of what's currently
__make_request to have it available for the next bio that's been
on the list. If it either can be merged now that we have the lock
and/or we optimize get_request_wait not to sleep in the fast path
we could get down to a single queue_lock roundtrip for each unplug.

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html