* [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
* 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
* [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 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).