block: modify __bio_add_page check to accept pages that don't start a new segment
PATCH RESEND v2 1/1 block: modify __bio_add_page check to accept pages that don't start a new segment
v2: changed a comment
The original behavior was to refuse all pages after the maximum number of segments has been reached. However, some drivers (like st) craft their buffers to potentially require exactly max segments and multiple pages in the last segment. This patch modifies the check to allow pages that can be merged into the last segment.
Fixes EBUSY failures when using large tape block size in high memory fragmentation condition.
This regression was introduced by commit
46081b166415acb66d4b3150ecefcd9460bb48a1
st: Increase success probability in driver buffer allocation
--- fs/bio.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)
fs/bio.c +17 -10
@@ -500,7 +500,6 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page *page, unsigned int len, unsigned int offset, unsigned short max_sectors) { -int retried_segments =0 ;struct bio_vec *bvec;/*
@@ -551,18 +550,13 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct pagereturn 0 ;/* -* we might lose a segment or two here, but rather that than -* make this too complex. +* The first part of the segment count check, +* reduce segment count if possible */ -while (bio->bi_phys_segments >=queue_max_segments (q)) { - -if (retried_segments) -return 0 ; - - retried_segments =1 ; +if (bio->bi_phys_segments >=queue_max_segments (q))blk_recount_segments (q, bio); - } +/* * setup the new entry, we might clear it again later if we
@@ -572,6 +566,19 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page bvec->bv_page = page; bvec->bv_len = len; bvec->bv_offset = offset; + +/* +* the other part of the segment count check, allow mergeable pages +*/ +if ((bio->bi_phys_segments >queue_max_segments (q)) || + ( (bio->bi_phys_segments ==queue_max_segments (q)) && + !BIOVEC_PHYS_MERGEABLE (bvec -1 , bvec))) { + bvec->bv_page =NULL ; + bvec->bv_len =0 ; + bvec->bv_offset =0 ; +return 0 ; + } +/* * if queue has other restrictions (eg varying max sector size
Signature · 1 line
1.8.1.4
Quoted 68 lines · Jan Vesely
v2: changed a comment
The original behavior was to refuse all pages after the maximum number of segments has been reached. However, some drivers (like st) craft their buffers to potentially require exactly max segments and multiple pages in the last segment. This patch modifies the check to allow pages that can be merged into the last segment.
Fixes EBUSY failures when using large tape block size in high memory fragmentation condition.
This regression was introduced by commit
46081b166415acb66d4b3150ecefcd9460bb48a1
st: Increase success probability in driver buffer allocationSigned-off-by Jan Vesely <[email protected]>CC Alexander Viro <[email protected]>CC FUJITA Tomonori <[email protected]>CC Kai Makisara <[email protected]>CC James Bottomley <[email protected]>CC Jens Axboe <[email protected]>--- fs/bio.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)fs/bio.c +7
-10@@ -500,7 +500,6 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page *page, unsigned int len, unsigned int offset, unsigned short max_sectors) { -int retried_segments =0 ;struct bio_vec *bvec;/* @@ -551,18 +550,13 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct pagereturn 0 ;/* -* we might lose a segment or two here, but rather that than -* make this too complex. +* The first part of the segment count check, +* reduce segment count if possible */ -while (bio->bi_phys_segments >=queue_max_segments (q)) { - -if (retried_segments) -return 0 ; - - retried_segments =1 ; +if (bio->bi_phys_segments >=queue_max_segments (q))blk_recount_segments (q, bio); - } +/* * setup the new entry, we might clear it again later if we @@ -572,6 +566,19 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page bvec->bv_page = page; bvec->bv_len = len; bvec->bv_offset = offset; + +/* +* the other part of the segment count check, allow mergeable pages
fs/bio.c static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page · line 581
Quoted 10 lines
fs/bio.c +10
-0+*/ +if ((bio->bi_phys_segments >queue_max_segments (q)) || + ( (bio->bi_phys_segments ==queue_max_segments (q)) && + !BIOVEC_PHYS_MERGEABLE (bvec -1 , bvec))) { + bvec->bv_page =NULL ; + bvec->bv_len =0 ; + bvec->bv_offset =0 ; +return 0 ; + } +
This is a bit messy, I think. bi_phys_segments should never be allowed to go beyond queue_ma_segments(), so the > test does not look right. Maybe it's an artifact of when we fall through with this patch, we bump bi_phys_segments even if the segments are physicall contig and mergeable.
What happens when the segment is physically mergeable, but the resulting merged segment is too large (bigger than q->limits.max_segment_size)?
Signature · 1 line
Jens Axboe
Quoted 77 lines · Jens Axboe
Quoted 76 lines · Jan Vesely
v2: changed a comment
The original behavior was to refuse all pages after the maximum number of segments has been reached. However, some drivers (like st) craft their buffers to potentially require exactly max segments and multiple pages in the last segment. This patch modifies the check to allow pages that can be merged into the last segment.
Fixes EBUSY failures when using large tape block size in high memory fragmentation condition.
This regression was introduced by commit
46081b166415acb66d4b3150ecefcd9460bb48a1
st: Increase success probability in driver buffer allocationSigned-off-by Jan Vesely <[email protected]>CC Alexander Viro <[email protected]>CC FUJITA Tomonori <[email protected]>CC Kai Makisara <[email protected]>CC James Bottomley <[email protected]>CC Jens Axboe <[email protected]>--- fs/bio.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)fs/bio.c +15
-10@@ -500,7 +500,6 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page *page, unsigned int len, unsigned int offset, unsigned short max_sectors) { -int retried_segments =0 ;struct bio_vec *bvec;/* @@ -551,18 +550,13 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct pagereturn 0 ;/* -* we might lose a segment or two here, but rather that than -* make this too complex. +* The first part of the segment count check, +* reduce segment count if possible */ -while (bio->bi_phys_segments >=queue_max_segments (q)) { - -if (retried_segments) -return 0 ; - - retried_segments =1 ; +if (bio->bi_phys_segments >=queue_max_segments (q))blk_recount_segments (q, bio); - } +/* * setup the new entry, we might clear it again later if we @@ -572,6 +566,19 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page bvec->bv_page = page; bvec->bv_len = len; bvec->bv_offset = offset; + +/* +* the other part of the segment count check, allow mergeable pages +*/ +if ((bio->bi_phys_segments >queue_max_segments (q)) || + ( (bio->bi_phys_segments ==queue_max_segments (q)) && + !BIOVEC_PHYS_MERGEABLE (bvec -1 , bvec))) { + bvec->bv_page =NULL ; + bvec->bv_len =0 ; + bvec->bv_offset =0 ; +return 0 ;
Quoted 8 lines
fs/bio.c static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page · line 581
Quoted 2 lines
fs/bio.c +2
-0+ } +This is a bit messy, I think.
bi_phys_segmentsshould never be allowed to go beyondqueue_ma_segments(), so the > test does not look right. Maybe it's an artifact of when we fall through with this patch, we bumpbi_phys_segmentseven if the segments are physicall contig and mergeable.
yeah. it is messy, I tried to go for the least invasive changes.
I took the '>' test from the original while loop '>='. The original behavior
guaranteed bio->bi_phys_segments <= max_segments, if the bio satisfied this condition to begin with.
I did not find any guarantees that the 'bio' parameter of this function has
to satisfy this condition in general.
My understanding is that if a caller of this function (or one of the two that call this one)
provides an invalid (segment-count-wise) bio, it will fail (return 0 added length),
and let the caller handle the situation.
I admit, I did not check all the call paths that use these functions.
Quoted 4 lines
What happens when the segment is physically mergeable, but the resulting merged segment is too large (bigger than
q->limits.max_segment_size)?
ah, yes. I guess I need a check that follows __blk_recalc_rq_segments more closely.
We know that at this point all pages are merged into segments, so a helper function that would be used
by both __blk_recalc_rq_segments and this check is possible.
I still assume that a temporary increase of bi_phys_segments above max_segments is ok.
If we want to avoid this situation we would need to merge tail pages right away. That's imo uglier.
thanks
--
Jan Vesely <[email protected]>
Quoted 97 lines · Jan Vesely
51506edc5741209311913Quoted 77 lines · Jens Axboe
Quoted 76 lines · Jan Vesely
v2: changed a comment
The original behavior was to refuse all pages after the maximum number of segments has been reached. However, some drivers (like st) craft their buffers to potentially require exactly max segments and multiple pages in the last segment. This patch modifies the check to allow pages that can be merged into the last segment.
Fixes EBUSY failures when using large tape block size in high memory fragmentation condition.
This regression was introduced by commit
46081b166415acb66d4b3150ecefcd9460bb48a1
st: Increase success probability in driver buffer allocationSigned-off-by Jan Vesely <[email protected]>CC Alexander Viro <[email protected]>CC FUJITA Tomonori <[email protected]>CC Kai Makisara <[email protected]>CC James Bottomley <[email protected]>CC Jens Axboe <[email protected]>--- fs/bio.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)fs/bio.c +15
-10@@ -500,7 +500,6 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page *page, unsigned int len, unsigned int offset, unsigned short max_sectors) { -int retried_segments =0 ;struct bio_vec *bvec;/* @@ -551,18 +550,13 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct pagereturn 0 ;/* -* we might lose a segment or two here, but rather that than -* make this too complex. +* The first part of the segment count check, +* reduce segment count if possible */ -while (bio->bi_phys_segments >=queue_max_segments (q)) { - -if (retried_segments) -return 0 ; - - retried_segments =1 ; +if (bio->bi_phys_segments >=queue_max_segments (q))blk_recount_segments (q, bio); - } +/* * setup the new entry, we might clear it again later if we @@ -572,6 +566,19 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page bvec->bv_page = page; bvec->bv_len = len; bvec->bv_offset = offset; + +/* +* the other part of the segment count check, allow mergeable pages +*/ +if ((bio->bi_phys_segments >queue_max_segments (q)) || + ( (bio->bi_phys_segments ==queue_max_segments (q)) && + !BIOVEC_PHYS_MERGEABLE (bvec -1 , bvec))) { + bvec->bv_page =NULL ; + bvec->bv_len =0 ; + bvec->bv_offset =0 ; +return 0 ;Quoted 8 lines
fs/bio.c static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page · line 581
Quoted 2 lines
fs/bio.c +2
-0+ } +This is a bit messy, I think.
bi_phys_segmentsshould never be allowed to go beyondqueue_ma_segments(), so the > test does not look right. Maybe it's an artifact of when we fall through with this patch, we bumpbi_phys_segmentseven if the segments are physicall contig and mergeable.yeah. it is messy, I tried to go for the least invasive changes.
I took the '>' test from the original while loop '>='. The original behavior guaranteed
bio->bi_phys_segments<=max_segments, if the bio satisfied this condition to begin with. I did not find any guarantees that the 'bio' parameter of this function has to satisfy this condition in general.
Quoted 5 lines
My understanding is that if a caller of this function (or one of the two that call this one) provides an invalid (segment-count-wise) bio, it will fail (return 0 added length), and let the caller handle the situation. I admit, I did not check all the call paths that use these functions.
Yes, that is how it works. So that should be fine.
Quoted 10 lines
Quoted 3 lines
What happens when the segment is physically mergeable, but the resulting merged segment is too large (bigger than
q->limits.max_segment_size)?ah, yes. I guess I need a check that follows
__blk_recalc_rq_segmentsmore closely. We know that at this point all pages are merged into segments, so a helper function that would be used by both__blk_recalc_rq_segmentsand this check is possible.
Quoted 3 lines
I still assume that a temporary increase of
bi_phys_segmentsabovemax_segmentsis ok. If we want to avoid this situation we would need to merge tail pages right away. That's imo uglier.
Yes, it's OK if we just ensure that we clear the valid segment flag. At least that would be the best sort of solution, to ensure that it's recalculated properly when someone checks it.
Signature · 1 line
Jens Axboe
Quoted 116 lines · Jens Axboe
Quoted 97 lines · Jan Vesely
51506edc5741209311913Quoted 77 lines · Jens Axboe
Quoted 76 lines · Jan Vesely
v2: changed a comment
The original behavior was to refuse all pages after the maximum number of segments has been reached. However, some drivers (like st) craft their buffers to potentially require exactly max segments and multiple pages in the last segment. This patch modifies the check to allow pages that can be merged into the last segment.
Fixes EBUSY failures when using large tape block size in high memory fragmentation condition.
This regression was introduced by commit
46081b166415acb66d4b3150ecefcd9460bb48a1
st: Increase success probability in driver buffer allocationSigned-off-by Jan Vesely <[email protected]>CC Alexander Viro <[email protected]>CC FUJITA Tomonori <[email protected]>CC Kai Makisara <[email protected]>CC James Bottomley <[email protected]>CC Jens Axboe <[email protected]>--- fs/bio.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)fs/bio.c +15
-10@@ -500,7 +500,6 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page *page, unsigned int len, unsigned int offset, unsigned short max_sectors) { -int retried_segments =0 ;struct bio_vec *bvec;/* @@ -551,18 +550,13 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct pagereturn 0 ;/* -* we might lose a segment or two here, but rather that than -* make this too complex. +* The first part of the segment count check, +* reduce segment count if possible */ -while (bio->bi_phys_segments >=queue_max_segments (q)) { - -if (retried_segments) -return 0 ; - - retried_segments =1 ; +if (bio->bi_phys_segments >=queue_max_segments (q))blk_recount_segments (q, bio); - } +/* * setup the new entry, we might clear it again later if we @@ -572,6 +566,19 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page bvec->bv_page = page; bvec->bv_len = len; bvec->bv_offset = offset; + +/* +* the other part of the segment count check, allow mergeable pages +*/ +if ((bio->bi_phys_segments >queue_max_segments (q)) || + ( (bio->bi_phys_segments ==queue_max_segments (q)) && + !BIOVEC_PHYS_MERGEABLE (bvec -1 , bvec))) { + bvec->bv_page =NULL ; + bvec->bv_len =0 ; + bvec->bv_offset =0 ; +return 0 ;Quoted 8 lines
fs/bio.c static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page · line 581
Quoted 2 lines
fs/bio.c +2
-0+ } +This is a bit messy, I think.
bi_phys_segmentsshould never be allowed to go beyondqueue_ma_segments(), so the > test does not look right. Maybe it's an artifact of when we fall through with this patch, we bumpbi_phys_segmentseven if the segments are physicall contig and mergeable.yeah. it is messy, I tried to go for the least invasive changes.
I took the '>' test from the original while loop '>='. The original behavior guaranteed
bio->bi_phys_segments<=max_segments, if the bio satisfied this condition to begin with. I did not find any guarantees that the 'bio' parameter of this function has to satisfy this condition in general.Quoted 5 lines
My understanding is that if a caller of this function (or one of the two that call this one) provides an invalid (segment-count-wise) bio, it will fail (return 0 added length), and let the caller handle the situation. I admit, I did not check all the call paths that use these functions.
Yes, that is how it works. So that should be fine.
Quoted 10 lines
Quoted 3 lines
What happens when the segment is physically mergeable, but the resulting merged segment is too large (bigger than
q->limits.max_segment_size)?ah, yes. I guess I need a check that follows
__blk_recalc_rq_segmentsmore closely. We know that at this point all pages are merged into segments, so a helper function that would be used by both__blk_recalc_rq_segmentsand this check is possible.
Quoted 8 lines
Quoted 3 lines
I still assume that a temporary increase of
bi_phys_segmentsabovemax_segmentsis ok. If we want to avoid this situation we would need to merge tail pages right away. That's imo uglier.Yes, it's OK if we just ensure that we clear the valid segment flag. At least that would be the best sort of solution, to ensure that it's recalculated properly when someone checks it.
Hi Jens,
v3 has been around for few months and I posted v4(whitespace changes) two weeks ago.
Let me know if there's something more I can do to get these patches merged.
regards,
--
Jan Vesely <[email protected]>