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

[PATCH V2 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.

Changes since v1:
- add a Fixes tag in individual patch.
- check end-of-device of bio in blk_partition_remap when the bi_partno
is not zero to avoid another partition lookup.

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          |   81 +++++++++++++++++++++-------------------------
 block/partition-generic.c |    6 ---
 drivers/block/pktcdvd.c   |    2 -
 include/linux/bio.h       |    4 +-
 4 files changed, 41 insertions(+), 52 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")
Cc: stable@vger.kernel.org
Reviewed-by: Omar Sandoval <osandov@fb.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 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.

Fixes: 74d46992e0d9 ("block: replace bi_bdev with a gendisk pointer and partitions index")
Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
Reviewed-by: Omar Sandoval <osandov@fb.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]

Fix a typo in pkt_start_recovery.

Fixes: 74d46992e0d9 ("block: replace bi_bdev with a gendisk pointer and partitions index")
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

[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 3/4] block: display the correct diskname for bio

[Christoph Hellwig]

Looks fine,

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

Re: [PATCH 4/4] block: fix a typo

[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]

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 V2 0/4] fix a few problems in block layer

[Jens Axboe]

On 2/27/18 5:07 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.

Applied 1-3 for 4.16, thanks.

-- 
Jens Axboe

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().