[PATCH v2 0/3] block: improvements for discard alignment

[PATCH v2 0/3] block: improvements for discard alignment

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

For ease of debugging, patch 1 adds a sysfs entry for discard_alignment.
Patch 2 adjusts the computation of the granularity-adjusted
max_discard_sectors so that it prepares for the new code in patch 3,
which 3 actually adjusts the split.

v1->v2: added patch 1, fixed line length

Paolo Bonzini (3):
  block: add sysfs entry for discard_alignment
  block: reorganize rounding of max_discard_sectors
  block: split discard into aligned requests

 block/blk-lib.c   |   41 ++++++++++++++++++++++++++++-------------
 block/blk-sysfs.c |   11 +++++++++++
 2 files changed, 39 insertions(+), 13 deletions(-)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 0/3] block: improvements for discard alignment

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

For ease of debugging, patch 1 adds a sysfs entry for discard_alignment.
Patch 2 adjusts the computation of the granularity-adjusted
max_discard_sectors so that it prepares for the new code in patch 3,
which 3 actually adjusts the split.

v1->v2: added patch 1, fixed line length

Paolo Bonzini (3):
  block: add sysfs entry for discard_alignment
  block: reorganize rounding of max_discard_sectors
  block: split discard into aligned requests

 block/blk-lib.c   |   41 ++++++++++++++++++++++++++++-------------
 block/blk-sysfs.c |   11 +++++++++++
 2 files changed, 39 insertions(+), 13 deletions(-)

[PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Paolo Bonzini]

The next patches will actually use the alignment, expose it in sysfs
for ease of debugging.

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

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index aa41b47..95e919c 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -146,6 +146,11 @@ static ssize_t queue_io_opt_show(struct request_queue *q, char *page)
 	return queue_var_show(queue_io_opt(q), page);
 }
 
+static ssize_t queue_discard_alignment_show(struct request_queue *q, char *page)
+{
+	return queue_var_show(q->limits.discard_alignment, page);
+}
+
 static ssize_t queue_discard_granularity_show(struct request_queue *q, char *page)
 {
 	return queue_var_show(q->limits.discard_granularity, page);
@@ -343,6 +348,11 @@ static struct queue_sysfs_entry queue_io_opt_entry = {
 	.show = queue_io_opt_show,
 };
 
+static struct queue_sysfs_entry queue_discard_alignment_entry = {
+	.attr = {.name = "discard_alignment", .mode = S_IRUGO },
+	.show = queue_discard_alignment_show,
+};
+
 static struct queue_sysfs_entry queue_discard_granularity_entry = {
 	.attr = {.name = "discard_granularity", .mode = S_IRUGO },
 	.show = queue_discard_granularity_show,
@@ -403,6 +413,7 @@ static struct attribute *default_attrs[] = {
 	&queue_io_min_entry.attr,
 	&queue_io_opt_entry.attr,
 	&queue_discard_granularity_entry.attr,
+	&queue_discard_alignment_entry.attr,
 	&queue_discard_max_entry.attr,
 	&queue_discard_zeroes_data_entry.attr,
 	&queue_nonrot_entry.attr,
-- 
1.7.1


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Paolo Bonzini]

The next patches will actually use the alignment, expose it in sysfs
for ease of debugging.

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

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index aa41b47..95e919c 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -146,6 +146,11 @@ static ssize_t queue_io_opt_show(struct request_queue *q, char *page)
 	return queue_var_show(queue_io_opt(q), page);
 }
 
+static ssize_t queue_discard_alignment_show(struct request_queue *q, char *page)
+{
+	return queue_var_show(q->limits.discard_alignment, page);
+}
+
 static ssize_t queue_discard_granularity_show(struct request_queue *q, char *page)
 {
 	return queue_var_show(q->limits.discard_granularity, page);
@@ -343,6 +348,11 @@ static struct queue_sysfs_entry queue_io_opt_entry = {
 	.show = queue_io_opt_show,
 };
 
+static struct queue_sysfs_entry queue_discard_alignment_entry = {
+	.attr = {.name = "discard_alignment", .mode = S_IRUGO },
+	.show = queue_discard_alignment_show,
+};
+
 static struct queue_sysfs_entry queue_discard_granularity_entry = {
 	.attr = {.name = "discard_granularity", .mode = S_IRUGO },
 	.show = queue_discard_granularity_show,
@@ -403,6 +413,7 @@ static struct attribute *default_attrs[] = {
 	&queue_io_min_entry.attr,
 	&queue_io_opt_entry.attr,
 	&queue_discard_granularity_entry.attr,
+	&queue_discard_alignment_entry.attr,
 	&queue_discard_max_entry.attr,
 	&queue_discard_zeroes_data_entry.attr,
 	&queue_nonrot_entry.attr,
-- 
1.7.1

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Vivek Goyal]

On Mon, Jul 02, 2012 at 03:20:23PM +0200, Paolo Bonzini wrote:
> The next patches will actually use the alignment, expose it in sysfs
> for ease of debugging.
> 

Don't we already have discard_alignment exported as device attribute.

/sys/block/<dev>/discard_alignment

Thanks
Vivek

> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/blk-sysfs.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index aa41b47..95e919c 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -146,6 +146,11 @@ static ssize_t queue_io_opt_show(struct request_queue *q, char *page)
>  	return queue_var_show(queue_io_opt(q), page);
>  }
>  
> +static ssize_t queue_discard_alignment_show(struct request_queue *q, char *page)
> +{
> +	return queue_var_show(q->limits.discard_alignment, page);
> +}
> +
>  static ssize_t queue_discard_granularity_show(struct request_queue *q, char *page)
>  {
>  	return queue_var_show(q->limits.discard_granularity, page);
> @@ -343,6 +348,11 @@ static struct queue_sysfs_entry queue_io_opt_entry = {
>  	.show = queue_io_opt_show,
>  };
>  
> +static struct queue_sysfs_entry queue_discard_alignment_entry = {
> +	.attr = {.name = "discard_alignment", .mode = S_IRUGO },
> +	.show = queue_discard_alignment_show,
> +};
> +
>  static struct queue_sysfs_entry queue_discard_granularity_entry = {
>  	.attr = {.name = "discard_granularity", .mode = S_IRUGO },
>  	.show = queue_discard_granularity_show,
> @@ -403,6 +413,7 @@ static struct attribute *default_attrs[] = {
>  	&queue_io_min_entry.attr,
>  	&queue_io_opt_entry.attr,
>  	&queue_discard_granularity_entry.attr,
> +	&queue_discard_alignment_entry.attr,
>  	&queue_discard_max_entry.attr,
>  	&queue_discard_zeroes_data_entry.attr,
>  	&queue_nonrot_entry.attr,
> -- 
> 1.7.1
> 
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Vivek Goyal]

On Mon, Jul 02, 2012 at 03:20:23PM +0200, Paolo Bonzini wrote:
> The next patches will actually use the alignment, expose it in sysfs
> for ease of debugging.
> 

Don't we already have discard_alignment exported as device attribute.

/sys/block/<dev>/discard_alignment

Thanks
Vivek

> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/blk-sysfs.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index aa41b47..95e919c 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -146,6 +146,11 @@ static ssize_t queue_io_opt_show(struct request_queue *q, char *page)
>  	return queue_var_show(queue_io_opt(q), page);
>  }
>  
> +static ssize_t queue_discard_alignment_show(struct request_queue *q, char *page)
> +{
> +	return queue_var_show(q->limits.discard_alignment, page);
> +}
> +
>  static ssize_t queue_discard_granularity_show(struct request_queue *q, char *page)
>  {
>  	return queue_var_show(q->limits.discard_granularity, page);
> @@ -343,6 +348,11 @@ static struct queue_sysfs_entry queue_io_opt_entry = {
>  	.show = queue_io_opt_show,
>  };
>  
> +static struct queue_sysfs_entry queue_discard_alignment_entry = {
> +	.attr = {.name = "discard_alignment", .mode = S_IRUGO },
> +	.show = queue_discard_alignment_show,
> +};
> +
>  static struct queue_sysfs_entry queue_discard_granularity_entry = {
>  	.attr = {.name = "discard_granularity", .mode = S_IRUGO },
>  	.show = queue_discard_granularity_show,
> @@ -403,6 +413,7 @@ static struct attribute *default_attrs[] = {
>  	&queue_io_min_entry.attr,
>  	&queue_io_opt_entry.attr,
>  	&queue_discard_granularity_entry.attr,
> +	&queue_discard_alignment_entry.attr,
>  	&queue_discard_max_entry.attr,
>  	&queue_discard_zeroes_data_entry.attr,
>  	&queue_nonrot_entry.attr,
> -- 
> 1.7.1
> 
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Mike Snitzer]

On Mon, Jul 02 2012 at 10:34pm -0400,
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Mon, Jul 02, 2012 at 03:20:23PM +0200, Paolo Bonzini wrote:
> > The next patches will actually use the alignment, expose it in sysfs
> > for ease of debugging.
> > 
> 
> Don't we already have discard_alignment exported as device attribute.
> 
> /sys/block/<dev>/discard_alignment

Yes we do.  It is documented in Documentation/ABI/testing/sysfs-block

So this patch isn't needed.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Mike Snitzer]

On Mon, Jul 02 2012 at 10:34pm -0400,
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Mon, Jul 02, 2012 at 03:20:23PM +0200, Paolo Bonzini wrote:
> > The next patches will actually use the alignment, expose it in sysfs
> > for ease of debugging.
> > 
> 
> Don't we already have discard_alignment exported as device attribute.
> 
> /sys/block/<dev>/discard_alignment

Yes we do.  It is documented in Documentation/ABI/testing/sysfs-block

So this patch isn't needed.

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Paolo Bonzini]

Il 03/07/2012 04:34, Vivek Goyal ha scritto:
>> > The next patches will actually use the alignment, expose it in sysfs
>> > for ease of debugging.
>> > 
> Don't we already have discard_alignment exported as device attribute.
> 
> /sys/block/<dev>/discard_alignment

Ah, interesting, I missed it completely.  I guess it's because queue/
directories only exist for full disks, and the correct alignment varies
for each partition.  So this patch is unnecessary.

Paolo

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Paolo Bonzini]

Il 03/07/2012 04:34, Vivek Goyal ha scritto:
>> > The next patches will actually use the alignment, expose it in sysfs
>> > for ease of debugging.
>> > 
> Don't we already have discard_alignment exported as device attribute.
> 
> /sys/block/<dev>/discard_alignment

Ah, interesting, I missed it completely.  I guess it's because queue/
directories only exist for full disks, and the correct alignment varies
for each partition.  So this patch is unnecessary.

Paolo

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Vivek Goyal]

On Tue, Jul 03, 2012 at 01:51:13PM +0200, Paolo Bonzini wrote:
> Il 03/07/2012 04:34, Vivek Goyal ha scritto:
> >> > The next patches will actually use the alignment, expose it in sysfs
> >> > for ease of debugging.
> >> > 
> > Don't we already have discard_alignment exported as device attribute.
> > 
> > /sys/block/<dev>/discard_alignment
> 
> Ah, interesting, I missed it completely.  I guess it's because queue/
> directories only exist for full disks, and the correct alignment varies
> for each partition.  So this patch is unnecessary.

Partition discard_alignments are available in
/sys/block/<disk>/<partition>/discard_alignment.

That raises an interesting question for patch3. If the discard is happening to
a partition, shouldn't you be looking at partition discard_alignment
instead of always looking at queue discard_alignment?

Thanks
Vivek

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Vivek Goyal]

On Tue, Jul 03, 2012 at 01:51:13PM +0200, Paolo Bonzini wrote:
> Il 03/07/2012 04:34, Vivek Goyal ha scritto:
> >> > The next patches will actually use the alignment, expose it in sysfs
> >> > for ease of debugging.
> >> > 
> > Don't we already have discard_alignment exported as device attribute.
> > 
> > /sys/block/<dev>/discard_alignment
> 
> Ah, interesting, I missed it completely.  I guess it's because queue/
> directories only exist for full disks, and the correct alignment varies
> for each partition.  So this patch is unnecessary.

Partition discard_alignments are available in
/sys/block/<disk>/<partition>/discard_alignment.

That raises an interesting question for patch3. If the discard is happening to
a partition, shouldn't you be looking at partition discard_alignment
instead of always looking at queue discard_alignment?

Thanks
Vivek

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Paolo Bonzini]

Il 03/07/2012 16:00, Vivek Goyal ha scritto:
>> > 
>> > Ah, interesting, I missed it completely.  I guess it's because queue/
>> > directories only exist for full disks, and the correct alignment varies
>> > for each partition.  So this patch is unnecessary.
> Partition discard_alignments are available in
> /sys/block/<disk>/<partition>/discard_alignment.

Yes.  If it were in queue/, it wouldn't be available.

> That raises an interesting question for patch3. If the discard is happening to
> a partition, shouldn't you be looking at partition discard_alignment
> instead of always looking at queue discard_alignment?

Good point!  Like this?

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ba43f40..3530764 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1125,6 +1125,16 @@ static inline int queue_limit_discard_alignment(struct queue_limits *lim, sector
 		& (lim->discard_granularity - 1);
 }
 
+static inline int bdev_discard_alignment(struct block_device *bdev)
+{
+	struct request_queue *q = bdev_get_queue(bdev);
+
+	if (bdev != bdev->bd_contains)
+		return bdev->bd_part->discard_alignment;
+
+	return q->limits.discard_alignment;
+}
+
 static inline unsigned int queue_discard_zeroes_data(struct request_queue *q)
 {
 	if (q->limits.max_discard_sectors && q->limits.discard_zeroes_data == 1)
diff --git a/block/blk-lib.c b/block/blk-lib.c
index b2bde5c..77d8869 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -58,7 +58,7 @@ 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;
+	alignment = bdev_discard_alignment(bdev) >> 9;
 
 	/*
 	 * Ensure that max_discard_sectors is of the proper

Paolo

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Paolo Bonzini]

Il 03/07/2012 16:00, Vivek Goyal ha scritto:
>> > 
>> > Ah, interesting, I missed it completely.  I guess it's because queue/
>> > directories only exist for full disks, and the correct alignment varies
>> > for each partition.  So this patch is unnecessary.
> Partition discard_alignments are available in
> /sys/block/<disk>/<partition>/discard_alignment.

Yes.  If it were in queue/, it wouldn't be available.

> That raises an interesting question for patch3. If the discard is happening to
> a partition, shouldn't you be looking at partition discard_alignment
> instead of always looking at queue discard_alignment?

Good point!  Like this?

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ba43f40..3530764 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1125,6 +1125,16 @@ static inline int queue_limit_discard_alignment(struct queue_limits *lim, sector
 		& (lim->discard_granularity - 1);
 }
 
+static inline int bdev_discard_alignment(struct block_device *bdev)
+{
+	struct request_queue *q = bdev_get_queue(bdev);
+
+	if (bdev != bdev->bd_contains)
+		return bdev->bd_part->discard_alignment;
+
+	return q->limits.discard_alignment;
+}
+
 static inline unsigned int queue_discard_zeroes_data(struct request_queue *q)
 {
 	if (q->limits.max_discard_sectors && q->limits.discard_zeroes_data == 1)
diff --git a/block/blk-lib.c b/block/blk-lib.c
index b2bde5c..77d8869 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -58,7 +58,7 @@ 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;
+	alignment = bdev_discard_alignment(bdev) >> 9;
 
 	/*
 	 * Ensure that max_discard_sectors is of the proper

Paolo

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Vivek Goyal]

On Tue, Jul 03, 2012 at 04:21:34PM +0200, Paolo Bonzini wrote:

[..]
> > That raises an interesting question for patch3. If the discard is happening to
> > a partition, shouldn't you be looking at partition discard_alignment
> > instead of always looking at queue discard_alignment?
> 
> Good point!  Like this?

This looks better.
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index ba43f40..3530764 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1125,6 +1125,16 @@ static inline int queue_limit_discard_alignment(struct queue_limits *lim, sector
>  		& (lim->discard_granularity - 1);
>  }
>  
> +static inline int bdev_discard_alignment(struct block_device *bdev)
> +{
> +	struct request_queue *q = bdev_get_queue(bdev);
> +
> +	if (bdev != bdev->bd_contains)
> +		return bdev->bd_part->discard_alignment;
> +
> +	return q->limits.discard_alignment;
> +}
> +
>  static inline unsigned int queue_discard_zeroes_data(struct request_queue *q)
>  {
>  	if (q->limits.max_discard_sectors && q->limits.discard_zeroes_data == 1)
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index b2bde5c..77d8869 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -58,7 +58,7 @@ 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;
> +	alignment = bdev_discard_alignment(bdev) >> 9;

Why are you removing AND with mask operation? I don't see any AND
operation being done in bdev_discard_alignment().

Thanks
Vivek

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Vivek Goyal]

On Tue, Jul 03, 2012 at 04:21:34PM +0200, Paolo Bonzini wrote:

[..]
> > That raises an interesting question for patch3. If the discard is happening to
> > a partition, shouldn't you be looking at partition discard_alignment
> > instead of always looking at queue discard_alignment?
> 
> Good point!  Like this?

This looks better.
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index ba43f40..3530764 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1125,6 +1125,16 @@ static inline int queue_limit_discard_alignment(struct queue_limits *lim, sector
>  		& (lim->discard_granularity - 1);
>  }
>  
> +static inline int bdev_discard_alignment(struct block_device *bdev)
> +{
> +	struct request_queue *q = bdev_get_queue(bdev);
> +
> +	if (bdev != bdev->bd_contains)
> +		return bdev->bd_part->discard_alignment;
> +
> +	return q->limits.discard_alignment;
> +}
> +
>  static inline unsigned int queue_discard_zeroes_data(struct request_queue *q)
>  {
>  	if (q->limits.max_discard_sectors && q->limits.discard_zeroes_data == 1)
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index b2bde5c..77d8869 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -58,7 +58,7 @@ 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;
> +	alignment = bdev_discard_alignment(bdev) >> 9;

Why are you removing AND with mask operation? I don't see any AND
operation being done in bdev_discard_alignment().

Thanks
Vivek

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Paolo Bonzini]

Il 03/07/2012 16:39, Vivek Goyal ha scritto:
>> > +static inline int bdev_discard_alignment(struct block_device *bdev)
>> > +{
>> > +	struct request_queue *q = bdev_get_queue(bdev);
>> > +
>> > +	if (bdev != bdev->bd_contains)
>> > +		return bdev->bd_part->discard_alignment;
>> > +
>> > +	return q->limits.discard_alignment;
>> > +}
>> > +
>> >  static inline unsigned int queue_discard_zeroes_data(struct request_queue *q)
>> >  {
>> >  	if (q->limits.max_discard_sectors && q->limits.discard_zeroes_data == 1)
>> > diff --git a/block/blk-lib.c b/block/blk-lib.c
>> > index b2bde5c..77d8869 100644
>> > --- a/block/blk-lib.c
>> > +++ b/block/blk-lib.c
>> > @@ -58,7 +58,7 @@ 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;
>> > +	alignment = bdev_discard_alignment(bdev) >> 9;
> Why are you removing AND with mask operation? I don't see any AND
> operation being done in bdev_discard_alignment().

For partitions it is done by queue_limits_discard_alignment.  For disks,
it shouldn't be necessary at all but I can leave it.

paolo

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Paolo Bonzini]

Il 03/07/2012 16:39, Vivek Goyal ha scritto:
>> > +static inline int bdev_discard_alignment(struct block_device *bdev)
>> > +{
>> > +	struct request_queue *q = bdev_get_queue(bdev);
>> > +
>> > +	if (bdev != bdev->bd_contains)
>> > +		return bdev->bd_part->discard_alignment;
>> > +
>> > +	return q->limits.discard_alignment;
>> > +}
>> > +
>> >  static inline unsigned int queue_discard_zeroes_data(struct request_queue *q)
>> >  {
>> >  	if (q->limits.max_discard_sectors && q->limits.discard_zeroes_data == 1)
>> > diff --git a/block/blk-lib.c b/block/blk-lib.c
>> > index b2bde5c..77d8869 100644
>> > --- a/block/blk-lib.c
>> > +++ b/block/blk-lib.c
>> > @@ -58,7 +58,7 @@ 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;
>> > +	alignment = bdev_discard_alignment(bdev) >> 9;
> Why are you removing AND with mask operation? I don't see any AND
> operation being done in bdev_discard_alignment().

For partitions it is done by queue_limits_discard_alignment.  For disks,
it shouldn't be necessary at all but I can leave it.

paolo

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Vivek Goyal]

On Tue, Jul 03, 2012 at 04:40:40PM +0200, Paolo Bonzini wrote:
> Il 03/07/2012 16:39, Vivek Goyal ha scritto:
> >> > +static inline int bdev_discard_alignment(struct block_device *bdev)
> >> > +{
> >> > +	struct request_queue *q = bdev_get_queue(bdev);
> >> > +
> >> > +	if (bdev != bdev->bd_contains)
> >> > +		return bdev->bd_part->discard_alignment;
> >> > +
> >> > +	return q->limits.discard_alignment;
> >> > +}
> >> > +
> >> >  static inline unsigned int queue_discard_zeroes_data(struct request_queue *q)
> >> >  {
> >> >  	if (q->limits.max_discard_sectors && q->limits.discard_zeroes_data == 1)
> >> > diff --git a/block/blk-lib.c b/block/blk-lib.c
> >> > index b2bde5c..77d8869 100644
> >> > --- a/block/blk-lib.c
> >> > +++ b/block/blk-lib.c
> >> > @@ -58,7 +58,7 @@ 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;
> >> > +	alignment = bdev_discard_alignment(bdev) >> 9;
> > Why are you removing AND with mask operation? I don't see any AND
> > operation being done in bdev_discard_alignment().
> 
> For partitions it is done by queue_limits_discard_alignment.  For disks,
> it shouldn't be necessary at all but I can leave it.

Ok. I am fine with both with and without mask.

Thanks
Vivek

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [dm-devel] [PATCH v2 1/3] block: add sysfs entry for discard_alignment

[Vivek Goyal]

On Tue, Jul 03, 2012 at 04:40:40PM +0200, Paolo Bonzini wrote:
> Il 03/07/2012 16:39, Vivek Goyal ha scritto:
> >> > +static inline int bdev_discard_alignment(struct block_device *bdev)
> >> > +{
> >> > +	struct request_queue *q = bdev_get_queue(bdev);
> >> > +
> >> > +	if (bdev != bdev->bd_contains)
> >> > +		return bdev->bd_part->discard_alignment;
> >> > +
> >> > +	return q->limits.discard_alignment;
> >> > +}
> >> > +
> >> >  static inline unsigned int queue_discard_zeroes_data(struct request_queue *q)
> >> >  {
> >> >  	if (q->limits.max_discard_sectors && q->limits.discard_zeroes_data == 1)
> >> > diff --git a/block/blk-lib.c b/block/blk-lib.c
> >> > index b2bde5c..77d8869 100644
> >> > --- a/block/blk-lib.c
> >> > +++ b/block/blk-lib.c
> >> > @@ -58,7 +58,7 @@ 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;
> >> > +	alignment = bdev_discard_alignment(bdev) >> 9;
> > Why are you removing AND with mask operation? I don't see any AND
> > operation being done in bdev_discard_alignment().
> 
> For partitions it is done by queue_limits_discard_alignment.  For disks,
> it shouldn't be necessary at all but I can leave it.

Ok. I am fine with both with and without mask.

Thanks
Vivek

[PATCH v2 2/3] block: reorganize 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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..16b06f6 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;
@@ -54,18 +55,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.1


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 2/3] block: reorganize 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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..16b06f6 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;
@@ -54,18 +55,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.1

Re: [dm-devel] [PATCH v2 2/3] block: reorganize rounding of max_discard_sectors

[Vivek Goyal]

On Mon, Jul 02, 2012 at 03:20:24PM +0200, 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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 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..16b06f6 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;
> @@ -54,18 +55,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);

This is kind of odd. If discard_granularity is zero, we assume that
discards are supported and granularity is 1. But if max_discard_sectors
is zero, we assume discards are disabled. Not sure if we should treat
max_discard_sectors and discard_granularity in same way or not.

Thanks
Vivek

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [dm-devel] [PATCH v2 2/3] block: reorganize rounding of max_discard_sectors

[Vivek Goyal]

On Mon, Jul 02, 2012 at 03:20:24PM +0200, 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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 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..16b06f6 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;
> @@ -54,18 +55,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);

This is kind of odd. If discard_granularity is zero, we assume that
discards are supported and granularity is 1. But if max_discard_sectors
is zero, we assume discards are disabled. Not sure if we should treat
max_discard_sectors and discard_granularity in same way or not.

Thanks
Vivek

Re: [dm-devel] [PATCH v2 2/3] block: reorganize rounding of max_discard_sectors

[Paolo Bonzini]

Il 03/07/2012 04:49, Vivek Goyal ha scritto:
>> > +	/* 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);
> This is kind of odd. If discard_granularity is zero, we assume that
> discards are supported and granularity is 1. But if max_discard_sectors
> is zero, we assume discards are disabled. Not sure if we should treat
> max_discard_sectors and discard_granularity in same way or not.

Yes, this keeps the same behavior as before.  It is also the one that is
consistent with drivers/scsi/sd.c.  sd_config_discard always sets
limits.discard_granularity and then uses limits.max_discard_sectors to
disable discards.

Paolo

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [dm-devel] [PATCH v2 2/3] block: reorganize rounding of max_discard_sectors

[Paolo Bonzini]

Il 03/07/2012 04:49, Vivek Goyal ha scritto:
>> > +	/* 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);
> This is kind of odd. If discard_granularity is zero, we assume that
> discards are supported and granularity is 1. But if max_discard_sectors
> is zero, we assume discards are disabled. Not sure if we should treat
> max_discard_sectors and discard_granularity in same way or not.

Yes, this keeps the same behavior as before.  It is also the one that is
consistent with drivers/scsi/sd.c.  sd_config_discard always sets
limits.discard_granularity and then uses limits.max_discard_sectors to
disable discards.

Paolo

[PATCH v2 3/3] block: split discard into aligned requests

[Paolo Bonzini]

When a disk has large discard_granularity and small max_discard_sectors,
discards are not split with optimal alignment.  In the limit case of
discard_granularity == max_discard_sectors, all requests might end up
with incorrect alignment, so that logical blocks are discarded at all.

Here is an example that shows the condition handled in the patch.
Suppose discard_granularity == 64, max_discard_sectors == 128,
discard_alignment == 0 (in sectors).  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.

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.

The patch will also take into account the discard_alignment.

Cc: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: fixed line length

 block/blk-lib.c |   34 ++++++++++++++++++++++++----------
 1 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 16b06f6..b2bde5c 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,37 @@ 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.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 3/3] block: split discard into aligned requests

[Paolo Bonzini]

When a disk has large discard_granularity and small max_discard_sectors,
discards are not split with optimal alignment.  In the limit case of
discard_granularity == max_discard_sectors, all requests might end up
with incorrect alignment, so that logical blocks are discarded at all.

Here is an example that shows the condition handled in the patch.
Suppose discard_granularity == 64, max_discard_sectors == 128,
discard_alignment == 0 (in sectors).  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.

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.

The patch will also take into account the discard_alignment.

Cc: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: fixed line length

 block/blk-lib.c |   34 ++++++++++++++++++++++++----------
 1 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 16b06f6..b2bde5c 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,37 @@ 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.1