[PATCH] rbd: set discard alignment to zero

[PATCH] rbd: set discard alignment to zero

[David Disseldorp]

RBD devices are currently incorrectly initialised with the block queue
discard_alignment set to the underlying RADOS object size.

As per Documentation/ABI/testing/sysfs-block:
  The discard_alignment parameter indicates how many bytes the beginning
  of the partition is offset from the internal allocation unit's natural
  alignment.

Correcting the discard_alignment parameter from the RADOS object size to
zero has no effect on how discard requests are propagated through the
block layer - @alignment in __blkdev_issue_discard() remains zero.
However, it does fix the UNMAP granularity alignment value advertised
to SCSI initiators via the Block Limits VPD.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 drivers/block/rbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index b640ad8a6d20..cd4c65843f77 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4423,7 +4423,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	/* enable the discard support */
 	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
 	q->limits.discard_granularity = segment_size;
-	q->limits.discard_alignment = segment_size;
+	q->limits.discard_alignment = 0;
 	blk_queue_max_discard_sectors(q, segment_size / SECTOR_SIZE);
 	blk_queue_max_write_zeroes_sectors(q, segment_size / SECTOR_SIZE);
 
-- 
2.13.6

Re: [PATCH] rbd: set discard alignment to zero

[Ilya Dryomov]

On Wed, Nov 1, 2017 at 1:24 PM, David Disseldorp <ddiss@suse.de> wrote:
> RBD devices are currently incorrectly initialised with the block queue
> discard_alignment set to the underlying RADOS object size.
>
> As per Documentation/ABI/testing/sysfs-block:
>   The discard_alignment parameter indicates how many bytes the beginning
>   of the partition is offset from the internal allocation unit's natural
>   alignment.
>
> Correcting the discard_alignment parameter from the RADOS object size to
> zero has no effect on how discard requests are propagated through the
> block layer - @alignment in __blkdev_issue_discard() remains zero.
> However, it does fix the UNMAP granularity alignment value advertised
> to SCSI initiators via the Block Limits VPD.
>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/block/rbd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index b640ad8a6d20..cd4c65843f77 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -4423,7 +4423,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>         /* enable the discard support */
>         queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
>         q->limits.discard_granularity = segment_size;
> -       q->limits.discard_alignment = segment_size;
> +       q->limits.discard_alignment = 0;
>         blk_queue_max_discard_sectors(q, segment_size / SECTOR_SIZE);
>         blk_queue_max_write_zeroes_sectors(q, segment_size / SECTOR_SIZE);

Hi David,

Yeah, discard_alignment == discard_granularity seems bogus.  The name
is misleading too, but that probably comes from SBC, which talks about
OPTIMAL UNMAP GRANULARITY and UNMAP GRANULARITY ALIGNMENT.

0 is the default -- let's just drop discard_alignment assignment?
Also, you quoted <disk>/<partition>/discard_alignment description, but
this is about <disk>/discard_alignment.  This had me confused for
a moment.

Thanks,

                Ilya

Re: [PATCH] rbd: set discard alignment to zero

[David Disseldorp]

Thanks for the feedback, Ilya.

On Wed, 1 Nov 2017 18:14:04 +0100, Ilya Dryomov wrote:

> Yeah, discard_alignment == discard_granularity seems bogus.  The name
> is misleading too, but that probably comes from SBC, which talks about
> OPTIMAL UNMAP GRANULARITY and UNMAP GRANULARITY ALIGNMENT.
> 
> 0 is the default -- let's just drop discard_alignment assignment?

Works for me, if you'd prefer. I'll send through a v2 with this.

> Also, you quoted <disk>/<partition>/discard_alignment description, but
> this is about <disk>/discard_alignment.  This had me confused for
> a moment.

Oops, my bad. Will use the <disk> only description in the next round.

Cheers, David

[PATCH v2] rbd: set discard alignment to zero

[David Disseldorp]

RBD devices are currently incorrectly initialised with the block queue
discard_alignment set to the underlying RADOS object size.

As per Documentation/ABI/testing/sysfs-block:
  The discard_alignment parameter indicates how many bytes the beginning
  of the device is offset from the internal allocation unit's natural
  alignment.

Correcting the discard_alignment parameter from the RADOS object size to
zero (the blk_set_default_limits() default) has no effect on how discard
requests are propagated through the block layer - @alignment in
__blkdev_issue_discard() remains zero. However, it does fix the UNMAP
granularity alignment value advertised to SCSI initiators via the Block
Limits VPD.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 drivers/block/rbd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index b640ad8a6d20..016542c75e1f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4423,7 +4423,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 	/* enable the discard support */
 	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
 	q->limits.discard_granularity = segment_size;
-	q->limits.discard_alignment = segment_size;
 	blk_queue_max_discard_sectors(q, segment_size / SECTOR_SIZE);
 	blk_queue_max_write_zeroes_sectors(q, segment_size / SECTOR_SIZE);
 
-- 
2.13.6

Re: [PATCH v2] rbd: set discard alignment to zero

[Ilya Dryomov]

On Thu, Nov 2, 2017 at 1:05 AM, David Disseldorp <ddiss@suse.de> wrote:
> RBD devices are currently incorrectly initialised with the block queue
> discard_alignment set to the underlying RADOS object size.
>
> As per Documentation/ABI/testing/sysfs-block:
>   The discard_alignment parameter indicates how many bytes the beginning
>   of the device is offset from the internal allocation unit's natural
>   alignment.
>
> Correcting the discard_alignment parameter from the RADOS object size to
> zero (the blk_set_default_limits() default) has no effect on how discard
> requests are propagated through the block layer - @alignment in
> __blkdev_issue_discard() remains zero. However, it does fix the UNMAP
> granularity alignment value advertised to SCSI initiators via the Block
> Limits VPD.
>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/block/rbd.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index b640ad8a6d20..016542c75e1f 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -4423,7 +4423,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>         /* enable the discard support */
>         queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
>         q->limits.discard_granularity = segment_size;
> -       q->limits.discard_alignment = segment_size;
>         blk_queue_max_discard_sectors(q, segment_size / SECTOR_SIZE);
>         blk_queue_max_write_zeroes_sectors(q, segment_size / SECTOR_SIZE);

LGTM, applied.

Thanks,

                Ilya