Re: [PATCH v3 1/2] bio: limit bio max size
> On 2021/01/26 18:37, Changheun Lee wrote:
> > bio size can grow up to 4GB when muli-page bvec is enabled.
> > but sometimes it would lead to inefficient behaviors.
> > in case of large chunk direct I/O, - 32MB chunk read in user space -
> > all pages for 32MB would be merged to a bio structure if the pages
> > physical addresses are contiguous. it makes some delay to submit
> > until merge complete. bio max size should be limited to a proper size.
> >
> > When 32MB chunk read with direct I/O option is coming from userspace,
> > kernel behavior is below now. it's timeline.
> >
> > | bio merge for 32MB. total 8,192 pages are merged.
> > | total elapsed time is over 2ms.
> > |-- ... --->|
> > | 8,192 pages merged a bio.
> > | at this time, first bio
> > submit is done.
> > | 1 bio is split to 32
> > read request and issue.
> > |--->
> > |--->
> >|--->
> > ..
> >
> > |--->
> >
> > |--->|
> > total 19ms elapsed to complete 32MB read done
> > from device. |
> >
> > If bio max size is limited with 1MB, behavior is changed below.
> >
> > | bio merge for 1MB. 256 pages are merged for each bio.
> > | total 32 bio will be made.
> > | total elapsed time is over 2ms. it's same.
> > | but, first bio submit timing is fast. about 100us.
> > |--->|--->|--->|---> ... -->|--->|--->|--->|--->|
> > | 256 pages merged a bio.
> > | at this time, first bio submit is done.
> > | and 1 read request is issued for 1 bio.
> > |--->
> >|--->
> > |--->
> > ..
> > |--->
> > |--->|
> > total 17ms elapsed to complete 32MB read done from device. |
> >
> > As a result, read request issue timing is faster if bio max size is limited.
> > Current kernel behavior with multipage bvec, super large bio can be created.
> > And it lead to delay first I/O request issue.
> >
> > Signed-off-by: Changheun Lee
> > ---
> > block/bio.c| 17 -
> > include/linux/bio.h| 4 +++-
> > include/linux/blkdev.h | 3 +++
> > 3 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/bio.c b/block/bio.c
> > index 1f2cc1fbe283..ec0281889045 100644
> > --- a/block/bio.c
> > +++ b/block/bio.c
> > @@ -287,6 +287,21 @@ void bio_init(struct bio *bio, struct bio_vec *table,
> > }
> > EXPORT_SYMBOL(bio_init);
> >
> > +unsigned int bio_max_size(struct bio *bio)
> > +{
> > + struct request_queue *q;
> > +
> > + if (!bio->bi_disk)
> > + return UINT_MAX;
> > +
> > + q = bio->bi_disk->queue;
> > + if (!blk_queue_limit_bio_size(q))
> > + return UINT_MAX;
> > +
> > + return blk_queue_get_max_sectors(q, bio_op(bio)) << SECTOR_SHIFT;
> > +}
> > +EXPORT_SYMBOL(bio_max_size);
>
> Why export this ? This is only used in this file and in bio.h in bio_full().
OK. I'll remove it.
>
> > +
> > /**
> > * bio_reset - reinitialize a bio
> > * @bio: bio to reset
> > @@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page
> > *page,
> > struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
> >
> > if (page_is_mergeable(bv, page, len, off, same_page)) {
> > - if (bio->bi_iter.bi_size > UINT_MAX - len) {
> > + if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
> > *same_page = false;
> > return false;
> > }
> > diff --git a/include/linux/bio.h b/include/linux/bio.h
> > index 1edda614f7ce..cdb134ca7bf5 100644
> > --- a/include/linux/bio.h
> > +++ b/include/linux/bio.h
> > @@ -100,6 +100,8 @@ static inline void *bio_data(struct bio *bio)
> > return NULL;
> > }
> >
> > +extern unsigned int bio_max_size(struct bio *);
>
> No need for extern.
It's just for compile warning in my test environment.
I'll remove it too. But I think compile warning could be in the other
.c file which includes bio.h. Is it OK?
>
> > +
> > /**
> > * bio_full - check if the bio is full
> > * @bio: bio to check
> > @@ -113,7 +115,7 @@ static inline bool bio_full(struct bio *bio, unsigned
> > len)
> > if (bio->bi_vcnt >= bio->bi_max_vecs)
> > return true;
> >
Re: [PATCH v3 1/2] bio: limit bio max size
> On Tue, Jan 26, 2021 at 06:26:02AM +, Damien Le Moal wrote:
> > On 2021/01/26 15:07, Ming Lei wrote:
> > > On Tue, Jan 26, 2021 at 04:06:06AM +, Damien Le Moal wrote:
> > >> On 2021/01/26 12:58, Ming Lei wrote:
> > >>> On Tue, Jan 26, 2021 at 10:32:34AM +0900, Changheun Lee wrote:
> > bio size can grow up to 4GB when muli-page bvec is enabled.
> > but sometimes it would lead to inefficient behaviors.
> > in case of large chunk direct I/O, - 32MB chunk read in user space -
> > all pages for 32MB would be merged to a bio structure if the pages
> > physical addresses are contiguous. it makes some delay to submit
> > until merge complete. bio max size should be limited to a proper size.
> >
> > When 32MB chunk read with direct I/O option is coming from userspace,
> > kernel behavior is below now. it's timeline.
> >
> > | bio merge for 32MB. total 8,192 pages are merged.
> > | total elapsed time is over 2ms.
> > |-- ... --->|
> > | 8,192 pages merged
> > a bio.
> > | at this time, first
> > bio submit is done.
> > | 1 bio is split to
> > 32 read request and issue.
> > |--->
> > |--->
> > |--->
> > ..
> >
> > |--->
> >
> > |--->|
> > total 19ms elapsed to complete 32MB read
> > done from device. |
> >
> > If bio max size is limited with 1MB, behavior is changed below.
> >
> > | bio merge for 1MB. 256 pages are merged for each bio.
> > | total 32 bio will be made.
> > | total elapsed time is over 2ms. it's same.
> > | but, first bio submit timing is fast. about 100us.
> > |--->|--->|--->|---> ... -->|--->|--->|--->|--->|
> > | 256 pages merged a bio.
> > | at this time, first bio submit is done.
> > | and 1 read request is issued for 1 bio.
> > |--->
> > |--->
> > |--->
> > ..
> > |--->
> > |--->|
> > total 17ms elapsed to complete 32MB read done from device. |
> >
> > As a result, read request issue timing is faster if bio max size is
> > limited.
> > Current kernel behavior with multipage bvec, super large bio can be
> > created.
> > And it lead to delay first I/O request issue.
> >
> > Signed-off-by: Changheun Lee
> > ---
> > block/bio.c| 17 -
> > include/linux/bio.h| 4 +++-
> > include/linux/blkdev.h | 3 +++
> > 3 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/bio.c b/block/bio.c
> > index 1f2cc1fbe283..ec0281889045 100644
> > --- a/block/bio.c
> > +++ b/block/bio.c
> > @@ -287,6 +287,21 @@ void bio_init(struct bio *bio, struct bio_vec
> > *table,
> > }
> > EXPORT_SYMBOL(bio_init);
> >
> > +unsigned int bio_max_size(struct bio *bio)
> > +{
> > + struct request_queue *q;
> > +
> > + if (!bio->bi_disk)
> > + return UINT_MAX;
> > +
> > + q = bio->bi_disk->queue;
> > + if (!blk_queue_limit_bio_size(q))
> > + return UINT_MAX;
> > +
> > + return blk_queue_get_max_sectors(q, bio_op(bio)) <<
> > SECTOR_SHIFT;
> > +}
> > +EXPORT_SYMBOL(bio_max_size);
> > +
> > /**
> > * bio_reset - reinitialize a bio
> > * @bio: bio to reset
> > @@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct
> > page *page,
> > struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
> >
> > if (page_is_mergeable(bv, page, len, off, same_page)) {
> > - if (bio->bi_iter.bi_size > UINT_MAX - len) {
> > + if (bio->bi_iter.bi_size > bio_max_size(bio) -
> > len) {
> > *same_page = false;
> > return false;
> > }
> > >>>
> > >>> So far we don't need bio->bi_disk or
Re: [PATCH v3 1/2] bio: limit bio max size
On 2021/01/27 9:36, Changheun Lee wrote:
>>> +
>>> /**
>>> * bio_reset - reinitialize a bio
>>> * @bio: bio to reset
>>> @@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page
>>> *page,
>>> struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
>>>
>>> if (page_is_mergeable(bv, page, len, off, same_page)) {
>>> - if (bio->bi_iter.bi_size > UINT_MAX - len) {
>>> + if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
>>> *same_page = false;
>>> return false;
>>> }
>>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>>> index 1edda614f7ce..cdb134ca7bf5 100644
>>> --- a/include/linux/bio.h
>>> +++ b/include/linux/bio.h
>>> @@ -100,6 +100,8 @@ static inline void *bio_data(struct bio *bio)
>>> return NULL;
>>> }
>>>
>>> +extern unsigned int bio_max_size(struct bio *);
>>
>> No need for extern.
>
> It's just for compile warning in my test environment.
> I'll remove it too. But I think compile warning could be in the other
> .c file which includes bio.h. Is it OK?
Hmmm... not having extern should not generate a compilation warning. There are
tons of functions declared without extern in header files in the kernel. What
compiler are you using ?
--
Damien Le Moal
Western Digital Research
Re: [PATCH v3 1/2] bio: limit bio max size
> On 2021/01/27 9:36, Changheun Lee wrote:
> >>> +
> >>> /**
> >>> * bio_reset - reinitialize a bio
> >>> * @bio: bio to reset
> >>> @@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct
> >>> page *page,
> >>> struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
> >>>
> >>> if (page_is_mergeable(bv, page, len, off, same_page)) {
> >>> - if (bio->bi_iter.bi_size > UINT_MAX - len) {
> >>> + if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
> >>> *same_page = false;
> >>> return false;
> >>> }
> >>> diff --git a/include/linux/bio.h b/include/linux/bio.h
> >>> index 1edda614f7ce..cdb134ca7bf5 100644
> >>> --- a/include/linux/bio.h
> >>> +++ b/include/linux/bio.h
> >>> @@ -100,6 +100,8 @@ static inline void *bio_data(struct bio *bio)
> >>> return NULL;
> >>> }
> >>>
> >>> +extern unsigned int bio_max_size(struct bio *);
> >>
> >> No need for extern.
> >
> > It's just for compile warning in my test environment.
> > I'll remove it too. But I think compile warning could be in the other
> > .c file which includes bio.h. Is it OK?
>
> Hmmm... not having extern should not generate a compilation warning. There are
> tons of functions declared without extern in header files in the kernel. What
> compiler are you using ?
>
Compiler imformation is below.
CROSS_COMPILE:
android/prebuilts/gcc/linux-x86/aarch64/aarch64-linux-android-4.9/bin/aarch64-linux-android-
CC: android/prebuilts/clang/host/linux-x86/clang-r383902/bin/clang
CLANG_TRIPLE:
android/prebuilts/clang/host/linux-x86/clang-r383902/bin/aarch64-linux-gnu-
CROSS_COMPILE_COMPAT:
android/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.9/bin/arm-linux-androideabi-
>
> --
> Damien Le Moal
> Western Digital Research
>
---
Changheun Lee
Samsung Electronics
Re: [PATCH v3 1/2] bio: limit bio max size
On Tue, Jan 26, 2021 at 11:57:48AM +0800, Ming Lei wrote: > > *same_page = false; > > return false; > > } > > So far we don't need bio->bi_disk or bio->bi_bdev(will be changed in > Christoph's patch) during adding page to bio, so there is null ptr > refereance risk. I've started resurrecting my old plan to always have a valid device in the bio from allocation time on. It is pretty invasive, but probably worth it. Sending out the first prep series right now, the actual changes should be ready for an RFC later this week.
[PATCH v3 1/2] bio: limit bio max size
bio size can grow up to 4GB when muli-page bvec is enabled.
but sometimes it would lead to inefficient behaviors.
in case of large chunk direct I/O, - 32MB chunk read in user space -
all pages for 32MB would be merged to a bio structure if the pages
physical addresses are contiguous. it makes some delay to submit
until merge complete. bio max size should be limited to a proper size.
When 32MB chunk read with direct I/O option is coming from userspace,
kernel behavior is below now. it's timeline.
| bio merge for 32MB. total 8,192 pages are merged.
| total elapsed time is over 2ms.
|-- ... --->|
| 8,192 pages merged a bio.
| at this time, first bio
submit is done.
| 1 bio is split to 32 read
request and issue.
|--->
|--->
|--->
..
|--->
|--->|
total 19ms elapsed to complete 32MB read done from
device. |
If bio max size is limited with 1MB, behavior is changed below.
| bio merge for 1MB. 256 pages are merged for each bio.
| total 32 bio will be made.
| total elapsed time is over 2ms. it's same.
| but, first bio submit timing is fast. about 100us.
|--->|--->|--->|---> ... -->|--->|--->|--->|--->|
| 256 pages merged a bio.
| at this time, first bio submit is done.
| and 1 read request is issued for 1 bio.
|--->
|--->
|--->
..
|--->
|--->|
total 17ms elapsed to complete 32MB read done from device. |
As a result, read request issue timing is faster if bio max size is limited.
Current kernel behavior with multipage bvec, super large bio can be created.
And it lead to delay first I/O request issue.
Signed-off-by: Changheun Lee
---
block/bio.c| 17 -
include/linux/bio.h| 4 +++-
include/linux/blkdev.h | 3 +++
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 1f2cc1fbe283..ec0281889045 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -287,6 +287,21 @@ void bio_init(struct bio *bio, struct bio_vec *table,
}
EXPORT_SYMBOL(bio_init);
+unsigned int bio_max_size(struct bio *bio)
+{
+ struct request_queue *q;
+
+ if (!bio->bi_disk)
+ return UINT_MAX;
+
+ q = bio->bi_disk->queue;
+ if (!blk_queue_limit_bio_size(q))
+ return UINT_MAX;
+
+ return blk_queue_get_max_sectors(q, bio_op(bio)) << SECTOR_SHIFT;
+}
+EXPORT_SYMBOL(bio_max_size);
+
/**
* bio_reset - reinitialize a bio
* @bio: bio to reset
@@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page
*page,
struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
if (page_is_mergeable(bv, page, len, off, same_page)) {
- if (bio->bi_iter.bi_size > UINT_MAX - len) {
+ if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
*same_page = false;
return false;
}
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 1edda614f7ce..cdb134ca7bf5 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -100,6 +100,8 @@ static inline void *bio_data(struct bio *bio)
return NULL;
}
+extern unsigned int bio_max_size(struct bio *);
+
/**
* bio_full - check if the bio is full
* @bio: bio to check
@@ -113,7 +115,7 @@ static inline bool bio_full(struct bio *bio, unsigned len)
if (bio->bi_vcnt >= bio->bi_max_vecs)
return true;
- if (bio->bi_iter.bi_size > UINT_MAX - len)
+ if (bio->bi_iter.bi_size > bio_max_size(bio) - len)
return true;
return false;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f94ee3089e01..3aeab9e7e97b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -621,6 +621,7 @@ struct request_queue {
#define QUEUE_FLAG_RQ_ALLOC_TIME 27/* record rq->alloc_time_ns */
#define QUEUE_FLAG_HCTX_ACTIVE 28 /* at least one blk-mq hctx is active */
#define QUEUE_FLAG_NOWAIT 29 /* device supports NOWAIT */
+#define QUEUE_FLAG_LIMIT_BIO_SIZE 30 /* limit bio size */
#define QUEUE_FLAG_MQ_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) |
Re: [PATCH v3 1/2] bio: limit bio max size
On Tue, Jan 26, 2021 at 06:26:02AM +, Damien Le Moal wrote:
> On 2021/01/26 15:07, Ming Lei wrote:
> > On Tue, Jan 26, 2021 at 04:06:06AM +, Damien Le Moal wrote:
> >> On 2021/01/26 12:58, Ming Lei wrote:
> >>> On Tue, Jan 26, 2021 at 10:32:34AM +0900, Changheun Lee wrote:
> bio size can grow up to 4GB when muli-page bvec is enabled.
> but sometimes it would lead to inefficient behaviors.
> in case of large chunk direct I/O, - 32MB chunk read in user space -
> all pages for 32MB would be merged to a bio structure if the pages
> physical addresses are contiguous. it makes some delay to submit
> until merge complete. bio max size should be limited to a proper size.
>
> When 32MB chunk read with direct I/O option is coming from userspace,
> kernel behavior is below now. it's timeline.
>
> | bio merge for 32MB. total 8,192 pages are merged.
> | total elapsed time is over 2ms.
> |-- ... --->|
> | 8,192 pages merged a
> bio.
> | at this time, first
> bio submit is done.
> | 1 bio is split to 32
> read request and issue.
> |--->
> |--->
> |--->
> ..
>
> |--->
>
> |--->|
> total 19ms elapsed to complete 32MB read done
> from device. |
>
> If bio max size is limited with 1MB, behavior is changed below.
>
> | bio merge for 1MB. 256 pages are merged for each bio.
> | total 32 bio will be made.
> | total elapsed time is over 2ms. it's same.
> | but, first bio submit timing is fast. about 100us.
> |--->|--->|--->|---> ... -->|--->|--->|--->|--->|
> | 256 pages merged a bio.
> | at this time, first bio submit is done.
> | and 1 read request is issued for 1 bio.
> |--->
> |--->
> |--->
> ..
> |--->
> |--->|
> total 17ms elapsed to complete 32MB read done from device. |
>
> As a result, read request issue timing is faster if bio max size is
> limited.
> Current kernel behavior with multipage bvec, super large bio can be
> created.
> And it lead to delay first I/O request issue.
>
> Signed-off-by: Changheun Lee
> ---
> block/bio.c| 17 -
> include/linux/bio.h| 4 +++-
> include/linux/blkdev.h | 3 +++
> 3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 1f2cc1fbe283..ec0281889045 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -287,6 +287,21 @@ void bio_init(struct bio *bio, struct bio_vec
> *table,
> }
> EXPORT_SYMBOL(bio_init);
>
> +unsigned int bio_max_size(struct bio *bio)
> +{
> +struct request_queue *q;
> +
> +if (!bio->bi_disk)
> +return UINT_MAX;
> +
> +q = bio->bi_disk->queue;
> +if (!blk_queue_limit_bio_size(q))
> +return UINT_MAX;
> +
> +return blk_queue_get_max_sectors(q, bio_op(bio)) <<
> SECTOR_SHIFT;
> +}
> +EXPORT_SYMBOL(bio_max_size);
> +
> /**
> * bio_reset - reinitialize a bio
> * @bio:bio to reset
> @@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct
> page *page,
> struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
>
> if (page_is_mergeable(bv, page, len, off, same_page)) {
> -if (bio->bi_iter.bi_size > UINT_MAX - len) {
> +if (bio->bi_iter.bi_size > bio_max_size(bio) -
> len) {
> *same_page = false;
> return false;
> }
> >>>
> >>> So far we don't need bio->bi_disk or bio->bi_bdev(will be changed in
> >>> Christoph's patch) during adding page to bio, so there is null ptr
> >>> refereance risk.
> >>>
> diff --git a/include/linux/bio.h
Re: [PATCH v3 1/2] bio: limit bio max size
On 2021/01/26 18:37, Changheun Lee wrote:
> bio size can grow up to 4GB when muli-page bvec is enabled.
> but sometimes it would lead to inefficient behaviors.
> in case of large chunk direct I/O, - 32MB chunk read in user space -
> all pages for 32MB would be merged to a bio structure if the pages
> physical addresses are contiguous. it makes some delay to submit
> until merge complete. bio max size should be limited to a proper size.
>
> When 32MB chunk read with direct I/O option is coming from userspace,
> kernel behavior is below now. it's timeline.
>
> | bio merge for 32MB. total 8,192 pages are merged.
> | total elapsed time is over 2ms.
> |-- ... --->|
> | 8,192 pages merged a bio.
> | at this time, first bio
> submit is done.
> | 1 bio is split to 32 read
> request and issue.
> |--->
> |--->
>|--->
> ..
>
> |--->
>
> |--->|
> total 19ms elapsed to complete 32MB read done from
> device. |
>
> If bio max size is limited with 1MB, behavior is changed below.
>
> | bio merge for 1MB. 256 pages are merged for each bio.
> | total 32 bio will be made.
> | total elapsed time is over 2ms. it's same.
> | but, first bio submit timing is fast. about 100us.
> |--->|--->|--->|---> ... -->|--->|--->|--->|--->|
> | 256 pages merged a bio.
> | at this time, first bio submit is done.
> | and 1 read request is issued for 1 bio.
> |--->
>|--->
> |--->
> ..
> |--->
> |--->|
> total 17ms elapsed to complete 32MB read done from device. |
>
> As a result, read request issue timing is faster if bio max size is limited.
> Current kernel behavior with multipage bvec, super large bio can be created.
> And it lead to delay first I/O request issue.
>
> Signed-off-by: Changheun Lee
> ---
> block/bio.c| 17 -
> include/linux/bio.h| 4 +++-
> include/linux/blkdev.h | 3 +++
> 3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 1f2cc1fbe283..ec0281889045 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -287,6 +287,21 @@ void bio_init(struct bio *bio, struct bio_vec *table,
> }
> EXPORT_SYMBOL(bio_init);
>
> +unsigned int bio_max_size(struct bio *bio)
> +{
> + struct request_queue *q;
> +
> + if (!bio->bi_disk)
> + return UINT_MAX;
> +
> + q = bio->bi_disk->queue;
> + if (!blk_queue_limit_bio_size(q))
> + return UINT_MAX;
> +
> + return blk_queue_get_max_sectors(q, bio_op(bio)) << SECTOR_SHIFT;
> +}
> +EXPORT_SYMBOL(bio_max_size);
Why export this ? This is only used in this file and in bio.h in bio_full().
> +
> /**
> * bio_reset - reinitialize a bio
> * @bio: bio to reset
> @@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page
> *page,
> struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
>
> if (page_is_mergeable(bv, page, len, off, same_page)) {
> - if (bio->bi_iter.bi_size > UINT_MAX - len) {
> + if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
> *same_page = false;
> return false;
> }
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 1edda614f7ce..cdb134ca7bf5 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -100,6 +100,8 @@ static inline void *bio_data(struct bio *bio)
> return NULL;
> }
>
> +extern unsigned int bio_max_size(struct bio *);
No need for extern.
> +
> /**
> * bio_full - check if the bio is full
> * @bio: bio to check
> @@ -113,7 +115,7 @@ static inline bool bio_full(struct bio *bio, unsigned len)
> if (bio->bi_vcnt >= bio->bi_max_vecs)
> return true;
>
> - if (bio->bi_iter.bi_size > UINT_MAX - len)
> + if (bio->bi_iter.bi_size > bio_max_size(bio) - len)
> return true;
>
> return false;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index f94ee3089e01..3aeab9e7e97b 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -621,6 +621,7 @@ struct request_queue {
> #define
Re: [PATCH v3 1/2] bio: limit bio max size
On 2021/01/26 15:07, Ming Lei wrote:
> On Tue, Jan 26, 2021 at 04:06:06AM +, Damien Le Moal wrote:
>> On 2021/01/26 12:58, Ming Lei wrote:
>>> On Tue, Jan 26, 2021 at 10:32:34AM +0900, Changheun Lee wrote:
bio size can grow up to 4GB when muli-page bvec is enabled.
but sometimes it would lead to inefficient behaviors.
in case of large chunk direct I/O, - 32MB chunk read in user space -
all pages for 32MB would be merged to a bio structure if the pages
physical addresses are contiguous. it makes some delay to submit
until merge complete. bio max size should be limited to a proper size.
When 32MB chunk read with direct I/O option is coming from userspace,
kernel behavior is below now. it's timeline.
| bio merge for 32MB. total 8,192 pages are merged.
| total elapsed time is over 2ms.
|-- ... --->|
| 8,192 pages merged a
bio.
| at this time, first bio
submit is done.
| 1 bio is split to 32
read request and issue.
|--->
|--->
|--->
..
|--->
|--->|
total 19ms elapsed to complete 32MB read done
from device. |
If bio max size is limited with 1MB, behavior is changed below.
| bio merge for 1MB. 256 pages are merged for each bio.
| total 32 bio will be made.
| total elapsed time is over 2ms. it's same.
| but, first bio submit timing is fast. about 100us.
|--->|--->|--->|---> ... -->|--->|--->|--->|--->|
| 256 pages merged a bio.
| at this time, first bio submit is done.
| and 1 read request is issued for 1 bio.
|--->
|--->
|--->
..
|--->
|--->|
total 17ms elapsed to complete 32MB read done from device. |
As a result, read request issue timing is faster if bio max size is
limited.
Current kernel behavior with multipage bvec, super large bio can be
created.
And it lead to delay first I/O request issue.
Signed-off-by: Changheun Lee
---
block/bio.c| 17 -
include/linux/bio.h| 4 +++-
include/linux/blkdev.h | 3 +++
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 1f2cc1fbe283..ec0281889045 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -287,6 +287,21 @@ void bio_init(struct bio *bio, struct bio_vec *table,
}
EXPORT_SYMBOL(bio_init);
+unsigned int bio_max_size(struct bio *bio)
+{
+ struct request_queue *q;
+
+ if (!bio->bi_disk)
+ return UINT_MAX;
+
+ q = bio->bi_disk->queue;
+ if (!blk_queue_limit_bio_size(q))
+ return UINT_MAX;
+
+ return blk_queue_get_max_sectors(q, bio_op(bio)) << SECTOR_SHIFT;
+}
+EXPORT_SYMBOL(bio_max_size);
+
/**
* bio_reset - reinitialize a bio
* @bio: bio to reset
@@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page
*page,
struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
if (page_is_mergeable(bv, page, len, off, same_page)) {
- if (bio->bi_iter.bi_size > UINT_MAX - len) {
+ if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
*same_page = false;
return false;
}
>>>
>>> So far we don't need bio->bi_disk or bio->bi_bdev(will be changed in
>>> Christoph's patch) during adding page to bio, so there is null ptr
>>> refereance risk.
>>>
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 1edda614f7ce..cdb134ca7bf5 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -100,6 +100,8 @@ static inline void *bio_data(struct bio *bio)
return NULL;
}
+extern unsigned int bio_max_size(struct bio *);
+
/**
* bio_full - check if the bio is full
* @bio: bio to check
@@ -113,7 +115,7 @@
Re: [PATCH v3 1/2] bio: limit bio max size
On Tue, Jan 26, 2021 at 04:06:06AM +, Damien Le Moal wrote:
> On 2021/01/26 12:58, Ming Lei wrote:
> > On Tue, Jan 26, 2021 at 10:32:34AM +0900, Changheun Lee wrote:
> >> bio size can grow up to 4GB when muli-page bvec is enabled.
> >> but sometimes it would lead to inefficient behaviors.
> >> in case of large chunk direct I/O, - 32MB chunk read in user space -
> >> all pages for 32MB would be merged to a bio structure if the pages
> >> physical addresses are contiguous. it makes some delay to submit
> >> until merge complete. bio max size should be limited to a proper size.
> >>
> >> When 32MB chunk read with direct I/O option is coming from userspace,
> >> kernel behavior is below now. it's timeline.
> >>
> >> | bio merge for 32MB. total 8,192 pages are merged.
> >> | total elapsed time is over 2ms.
> >> |-- ... --->|
> >> | 8,192 pages merged a
> >> bio.
> >> | at this time, first bio
> >> submit is done.
> >> | 1 bio is split to 32
> >> read request and issue.
> >> |--->
> >> |--->
> >>|--->
> >> ..
> >>
> >> |--->
> >>
> >> |--->|
> >> total 19ms elapsed to complete 32MB read done
> >> from device. |
> >>
> >> If bio max size is limited with 1MB, behavior is changed below.
> >>
> >> | bio merge for 1MB. 256 pages are merged for each bio.
> >> | total 32 bio will be made.
> >> | total elapsed time is over 2ms. it's same.
> >> | but, first bio submit timing is fast. about 100us.
> >> |--->|--->|--->|---> ... -->|--->|--->|--->|--->|
> >> | 256 pages merged a bio.
> >> | at this time, first bio submit is done.
> >> | and 1 read request is issued for 1 bio.
> >> |--->
> >>|--->
> >> |--->
> >> ..
> >> |--->
> >> |--->|
> >> total 17ms elapsed to complete 32MB read done from device. |
> >>
> >> As a result, read request issue timing is faster if bio max size is
> >> limited.
> >> Current kernel behavior with multipage bvec, super large bio can be
> >> created.
> >> And it lead to delay first I/O request issue.
> >>
> >> Signed-off-by: Changheun Lee
> >> ---
> >> block/bio.c| 17 -
> >> include/linux/bio.h| 4 +++-
> >> include/linux/blkdev.h | 3 +++
> >> 3 files changed, 22 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block/bio.c b/block/bio.c
> >> index 1f2cc1fbe283..ec0281889045 100644
> >> --- a/block/bio.c
> >> +++ b/block/bio.c
> >> @@ -287,6 +287,21 @@ void bio_init(struct bio *bio, struct bio_vec *table,
> >> }
> >> EXPORT_SYMBOL(bio_init);
> >>
> >> +unsigned int bio_max_size(struct bio *bio)
> >> +{
> >> + struct request_queue *q;
> >> +
> >> + if (!bio->bi_disk)
> >> + return UINT_MAX;
> >> +
> >> + q = bio->bi_disk->queue;
> >> + if (!blk_queue_limit_bio_size(q))
> >> + return UINT_MAX;
> >> +
> >> + return blk_queue_get_max_sectors(q, bio_op(bio)) << SECTOR_SHIFT;
> >> +}
> >> +EXPORT_SYMBOL(bio_max_size);
> >> +
> >> /**
> >> * bio_reset - reinitialize a bio
> >> * @bio: bio to reset
> >> @@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page
> >> *page,
> >>struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
> >>
> >>if (page_is_mergeable(bv, page, len, off, same_page)) {
> >> - if (bio->bi_iter.bi_size > UINT_MAX - len) {
> >> + if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
> >>*same_page = false;
> >>return false;
> >>}
> >
> > So far we don't need bio->bi_disk or bio->bi_bdev(will be changed in
> > Christoph's patch) during adding page to bio, so there is null ptr
> > refereance risk.
> >
> >> diff --git a/include/linux/bio.h b/include/linux/bio.h
> >> index 1edda614f7ce..cdb134ca7bf5 100644
> >> --- a/include/linux/bio.h
> >> +++ b/include/linux/bio.h
> >> @@ -100,6 +100,8 @@ static inline void *bio_data(struct bio *bio)
> >>return NULL;
> >> }
> >>
> >> +extern unsigned int bio_max_size(struct bio *);
> >> +
> >> /**
> >> * bio_full - check if the bio is full
> >> * @bio: bio to check
> >> @@ -113,7 +115,7 @@ static inline bool bio_full(struct bio
Re: [PATCH v3 1/2] bio: limit bio max size
On 2021/01/26 12:58, Ming Lei wrote:
> On Tue, Jan 26, 2021 at 10:32:34AM +0900, Changheun Lee wrote:
>> bio size can grow up to 4GB when muli-page bvec is enabled.
>> but sometimes it would lead to inefficient behaviors.
>> in case of large chunk direct I/O, - 32MB chunk read in user space -
>> all pages for 32MB would be merged to a bio structure if the pages
>> physical addresses are contiguous. it makes some delay to submit
>> until merge complete. bio max size should be limited to a proper size.
>>
>> When 32MB chunk read with direct I/O option is coming from userspace,
>> kernel behavior is below now. it's timeline.
>>
>> | bio merge for 32MB. total 8,192 pages are merged.
>> | total elapsed time is over 2ms.
>> |-- ... --->|
>> | 8,192 pages merged a bio.
>> | at this time, first bio
>> submit is done.
>> | 1 bio is split to 32 read
>> request and issue.
>> |--->
>> |--->
>>|--->
>> ..
>>
>> |--->
>>
>> |--->|
>> total 19ms elapsed to complete 32MB read done from
>> device. |
>>
>> If bio max size is limited with 1MB, behavior is changed below.
>>
>> | bio merge for 1MB. 256 pages are merged for each bio.
>> | total 32 bio will be made.
>> | total elapsed time is over 2ms. it's same.
>> | but, first bio submit timing is fast. about 100us.
>> |--->|--->|--->|---> ... -->|--->|--->|--->|--->|
>> | 256 pages merged a bio.
>> | at this time, first bio submit is done.
>> | and 1 read request is issued for 1 bio.
>> |--->
>>|--->
>> |--->
>> ..
>> |--->
>> |--->|
>> total 17ms elapsed to complete 32MB read done from device. |
>>
>> As a result, read request issue timing is faster if bio max size is limited.
>> Current kernel behavior with multipage bvec, super large bio can be created.
>> And it lead to delay first I/O request issue.
>>
>> Signed-off-by: Changheun Lee
>> ---
>> block/bio.c| 17 -
>> include/linux/bio.h| 4 +++-
>> include/linux/blkdev.h | 3 +++
>> 3 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index 1f2cc1fbe283..ec0281889045 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -287,6 +287,21 @@ void bio_init(struct bio *bio, struct bio_vec *table,
>> }
>> EXPORT_SYMBOL(bio_init);
>>
>> +unsigned int bio_max_size(struct bio *bio)
>> +{
>> +struct request_queue *q;
>> +
>> +if (!bio->bi_disk)
>> +return UINT_MAX;
>> +
>> +q = bio->bi_disk->queue;
>> +if (!blk_queue_limit_bio_size(q))
>> +return UINT_MAX;
>> +
>> +return blk_queue_get_max_sectors(q, bio_op(bio)) << SECTOR_SHIFT;
>> +}
>> +EXPORT_SYMBOL(bio_max_size);
>> +
>> /**
>> * bio_reset - reinitialize a bio
>> * @bio:bio to reset
>> @@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page
>> *page,
>> struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
>>
>> if (page_is_mergeable(bv, page, len, off, same_page)) {
>> -if (bio->bi_iter.bi_size > UINT_MAX - len) {
>> +if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
>> *same_page = false;
>> return false;
>> }
>
> So far we don't need bio->bi_disk or bio->bi_bdev(will be changed in
> Christoph's patch) during adding page to bio, so there is null ptr
> refereance risk.
>
>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>> index 1edda614f7ce..cdb134ca7bf5 100644
>> --- a/include/linux/bio.h
>> +++ b/include/linux/bio.h
>> @@ -100,6 +100,8 @@ static inline void *bio_data(struct bio *bio)
>> return NULL;
>> }
>>
>> +extern unsigned int bio_max_size(struct bio *);
>> +
>> /**
>> * bio_full - check if the bio is full
>> * @bio:bio to check
>> @@ -113,7 +115,7 @@ static inline bool bio_full(struct bio *bio, unsigned
>> len)
>> if (bio->bi_vcnt >= bio->bi_max_vecs)
>> return true;
>>
>> -if (bio->bi_iter.bi_size > UINT_MAX - len)
>> +if (bio->bi_iter.bi_size > bio_max_size(bio) - len)
>> return true;
>>
>> return false;
>> diff
Re: [PATCH v3 1/2] bio: limit bio max size
On Tue, Jan 26, 2021 at 10:32:34AM +0900, Changheun Lee wrote:
> bio size can grow up to 4GB when muli-page bvec is enabled.
> but sometimes it would lead to inefficient behaviors.
> in case of large chunk direct I/O, - 32MB chunk read in user space -
> all pages for 32MB would be merged to a bio structure if the pages
> physical addresses are contiguous. it makes some delay to submit
> until merge complete. bio max size should be limited to a proper size.
>
> When 32MB chunk read with direct I/O option is coming from userspace,
> kernel behavior is below now. it's timeline.
>
> | bio merge for 32MB. total 8,192 pages are merged.
> | total elapsed time is over 2ms.
> |-- ... --->|
> | 8,192 pages merged a bio.
> | at this time, first bio
> submit is done.
> | 1 bio is split to 32 read
> request and issue.
> |--->
> |--->
>|--->
> ..
>
> |--->
>
> |--->|
> total 19ms elapsed to complete 32MB read done from
> device. |
>
> If bio max size is limited with 1MB, behavior is changed below.
>
> | bio merge for 1MB. 256 pages are merged for each bio.
> | total 32 bio will be made.
> | total elapsed time is over 2ms. it's same.
> | but, first bio submit timing is fast. about 100us.
> |--->|--->|--->|---> ... -->|--->|--->|--->|--->|
> | 256 pages merged a bio.
> | at this time, first bio submit is done.
> | and 1 read request is issued for 1 bio.
> |--->
>|--->
> |--->
> ..
> |--->
> |--->|
> total 17ms elapsed to complete 32MB read done from device. |
>
> As a result, read request issue timing is faster if bio max size is limited.
> Current kernel behavior with multipage bvec, super large bio can be created.
> And it lead to delay first I/O request issue.
>
> Signed-off-by: Changheun Lee
> ---
> block/bio.c| 17 -
> include/linux/bio.h| 4 +++-
> include/linux/blkdev.h | 3 +++
> 3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 1f2cc1fbe283..ec0281889045 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -287,6 +287,21 @@ void bio_init(struct bio *bio, struct bio_vec *table,
> }
> EXPORT_SYMBOL(bio_init);
>
> +unsigned int bio_max_size(struct bio *bio)
> +{
> + struct request_queue *q;
> +
> + if (!bio->bi_disk)
> + return UINT_MAX;
> +
> + q = bio->bi_disk->queue;
> + if (!blk_queue_limit_bio_size(q))
> + return UINT_MAX;
> +
> + return blk_queue_get_max_sectors(q, bio_op(bio)) << SECTOR_SHIFT;
> +}
> +EXPORT_SYMBOL(bio_max_size);
> +
> /**
> * bio_reset - reinitialize a bio
> * @bio: bio to reset
> @@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page
> *page,
> struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
>
> if (page_is_mergeable(bv, page, len, off, same_page)) {
> - if (bio->bi_iter.bi_size > UINT_MAX - len) {
> + if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
> *same_page = false;
> return false;
> }
So far we don't need bio->bi_disk or bio->bi_bdev(will be changed in
Christoph's patch) during adding page to bio, so there is null ptr
refereance risk.
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 1edda614f7ce..cdb134ca7bf5 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -100,6 +100,8 @@ static inline void *bio_data(struct bio *bio)
> return NULL;
> }
>
> +extern unsigned int bio_max_size(struct bio *);
> +
> /**
> * bio_full - check if the bio is full
> * @bio: bio to check
> @@ -113,7 +115,7 @@ static inline bool bio_full(struct bio *bio, unsigned len)
> if (bio->bi_vcnt >= bio->bi_max_vecs)
> return true;
>
> - if (bio->bi_iter.bi_size > UINT_MAX - len)
> + if (bio->bi_iter.bi_size > bio_max_size(bio) - len)
> return true;
>
> return false;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index f94ee3089e01..3aeab9e7e97b 100644
> --- a/include/linux/blkdev.h
> +++
[PATCH v3 1/2] bio: limit bio max size
bio size can grow up to 4GB when muli-page bvec is enabled.
but sometimes it would lead to inefficient behaviors.
in case of large chunk direct I/O, - 32MB chunk read in user space -
all pages for 32MB would be merged to a bio structure if the pages
physical addresses are contiguous. it makes some delay to submit
until merge complete. bio max size should be limited to a proper size.
When 32MB chunk read with direct I/O option is coming from userspace,
kernel behavior is below now. it's timeline.
| bio merge for 32MB. total 8,192 pages are merged.
| total elapsed time is over 2ms.
|-- ... --->|
| 8,192 pages merged a bio.
| at this time, first bio
submit is done.
| 1 bio is split to 32 read
request and issue.
|--->
|--->
|--->
..
|--->
|--->|
total 19ms elapsed to complete 32MB read done from
device. |
If bio max size is limited with 1MB, behavior is changed below.
| bio merge for 1MB. 256 pages are merged for each bio.
| total 32 bio will be made.
| total elapsed time is over 2ms. it's same.
| but, first bio submit timing is fast. about 100us.
|--->|--->|--->|---> ... -->|--->|--->|--->|--->|
| 256 pages merged a bio.
| at this time, first bio submit is done.
| and 1 read request is issued for 1 bio.
|--->
|--->
|--->
..
|--->
|--->|
total 17ms elapsed to complete 32MB read done from device. |
As a result, read request issue timing is faster if bio max size is limited.
Current kernel behavior with multipage bvec, super large bio can be created.
And it lead to delay first I/O request issue.
Signed-off-by: Changheun Lee
---
block/bio.c| 17 -
include/linux/bio.h| 4 +++-
include/linux/blkdev.h | 3 +++
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 1f2cc1fbe283..ec0281889045 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -287,6 +287,21 @@ void bio_init(struct bio *bio, struct bio_vec *table,
}
EXPORT_SYMBOL(bio_init);
+unsigned int bio_max_size(struct bio *bio)
+{
+ struct request_queue *q;
+
+ if (!bio->bi_disk)
+ return UINT_MAX;
+
+ q = bio->bi_disk->queue;
+ if (!blk_queue_limit_bio_size(q))
+ return UINT_MAX;
+
+ return blk_queue_get_max_sectors(q, bio_op(bio)) << SECTOR_SHIFT;
+}
+EXPORT_SYMBOL(bio_max_size);
+
/**
* bio_reset - reinitialize a bio
* @bio: bio to reset
@@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page
*page,
struct bio_vec *bv = >bi_io_vec[bio->bi_vcnt - 1];
if (page_is_mergeable(bv, page, len, off, same_page)) {
- if (bio->bi_iter.bi_size > UINT_MAX - len) {
+ if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
*same_page = false;
return false;
}
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 1edda614f7ce..cdb134ca7bf5 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -100,6 +100,8 @@ static inline void *bio_data(struct bio *bio)
return NULL;
}
+extern unsigned int bio_max_size(struct bio *);
+
/**
* bio_full - check if the bio is full
* @bio: bio to check
@@ -113,7 +115,7 @@ static inline bool bio_full(struct bio *bio, unsigned len)
if (bio->bi_vcnt >= bio->bi_max_vecs)
return true;
- if (bio->bi_iter.bi_size > UINT_MAX - len)
+ if (bio->bi_iter.bi_size > bio_max_size(bio) - len)
return true;
return false;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f94ee3089e01..3aeab9e7e97b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -621,6 +621,7 @@ struct request_queue {
#define QUEUE_FLAG_RQ_ALLOC_TIME 27/* record rq->alloc_time_ns */
#define QUEUE_FLAG_HCTX_ACTIVE 28 /* at least one blk-mq hctx is active */
#define QUEUE_FLAG_NOWAIT 29 /* device supports NOWAIT */
+#define QUEUE_FLAG_LIMIT_BIO_SIZE 30 /* limit bio size */
#define QUEUE_FLAG_MQ_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) |
