On Wed, Mar 02, 2022 at 02:03:28PM +0100, Andreas Gruenbacher wrote: > On Wed, Mar 2, 2022 at 11:17 AM Filipe Manana <fdmanana@xxxxxxxxxx> wrote: > > On Tue, Mar 01, 2022 at 09:38:30AM +1100, Dave Chinner wrote: > > > On Mon, Feb 28, 2022 at 02:32:03PM +0000, fdmanana@xxxxxxxxxx wrote: > > > > From: Filipe Manana <fdmanana@xxxxxxxx> > > > ..... > > > > > > > 11) At iomap_dio_complete() we adjust the iocb->ki_pos from X to X + 4K > > > > and return 4K (the amount of io done) to iomap_dio_complete_work(); > > > > > > > > 12) iomap_dio_complete_work() calls the iocb completion callback, > > > > iocb->ki_complete() with a second argument value of 4K (total io > > > > done) and the iocb with the adjust ki_pos of X + 4K. This results > > > > in completing the read request for io_uring, leaving it with a > > > > result of 4K bytes read, and only the first page of the buffer > > > > filled in, while the remaining 3 pages, corresponding to the other > > > > 3 extents, were not filled; > > > > > > > > 13) For the application, the result is unexpected because if we ask > > > > to read N bytes, it expects to get N bytes read as long as those > > > > N bytes don't cross the EOF (i_size). > > > > > > Yeah, that's exactly the sort of problem we were having with XFS > > > with partial DIO completions due to needing multiple iomap iteration > > > loops to complete a single IO. Hence IOMAP_NOWAIT now triggers the > > > above range check and aborts before we start... > > > > Interesting. > > Dave, this seems to affect all users of iomap_dio_rw in the same way, > so would it make sense to move this check there? Perhaps, but I'm not sure it makes sense because filesystems need to abort ->iomap_begin with -EAGAIN in IOMAP_NOWAIT contexts before they make any changes. Hence detecting short extents in the generic code becomes ... difficult because we might now need to undo changes that have been made in ->iomap_begin. e.g. if the filesystem allocates space and the iomap core says "not long enough" because IOMAP_NOWAIT is set, then we may have to punch out that allocation in ->iomap_end beforei returning -EAGAIN. That means filesystems like XFS may now need to supply a ->iomap_end function to undo stuff the core decides it shouldn't have done, instead of the filesystem ensuring it never does the operation it in the first place... IOWs, the correct behaviour here is for the filesystem ->iomap_begin method to see that it needs to allocate and return -EAGAIN if IOMAP_NOWAIT is set, not do the allocation and hope it that it ends up being long enough to cover the entire IO we have to do. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
