Re: [PATCH 2/2] target: fix max discard sectors calculation

[Mike Christie <mchristi@redhat.com>]

On 05/18/2016 11:56 AM, Bart Van Assche wrote:
> On 05/17/2016 02:49 PM, Mike Christie wrote:
>> --- a/drivers/target/target_core_device.c
>> +++ b/drivers/target/target_core_device.c
>> @@ -821,13 +821,15 @@ struct se_device *target_alloc_device(struct
>> se_hba *hba, const char *name)
>>    * in ATA and we need to set TPE=1
>>    */
>>   bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
>> -                       struct request_queue *q, int block_size)
>> +                       struct request_queue *q)
>>   {
>> +    unsigned short block_size = queue_logical_block_size(q);
>> +
>>       if (!blk_queue_discard(q))
>>           return false;
>>
>> -    attrib->max_unmap_lba_count = (q->limits.max_discard_sectors << 9) /
>> -                                block_size;
>> +    attrib->max_unmap_lba_count = queue_sector_to_logical(q,
>> +                        q->limits.max_discard_sectors);
>>       /*
>>        * Currently hardcoded to 1 in Linux/SCSI code..
>>        */
>> diff --git a/drivers/target/target_core_file.c
>> b/drivers/target/target_core_file.c
>> index 75f0f08..7929186 100644
>> --- a/drivers/target/target_core_file.c
>> +++ b/drivers/target/target_core_file.c
>> @@ -161,8 +161,7 @@ static int fd_configure_device(struct se_device *dev)
>>               dev_size, div_u64(dev_size, fd_dev->fd_block_size),
>>               fd_dev->fd_block_size);
>>
>> -        if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
>> -                              fd_dev->fd_block_size))
>> +        if (target_configure_unmap_from_queue(&dev->dev_attrib, q))
>>               pr_debug("IFILE: BLOCK Discard support available,"
>>                    " disabled by default\n");
>>           /*
>> diff --git a/drivers/target/target_core_iblock.c
>> b/drivers/target/target_core_iblock.c
>> index 026a758..3cbe060 100644
>> --- a/drivers/target/target_core_iblock.c
>> +++ b/drivers/target/target_core_iblock.c
>> @@ -121,8 +121,7 @@ static int iblock_configure_device(struct
>> se_device *dev)
>>       dev->dev_attrib.hw_max_sectors = queue_max_hw_sectors(q);
>>       dev->dev_attrib.hw_queue_depth = q->nr_requests;
>>
>> -    if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
>> -                          dev->dev_attrib.hw_block_size))
>> +    if (target_configure_unmap_from_queue(&dev->dev_attrib, q))
>>           pr_debug("IBLOCK: BLOCK Discard support available,"
>>                " disabled by default\n");
>>
>> [ ... ]
> 
> Hello Mike,
> 
> Are you sure that LIO guarantees that dev_attrib.hw_block_size is
> identical to queue_logical_block_size(q)? The other changes in this
> patch look like a nice improvement to me.
> 

Yeah. In iblock a line above where patch/diff started from above we do:

        dev->dev_attrib.hw_block_size = bdev_logical_block_size(bd);


For the file setup code path we do:

fd_dev->fd_block_size = bdev_logical_block_size(inode->i_bdev);

and then a couple lines later we did:

if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
				fd_dev->fd_block_size))