From: Jens Axboe <axboe@fb.com>
To: xiakaixu <xiakaixu@huawei.com>
Cc: <linux-kernel@vger.kernel.org>, <linux-fsdevel@vger.kernel.org>,
<linux-block@vger.kernel.org>, <jack@suse.cz>,
<dchinner@redhat.com>, "miaoxie (A)" <miaoxie@huawei.com>,
Bintian <bintian.wang@huawei.com>, Huxinwei <huxinwei@huawei.com>
Subject: Re: [PATCH 8/8] writeback: throttle buffered writeback
Date: Mon, 25 Apr 2016 08:37:49 -0600 [thread overview]
Message-ID: <571E2BBD.7040804@fb.com> (raw)
In-Reply-To: <571E024C.2020307@huawei.com>
On 04/25/2016 05:41 AM, xiakaixu wrote:
> 于 2016/4/24 5:37, Jens Axboe 写道:
>> On 04/23/2016 02:21 AM, xiakaixu wrote:
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index 40b57bf4852c..d941f69dfb4b 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -39,6 +39,7 @@
>>>>
>>>> #include "blk.h"
>>>> #include "blk-mq.h"
>>>> +#include "blk-wb.h"
>>>>
>>>> EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_remap);
>>>> EXPORT_TRACEPOINT_SYMBOL_GPL(block_rq_remap);
>>>> @@ -880,6 +881,7 @@ blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
>>>>
>>>> fail:
>>>> blk_free_flush_queue(q->fq);
>>>> + blk_wb_exit(q);
>>>> return NULL;
>>>> }
>>>> EXPORT_SYMBOL(blk_init_allocated_queue);
>>>> @@ -1395,6 +1397,7 @@ void blk_requeue_request(struct request_queue *q, struct request *rq)
>>>> blk_delete_timer(rq);
>>>> blk_clear_rq_complete(rq);
>>>> trace_block_rq_requeue(q, rq);
>>>> + blk_wb_requeue(q->rq_wb, rq);
>>>>
>>>> if (rq->cmd_flags & REQ_QUEUED)
>>>> blk_queue_end_tag(q, rq);
>>>> @@ -1485,6 +1488,8 @@ void __blk_put_request(struct request_queue *q, struct request *req)
>>>> /* this is a bio leak */
>>>> WARN_ON(req->bio != NULL);
>>>>
>>>> + blk_wb_done(q->rq_wb, req);
>>>> +
>>>> /*
>>>> * Request may not have originated from ll_rw_blk. if not,
>>>> * it didn't come out of our reserved rq pools
>>>> @@ -1714,6 +1719,7 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
>>>> int el_ret, rw_flags, where = ELEVATOR_INSERT_SORT;
>>>> struct request *req;
>>>> unsigned int request_count = 0;
>>>> + bool wb_acct;
>>>>
>>>> /*
>>>> * low level driver can indicate that it wants pages above a
>>>> @@ -1766,6 +1772,8 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
>>>> }
>>>>
>>>> get_rq:
>>>> + wb_acct = blk_wb_wait(q->rq_wb, bio, q->queue_lock);
>>>> +
>>>> /*
>>>> * This sync check and mask will be re-done in init_request_from_bio(),
>>>> * but we need to set it earlier to expose the sync flag to the
>>>> @@ -1781,11 +1789,16 @@ get_rq:
>>>> */
>>>> req = get_request(q, rw_flags, bio, GFP_NOIO);
>>>> if (IS_ERR(req)) {
>>>> + if (wb_acct)
>>>> + __blk_wb_done(q->rq_wb);
>>>> bio->bi_error = PTR_ERR(req);
>>>> bio_endio(bio);
>>>> goto out_unlock;
>>>> }
>>>>
>>>> + if (wb_acct)
>>>> + req->cmd_flags |= REQ_BUF_INFLIGHT;
>>>> +
>>>> /*
>>>> * After dropping the lock and possibly sleeping here, our request
>>>> * may now be mergeable after it had proven unmergeable (above).
>>>> @@ -2515,6 +2528,7 @@ void blk_start_request(struct request *req)
>>>> blk_dequeue_request(req);
>>>>
>>>> req->issue_time = ktime_to_ns(ktime_get());
>>>> + blk_wb_issue(req->q->rq_wb, req);
>>>>
>>>> /*
>>>> * We are now handing the request to the hardware, initialize
>>>> @@ -2751,6 +2765,7 @@ void blk_finish_request(struct request *req, int error)
>>>> blk_unprep_request(req);
>>>>
>>>> blk_account_io_done(req);
>>>> + blk_wb_done(req->q->rq_wb, req);
>>>
>>> Hi Jens,
>>>
>>> Seems the function blk_wb_done() will be executed twice even if the end_io
>>> callback is set.
>>> Maybe the same thing would happen in blk-mq.c.
>>
>> Yeah, that was a mistake, the current version has it fixed. It was inadvertently added when I discovered that the flush request didn't work properly. Now it just duplicates the call inside the check for if it has an ->end_io() defined, since we don't use the normal path for that.
>>
> Hi Jens,
>
> I have checked the wb-buf-throttle branch in your block git repo. I am not sure it is the completed version.
> Seems only the problem is fixed in blk-mq.c. The function blk_wb_done() still would be executed twice in blk-core.c.
> (the functions blk_finish_request() and __blk_put_request())
> Maybe we can add a flag to mark whether blk_wb_done() has been done or not.
Good catch, looks like I did only patch up the mq bits. It's still not
perfect, since we could potentially double account a request that has a
private end_io(), if it was allocated through the normal block rq
allocator. It'll skew the unrelated-io-timestamp a bit, but it's not a
big deal. The count for inflight will be consistent, which is the
important part.
We currently have just 1 bit to tell if the request is tracked or not,
so we don't know if it was tracked but already seen.
I'll fix up the blk-core part to be identical to the blk-mq fix.
--
Jens Axboe
next prev parent reply other threads:[~2016-04-25 14:38 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-18 4:24 [PATCHSET v4 0/8] Make background writeback not suck Jens Axboe
2016-04-18 4:24 ` [PATCH 1/8] block: add WRITE_BG Jens Axboe
2016-04-18 4:24 ` [PATCH 2/8] writeback: add wbc_to_write_cmd() Jens Axboe
2016-04-18 15:12 ` Jan Kara
2016-04-18 20:18 ` Jens Axboe
2016-04-18 4:24 ` [PATCH 3/8] writeback: use WRITE_BG for kupdate and background writeback Jens Axboe
2016-04-18 4:24 ` [PATCH 4/8] writeback: track if we're sleeping on progress in balance_dirty_pages() Jens Axboe
2016-04-18 4:24 ` [PATCH 5/8] writeback: increment page wait count when waiting Jens Axboe
2016-04-18 4:24 ` [PATCH 6/8] block: add code to track actual device queue depth Jens Axboe
2016-04-18 4:24 ` [PATCH 7/8] block: add scalable completion tracking of requests Jens Axboe
2016-04-18 4:24 ` [PATCH 8/8] writeback: throttle buffered writeback Jens Axboe
2016-04-23 8:21 ` xiakaixu
2016-04-23 21:37 ` Jens Axboe
2016-04-25 11:41 ` xiakaixu
2016-04-25 14:37 ` Jens Axboe [this message]
2016-04-26 7:04 ` [PATCHSET v4 0/8] Make background writeback not suck Sedat Dilek
2016-04-26 15:07 ` Jens Axboe
-- strict thread matches above, loose matches on Subject: below --
2016-09-07 14:46 [PATCH 0/8] Throttled background buffered writeback v7 Jens Axboe
2016-09-07 14:46 ` [PATCH 8/8] writeback: throttle buffered writeback Jens Axboe
2016-08-31 17:05 [PATCHSET v6] Throttled background " Jens Axboe
2016-08-31 17:05 ` [PATCH 8/8] writeback: throttle " Jens Axboe
2016-04-26 15:55 [PATCHSET v5] Make background writeback great again for the first time Jens Axboe
2016-04-26 15:55 ` [PATCH 8/8] writeback: throttle buffered writeback Jens Axboe
2016-03-23 15:25 Jens Axboe
2016-03-23 15:25 ` [PATCH 8/8] writeback: throttle buffered writeback Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=571E2BBD.7040804@fb.com \
--to=axboe@fb.com \
--cc=bintian.wang@huawei.com \
--cc=dchinner@redhat.com \
--cc=huxinwei@huawei.com \
--cc=jack@suse.cz \
--cc=linux-block@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=miaoxie@huawei.com \
--cc=xiakaixu@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).