Re: madvise/fadvise 32-bit length

[Jens Axboe <axboe@kernel.dk>]

On 6/2/24 2:58 AM, Stefan wrote:
> On 1/6/2024 20:33, Jens Axboe wrote:
>> On 6/1/24 9:51 AM, Stefan wrote:
>>> On 1/6/2024 17:35, Jens Axboe wrote:
>>>> On 6/1/24 9:22 AM, Stefan wrote:
>>>>> On 1/6/2024 17:05, Jens Axboe wrote:
>>>>>> On 6/1/24 8:19 AM, Jens Axboe wrote:
>>>>>>> On 6/1/24 3:43 AM, Stefan wrote:
>>>>>>>> io_uring uses the __u32 len field in order to pass the length to
>>>>>>>> madvise and fadvise, but these calls use an off_t, which is 64bit on
>>>>>>>> 64bit platforms.
>>>>>>>>
>>>>>>>> When using liburing, the length is silently truncated to 32bits (so
>>>>>>>> 8GB length would become zero, which has a different meaning of "until
>>>>>>>> the end of the file" for fadvise).
>>>>>>>>
>>>>>>>> If my understanding is correct, we could fix this by introducing new
>>>>>>>> operations MADVISE64 and FADVISE64, which use the addr3 field instead
>>>>>>>> of the length field for length.
>>>>>>>
>>>>>>> We probably just want to introduce a flag and ensure that older stable
>>>>>>> kernels check it, and then use a 64-bit field for it when the flag is
>>>>>>> set.
>>>>>>
>>>>>> I think this should do it on the kernel side, as we already check these
>>>>>> fields and return -EINVAL as needed. Should also be trivial to backport.
>>>>>> Totally untested... Might want a FEAT flag for this, or something where
>>>>>> it's detectable, to make the liburing change straight forward.
>>>>>>
>>>>>>
>>>>>> diff --git a/io_uring/advise.c b/io_uring/advise.c
>>>>>> index 7085804c513c..cb7b881665e5 100644
>>>>>> --- a/io_uring/advise.c
>>>>>> +++ b/io_uring/advise.c
>>>>>> @@ -17,14 +17,14 @@
>>>>>>     struct io_fadvise {
>>>>>>         struct file            *file;
>>>>>>         u64                offset;
>>>>>> -    u32                len;
>>>>>> +    u64                len;
>>>>>>         u32                advice;
>>>>>>     };
>>>>>>       struct io_madvise {
>>>>>>         struct file            *file;
>>>>>>         u64                addr;
>>>>>> -    u32                len;
>>>>>> +    u64                len;
>>>>>>         u32                advice;
>>>>>>     };
>>>>>>     @@ -33,11 +33,13 @@ int io_madvise_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>>>>     #if defined(CONFIG_ADVISE_SYSCALLS) && defined(CONFIG_MMU)
>>>>>>         struct io_madvise *ma = io_kiocb_to_cmd(req, struct io_madvise);
>>>>>>     -    if (sqe->buf_index || sqe->off || sqe->splice_fd_in)
>>>>>> +    if (sqe->buf_index || sqe->splice_fd_in)
>>>>>>             return -EINVAL;
>>>>>>           ma->addr = READ_ONCE(sqe->addr);
>>>>>> -    ma->len = READ_ONCE(sqe->len);
>>>>>> +    ma->len = READ_ONCE(sqe->off);
>>>>>> +    if (!ma->len)
>>>>>> +        ma->len = READ_ONCE(sqe->len);
>>>>>>         ma->advice = READ_ONCE(sqe->fadvise_advice);
>>>>>>         req->flags |= REQ_F_FORCE_ASYNC;
>>>>>>         return 0;
>>>>>> @@ -78,11 +80,13 @@ int io_fadvise_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>>>>     {
>>>>>>         struct io_fadvise *fa = io_kiocb_to_cmd(req, struct io_fadvise);
>>>>>>     -    if (sqe->buf_index || sqe->addr || sqe->splice_fd_in)
>>>>>> +    if (sqe->buf_index || sqe->splice_fd_in)
>>>>>>             return -EINVAL;
>>>>>>           fa->offset = READ_ONCE(sqe->off);
>>>>>> -    fa->len = READ_ONCE(sqe->len);
>>>>>> +    fa->len = READ_ONCE(sqe->addr);
>>>>>> +    if (!fa->len)
>>>>>> +        fa->len = READ_ONCE(sqe->len);
>>>>>>         fa->advice = READ_ONCE(sqe->fadvise_advice);
>>>>>>         if (io_fadvise_force_async(fa))
>>>>>>             req->flags |= REQ_F_FORCE_ASYNC;
>>>>>>
>>>>>
>>>>>
>>>>> If we want to have the length in the same field in both *ADVISE
>>>>> operations, we can put a flag in splice_fd_in/optlen.
>>>>
>>>> I don't think that part matters that much.
>>>>
>>>>> Maybe the explicit flag is a bit clearer for users of the API
>>>>> compared to the implicit flag when setting sqe->len to zero?
>>>>
>>>> We could go either way. The unused fields returning -EINVAL if set right
>>>> now can serve as the flag field - if you have it set, then that is your
>>>> length. If not, then the old style is the length. That's the approach I
>>>> took, rather than add an explicit flag to it. Existing users that would
>>>> set the 64-bit length fields would get -EINVAL already. And since the
>>>> normal flags field is already used for advice flags, I'd prefer just
>>>> using the existing 64-bit zero fields for it rather than add a flag in
>>>> an odd location. Would also make for an easier backport to stable.
>>>>
>>>> But don't feel that strongly about that part.
>>>>
>>>> Attached kernel patch with FEAT added, and liburing patch with 64
>>>> versions added.
>>>>
>>>
>>> Sounds good!
>>> Do we want to do anything about the current (32-bit) functions in
>>> liburing? They silently truncate the user's values, so either marking
>>> them deprecated or changing the type of length in the arguments to a
>>> __u32 could help.
>>
>> I like changing it to an __u32, and then we'll add a note to the man
>> page for them as well (with references to the 64-bit variants).
>>
>> I still need to write a test and actually test the patches, but I'll get
>> to that Monday. If you want to write a test case that checks the 64-bit
>> range, then please do!
>>
> 
> Maybe something like the following for madvise?
> Create an 8GB file initialized with 0xaa, punch a (8GB - page_size)
> hole using MADV_REMOVE, and check the contents. It requires support
> for FALLOC_FL_PUNCH_HOLE in the filesystem.

I think that looks very reasonable, and it's better than the DONTNEED
and timings, it was always a pretty shitty test. We just need to ensure
that we return T_EXIT_SKIP if the fs it's being run on doesn't support
punching holes.

FWIW, I did put the liburing changes in an 'advise' branch, so you could
generate a patch against that. Once we're happy with it, it can get
pulled into master.

-- 
Jens Axboe