[PATCH 0/4] fix a few problems in block layer

[PATCH 0/4] fix a few problems in block layer

[Jiufei Xue]

I have found a few problems while reviewing the patch 74d46992e0d9
("block: replace bi_bdev with a gendisk pointer and partitions index"),
So fix them.

Jiufei Xue (4)
block: fix the count of PGPGOUT for WRITE_SAME
block: bio_check_eod() needs to consider partition
block: display the correct disk name for bio
block: fix a typo and modify the documentation for bio

block/blk-core.c          |   18 +++++-------------
block/partition-generic.c |    6 ------
drivers/block/pktcdvd.c   |    2 +-
include/linux/bio.h       |    4 +++-
4 files changed, 9 insertions(+), 21 deletions(-)

[PATCH 1/4] block: fix the count of PGPGOUT for WRITE_SAME

[Jiufei Xue]

The vm counters is counted in sectors, so we should do the conversation
in submit_bio.

Fixes: 74d46992e0d9 ("block: replace bi_bdev with a gendisk pointer and
partitions index")

Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
---
 block/blk-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2d1a7bb..6d82c4f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2434,7 +2434,7 @@ blk_qc_t submit_bio(struct bio *bio)
 		unsigned int count;
 
 		if (unlikely(bio_op(bio) == REQ_OP_WRITE_SAME))
-			count = queue_logical_block_size(bio->bi_disk->queue);
+			count = queue_logical_block_size(bio->bi_disk->queue) >> 9;
 		else
 			count = bio_sectors(bio);
 
-- 
1.9.4

[PATCH 2/4] block: bio_check_eod() needs to consider partition

[Jiufei Xue]

bio_check_eod() should get the capacity of partiton, not the whole
disk.

Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
---
 block/blk-core.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 6d82c4f..ef6677d 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2023,7 +2023,7 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
 	return BLK_QC_T_NONE;
 }
 
-static void handle_bad_sector(struct bio *bio)
+static void handle_bad_sector(struct bio *bio, sector_t maxsector)
 {
 	char b[BDEVNAME_SIZE];
 
@@ -2031,7 +2031,7 @@ static void handle_bad_sector(struct bio *bio)
 	printk(KERN_INFO "%s: rw=%d, want=%Lu, limit=%Lu\n",
 			bio_devname(bio, b), bio->bi_opf,
 			(unsigned long long)bio_end_sector(bio),
-			(long long)get_capacity(bio->bi_disk));
+			(long long)maxsector);
 }
 
 #ifdef CONFIG_FAIL_MAKE_REQUEST
@@ -2131,12 +2131,20 @@ static inline int blk_partition_remap(struct bio *bio)
 static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
 {
 	sector_t maxsector;
+	struct hd_struct *part;
 
 	if (!nr_sectors)
 		return 0;
 
 	/* Test device or partition size, when known. */
-	maxsector = get_capacity(bio->bi_disk);
+	rcu_read_lock();
+	part = __disk_get_part(bio->bi_disk, bio->bi_partno);
+	if (part)
+		maxsector = part_nr_sects_read(part);
+	else
+		maxsector = get_capacity(bio->bi_disk);
+	rcu_read_unlock();
+
 	if (maxsector) {
 		sector_t sector = bio->bi_iter.bi_sector;
 
@@ -2146,7 +2154,7 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
 			 * without checking the size of the device, e.g., when
 			 * mounting a device.
 			 */
-			handle_bad_sector(bio);
+			handle_bad_sector(bio, maxsector);
 			return 1;
 		}
 	}
-- 
1.9.4

[PATCH 3/4] block: display the correct diskname for bio

[Jiufei Xue]

bio_devname use __bdevname to display the device name, and can
only show the major and minor of the part0.
Fix this by using disk_name to display the correct name.

Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
---
 block/partition-generic.c | 6 ++++++
 include/linux/bio.h       | 4 +---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/partition-generic.c b/block/partition-generic.c
index 91622db..08dabcd 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -51,6 +51,12 @@ const char *bdevname(struct block_device *bdev, char *buf)
 
 EXPORT_SYMBOL(bdevname);
 
+const char *bio_devname(struct bio *bio, char *buf)
+{
+	return disk_name(bio->bi_disk, bio->bi_partno, buf);
+}
+EXPORT_SYMBOL(bio_devname);
+
 /*
  * There's very little reason to use this, you should really
  * have a struct block_device just about everywhere and use
diff --git a/include/linux/bio.h b/include/linux/bio.h
index d0eb659..ce547a2 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -511,6 +511,7 @@ extern struct bio *bio_copy_user_iov(struct request_queue *,
 extern struct bio_vec *bvec_alloc(gfp_t, int, unsigned long *, mempool_t *);
 extern void bvec_free(mempool_t *, struct bio_vec *, unsigned int);
 extern unsigned int bvec_nr_vecs(unsigned short idx);
+extern const char *bio_devname(struct bio *bio, char *buffer);
 
 #define bio_set_dev(bio, bdev) 			\
 do {						\
@@ -529,9 +530,6 @@ extern struct bio *bio_copy_user_iov(struct request_queue *,
 #define bio_dev(bio) \
 	disk_devt((bio)->bi_disk)
 
-#define bio_devname(bio, buf) \
-	__bdevname(bio_dev(bio), (buf))
-
 #ifdef CONFIG_BLK_CGROUP
 int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css);
 void bio_disassociate_task(struct bio *bio);
-- 
1.9.4

[PATCH 4/4] block: fix a typo

[Jiufei Xue]

Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
---
 drivers/block/pktcdvd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 531a091..c61d20c 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -1122,7 +1122,7 @@ static int pkt_start_recovery(struct packet_data *pkt)
 	pkt->sector = new_sector;
 
 	bio_reset(pkt->bio);
-	bio_set_set(pkt->bio, pd->bdev);
+	bio_set_dev(pkt->bio, pd->bdev);
 	bio_set_op_attrs(pkt->bio, REQ_OP_WRITE, 0);
 	pkt->bio->bi_iter.bi_sector = new_sector;
 	pkt->bio->bi_iter.bi_size = pkt->frames * CD_FRAMESIZE;
-- 
1.9.4

Re: [PATCH 0/4] fix a few problems in block layer

[Jens Axboe]

On 2/26/18 5:01 AM, Jiufei Xue wrote:
> I have found a few problems while reviewing the patch 74d46992e0d9
> ("block: replace bi_bdev with a gendisk pointer and partitions index"),
> So fix them.

Looks good to me. It's important to include a Fixes line in
the individual patches, though. Otherwise this information
is lost, and it's very useful for folks backporting fixes.

Also, patch 4/4 should have some small sensible commit
message.

Care to resend with these items fixed?

-- 
Jens Axboe

Re: [PATCH 1/4] block: fix the count of PGPGOUT for WRITE_SAME

[Omar Sandoval]

On Mon, Feb 26, 2018 at 08:04:35PM +0800, Jiufei Xue wrote:
> The vm counters is counted in sectors, so we should do the conversation
> in submit_bio.
> 
> Fixes: 74d46992e0d9 ("block: replace bi_bdev with a gendisk pointer and
> partitions index")

The Fixes line shouldn't be wrapped. Besides that,

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
> ---
>  block/blk-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 2d1a7bb..6d82c4f 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2434,7 +2434,7 @@ blk_qc_t submit_bio(struct bio *bio)
>  		unsigned int count;
>  
>  		if (unlikely(bio_op(bio) == REQ_OP_WRITE_SAME))
> -			count = queue_logical_block_size(bio->bi_disk->queue);
> +			count = queue_logical_block_size(bio->bi_disk->queue) >> 9;
>  		else
>  			count = bio_sectors(bio);
>  
> -- 
> 1.9.4
> 

Re: [PATCH 2/4] block: bio_check_eod() needs to consider partition

[Omar Sandoval]

On Mon, Feb 26, 2018 at 08:04:38PM +0800, Jiufei Xue wrote:
> bio_check_eod() should get the capacity of partiton, not the whole
> disk.
> 
> Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
> ---
>  block/blk-core.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 6d82c4f..ef6677d 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2023,7 +2023,7 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
>  	return BLK_QC_T_NONE;
>  }
>  
> -static void handle_bad_sector(struct bio *bio)
> +static void handle_bad_sector(struct bio *bio, sector_t maxsector)
>  {
>  	char b[BDEVNAME_SIZE];
>  
> @@ -2031,7 +2031,7 @@ static void handle_bad_sector(struct bio *bio)
>  	printk(KERN_INFO "%s: rw=%d, want=%Lu, limit=%Lu\n",
>  			bio_devname(bio, b), bio->bi_opf,
>  			(unsigned long long)bio_end_sector(bio),
> -			(long long)get_capacity(bio->bi_disk));
> +			(long long)maxsector);
>  }
>  
>  #ifdef CONFIG_FAIL_MAKE_REQUEST
> @@ -2131,12 +2131,20 @@ static inline int blk_partition_remap(struct bio *bio)
>  static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
>  {
>  	sector_t maxsector;
> +	struct hd_struct *part;
>  
>  	if (!nr_sectors)
>  		return 0;
>  
>  	/* Test device or partition size, when known. */
> -	maxsector = get_capacity(bio->bi_disk);
> +	rcu_read_lock();
> +	part = __disk_get_part(bio->bi_disk, bio->bi_partno);
> +	if (part)
> +		maxsector = part_nr_sects_read(part);
> +	else
> +		maxsector = get_capacity(bio->bi_disk);

This case means that bio->bi_partno was invalid, right? Shouldn't this
be an error? I see that this is copied from guard_bio_eod(), but that
serves a slightly different purpose.

> +	rcu_read_unlock();
> +
>  	if (maxsector) {
>  		sector_t sector = bio->bi_iter.bi_sector;
>  
> @@ -2146,7 +2154,7 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
>  			 * without checking the size of the device, e.g., when
>  			 * mounting a device.
>  			 */
> -			handle_bad_sector(bio);
> +			handle_bad_sector(bio, maxsector);
>  			return 1;
>  		}
>  	}
> -- 
> 1.9.4
> 
> 

Re: [PATCH 3/4] block: display the correct diskname for bio

[Omar Sandoval]

On Mon, Feb 26, 2018 at 08:04:41PM +0800, Jiufei Xue wrote:
> bio_devname use __bdevname to display the device name, and can
> only show the major and minor of the part0.
> Fix this by using disk_name to display the correct name.

Besides adding a Fixes: tag like Jens said,

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
> ---
>  block/partition-generic.c | 6 ++++++
>  include/linux/bio.h       | 4 +---
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 91622db..08dabcd 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -51,6 +51,12 @@ const char *bdevname(struct block_device *bdev, char *buf)
>  
>  EXPORT_SYMBOL(bdevname);
>  
> +const char *bio_devname(struct bio *bio, char *buf)
> +{
> +	return disk_name(bio->bi_disk, bio->bi_partno, buf);
> +}
> +EXPORT_SYMBOL(bio_devname);
> +
>  /*
>   * There's very little reason to use this, you should really
>   * have a struct block_device just about everywhere and use
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index d0eb659..ce547a2 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -511,6 +511,7 @@ extern struct bio *bio_copy_user_iov(struct request_queue *,
>  extern struct bio_vec *bvec_alloc(gfp_t, int, unsigned long *, mempool_t *);
>  extern void bvec_free(mempool_t *, struct bio_vec *, unsigned int);
>  extern unsigned int bvec_nr_vecs(unsigned short idx);
> +extern const char *bio_devname(struct bio *bio, char *buffer);
>  
>  #define bio_set_dev(bio, bdev) 			\
>  do {						\
> @@ -529,9 +530,6 @@ extern struct bio *bio_copy_user_iov(struct request_queue *,
>  #define bio_dev(bio) \
>  	disk_devt((bio)->bi_disk)
>  
> -#define bio_devname(bio, buf) \
> -	__bdevname(bio_dev(bio), (buf))
> -
>  #ifdef CONFIG_BLK_CGROUP
>  int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css);
>  void bio_disassociate_task(struct bio *bio);
> -- 
> 1.9.4
> 
> 

Re: [PATCH 1/4] block: fix the count of PGPGOUT for WRITE_SAME

[Bart Van Assche]

T24gTW9uLCAyMDE4LTAyLTI2IGF0IDIwOjA0ICswODAwLCBKaXVmZWkgWHVlIHdyb3RlOg0KPiBU
aGUgdm0gY291bnRlcnMgaXMgY291bnRlZCBpbiBzZWN0b3JzLCBzbyB3ZSBzaG91bGQgZG8gdGhl
IGNvbnZlcnNhdGlvbg0KPiBpbiBzdWJtaXRfYmlvLg0KPiANCj4gRml4ZXM6IDc0ZDQ2OTkyZTBk
OSAoImJsb2NrOiByZXBsYWNlIGJpX2JkZXYgd2l0aCBhIGdlbmRpc2sgcG9pbnRlciBhbmQNCj4g
cGFydGl0aW9ucyBpbmRleCIpDQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBKaXVmZWkgWHVlIDxqaXVm
ZWkueHVlQGxpbnV4LmFsaWJhYmEuY29tPg0KPiAtLS0NCj4gIGJsb2NrL2Jsay1jb3JlLmMgfCAy
ICstDQo+ICAxIGZpbGUgY2hhbmdlZCwgMSBpbnNlcnRpb24oKyksIDEgZGVsZXRpb24oLSkNCj4g
DQo+IGRpZmYgLS1naXQgYS9ibG9jay9ibGstY29yZS5jIGIvYmxvY2svYmxrLWNvcmUuYw0KPiBp
bmRleCAyZDFhN2JiLi42ZDgyYzRmIDEwMDY0NA0KPiAtLS0gYS9ibG9jay9ibGstY29yZS5jDQo+
ICsrKyBiL2Jsb2NrL2Jsay1jb3JlLmMNCj4gQEAgLTI0MzQsNyArMjQzNCw3IEBAIGJsa19xY190
IHN1Ym1pdF9iaW8oc3RydWN0IGJpbyAqYmlvKQ0KPiAgCQl1bnNpZ25lZCBpbnQgY291bnQ7DQo+
ICANCj4gIAkJaWYgKHVubGlrZWx5KGJpb19vcChiaW8pID09IFJFUV9PUF9XUklURV9TQU1FKSkN
Cj4gLQkJCWNvdW50ID0gcXVldWVfbG9naWNhbF9ibG9ja19zaXplKGJpby0+YmlfZGlzay0+cXVl
dWUpOw0KPiArCQkJY291bnQgPSBxdWV1ZV9sb2dpY2FsX2Jsb2NrX3NpemUoYmlvLT5iaV9kaXNr
LT5xdWV1ZSkgPj4gOTsNCj4gIAkJZWxzZQ0KPiAgCQkJY291bnQgPSBiaW9fc2VjdG9ycyhiaW8p
Ow0KDQpTaW5jZSB0aGlzIGlzIGEgZml4IGZvciBhIGtlcm5lbCB2NC4xNCBjaGFuZ2UsIHBsZWFz
ZSBhZGQgYQ0KIkNjOiBzdGFibGVAdmdlci5rZXJuZWwub3JnIiB0YWcuDQoNClRoYW5rcywNCg0K
QmFydC4NCg0KDQo=

Re: [PATCH 1/4] block: fix the count of PGPGOUT for WRITE_SAME

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/4] block: bio_check_eod() needs to consider partition

[Christoph Hellwig]

The issue looks real, but please try to do this without another
partition lookup. 

Re: [PATCH 2/4] block: bio_check_eod() needs to consider partition

[Jiufei Xue]

On 2018/2/27 上午8:11, Christoph Hellwig wrote:
> The issue looks real, but please try to do this without another
> partition lookup. 
> 
Yes, I think the partition size test can be moved to
blk_partition_remap(). I will send version 2 later.

[PATCH 2/4] block: bio_check_eod() needs to consider partition

[Jiufei Xue]

bio_check_eod() should check partiton size not the whole
disk if bio->bi_partno is not zero.

Fixes: 74d46992e0d9 ("block: replace bi_bdev with a gendisk pointer and partitions index")
Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
---
 block/blk-core.c | 79 ++++++++++++++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 6d82c4f..5fb5278 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2023,7 +2023,7 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
 	return BLK_QC_T_NONE;
 }
 
-static void handle_bad_sector(struct bio *bio)
+static void handle_bad_sector(struct bio *bio, sector_t maxsector)
 {
 	char b[BDEVNAME_SIZE];
 
@@ -2031,7 +2031,7 @@ static void handle_bad_sector(struct bio *bio)
 	printk(KERN_INFO "%s: rw=%d, want=%Lu, limit=%Lu\n",
 			bio_devname(bio, b), bio->bi_opf,
 			(unsigned long long)bio_end_sector(bio),
-			(long long)get_capacity(bio->bi_disk));
+			(long long)maxsector);
 }
 
 #ifdef CONFIG_FAIL_MAKE_REQUEST
@@ -2093,11 +2093,45 @@ static noinline int should_fail_bio(struct bio *bio)
 ALLOW_ERROR_INJECTION(should_fail_bio, ERRNO);
 
 /*
+ * Check whether this bio extends beyond the end of the device.
+ */
+static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors,
+				struct hd_struct *part)
+{
+	sector_t maxsector;
+
+	if (!nr_sectors)
+		return 0;
+
+	/* Test device or partition size, when known. */
+	if (part->partno)
+		maxsector = part_nr_sects_read(part);
+	else
+		maxsector = get_capacity(bio->bi_disk);
+	if (maxsector) {
+		sector_t sector = bio->bi_iter.bi_sector;
+
+		if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
+			/*
+			 * This may well happen - the kernel calls bread()
+			 * without checking the size of the device, e.g., when
+			 * mounting a device.
+			 */
+			handle_bad_sector(bio, maxsector);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+/*
  * Remap block n of partition p to block n+start(p) of the disk.
  */
 static inline int blk_partition_remap(struct bio *bio)
 {
 	struct hd_struct *p;
+	int nr_sectors = bio_sectors(bio);
 	int ret = 0;
 
 	rcu_read_lock();
@@ -2108,11 +2142,16 @@ static inline int blk_partition_remap(struct bio *bio)
 		goto out;
 	}
 
+	if (bio_check_eod(bio, nr_sectors, p)) {
+		ret = -EIO;
+		goto out;
+	}
+
 	/*
 	 * Zone reset does not include bi_size so bio_sectors() is always 0.
 	 * Include a test for the reset op code and perform the remap if needed.
 	 */
-	if (!bio_sectors(bio) && bio_op(bio) != REQ_OP_ZONE_RESET)
+	if (!nr_sectors && bio_op(bio) != REQ_OP_ZONE_RESET)
 		goto out;
 
 	bio->bi_iter.bi_sector += p->start_sect;
@@ -2125,35 +2164,6 @@ static inline int blk_partition_remap(struct bio *bio)
 	return ret;
 }
 
-/*
- * Check whether this bio extends beyond the end of the device.
- */
-static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
-{
-	sector_t maxsector;
-
-	if (!nr_sectors)
-		return 0;
-
-	/* Test device or partition size, when known. */
-	maxsector = get_capacity(bio->bi_disk);
-	if (maxsector) {
-		sector_t sector = bio->bi_iter.bi_sector;
-
-		if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
-			/*
-			 * This may well happen - the kernel calls bread()
-			 * without checking the size of the device, e.g., when
-			 * mounting a device.
-			 */
-			handle_bad_sector(bio);
-			return 1;
-		}
-	}
-
-	return 0;
-}
-
 static noinline_for_stack bool
 generic_make_request_checks(struct bio *bio)
 {
@@ -2164,9 +2174,6 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
 
 	might_sleep();
 
-	if (bio_check_eod(bio, nr_sectors))
-		goto end_io;
-
 	q = bio->bi_disk->queue;
 	if (unlikely(!q)) {
 		printk(KERN_ERR
@@ -2194,7 +2201,7 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
 			goto end_io;
 	}
 
-	if (bio_check_eod(bio, nr_sectors))
+	if (bio_check_eod(bio, nr_sectors, &bio->bi_disk->part0))
 		goto end_io;
 
 	/*
-- 
1.9.4

Re: [PATCH 2/4] block: bio_check_eod() needs to consider partition

[Christoph Hellwig]

Hmm.  I'd rather just kill off bio_check_eod and move the check
to blk_partition_remap so that we only have to check once.

What do you think of this version?  Probably needs to be split into
one or two prep patches and the real change.

diff --git a/block/blk-core.c b/block/blk-core.c
index 3ba4326a63b5..36a3cb042ca7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2009,7 +2009,7 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
 	return BLK_QC_T_NONE;
 }
 
-static void handle_bad_sector(struct bio *bio)
+static void handle_bad_sector(struct bio *bio, sector_t maxsector)
 {
 	char b[BDEVNAME_SIZE];
 
@@ -2017,7 +2017,7 @@ static void handle_bad_sector(struct bio *bio)
 	printk(KERN_INFO "%s: rw=%d, want=%Lu, limit=%Lu\n",
 			bio_devname(bio, b), bio->bi_opf,
 			(unsigned long long)bio_end_sector(bio),
-			(long long)get_capacity(bio->bi_disk));
+			(long long)maxsector);
 }
 
 #ifdef CONFIG_FAIL_MAKE_REQUEST
@@ -2060,57 +2060,47 @@ static inline bool should_fail_request(struct hd_struct *part,
  */
 static inline int blk_partition_remap(struct bio *bio)
 {
-	struct hd_struct *p;
-	int ret = 0;
+	sector_t maxsector = get_capacity(bio->bi_disk);
+	int nr_sectors = bio_sectors(bio);
 
 	/*
 	 * Zone reset does not include bi_size so bio_sectors() is always 0.
 	 * Include a test for the reset op code and perform the remap if needed.
 	 */
-	if (!bio->bi_partno ||
-	    (!bio_sectors(bio) && bio_op(bio) != REQ_OP_ZONE_RESET))
+	if (!nr_sectors && bio_op(bio) != REQ_OP_ZONE_RESET)
 		return 0;
 
-	rcu_read_lock();
-	p = __disk_get_part(bio->bi_disk, bio->bi_partno);
-	if (likely(p && !should_fail_request(p, bio->bi_iter.bi_size))) {
+	if (bio->bi_partno) {
+		struct hd_struct *p;
+
+		rcu_read_lock();
+		p = __disk_get_part(bio->bi_disk, bio->bi_partno);
+		if (unlikely(!p ||
+			     should_fail_request(p, bio->bi_iter.bi_size))) {
+			rcu_read_unlock();
+			pr_info("%s: fail for partition %d\n",
+				__func__, bio->bi_partno);
+			return -EIO;
+		}
+
 		bio->bi_iter.bi_sector += p->start_sect;
 		bio->bi_partno = 0;
 		trace_block_bio_remap(bio->bi_disk->queue, bio, part_devt(p),
 				bio->bi_iter.bi_sector - p->start_sect);
-	} else {
-		printk("%s: fail for partition %d\n", __func__, bio->bi_partno);
-		ret = -EIO;
+		maxsector = part_nr_sects_read(p);
+		rcu_read_unlock();
 	}
-	rcu_read_unlock();
 
-	return ret;
-}
-
-/*
- * Check whether this bio extends beyond the end of the device.
- */
-static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
-{
-	sector_t maxsector;
-
-	if (!nr_sectors)
-		return 0;
-
-	/* Test device or partition size, when known. */
-	maxsector = get_capacity(bio->bi_disk);
-	if (maxsector) {
-		sector_t sector = bio->bi_iter.bi_sector;
-
-		if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
-			/*
-			 * This may well happen - the kernel calls bread()
-			 * without checking the size of the device, e.g., when
-			 * mounting a device.
-			 */
-			handle_bad_sector(bio);
-			return 1;
-		}
+	/*
+	 * Check whether this bio extends beyond the end of the device or
+	 * partition.  This may well happen - the kernel calls bread() without
+	 * checking the size of the device, e.g., when mounting a file system.
+	 */
+	if (nr_sectors && maxsector &&
+	    (nr_sectors > maxsector ||
+	     bio->bi_iter.bi_sector > maxsector - nr_sectors)) {
+		handle_bad_sector(bio, maxsector);
+		return -EIO;
 	}
 
 	return 0;
@@ -2126,9 +2116,6 @@ generic_make_request_checks(struct bio *bio)
 
 	might_sleep();
 
-	if (bio_check_eod(bio, nr_sectors))
-		goto end_io;
-
 	q = bio->bi_disk->queue;
 	if (unlikely(!q)) {
 		printk(KERN_ERR
@@ -2152,9 +2139,6 @@ generic_make_request_checks(struct bio *bio)
 	if (blk_partition_remap(bio))
 		goto end_io;
 
-	if (bio_check_eod(bio, nr_sectors))
-		goto end_io;
-
 	/*
 	 * Filter flush bio's early so that make_request based
 	 * drivers without flush support don't have to worry

Re: [PATCH 2/4] block: bio_check_eod() needs to consider partition

[Jiufei Xue]

Hi Christoph,

thanks for your quick reply.

On 2018/3/1 上午1:48, Christoph Hellwig wrote:
> Hmm.  I'd rather just kill off bio_check_eod and move the check
> to blk_partition_remap so that we only have to check once.
> 
I think the check should be done twice if the bi_partno is not zero,
one for the partition, and another for the whole disk after remap which
is add in the commit 5ddfe9691c91
("md: check bio address after mapping through partitions").

Thanks,
Jiufei

> What do you think of this version?  Probably needs to be split into
> one or two prep patches and the real change.
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 3ba4326a63b5..36a3cb042ca7 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2009,7 +2009,7 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
>  	return BLK_QC_T_NONE;
>  }
>  
> -static void handle_bad_sector(struct bio *bio)
> +static void handle_bad_sector(struct bio *bio, sector_t maxsector)
>  {
>  	char b[BDEVNAME_SIZE];
>  
> @@ -2017,7 +2017,7 @@ static void handle_bad_sector(struct bio *bio)
>  	printk(KERN_INFO "%s: rw=%d, want=%Lu, limit=%Lu\n",
>  			bio_devname(bio, b), bio->bi_opf,
>  			(unsigned long long)bio_end_sector(bio),
> -			(long long)get_capacity(bio->bi_disk));
> +			(long long)maxsector);
>  }
>  
>  #ifdef CONFIG_FAIL_MAKE_REQUEST
> @@ -2060,57 +2060,47 @@ static inline bool should_fail_request(struct hd_struct *part,
>   */
>  static inline int blk_partition_remap(struct bio *bio)
>  {
> -	struct hd_struct *p;
> -	int ret = 0;
> +	sector_t maxsector = get_capacity(bio->bi_disk);
> +	int nr_sectors = bio_sectors(bio);
>  
>  	/*
>  	 * Zone reset does not include bi_size so bio_sectors() is always 0.
>  	 * Include a test for the reset op code and perform the remap if needed.
>  	 */
> -	if (!bio->bi_partno ||
> -	    (!bio_sectors(bio) && bio_op(bio) != REQ_OP_ZONE_RESET))
> +	if (!nr_sectors && bio_op(bio) != REQ_OP_ZONE_RESET)
>  		return 0;
>  
> -	rcu_read_lock();
> -	p = __disk_get_part(bio->bi_disk, bio->bi_partno);
> -	if (likely(p && !should_fail_request(p, bio->bi_iter.bi_size))) {
> +	if (bio->bi_partno) {
> +		struct hd_struct *p;
> +
> +		rcu_read_lock();
> +		p = __disk_get_part(bio->bi_disk, bio->bi_partno);
> +		if (unlikely(!p ||
> +			     should_fail_request(p, bio->bi_iter.bi_size))) {
> +			rcu_read_unlock();
> +			pr_info("%s: fail for partition %d\n",
> +				__func__, bio->bi_partno);
> +			return -EIO;
> +		}
> +
>  		bio->bi_iter.bi_sector += p->start_sect;
>  		bio->bi_partno = 0;
>  		trace_block_bio_remap(bio->bi_disk->queue, bio, part_devt(p),
>  				bio->bi_iter.bi_sector - p->start_sect);
> -	} else {
> -		printk("%s: fail for partition %d\n", __func__, bio->bi_partno);
> -		ret = -EIO;
> +		maxsector = part_nr_sects_read(p);
> +		rcu_read_unlock();
>  	}
> -	rcu_read_unlock();
>  
> -	return ret;
> -}
> -
> -/*
> - * Check whether this bio extends beyond the end of the device.
> - */
> -static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
> -{
> -	sector_t maxsector;
> -
> -	if (!nr_sectors)
> -		return 0;
> -
> -	/* Test device or partition size, when known. */
> -	maxsector = get_capacity(bio->bi_disk);
> -	if (maxsector) {
> -		sector_t sector = bio->bi_iter.bi_sector;
> -
> -		if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
> -			/*
> -			 * This may well happen - the kernel calls bread()
> -			 * without checking the size of the device, e.g., when
> -			 * mounting a device.
> -			 */
> -			handle_bad_sector(bio);
> -			return 1;
> -		}
> +	/*
> +	 * Check whether this bio extends beyond the end of the device or
> +	 * partition.  This may well happen - the kernel calls bread() without
> +	 * checking the size of the device, e.g., when mounting a file system.
> +	 */
> +	if (nr_sectors && maxsector &&
> +	    (nr_sectors > maxsector ||
> +	     bio->bi_iter.bi_sector > maxsector - nr_sectors)) {
> +		handle_bad_sector(bio, maxsector);
> +		return -EIO;
>  	}
>  
>  	return 0;
> @@ -2126,9 +2116,6 @@ generic_make_request_checks(struct bio *bio)
>  
>  	might_sleep();
>  
> -	if (bio_check_eod(bio, nr_sectors))
> -		goto end_io;
> -
>  	q = bio->bi_disk->queue;
>  	if (unlikely(!q)) {
>  		printk(KERN_ERR
> @@ -2152,9 +2139,6 @@ generic_make_request_checks(struct bio *bio)
>  	if (blk_partition_remap(bio))
>  		goto end_io;
>  
> -	if (bio_check_eod(bio, nr_sectors))
> -		goto end_io;
> -
>  	/*
>  	 * Filter flush bio's early so that make_request based
>  	 * drivers without flush support don't have to worry
> 

Re: [PATCH 2/4] block: bio_check_eod() needs to consider partition

[Christoph Hellwig]

On Thu, Mar 01, 2018 at 09:23:06AM +0800, Jiufei Xue wrote:
> I think the check should be done twice if the bi_partno is not zero,
> one for the partition, and another for the whole disk after remap which
> is add in the commit 5ddfe9691c91
> ("md: check bio address after mapping through partitions").

Why?  If the partitions has an incorrect size we already catch it during
parsing in rescan_partitions().