Re: [PATCH v2] fs: replace the ki_complete two integer arguments with a single argument

[Jens Axboe <axboe@kernel.dk>]

On 10/22/21 9:29 AM, Jens Axboe wrote:
> On 10/22/21 8:19 AM, Jens Axboe wrote:
>> On 10/21/21 3:03 PM, Jeff Moyer wrote:
>>> Jeff Moyer <jmoyer@redhat.com> writes:
>>>
>>>> Jens Axboe <axboe@kernel.dk> writes:
>>>>
>>>>> On 10/21/21 12:05 PM, Jeff Moyer wrote:
>>>>>>
>>>>>>>> I'll follow up if there are issues.
>>>>>>
>>>>>> s390 (big endian, 64 bit) is failing libaio test 21:
>>>>>>
>>>>>> # harness/cases/21.p
>>>>>> Expected -EAGAIN, got 4294967285
>>>>>>
>>>>>> If I print out both res and res2 using %lx, you'll see what happened:
>>>>>>
>>>>>> Expected -EAGAIN, got fffffff5,ffffffff
>>>>>>
>>>>>> The sign extension is being split up.
>>>>>
>>>>> Funky, does it work if you apply this on top?
>>>>>
>>>>> diff --git a/fs/aio.c b/fs/aio.c
>>>>> index 3674abc43788..c56437908339 100644
>>>>> --- a/fs/aio.c
>>>>> +++ b/fs/aio.c
>>>>> @@ -1442,8 +1442,8 @@ static void aio_complete_rw(struct kiocb *kiocb, u64 res)
>>>>>  	 * 32-bits of value at most for either value, bundle these up and
>>>>>  	 * pass them in one u64 value.
>>>>>  	 */
>>>>> -	iocb->ki_res.res = lower_32_bits(res);
>>>>> -	iocb->ki_res.res2 = upper_32_bits(res);
>>>>> +	iocb->ki_res.res = (long) (res & 0xffffffff);
>>>>> +	iocb->ki_res.res2 = (long) (res >> 32);
>>>>>  	iocb_put(iocb);
>>>>>  }
>>>>
>>>> I think you'll also need to clamp any ki_complete() call sites to 32
>>>> bits (cast to int, or what have you).  Otherwise that sign extension
>>>> will spill over into res2.
>>>>
>>>> fwiw, I tested with this:
>>>>
>>>> 	iocb->ki_res.res = (long)(int)lower_32_bits(res);
>>>> 	iocb->ki_res.res2 = (long)(int)upper_32_bits(res);
>>>>
>>>> Coupled with the call site changes, that made things work for me.
>>>
>>> This is all starting to feel like a minefield.  If you don't have any
>>> concrete numbers to show that there is a speedup, I think we should
>>> shelf this change.
>>
>> It's really not a minefield at all, we just need a proper help to encode
>> the value. I'm out until Tuesday, but I'll sort it out when I get back.
>> Can also provide some numbers on this.
> 
> I think this incremental should fix it, also providing a helper to
> properly pack these. The more I look at the gadget stuff the more I also
> get the feeling that it really is wonky and nobody uses res2, which
> would be a nice cleanup to continue. But I think it should be separate.

For the record, deferring all of this until next week when I'm back.
I'll pick it back up then, obviously this isn't an urgent thing at all,
would just love to sort out the useless argument going down the line.
We'd need to pack all of them, not just the odd ones out.

-- 
Jens Axboe