[Devel] [PATCH RH9 v2 2/8] dm: add llseek_hole infrastructure

[Andrey Zhadchenko]

On 8/21/23 14:07, Alexander Atanasov wrote:
> On 16.08.23 12:32, Andrey Zhadchenko wrote:
>> Add new function to target_type, so any dm devices may realize it
>> Implement intermediate llseek_hole() on device-mapper layer, do some 
>> simple
>> checks.
>>
>> Feature: dm: implement SEEK_HOLE for dm-qcow2 and dm-ploop
>> https://jira.vzint.dev/browse/PSBM-145746
>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko at virtuozzo.com>
>>
>> ---
>> v2: fix return and goto inconsistency
>>
>>   drivers/md/dm.c               | 41 +++++++++++++++++++++++++++++++++++
>>   include/linux/device-mapper.h |  4 ++++
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>> index 7e5c61a75eeb..d592c023b090 100644
>> --- a/drivers/md/dm.c
>> +++ b/drivers/md/dm.c
>> @@ -484,6 +484,45 @@ static int dm_blk_ioctl(struct block_device 
>> *bdev, fmode_t mode,
>>       return r;
>>   }
>> +static loff_t dm_llseek_hole(struct block_device *bdev, loff_t 
>> offset, int whence)
>> +{
>> +    struct mapped_device *md = bdev->bd_disk->private_data;
>> +    struct dm_table *table;
>> +    struct dm_target *ti;
>> +    int srcu_idx;
>> +    loff_t ret;
>> +
>> +    table = dm_get_live_table(md, &srcu_idx);
>> +    if (!table || !dm_table_get_size(table))
>> +        return -ENOTTY;
>> +
>> +    /*
>> +     * For now we only support devices that have a single target.
>> +     * But probably it is not hard to break it to a few requests to a 
>> different
>> +     * targets
>> +     */
>> +    if (table->num_targets != 1)
>> +        return -EOPNOTSUPP;
> 
> 
> multipath, tracking and push_backup are all possible targets (that get 
> used) so any combination can increase the count - shouldn't this be TODO?

Maybe I am mistaken, but I always thought that table->num_targets is 
amount of targets within single device. e.g. linear dm made by two 
devices concatenation will have two as the number of targets. And I 
think dm-ploop and dm-qcow2 num targets is always one, at least for now

> 
>> +    ti = dm_table_get_target(table, 0);
>> +
>> +    if (dm_suspended_md(md)) {
>> +        ret = -EAGAIN;
>> +        goto out;
>> +    }
>> +
>> +    if (!ti->type->llseek_hole) {
>> +        ret = -EINVAL;
> 
> EOPNOTSUPP - may be swap with num_targets != 1 error - which will be 
> EINVAL due to the argument - while here it is not supported - not an 
> issue with the arguments ?

-EINVAL is here to preserve default SEEK_DATA or SEEK_HOLE behavior with 
block devices. Previously fixed_size_llseek() was called in all cases 
and returned -EINVAL when seeking holes or data. So dm devices without 
llseek_hole implementation should return this value too.

The same comment goes for 3/8 patch.

> 
>> +        goto out;
>> +    }
>> +
>> +    ret = ti->type->llseek_hole(ti, offset, whence);
>> +
>> +out:
>> +    dm_put_live_table(md, srcu_idx);
>> +    return ret;
>> +}
>> +
>>   u64 dm_start_time_ns_from_clone(struct bio *bio)
>>   {
>>       return jiffies_to_nsecs(clone_to_tio(bio)->io->start_time);
>> @@ -3255,6 +3294,7 @@ static const struct block_device_operations 
>> dm_blk_dops = {
>>       .getgeo = dm_blk_getgeo,
>>       .report_zones = dm_blk_report_zones,
>>       .pr_ops = &dm_pr_ops,
>> +    .llseek_hole = &dm_llseek_hole,
> 
> Why &dm_llseek_hole and not just dm_llseek_hole - like the other 
> function assignments?
> 
>>       .owner = THIS_MODULE
>>   };
>> @@ -3264,6 +3304,7 @@ static const struct block_device_operations 
>> dm_rq_blk_dops = {
>>       .ioctl = dm_blk_ioctl,
>>       .getgeo = dm_blk_getgeo,
>>       .pr_ops = &dm_pr_ops,
>> +    .llseek_hole = &dm_llseek_hole,
> 
> ditto
> 
>