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

[Matthew Wilcox <willy@infradead.org>]

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?