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

[Jens Axboe <axboe@kernel.dk>]

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.

-- 
Jens Axboe