block:Remove extra condition in end of disk check

PATCH In review Raghavendra K T 2 participants 4 messages Jun 23, 2013 – Jun 25, 2013 Block layer

lore

Share & tools

lore

Raghavendra K T

author

2013-06-23 17:13 UTC #0 lore raw

PATCH 1/1 block:Remove extra condition in end of disk check

From: Raghavendra K T <[email protected]>

Since #sector is always positive the reduced condition check encompasses maxsector < nr_sectors check.

Signed-off-by Raghavendra K T <[email protected]>

---
An userspace program looping with:
maxsector - 0-9999
nr_sector - 0-999
sector - 0-999 gave 6.4% improvement with new condition though I agree that it is not the best way to test it perhaps :)

block/blk-core.c +1 -1

index 33c33bc..4a78583 100644
@@ -1656,7 +1656,7 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
 	if (maxsector) {
 		sector_t sector = bio->bi_sector;
 
-		if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
+		if (maxsector - nr_sectors < sector) {
 			/*
 			 * This may well happen - the kernel calls bread()
 			 * without checking the size of the device, e.g., when

Raghavendra K T

author

2013-06-24 13:46 UTC #1 lore raw

CCing more relevant people (with the help of get_maintainer --git-blame)

Quoted 28 lines · Raghavendra K T

From: Raghavendra K T <[email protected]>
Since #sector is always positive the reduced condition check encompasses maxsector < nr_sectors check.
Signed-off-by Raghavendra K T <[email protected]>
---
An userspace program looping with:
maxsector - 0-9999
nr_sector - 0-999
sector - 0-999 gave 6.4% improvement with new condition though I agree that it is not the best way to test it perhaps :)
block/blk-core.c +1 -1
index 33c33bc..4a78583 100644
@@ -1656,7 +1656,7 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
  	if (maxsector) {
  		sector_t sector = bio->bi_sector;
 
-		if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
+		if (maxsector - nr_sectors < sector) {
  			/*
  			 * This may well happen - the kernel calls bread()
  			 * without checking the size of the device, e.g., when

Tejun Heo

2013-06-24 22:51 UTC #2 lore raw

Quoted 6 lines · Raghavendra K T

block/blk-core.c static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors) · line 1659
Quoted 6 lines
block/blk-core.c +1 -1
@@ -1656,7 +1656,7 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
 	if (maxsector) {
 		sector_t sector = bio->bi_sector;
 
-		if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
+		if (maxsector - nr_sectors < sector) {

If maxsector < nr_sectors, the subtraction will underflow making it a very large number and fail to detect the invalid condition, no?

Thanks.

Signature · 1 line

tejun

Raghavendra K T

author

2013-06-25 05:06 UTC #3 lore raw

Quoted 11 lines · Tejun Heo

Quoted 6 lines · Raghavendra K T
block/blk-core.c static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors) · line 1659
Quoted 6 lines
block/blk-core.c +1 -1
@@ -1656,7 +1656,7 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
  	if (maxsector) {
  		sector_t sector = bio->bi_sector;
 
-		if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
+		if (maxsector - nr_sectors < sector) {
If maxsector < nr_sectors, the subtraction will underflow making it a very large number and fail to detect the invalid condition, no?

Hi Tejun,
Thanks for the reply and explanation. You are right. underflow results in invalid condition.

Considering maxsector and sectors are unsigned long, and nr_sector is unsigned int, probably safer bet is
(max_sector < sector + nr_sector), but still it would leave scope for overflow.

Thanks again,
Raghu.