[PATCH 0/3] block: make sure discard/writesame bio is aligned with logical block size

[PATCH 0/3] block: make sure discard/writesame bio is aligned with logical block size

[Ming Lei]

Hi,

The 1st & 3rd patch fixes bio size alignment issue.

The 2nd patch cleans up __blkdev_issue_discard() a bit.

Thanks,


Ming Lei (3):
  block: make sure discard bio is aligned with logical block size
  block: cleanup __blkdev_issue_discard()
  block: make sure writesame bio is aligned with logical block size

 block/blk-lib.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

Cc: Rui Salvaterra <rsalvaterra@gmail.com>
Cc: stable@vger.kernel.org
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Xiao Ni <xni@redhat.com>
Cc: Mariusz Dabrowski <mariusz.dabrowski@intel.com>

-- 
2.9.5

[PATCH 1/3] block: make sure discard bio is aligned with logical block size

[Ming Lei]

Obviously the created discard bio has to be aligned with logical block
size.

Fixes: 744889b7cbb56a6 ("block: don't deal with discard limit in blkdev_issue_discard()")
Reported-by: Rui Salvaterra <rsalvaterra@gmail.com>
Cc: Rui Salvaterra <rsalvaterra@gmail.com>
Cc: stable@vger.kernel.org
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Xiao Ni <xni@redhat.com>
Cc: Mariusz Dabrowski <mariusz.dabrowski@intel.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index bbd44666f2b5..aa3944946b2f 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -59,7 +59,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		if (!req_sects)
 			goto fail;
 		if (req_sects > UINT_MAX >> 9)
-			req_sects = UINT_MAX >> 9;
+			req_sects = (UINT_MAX >> 9) & ~bs_mask;
 
 		end_sect = sector + req_sects;
 
-- 
2.9.5

Re: [PATCH 1/3] block: make sure discard bio is aligned with logical block size

[Christoph Hellwig]

>  		if (req_sects > UINT_MAX >> 9)
> -			req_sects = UINT_MAX >> 9;
> +			req_sects = (UINT_MAX >> 9) & ~bs_mask;

Given that we have this same thing duplicated in write zeroes
what about a documented helper?

Re: [PATCH 1/3] block: make sure discard bio is aligned with logical block size

[Ming Lei]

On Fri, Oct 26, 2018 at 09:44:15AM +0200, Christoph Hellwig wrote:
> >  		if (req_sects > UINT_MAX >> 9)
> > -			req_sects = UINT_MAX >> 9;
> > +			req_sects = (UINT_MAX >> 9) & ~bs_mask;
> 
> Given that we have this same thing duplicated in write zeroes
> what about a documented helper?

IMO, using UINT_MAX & bs_mask is better because it is self-explanatory
in the context.

If we introduce one helper, it may not be easy to find a better
name than UINT_MAX.

thanks,
Ming

Re: [PATCH 1/3] block: make sure discard bio is aligned with logical block size

[Christoph Hellwig]

On Sun, Oct 28, 2018 at 08:51:31AM +0800, Ming Lei wrote:
> On Fri, Oct 26, 2018 at 09:44:15AM +0200, Christoph Hellwig wrote:
> > >  		if (req_sects > UINT_MAX >> 9)
> > > -			req_sects = UINT_MAX >> 9;
> > > +			req_sects = (UINT_MAX >> 9) & ~bs_mask;
> > 
> > Given that we have this same thing duplicated in write zeroes
> > what about a documented helper?
> 
> IMO, using UINT_MAX & bs_mask is better because it is self-explanatory
> in the context.

I don't think it is in any way.  I understand it because I know the
code, but there is nothing that documents why we do that.

Re: [PATCH 1/3] block: make sure discard bio is aligned with logical block size

[Ming Lei]

On Sun, Oct 28, 2018 at 04:49:47PM +0100, Christoph Hellwig wrote:
> On Sun, Oct 28, 2018 at 08:51:31AM +0800, Ming Lei wrote:
> > On Fri, Oct 26, 2018 at 09:44:15AM +0200, Christoph Hellwig wrote:
> > > >  		if (req_sects > UINT_MAX >> 9)
> > > > -			req_sects = UINT_MAX >> 9;
> > > > +			req_sects = (UINT_MAX >> 9) & ~bs_mask;
> > > 
> > > Given that we have this same thing duplicated in write zeroes
> > > what about a documented helper?
> > 
> > IMO, using UINT_MAX & bs_mask is better because it is self-explanatory
> > in the context.
> 
> I don't think it is in any way.  I understand it because I know the
> code, but there is nothing that documents why we do that.

Then how about introducing this helper?

 /*
+ * The max sectors one bio can handle is 'UINT_MAX >> 9' becasue
+ * bvec_iter.bi_size is defined as 'unsigned int', also it has to aligned
+ * to with logical block size which is minimum accepted unit by hardware.
+ */
+static inline unsigned int blk_max_allowed_max_secotrs(struct request_queue *q)
+{
+       return round_down(UINT_MAX, queue_logical_block_size(q)) >> 9;
+}
+
+/*

Thanks,
Ming

[PATCH 2/3] block: cleanup __blkdev_issue_discard()

[Ming Lei]

Cleanup __blkdev_issue_discard().

Cc: Rui Salvaterra <rsalvaterra@gmail.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Xiao Ni <xni@redhat.com>
Cc: Mariusz Dabrowski <mariusz.dabrowski@intel.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-lib.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index aa3944946b2f..93011785f710 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -52,16 +52,11 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	if ((sector | nr_sects) & bs_mask)
 		return -EINVAL;
 
-	while (nr_sects) {
-		unsigned int req_sects = nr_sects;
-		sector_t end_sect;
-
-		if (!req_sects)
-			goto fail;
-		if (req_sects > UINT_MAX >> 9)
-			req_sects = (UINT_MAX >> 9) & ~bs_mask;
+	if (!nr_sects)
+		return -EINVAL;
 
-		end_sect = sector + req_sects;
+	while (nr_sects) {
+		unsigned int req_sects = min(nr_sects, (UINT_MAX >> 9) & ~bs_mask);
 
 		bio = next_bio(bio, 0, gfp_mask);
 		bio->bi_iter.bi_sector = sector;
@@ -69,8 +64,8 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		bio_set_op_attrs(bio, op, 0);
 
 		bio->bi_iter.bi_size = req_sects << 9;
+		sector += req_sects;
 		nr_sects -= req_sects;
-		sector = end_sect;
 
 		/*
 		 * We can loop for a long time in here, if someone does
@@ -83,14 +78,6 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 
 	*biop = bio;
 	return 0;
-
-fail:
-	if (bio) {
-		submit_bio_wait(bio);
-		bio_put(bio);
-	}
-	*biop = NULL;
-	return -EOPNOTSUPP;
 }
 EXPORT_SYMBOL(__blkdev_issue_discard);
 
-- 
2.9.5

Re: [PATCH 2/3] block: cleanup __blkdev_issue_discard()

[Christoph Hellwig]

On Fri, Oct 26, 2018 at 02:24:34PM +0800, Ming Lei wrote:
> Cleanup __blkdev_issue_discard().

It would help to explain what you clean up..

> +		unsigned int req_sects = min(nr_sects, (UINT_MAX >> 9) & ~bs_mask);

This creates an overly long line.

[PATCH 3/3] block: make sure writesame bio is aligned with logical block size

[Ming Lei]

Obviously the created writesame bio has to be aligned with logical block
size.

Fixes: b49a0871be31a745b2ef ("block: remove split code in blkdev_issue_{discard,write_same}")
Cc: Rui Salvaterra <rsalvaterra@gmail.com>
Cc: stable@vger.kernel.org
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Xiao Ni <xni@redhat.com>
Cc: Mariusz Dabrowski <mariusz.dabrowski@intel.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 93011785f710..1750f0e480c0 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -149,7 +149,7 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
 		return -EOPNOTSUPP;
 
 	/* Ensure that max_write_same_sectors doesn't overflow bi_size */
-	max_write_same_sectors = UINT_MAX >> 9;
+	max_write_same_sectors = (UINT_MAX >> 9) & ~bs_mask;
 
 	while (nr_sects) {
 		bio = next_bio(bio, 1, gfp_mask);
-- 
2.9.5