* [PATCH 0/2] block/loop: improve performance
@ 2017-08-23 23:49 Shaohua Li
2017-08-23 23:49 ` [PATCH 1/2] block/loop: set hw_sectors Shaohua Li
2017-08-23 23:49 ` [PATCH 2/2] block/loop: allow request merge for directio mode Shaohua Li
0 siblings, 2 replies; 6+ messages in thread
From: Shaohua Li @ 2017-08-23 23:49 UTC (permalink / raw)
To: linux-block; +Cc: axboe, Kernel-team, Shaohua Li
From: Shaohua Li <shli@fb.com>
two small patches to improve performance for loop in directio mode. The goal is
to increase IO size sending to underlayer disks.
Thanks,
Shaohua
Shaohua Li (2):
block/loop: set hw_sectors
block/loop: allow request merge for directio mode
drivers/block/loop.c | 44 ++++++++++++++++++++++++++++++++++++--------
drivers/block/loop.h | 1 +
2 files changed, 37 insertions(+), 8 deletions(-)
--
2.9.5
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] block/loop: set hw_sectors
2017-08-23 23:49 [PATCH 0/2] block/loop: improve performance Shaohua Li
@ 2017-08-23 23:49 ` Shaohua Li
2017-08-24 17:36 ` Omar Sandoval
2017-08-23 23:49 ` [PATCH 2/2] block/loop: allow request merge for directio mode Shaohua Li
1 sibling, 1 reply; 6+ messages in thread
From: Shaohua Li @ 2017-08-23 23:49 UTC (permalink / raw)
To: linux-block; +Cc: axboe, Kernel-team, Shaohua Li
From: Shaohua Li <shli@fb.com>
Loop can handle any size of request. Limiting it to 255 sectors just
burns the CPU for bio split and request merge for underlayer disk and
also cause bad fs block allocation in directio mode.
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/block/loop.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index b55a1f8..428da07 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1799,6 +1799,7 @@ static int loop_add(struct loop_device **l, int i)
}
lo->lo_queue->queuedata = lo;
+ blk_queue_max_hw_sectors(lo->lo_queue, BLK_DEF_MAX_SECTORS);
/*
* It doesn't make sense to enable merge because the I/O
* submitted to backing file is handled page by page.
--
2.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] block/loop: allow request merge for directio mode
2017-08-23 23:49 [PATCH 0/2] block/loop: improve performance Shaohua Li
2017-08-23 23:49 ` [PATCH 1/2] block/loop: set hw_sectors Shaohua Li
@ 2017-08-23 23:49 ` Shaohua Li
2017-08-24 17:57 ` Omar Sandoval
1 sibling, 1 reply; 6+ messages in thread
From: Shaohua Li @ 2017-08-23 23:49 UTC (permalink / raw)
To: linux-block; +Cc: axboe, Kernel-team, Shaohua Li
From: Shaohua Li <shli@fb.com>
Currently loop disables merge. While it makes sense for buffer IO mode,
directio mode can benefit from request merge. Without merge, loop could
send small size IO to underlayer disk and harm performance.
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/block/loop.c | 43 +++++++++++++++++++++++++++++++++++--------
drivers/block/loop.h | 1 +
2 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 428da07..1bfa3ff 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -213,10 +213,13 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
*/
blk_mq_freeze_queue(lo->lo_queue);
lo->use_dio = use_dio;
- if (use_dio)
+ if (use_dio) {
+ queue_flag_clear_unlocked(QUEUE_FLAG_NOMERGES, lo->lo_queue);
lo->lo_flags |= LO_FLAGS_DIRECT_IO;
- else
+ } else {
+ queue_flag_set_unlocked(QUEUE_FLAG_NOMERGES, lo->lo_queue);
lo->lo_flags &= ~LO_FLAGS_DIRECT_IO;
+ }
blk_mq_unfreeze_queue(lo->lo_queue);
}
@@ -464,6 +467,8 @@ static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2)
{
struct loop_cmd *cmd = container_of(iocb, struct loop_cmd, iocb);
+ kfree(cmd->bvec);
+ cmd->bvec = NULL;
cmd->ret = ret;
blk_mq_complete_request(cmd->rq);
}
@@ -473,22 +478,44 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
{
struct iov_iter iter;
struct bio_vec *bvec;
- struct bio *bio = cmd->rq->bio;
+ struct request *rq = cmd->rq;
+ struct bio *bio = rq->bio;
struct file *file = lo->lo_backing_file;
+ unsigned int offset;
+ int segments = 0;
int ret;
- /* nomerge for loop request queue */
- WARN_ON(cmd->rq->bio != cmd->rq->biotail);
+ if (rq->bio != rq->biotail) {
+ struct req_iterator iter;
+ struct bio_vec tmp;
+
+ __rq_for_each_bio(bio, rq)
+ segments += bio_segments(bio);
+ bvec = kmalloc(sizeof(struct bio_vec) * segments, GFP_KERNEL);
+ if (!bvec)
+ return -EIO;
+ cmd->bvec = bvec;
+
+ rq_for_each_segment(tmp, rq, iter) {
+ *bvec = tmp;
+ bvec++;
+ }
+ bvec = cmd->bvec;
+ offset = 0;
+ } else {
+ offset = bio->bi_iter.bi_bvec_done;
+ bvec = __bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter);
+ segments = bio_segments(bio);
+ }
- bvec = __bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter);
iov_iter_bvec(&iter, ITER_BVEC | rw, bvec,
- bio_segments(bio), blk_rq_bytes(cmd->rq));
+ segments, blk_rq_bytes(rq));
/*
* This bio may be started from the middle of the 'bvec'
* because of bio splitting, so offset from the bvec must
* be passed to iov iterator
*/
- iter.iov_offset = bio->bi_iter.bi_bvec_done;
+ iter.iov_offset = offset;
cmd->iocb.ki_pos = pos;
cmd->iocb.ki_filp = file;
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index fecd3f9..bc9cf91 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -72,6 +72,7 @@ struct loop_cmd {
bool use_aio; /* use AIO interface to handle I/O */
long ret;
struct kiocb iocb;
+ struct bio_vec *bvec;
};
/* Support for loadable transfer modules */
--
2.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] block/loop: set hw_sectors
2017-08-23 23:49 ` [PATCH 1/2] block/loop: set hw_sectors Shaohua Li
@ 2017-08-24 17:36 ` Omar Sandoval
0 siblings, 0 replies; 6+ messages in thread
From: Omar Sandoval @ 2017-08-24 17:36 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-block, axboe, Kernel-team, Shaohua Li
On Wed, Aug 23, 2017 at 04:49:23PM -0700, Shaohua Li wrote:
> From: Shaohua Li <shli@fb.com>
>
> Loop can handle any size of request. Limiting it to 255 sectors just
> burns the CPU for bio split and request merge for underlayer disk and
> also cause bad fs block allocation in directio mode.
Reviewed-by: Omar Sandoval <osandov@fb.com>
Note that this will conflict with my loop blocksize series, we can fix
up whichever series goes in second.
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/block/loop.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index b55a1f8..428da07 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1799,6 +1799,7 @@ static int loop_add(struct loop_device **l, int i)
> }
> lo->lo_queue->queuedata = lo;
>
> + blk_queue_max_hw_sectors(lo->lo_queue, BLK_DEF_MAX_SECTORS);
> /*
> * It doesn't make sense to enable merge because the I/O
> * submitted to backing file is handled page by page.
> --
> 2.9.5
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] block/loop: allow request merge for directio mode
2017-08-23 23:49 ` [PATCH 2/2] block/loop: allow request merge for directio mode Shaohua Li
@ 2017-08-24 17:57 ` Omar Sandoval
2017-08-24 18:13 ` Shaohua Li
0 siblings, 1 reply; 6+ messages in thread
From: Omar Sandoval @ 2017-08-24 17:57 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-block, axboe, Kernel-team, Shaohua Li
On Wed, Aug 23, 2017 at 04:49:24PM -0700, Shaohua Li wrote:
> From: Shaohua Li <shli@fb.com>
>
> Currently loop disables merge. While it makes sense for buffer IO mode,
> directio mode can benefit from request merge. Without merge, loop could
> send small size IO to underlayer disk and harm performance.
A couple of nits on comments below, besides that
Reviewed-by: Omar Sandoval <osandov@fb.com>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/block/loop.c | 43 +++++++++++++++++++++++++++++++++++--------
> drivers/block/loop.h | 1 +
> 2 files changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 428da07..1bfa3ff 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -213,10 +213,13 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
> */
> blk_mq_freeze_queue(lo->lo_queue);
> lo->use_dio = use_dio;
> - if (use_dio)
> + if (use_dio) {
> + queue_flag_clear_unlocked(QUEUE_FLAG_NOMERGES, lo->lo_queue);
> lo->lo_flags |= LO_FLAGS_DIRECT_IO;
> - else
> + } else {
> + queue_flag_set_unlocked(QUEUE_FLAG_NOMERGES, lo->lo_queue);
> lo->lo_flags &= ~LO_FLAGS_DIRECT_IO;
> + }
Could you please also update the comment in loop_add() where we set
QUEUE_FLAG_NOMERGES to say something like
/*
* By default we do buffered I/O, so it doesn't make sense to
* enable merging because the I/O submitted to backing file is
* handled page by page. We enable merging when switching to
* direct I/O mode.
*/
> blk_mq_unfreeze_queue(lo->lo_queue);
> }
>
> @@ -464,6 +467,8 @@ static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2)
> {
> struct loop_cmd *cmd = container_of(iocb, struct loop_cmd, iocb);
>
> + kfree(cmd->bvec);
> + cmd->bvec = NULL;
> cmd->ret = ret;
> blk_mq_complete_request(cmd->rq);
> }
> @@ -473,22 +478,44 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
> {
> struct iov_iter iter;
> struct bio_vec *bvec;
> - struct bio *bio = cmd->rq->bio;
> + struct request *rq = cmd->rq;
> + struct bio *bio = rq->bio;
> struct file *file = lo->lo_backing_file;
> + unsigned int offset;
> + int segments = 0;
> int ret;
>
> - /* nomerge for loop request queue */
> - WARN_ON(cmd->rq->bio != cmd->rq->biotail);
> + if (rq->bio != rq->biotail) {
> + struct req_iterator iter;
> + struct bio_vec tmp;
> +
> + __rq_for_each_bio(bio, rq)
> + segments += bio_segments(bio);
> + bvec = kmalloc(sizeof(struct bio_vec) * segments, GFP_KERNEL);
> + if (!bvec)
> + return -EIO;
> + cmd->bvec = bvec;
> +
> + rq_for_each_segment(tmp, rq, iter) {
> + *bvec = tmp;
> + bvec++;
> + }
> + bvec = cmd->bvec;
> + offset = 0;
> + } else {
> + offset = bio->bi_iter.bi_bvec_done;
> + bvec = __bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter);
> + segments = bio_segments(bio);
> + }
>
> - bvec = __bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter);
> iov_iter_bvec(&iter, ITER_BVEC | rw, bvec,
> - bio_segments(bio), blk_rq_bytes(cmd->rq));
> + segments, blk_rq_bytes(rq));
> /*
> * This bio may be started from the middle of the 'bvec'
> * because of bio splitting, so offset from the bvec must
> * be passed to iov iterator
> */
This comment should be moved above to where you do
offset = bio->bi_iter.bi_bvec_done;
> - iter.iov_offset = bio->bi_iter.bi_bvec_done;
> + iter.iov_offset = offset;
>
> cmd->iocb.ki_pos = pos;
> cmd->iocb.ki_filp = file;
> diff --git a/drivers/block/loop.h b/drivers/block/loop.h
> index fecd3f9..bc9cf91 100644
> --- a/drivers/block/loop.h
> +++ b/drivers/block/loop.h
> @@ -72,6 +72,7 @@ struct loop_cmd {
> bool use_aio; /* use AIO interface to handle I/O */
> long ret;
> struct kiocb iocb;
> + struct bio_vec *bvec;
> };
>
> /* Support for loadable transfer modules */
> --
> 2.9.5
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] block/loop: allow request merge for directio mode
2017-08-24 17:57 ` Omar Sandoval
@ 2017-08-24 18:13 ` Shaohua Li
0 siblings, 0 replies; 6+ messages in thread
From: Shaohua Li @ 2017-08-24 18:13 UTC (permalink / raw)
To: Omar Sandoval; +Cc: linux-block, axboe, Kernel-team, Shaohua Li
On Thu, Aug 24, 2017 at 10:57:39AM -0700, Omar Sandoval wrote:
> On Wed, Aug 23, 2017 at 04:49:24PM -0700, Shaohua Li wrote:
> > From: Shaohua Li <shli@fb.com>
> >
> > Currently loop disables merge. While it makes sense for buffer IO mode,
> > directio mode can benefit from request merge. Without merge, loop could
> > send small size IO to underlayer disk and harm performance.
>
> A couple of nits on comments below, besides that
>
> Reviewed-by: Omar Sandoval <osandov@fb.com>
>
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> > drivers/block/loop.c | 43 +++++++++++++++++++++++++++++++++++--------
> > drivers/block/loop.h | 1 +
> > 2 files changed, 36 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 428da07..1bfa3ff 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -213,10 +213,13 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
> > */
> > blk_mq_freeze_queue(lo->lo_queue);
> > lo->use_dio = use_dio;
> > - if (use_dio)
> > + if (use_dio) {
> > + queue_flag_clear_unlocked(QUEUE_FLAG_NOMERGES, lo->lo_queue);
> > lo->lo_flags |= LO_FLAGS_DIRECT_IO;
> > - else
> > + } else {
> > + queue_flag_set_unlocked(QUEUE_FLAG_NOMERGES, lo->lo_queue);
> > lo->lo_flags &= ~LO_FLAGS_DIRECT_IO;
> > + }
>
> Could you please also update the comment in loop_add() where we set
> QUEUE_FLAG_NOMERGES to say something like
>
> /*
> * By default we do buffered I/O, so it doesn't make sense to
> * enable merging because the I/O submitted to backing file is
> * handled page by page. We enable merging when switching to
> * direct I/O mode.
> */
sure, will add
> > blk_mq_unfreeze_queue(lo->lo_queue);
> > }
> >
> > @@ -464,6 +467,8 @@ static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2)
> > {
> > struct loop_cmd *cmd = container_of(iocb, struct loop_cmd, iocb);
> >
> > + kfree(cmd->bvec);
> > + cmd->bvec = NULL;
> > cmd->ret = ret;
> > blk_mq_complete_request(cmd->rq);
> > }
> > @@ -473,22 +478,44 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
> > {
> > struct iov_iter iter;
> > struct bio_vec *bvec;
> > - struct bio *bio = cmd->rq->bio;
> > + struct request *rq = cmd->rq;
> > + struct bio *bio = rq->bio;
> > struct file *file = lo->lo_backing_file;
> > + unsigned int offset;
> > + int segments = 0;
> > int ret;
> >
> > - /* nomerge for loop request queue */
> > - WARN_ON(cmd->rq->bio != cmd->rq->biotail);
> > + if (rq->bio != rq->biotail) {
> > + struct req_iterator iter;
> > + struct bio_vec tmp;
> > +
> > + __rq_for_each_bio(bio, rq)
> > + segments += bio_segments(bio);
> > + bvec = kmalloc(sizeof(struct bio_vec) * segments, GFP_KERNEL);
> > + if (!bvec)
> > + return -EIO;
> > + cmd->bvec = bvec;
> > +
> > + rq_for_each_segment(tmp, rq, iter) {
> > + *bvec = tmp;
> > + bvec++;
> > + }
> > + bvec = cmd->bvec;
> > + offset = 0;
> > + } else {
> > + offset = bio->bi_iter.bi_bvec_done;
> > + bvec = __bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter);
> > + segments = bio_segments(bio);
> > + }
> >
> > - bvec = __bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter);
> > iov_iter_bvec(&iter, ITER_BVEC | rw, bvec,
> > - bio_segments(bio), blk_rq_bytes(cmd->rq));
> > + segments, blk_rq_bytes(rq));
> > /*
> > * This bio may be started from the middle of the 'bvec'
> > * because of bio splitting, so offset from the bvec must
> > * be passed to iov iterator
> > */
>
> This comment should be moved above to where you do
>
> offset = bio->bi_iter.bi_bvec_done;
This comment actually applies to 'rq->bio != rq->biotail' case too for
each bio of the request. That's why I don't just copy bio->bi_io_vec.
I'll explain this in different way.
Thanks,
Shaohua
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-24 18:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-23 23:49 [PATCH 0/2] block/loop: improve performance Shaohua Li
2017-08-23 23:49 ` [PATCH 1/2] block/loop: set hw_sectors Shaohua Li
2017-08-24 17:36 ` Omar Sandoval
2017-08-23 23:49 ` [PATCH 2/2] block/loop: allow request merge for directio mode Shaohua Li
2017-08-24 17:57 ` Omar Sandoval
2017-08-24 18:13 ` Shaohua Li
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).