Re: [PATCH v7 06/15] iomap: Return error code from iomap_write_iter()

[Stefan Roesch <shr@fb.com>]

On 6/6/22 1:30 PM, Matthew Wilcox wrote:
> On Mon, Jun 06, 2022 at 12:28:04PM -0700, Stefan Roesch wrote:
>> On 6/6/22 12:25 PM, Matthew Wilcox wrote:
>>> On Mon, Jun 06, 2022 at 12:21:28PM -0700, Stefan Roesch wrote:
>>>> On 6/6/22 12:18 PM, Matthew Wilcox wrote:
>>>>> On Mon, Jun 06, 2022 at 09:39:03AM -0700, Stefan Roesch wrote:
>>>>>> On 6/2/22 5:38 AM, Matthew Wilcox wrote:
>>>>>>> On Wed, Jun 01, 2022 at 02:01:32PM -0700, Stefan Roesch wrote:
>>>>>>>> Change the signature of iomap_write_iter() to return an error code. In
>>>>>>>> case we cannot allocate a page in iomap_write_begin(), we will not retry
>>>>>>>> the memory alloction in iomap_write_begin().
>>>>>>>
>>>>>>> loff_t can already represent an error code.  And it's already used like
>>>>>>> that.
>>>>>>>
>>>>>>>> @@ -829,7 +830,8 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>>>>>>>>  		length -= status;
>>>>>>>>  	} while (iov_iter_count(i) && length);
>>>>>>>>  
>>>>>>>> -	return written ? written : status;
>>>>>>>> +	*processed = written ? written : error;
>>>>>>>> +	return error;
>>>>>>>
>>>>>>> I think the change you really want is:
>>>>>>>
>>>>>>> 	if (status == -EAGAIN)
>>>>>>> 		return -EAGAIN;
>>>>>>> 	if (written)
>>>>>>> 		return written;
>>>>>>> 	return status;
>>>>>>>
>>>>>>
>>>>>> I think the change needs to be:
>>>>>>
>>>>>> -    return written ? written : status;
>>>>>> +    if (status == -EAGAIN) {
>>>>>> +        iov_iter_revert(i, written);
>>>>>> +        return -EAGAIN;
>>>>>> +    }
>>>>>> +    if (written)
>>>>>> +        return written;
>>>>>> +    return status;
>>>>>
>>>>> Ah yes, I think you're right.
>>>>>
>>>>> Does it work to leave everything the way it is, call back into the
>>>>> iomap_write_iter() after having done a short write, get the -EAGAIN at
>>>>> that point and pass the already-advanced iterator to the worker thread?
>>>>> I haven't looked into the details of how that works, so maybe you just
>>>>> can't do that.
>>>>
>>>> With the above change, short writes are handled correctly.
>>>
>>> Are they though?  What about a write that crosses an extent boundary?
>>> iomap_write_iter() gets called once per extent and I have a feeling that
>>> you really need to revert the entire write, rather than just the part
>>> of the write that was to that extent.
>>>
>>> Anyway, my question was whether we need to revert or whether the worker
>>> thread can continue from where we left off.
>>
>> Without iov_iter_revert() fsx fails with errors in short writes and also my test
>> program which issues short writes fails.
> 
> Does your test program include starting in one extent, completing the
> portion of the write which is in that extent successfully, and having
> the portion of the write which is in the second extent be short?

I do a 3k write, where the first 2k write is serviced from one extent and
the last 1k is served from another extent.

Does that answer the question?