On 2/6/2024 16:49, Jens Axboe wrote:
> 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.
> 

Here's the patch against your branch.