[Devel] [PATCH VZ9 3/3] block/blk-cbt: add BLKCBTLIST ioctl

[Andrey Zhadchenko]

On 1/23/25 09:15, Pavel Tikhomirov wrote:
> 
> 
> On 1/20/25 16:34, Andrey Zhadchenko wrote:
>> so user can query the list of active bitmaps.
>>
>> https://virtuozzo.atlassian.net/browse/VSTOR-96269
>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko at virtuozzo.com>
>> ---
>>   block/blk-cbt.c         | 38 ++++++++++++++++++++++++++++++++++++++
>>   block/ioctl.c           |  1 +
>>   include/uapi/linux/fs.h |  9 +++++++++
>>   3 files changed, 48 insertions(+)
>>
>> diff --git a/block/blk-cbt.c b/block/blk-cbt.c
>> index 8b0d07ee757c..af242633acc9 100644
>> --- a/block/blk-cbt.c
>> +++ b/block/blk-cbt.c
>> @@ -979,6 +979,42 @@ static int cbt_ioc_set(struct block_device *bdev, 
>> struct blk_user_cbt_info __use
>>       return ret;
>>   }
>> +static int cbt_ioc_list(struct block_device *bdev, void __user *arg)
>> +{
>> +    void __user *name_ptr = (void *)((size_t)arg + sizeof(struct 
>> blk_user_cbt_list));
>> +    struct request_queue *q = bdev_get_queue(bdev);
>> +    struct blk_user_cbt_list lst;
>> +    __u32 total = 0, copied = 0;
>> +    struct cbt_info *cbt;
>> +
>> +    if (copy_from_user(&lst, arg, sizeof(lst)))
>> +        return -EFAULT;
>> +
>> +    if (!access_ok(name_ptr,
>> +               lst.count * CBT_NAME_LENGTH))
>> +        return -EFAULT;
>> +
>> +    rcu_read_lock();
>> +    list_for_each_entry_rcu(cbt, &q->cbt_list, list) {
>> +        if (copied < lst.count) {
>> +            if (copy_to_user(name_ptr, cbt->name, CBT_NAME_LENGTH))
>> +                return -EFAULT;
>> +            copied++;
>> +            name_ptr = (void *)((size_t)name_ptr + CBT_NAME_LENGTH);
>> +        }
>> +        total++;
>> +    }
>> +    rcu_read_unlock();
>> +
>> +    lst.count = copied;
>> +    lst.total_count = total;
>> +
>> +    if (copy_to_user(arg, &lst, sizeof(lst)))
>> +        return -EFAULT;
>> +
>> +    return 0;
>> +}
>> +
>>   static int cbt_ioc_misc(struct block_device *bdev, void __user *arg)
>>   {
>>       struct request_queue *q = bdev_get_queue(bdev);
>> @@ -1024,6 +1060,8 @@ int blk_cbt_ioctl(struct block_device *bdev, 
>> unsigned cmd, char __user *arg)
>>           return cbt_ioc_set(bdev, ucbt_ioc, 0);
>>       case BLKCBTMISC:
>>           return cbt_ioc_misc(bdev, arg);
>> +    case BLKCBTLIST:
>> +        return cbt_ioc_list(bdev, arg);
>>       default:
>>           BUG();
>>       }
>> diff --git a/block/ioctl.c b/block/ioctl.c
>> index b2d3dbe5bef5..6e76d55790ea 100644
>> --- a/block/ioctl.c
>> +++ b/block/ioctl.c
>> @@ -569,6 +569,7 @@ static int blkdev_common_ioctl(struct block_device 
>> *bdev, blk_mode_t mode,
>>       case BLKCBTSET:
>>       case BLKCBTCLR:
>>       case BLKCBTMISC:
>> +    case BLKCBTLIST:
>>           return blk_cbt_ioctl(bdev, cmd, (char __user *)arg);
>>       default:
>>           return -ENOIOCTLCMD;
>> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
>> index 667157fe864a..d4641a8a70a7 100644
>> --- a/include/uapi/linux/fs.h
>> +++ b/include/uapi/linux/fs.h
>> @@ -169,12 +169,21 @@ struct blk_user_cbt_snap_create {
>>       __u64 size;
>>   };
>> +struct blk_user_cbt_list {
>> +    __u32 count;        /* Amount of name entries in names
>> +                 * in - available, out - filled
>> +                 */
>> +    __u32 total_count;    /* Total bitmap count of this device (out) */
> 
> Why do we need to know how many cbts were filled? It would always be 
> min(available, total). So it seems we can use just one "count" variable 
> with the same effect.

The only important case is when the amount of cbts is bigger then the 
provided buffer. e.g. someone added a cbt in-between calls or someone 
supplied static size buffer with the hopes that it will be enough

> 
> Also it might be a good idea to consider actual name lengths, so that 
> buffer size can be decreased, to save (CBT_NAME_LENGTH - strlen(name) - 
> 1) memory for each name (e.g. separating names with spaces).

Well user does not know the size of names beforehand anyway, so I think 
this is excess complexity.

> 
> Another idea is to make this ioctl iterative, remembering position, so 
> that we can read the list one by one using relatively small buffer.

I think userspace memory if rather "free"
If we make it iterative, we will have to solve a few problems like
  - Holding cbt pointer so we can get next cbt (what if it someone 
deletes previous)
  - When to reset
  - How to distinguish users

> 
>> +    __u8 names[0];        /* Array of CBT_NAME_LENGTH long names */
>> +};
>> +
>>   #define BLKCBTSTART _IOR(0x12,200, struct blk_user_cbt_info)
>>   #define BLKCBTSTOP _IOR(0x12, 201, struct blk_user_cbt_info)
>>   #define BLKCBTGET _IOWR(0x12,202,struct blk_user_cbt_info)
>>   #define BLKCBTSET _IOR(0x12,203,struct blk_user_cbt_info)
>>   #define BLKCBTCLR _IOR(0x12,204,struct blk_user_cbt_info)
>>   #define BLKCBTMISC _IOWR(0x12,205,struct blk_user_cbt_misc_info)
>> +#define BLKCBTLIST _IOWR(0x12, 206, struct blk_user_cbt_list)
>>   /*
>>    * Flags for the fsx_xflags field
>