[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

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

[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

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

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

[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