On 6/26/20 2:49 PM, David Sterba wrote:
> On Fri, Jun 26, 2020 at 02:06:01PM +0200, Hans van Kranenburg wrote:
>>>> @@ -250,10 +250,19 @@ struct btrfs_ioctl_fs_info_args {
>>>> __u32 nodesize; /* out */
>>>> __u32 sectorsize; /* out */
>>>> __u32 clone_alignment; /* out */
>>>> - __u32 reserved32;
>>>> - __u64 reserved[122]; /* pad to 1k */
>>>> + __u32 flags; /* out */
>>>> + __u16 csum_type; /* out */
>>>> + __u8 reserved[974]; /* pad to 1k */
>>>
>>> I think 32 bits for out flags should be enough (or at least for a long
>>> time).
>>
>> Otherwise just add a flags2 later somewhere further on, and continue in
>> there?. :) Yolo.
>>
>>> I'm not sure if we should make the flags also input.
>>
>> Remember that the only thing that the ioctl code now does is blindly
>> copying the output data over the provided user buf.
>>
>> This is the famous "there's no check if user provided buf was properly
>> zeroed".
>
> Should there such check? Zeroing the ioctl buffers is expected as a
> convention to make future extensions possible.
>
>> Combine this with "what to do if a bit is set to ask for a
>> feature that's not implemented yet?" -> ENOENT? That would mean that any
>> user who's calling the ioctl with a buffer filled with garbage will
>> likely hit it.
>
> Yes, users filling garbage values are either fuzzing or not the using
> the interface correctly, so any sane error is fine.
>
>> So, there you go, BTRFS_IOC_FS_INFO_V2. I don't think it's worth it.
>
> That I don't what to happen of course, but so far I'm not conviced that
> it would be needed. If you have another counter example, other than
> filling with garbage values, please post it.
My reference is commit d24a67b2d997 in which LOGICAL_INO_V2 was created
for this reason. Just allocating a memory buffer without explicitly
zeroing it. Apparently that happens, and it would change the behavior
(getting errors, call getting rejected) without the user changing their
code. Does that fall in the category of 'not using the interface
correctly', or in the category 'you're not a pro coder, but we have
mercy with you'?
>>> Right now I
>>> think not and if we need to pass flags to request potentially expensive
>>> data to retrieve, we'd add another member for that. I don't have a
>>> concrete example and the whole FS_INFO ioctl is supposed to be
>>> lightweight so as it is now looks good to me.
>>
>> As a user of this ioctl, my impression also always has been that there's
>> just a collection of simple interesting values returned, that can't be
>> easily read from other metadata, so mostly stuff from the super block.
>
> Yes that is how I understand it too.
>
>> E.g. a generation field would also be interesting, to see what the last
>> committed transaction is. One could e.g. create a monitoring graph that
>> shows how often they happen.
>
> So the FS_INFO is clearly underused and can be extended with at least:
>
> - generation
> - metadata_uuid
>
> and now that I'm reading the output of dump-super, we maybe should also
> add the 'csum_size' so the user does not need to do the translation
> type -> size.
>
> The compat features or label have their own ioctls, the stats about
> roots are IMHO not that useful and devices have own ioctl. Maybe
> total_bytes and that's probably all.
Hans
