[PATCH 0/2] block: align start sector of a discard request

[PATCH 0/2] block: align start sector of a discard request

[Paolo Bonzini]

When a disk has a large discard_granularity, discards are not split with
optimal alignment; the pessimization gets bigger as discard_granularity
and max_discard_sectors become closer.

Take the limit case of discard_granularity == max_discard_sectors == 64.
Then, if a request is submitted for 256 sectors 2..257 it will be split
like this: 2..65, 66..129, 130..193, 194..257.  None of these requests
is aligned, so in fact you might end up with no discarded logical blocks
at all.  With this patch, the split will be 2..63, 64..127, 128..191,
192..255, 256..257.  The patches also take the discard_alignment into
consideration.

Patch 1 adjusts the computation of the granularity-adjusted
max_discard_sectors so that it prepares for the new code in patch 2,
while patch 2 actually adjusts the split.

Paolo Bonzini (2):
  block: tweak rounding of max_discard_sectors
  block: split discard into aligned requests

 block/blk-lib.c    |   41 +++++++++++++++++++++++++++--------------
 1 files changed, 27 insertions(+), 14 deletions(-)

-- 
1.7.7.6

[PATCH 1/2] block: tweak rounding of max_discard_sectors

[Paolo Bonzini]

Mostly a preparation for the next patch.

In principle this fixes an infinite loop if max_discard_sectors < granularity,
but that really shouldn't happen.

Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/blk-lib.c    |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 2b461b4..d4bb160 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -44,6 +44,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	struct request_queue *q = bdev_get_queue(bdev);
 	int type = REQ_WRITE | REQ_DISCARD;
 	unsigned int max_discard_sectors;
+	unsigned int granularity;
 	struct bio_batch bb;
 	struct bio *bio;
 	int ret = 0;
@@ -53,18 +54,18 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	if (!blk_queue_discard(q))
 		return -EOPNOTSUPP;
 
+	/* Zero-sector (unknown) and one-sector granularities are the same.  */
+	granularity = max(q->limits.discard_granularity >> 9, 1U);
+
 	/*
 	 * Ensure that max_discard_sectors is of the proper
 	 * granularity
 	 */
 	max_discard_sectors = min(q->limits.max_discard_sectors, UINT_MAX >> 9);
+	max_discard_sectors = round_down(max_discard_sectors, granularity);
 	if (unlikely(!max_discard_sectors)) {
 		/* Avoid infinite loop below. Being cautious never hurts. */
 		return -EOPNOTSUPP;
-	} else if (q->limits.discard_granularity) {
-		unsigned int disc_sects = q->limits.discard_granularity >> 9;
-
-		max_discard_sectors &= ~(disc_sects - 1);
 	}
 
 	if (flags & BLKDEV_DISCARD_SECURE) {
-- 
1.7.7.6

Re: [PATCH 1/2] block: tweak rounding of max_discard_sectors

[Christoph Hellwig]

On Wed, Mar 14, 2012 at 06:12:27PM +0100, Paolo Bonzini wrote:
> Mostly a preparation for the next patch.
> 
> In principle this fixes an infinite loop if max_discard_sectors < granularity,
> but that really shouldn't happen.
> 
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Looks good, thanks for fixing this.

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

[PATCH 2/2] block: split discard into aligned requests

[Paolo Bonzini]

When a disk has a relatively large discard_granularity or small
max_discard_sectors, discards are not split with optimal alignment.
In the limit case of discard_granularity == max_discard_sectors, all
requests might be aligned incorrectly, so in fact you might end up
with no discarded logical blocks at all.

One example that helps showing how the patch works is with
discard_granularity == 64, max_discard_sectors == 128.  A request that is
submitted for 256 sectors 2..257 will be split in two: 2..129, 130..257.
However, only 2 aligned blocks out of 3 are included in the request;
128..191 may be left intact and not discarded.  With this patch, the
first request will be truncated to ensure good alignment of what's left,
and the split will be 2..127, 128..255, 256..257.  discard_alignment is
also taken into account.

At most one extra request will be introduced, because the first request
will be reduced by at most granularity-1 sectors, and granularity
must be less than max_discard_sectors.  Subsequent requests will run
on round_down(max_discard_sectors, granularity) sectors, as in the
current code.

Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/blk-lib.c |   32 ++++++++++++++++++++++----------
 1 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index d4bb160..837ce96 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -44,7 +44,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	struct request_queue *q = bdev_get_queue(bdev);
 	int type = REQ_WRITE | REQ_DISCARD;
 	unsigned int max_discard_sectors;
-	unsigned int granularity;
+	unsigned int granularity, alignment, mask;
 	struct bio_batch bb;
 	struct bio *bio;
 	int ret = 0;
@@ -57,10 +57,12 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 
 	/* Zero-sector (unknown) and one-sector granularities are the same.  */
 	granularity = max(q->limits.discard_granularity >> 9, 1U);
+	mask = granularity - 1;
+	alignment = (q->limits.discard_alignment >> 9) & mask;
 
 	/*
 	 * Ensure that max_discard_sectors is of the proper
-	 * granularity
+	 * granularity, so that requests stay aligned after a split.
 	 */
 	max_discard_sectors = min(q->limits.max_discard_sectors, UINT_MAX >> 9);
 	max_discard_sectors = round_down(max_discard_sectors, granularity);
@@ -80,25 +82,35 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	bb.wait = &wait;
 
 	while (nr_sects) {
+		unsigned int req_sects;
+		sector_t end_sect;
+
 		bio = bio_alloc(gfp_mask, 1);
 		if (!bio) {
 			ret = -ENOMEM;
 			break;
 		}
 
+		req_sects = min_t(sector_t, nr_sects, max_discard_sectors);
+
+		/*
+		 * If splitting a request, and the next starting sector would be
+		 * misaligned, stop the discard at the previous aligned sector.
+		 */
+		end_sect = sector + req_sects;
+		if (req_sects < nr_sects && (end_sect & mask) != alignment) {
+			end_sect = round_down(end_sect - alignment, granularity) + alignment;
+			req_sects = end_sect - sector;
+		}
+
 		bio->bi_sector = sector;
 		bio->bi_end_io = bio_batch_end_io;
 		bio->bi_bdev = bdev;
 		bio->bi_private = &bb;
 
-		if (nr_sects > max_discard_sectors) {
-			bio->bi_size = max_discard_sectors << 9;
-			nr_sects -= max_discard_sectors;
-			sector += max_discard_sectors;
-		} else {
-			bio->bi_size = nr_sects << 9;
-			nr_sects = 0;
-		}
+		bio->bi_size = req_sects << 9;
+		nr_sects -= req_sects;
+		sector = end_sect;
 
 		atomic_inc(&bb.done);
 		submit_bio(type, bio);
-- 
1.7.7.6

Re: [PATCH 2/2] block: split discard into aligned requests

[Christoph Hellwig]

On Wed, Mar 14, 2012 at 06:12:28PM +0100, Paolo Bonzini wrote:
> When a disk has a relatively large discard_granularity or small
> max_discard_sectors, discards are not split with optimal alignment.
> In the limit case of discard_granularity == max_discard_sectors, all
> requests might be aligned incorrectly, so in fact you might end up
> with no discarded logical blocks at all.
> 
> One example that helps showing how the patch works is with
> discard_granularity == 64, max_discard_sectors == 128.  A request that is
> submitted for 256 sectors 2..257 will be split in two: 2..129, 130..257.
> However, only 2 aligned blocks out of 3 are included in the request;
> 128..191 may be left intact and not discarded.  With this patch, the
> first request will be truncated to ensure good alignment of what's left,
> and the split will be 2..127, 128..255, 256..257.  discard_alignment is
> also taken into account.
> 
> At most one extra request will be introduced, because the first request
> will be reduced by at most granularity-1 sectors, and granularity
> must be less than max_discard_sectors.  Subsequent requests will run
> on round_down(max_discard_sectors, granularity) sectors, as in the
> current code.
> 
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/blk-lib.c |   32 ++++++++++++++++++++++----------
>  1 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index d4bb160..837ce96 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -44,7 +44,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  	struct request_queue *q = bdev_get_queue(bdev);
>  	int type = REQ_WRITE | REQ_DISCARD;
>  	unsigned int max_discard_sectors;
> -	unsigned int granularity;
> +	unsigned int granularity, alignment, mask;
>  	struct bio_batch bb;
>  	struct bio *bio;
>  	int ret = 0;
> @@ -57,10 +57,12 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  
>  	/* Zero-sector (unknown) and one-sector granularities are the same.  */
>  	granularity = max(q->limits.discard_granularity >> 9, 1U);
> +	mask = granularity - 1;

Given that ~ (aligment -1 is a common pattern I thin kthe code would be
better readable without the mask variable.

> +		 * If splitting a request, and the next starting sector would be
> +		 * misaligned, stop the discard at the previous aligned sector.
> +		 */
> +		end_sect = sector + req_sects;
> +		if (req_sects < nr_sects && (end_sect & mask) != alignment) {
> +			end_sect = round_down(end_sect - alignment, granularity) + alignment;

Please avoid overlong lines.

Otherwise the changes look good to me,


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

Re: [PATCH 0/2] block: align start sector of a discard request

[Paolo Bonzini]

Il 14/03/2012 18:12, Paolo Bonzini ha scritto:
> When a disk has a large discard_granularity, discards are not split with
> optimal alignment; the pessimization gets bigger as discard_granularity
> and max_discard_sectors become closer.
> 
> Take the limit case of discard_granularity == max_discard_sectors == 64.
> Then, if a request is submitted for 256 sectors 2..257 it will be split
> like this: 2..65, 66..129, 130..193, 194..257.  None of these requests
> is aligned, so in fact you might end up with no discarded logical blocks
> at all.  With this patch, the split will be 2..63, 64..127, 128..191,
> 192..255, 256..257.  The patches also take the discard_alignment into
> consideration.
> 
> Patch 1 adjusts the computation of the granularity-adjusted
> max_discard_sectors so that it prepares for the new code in patch 2,
> while patch 2 actually adjusts the split.
> 
> Paolo Bonzini (2):
>   block: tweak rounding of max_discard_sectors
>   block: split discard into aligned requests
> 
>  block/blk-lib.c    |   41 +++++++++++++++++++++++++++--------------
>  1 files changed, 27 insertions(+), 14 deletions(-)
> 

Ping?

Paolo

Re: [PATCH 0/2] block: align start sector of a discard request

[Paolo Bonzini]

Il 14/03/2012 18:12, Paolo Bonzini ha scritto:
> When a disk has a large discard_granularity, discards are not split with
> optimal alignment; the pessimization gets bigger as discard_granularity
> and max_discard_sectors become closer.
> 
> Take the limit case of discard_granularity == max_discard_sectors == 64.
> Then, if a request is submitted for 256 sectors 2..257 it will be split
> like this: 2..65, 66..129, 130..193, 194..257.  None of these requests
> is aligned, so in fact you might end up with no discarded logical blocks
> at all.  With this patch, the split will be 2..63, 64..127, 128..191,
> 192..255, 256..257.  The patches also take the discard_alignment into
> consideration.
> 
> Patch 1 adjusts the computation of the granularity-adjusted
> max_discard_sectors so that it prepares for the new code in patch 2,
> while patch 2 actually adjusts the split.
> 
> Paolo Bonzini (2):
>   block: tweak rounding of max_discard_sectors
>   block: split discard into aligned requests
> 
>  block/blk-lib.c    |   41 +++++++++++++++++++++++++++--------------
>  1 files changed, 27 insertions(+), 14 deletions(-)
> 

Christoph, since you talked about this on the qemu-devel mailing list,
do you have a few free cycles to review this?

Paolo

Re: [PATCH 0/2] block: align start sector of a discard request

[Christoph Hellwig]

On Tue, Mar 27, 2012 at 08:43:11AM +0200, Paolo Bonzini wrote:
> Christoph, since you talked about this on the qemu-devel mailing list,
> do you have a few free cycles to review this?

Oh, I completely missed that you already sent patches for the issue.
I'll take a look.