Virtual I/O Device (VIRTIO) Version 1.1
https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
> discard_sector_alignment can be used by OS when splitting a request
> based on alignment.

According to Documentation/ABI/stable/sysfs-block, the corresponding
field in the kernel is, confusingly, discard_granularity, not
discard_alignment.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 drivers/block/virtio_blk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index c443cd64fc9b..1fb3c89900e3 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -913,11 +913,9 @@ static int virtblk_probe(struct virtio_device *vdev)
 		blk_queue_io_opt(q, blk_size * opt_io_size);
 
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
-		q->limits.discard_granularity = blk_size;
-
 		virtio_cread(vdev, struct virtio_blk_config,
 			     discard_sector_alignment, &v);
-		q->limits.discard_alignment = v ? v << SECTOR_SHIFT : 0;
+		q->limits.discard_granularity = v ? v << SECTOR_SHIFT : 0;
 
 		virtio_cread(vdev, struct virtio_blk_config,
 			     max_discard_sectors, &v);
-- 
2.35.1

On Thu, Feb 24, 2022 at 06:38:02PM +0900, Akihiko Odaki wrote:
> Virtual I/O Device (VIRTIO) Version 1.1
> https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
> > discard_sector_alignment can be used by OS when splitting a request
> > based on alignment.
> 
> According to Documentation/ABI/stable/sysfs-block, the corresponding
> field in the kernel is, confusingly, discard_granularity, not
> discard_alignment.

Good catch, struct virtio_blk_config->discard_sector_alignment is Linux
q->limits.discard_granularity.

> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> ---
>  drivers/block/virtio_blk.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index c443cd64fc9b..1fb3c89900e3 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -913,11 +913,9 @@ static int virtblk_probe(struct virtio_device *vdev)
>  		blk_queue_io_opt(q, blk_size * opt_io_size);
>  
>  	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
> -		q->limits.discard_granularity = blk_size;
> -
>  		virtio_cread(vdev, struct virtio_blk_config,
>  			     discard_sector_alignment, &v);
> -		q->limits.discard_alignment = v ? v << SECTOR_SHIFT : 0;

Should we use struct virtio_blk_config->topology.alignment_offset
("offset of first aligned logical block" and used for Linux
blk_queue_alignment_offset()) for q->limits.discard_alignment?

On 2022/02/28 19:51, Stefan Hajnoczi wrote:
> On Thu, Feb 24, 2022 at 06:38:02PM +0900, Akihiko Odaki wrote:
>> Virtual I/O Device (VIRTIO) Version 1.1
>> https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
>>> discard_sector_alignment can be used by OS when splitting a request
>>> based on alignment.
>>
>> According to Documentation/ABI/stable/sysfs-block, the corresponding
>> field in the kernel is, confusingly, discard_granularity, not
>> discard_alignment.
> 
> Good catch, struct virtio_blk_config->discard_sector_alignment is Linux
> q->limits.discard_granularity.
> 
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
>> ---
>>   drivers/block/virtio_blk.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index c443cd64fc9b..1fb3c89900e3 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -913,11 +913,9 @@ static int virtblk_probe(struct virtio_device *vdev)
>>   		blk_queue_io_opt(q, blk_size * opt_io_size);
>>   
>>   	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
>> -		q->limits.discard_granularity = blk_size;
>> -
>>   		virtio_cread(vdev, struct virtio_blk_config,
>>   			     discard_sector_alignment, &v);
>> -		q->limits.discard_alignment = v ? v << SECTOR_SHIFT : 0;
> 
> Should we use struct virtio_blk_config->topology.alignment_offset
> ("offset of first aligned logical block" and used for Linux
> blk_queue_alignment_offset()) for q->limits.discard_alignment?

Maybe but I'm not sure. I had looked at the code of QEMU
(commit 5c1ee569660d4a205dced9cb4d0306b907fb7599) but it apparently 
always sets 0 for virtio_blk_config->topology.alignment_offset.
I don't have a hardware which requires discard_alignment either so I 
cannot test it.

I'd like to leave this patch as is since I cannot deny the possibility 
that the host has a different alignment offset for discarding and other 
operations.

On Tue, Mar 01, 2022 at 02:43:55PM +0900, Akihiko Odaki wrote:
> On 2022/02/28 19:51, Stefan Hajnoczi wrote:
> > On Thu, Feb 24, 2022 at 06:38:02PM +0900, Akihiko Odaki wrote:
> > > Virtual I/O Device (VIRTIO) Version 1.1
> > > https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
> > > > discard_sector_alignment can be used by OS when splitting a request
> > > > based on alignment.
> > > 
> > > According to Documentation/ABI/stable/sysfs-block, the corresponding
> > > field in the kernel is, confusingly, discard_granularity, not
> > > discard_alignment.
> > 
> > Good catch, struct virtio_blk_config->discard_sector_alignment is Linux
> > q->limits.discard_granularity.
> > 
> > > 
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> > > ---
> > >   drivers/block/virtio_blk.c | 4 +---
> > >   1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > index c443cd64fc9b..1fb3c89900e3 100644
> > > --- a/drivers/block/virtio_blk.c
> > > +++ b/drivers/block/virtio_blk.c
> > > @@ -913,11 +913,9 @@ static int virtblk_probe(struct virtio_device *vdev)
> > >   		blk_queue_io_opt(q, blk_size * opt_io_size);
> > >   	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
> > > -		q->limits.discard_granularity = blk_size;
> > > -
> > >   		virtio_cread(vdev, struct virtio_blk_config,
> > >   			     discard_sector_alignment, &v);
> > > -		q->limits.discard_alignment = v ? v << SECTOR_SHIFT : 0;
> > 
> > Should we use struct virtio_blk_config->topology.alignment_offset
> > ("offset of first aligned logical block" and used for Linux
> > blk_queue_alignment_offset()) for q->limits.discard_alignment?
> 
> Maybe but I'm not sure. I had looked at the code of QEMU
> (commit 5c1ee569660d4a205dced9cb4d0306b907fb7599) but it apparently always
> sets 0 for virtio_blk_config->topology.alignment_offset.
> I don't have a hardware which requires discard_alignment either so I cannot
> test it.
> 
> I'd like to leave this patch as is since I cannot deny the possibility that
> the host has a different alignment offset for discarding and other
> operations.

Fair enough. To do it properly we'd need to add a new configuration
space field to virtio-blk.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Akihiko,

> I'd like to leave this patch as is since I cannot deny the possibility
> that the host has a different alignment offset for discarding and
> other operations.

That's correct.

SCSI explicitly reports separate values for physical block alignment and
"discard" alignment. The queue limits reflect this distinction.

While it is not super common for these two to be different, it does
happen. There are several disk arrays that do not have an internal
allocation unit (discard granularity) which is a power of two, for
instance.

I am not particularly happy that we have to deal with two distinct types
of alignment in the stack but that is the reality of the hardware we
have to support.

-- 
Martin K. Petersen	Oracle Linux Engineering