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

[Andrey Zhadchenko]

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?

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