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

[Pavel Tikhomirov]

On 1/23/25 18:47, Andrey Zhadchenko wrote:
> 
> 
> On 1/23/25 10:53, Pavel Tikhomirov wrote:
>>
>>
>> On 1/23/25 17:21, Andrey Zhadchenko wrote:
>>>
>>>
>>> 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
>>
>> Still, my point is that you don't need two variables to handle it.
> 
> Why not? If you want to list every bitmap and someone does create a new 
> one between first and second ioctls, you won't know.
> But I do not check it after second ioctl anyway, so probably we should 
> reduce it to a single field
> 
>>
>>>
>>>>
>>>> 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.
>>
>> First user calls with zero buffer, gets the size, allocates buffer of 
>> the size and does real call (same as in your example in other thread, 
>> but names are put in a more compact manner). But yes, I don't insist, 
>> probably for now simpler is better and if we face size problems in 
>> future we can compact it (though we might need to put version to the 
>> structure to have smooth switch to different format).
>>
>>>
>>>>
>>>> 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
>>
>> yes, going in this direction is hard
>>
>> Thus I think restricting maximum number of cbts is the best option.
> 
> So, do you want to have some inner hard cap, or should it be a new IOCTL 
> to set/get bitmap maximum count?

I thought of hard cap, how many cbts do we expect?

> 
>>
>>>
>>>>
>>>>> +    __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
>>>>
>>

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.