[Devel] [PATCH VZ9 3/3] block/blk-cbt: add BLKCBTLIST ioctl
Andrey Zhadchenko
andrey.zhadchenko at virtuozzo.com
Fri Jan 24 12:15:28 MSK 2025
On 1/23/25 12:24, Pavel Tikhomirov wrote:
>
>
> 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?
Currently there are no hard cap in VHI for backup bitmaps.
So between zero and 65536 (hard cap for QEMU).
Initial discussion gets us somewhere about 8 bitmaps.
>
>>
>>>
>>>>
>>>>>
>>>>>> + __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
>>>>>
>>>
>
More information about the Devel
mailing list