Re: [PATCH RFC] iomap: only return IO error if no data has been transferred

From: Jens Axboe <axboe@kernel.dk>
To: Dave Chinner <david@fromorbit.com>
Cc: Christoph Hellwig <hch@lst.de>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH RFC] iomap: only return IO error if no data has been transferred
Date: Wed, 18 Nov 2020 08:26:50 -0700	[thread overview]
Message-ID: <9ef0f890-f115-41f3-15fc-28f21810379f@kernel.dk> (raw)
In-Reply-To: <20201118071941.GN7391@dread.disaster.area>

On 11/18/20 12:19 AM, Dave Chinner wrote:
> On Tue, Nov 17, 2020 at 03:17:18PM -0700, Jens Axboe wrote:
>> If we've successfully transferred some data in __iomap_dio_rw(),
>> don't mark an error for a latter segment in the dio.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> Debugging an issue with io_uring, which uses IOCB_NOWAIT for the
>> IO. If we do parts of an IO, then once that completes, we still
>> return -EAGAIN if we ran into a problem later on. That seems wrong,
>> normal convention would be to return the short IO instead. For the
>> -EAGAIN case, io_uring will retry later parts without IOCB_NOWAIT
>> and complete it successfully.
> 
> So you are getting a write IO that is split across an allocated
> extent and a hole, and the second mapping is returning EAGAIN
> because allocation would be required? This sort of split extent IO
> is fairly common, so I'm not sure that splitting them into two
> separate IOs may not be the best approach.

The case I seem to be hitting is this one:

if (iocb->ki_flags & IOCB_NOWAIT) {
	if (filemap_range_has_page(mapping, pos, end)) {
                  ret = -EAGAIN;
                  goto out_free_dio;
	}
	flags |= IOMAP_NOWAIT;
}

in __iomap_dio_rw(), which isn't something we can detect upfront like IO
over a multiple extents...

> I'd kinda like to avoid have NOWAIT IO return different results to a
> non-NOWAIT IO with exactly the same setup contexts i.e. either we
> get -EAGAIN or the IO completes as a whole just like a non-NOWAIT IO
> would.
> 
> So perhaps it would be better to fix the IOMAP_NOWAIT handling in XFS
> to return EAGAIN if the mapping found doesn't span the entire range
> of the IO. That way we avoid the potential "partial NOWAIT"
> behaviour for IOs that span extent boundaries....
> 
> Thoughts?

I don't think it's unreasonable to expect NOWAIT to return short IO,
there are various cases that can lead to that (we've identified two
already). There's just no way to always know upfront if we'd need to
block to satisfy a given range, and returning -EAGAIN in general when IO
has been done is misleading imho.

-- 
Jens Axboe