Re: [PATCH 3/3] iomap: use filemap_range_needs_writeback() for O_DIRECT reads

[Jens Axboe <axboe@kernel.dk>]

On 2/9/21 12:51 AM, Christoph Hellwig wrote:
> On Mon, Feb 08, 2021 at 07:30:08PM -0700, Jens Axboe wrote:
>> +		if (iocb->ki_flags & IOCB_NOWAIT) {
>> +			if (filemap_range_needs_writeback(mapping, pos, end)) {
>> +				ret = -EAGAIN;
>> +				goto out_free_dio;
>> +			}
>> +			flags |= IOMAP_NOWAIT;
>> +		}
>>  		if (iter_is_iovec(iter))
>>  			dio->flags |= IOMAP_DIO_DIRTY;
>>  	} else {
>> +		if (iocb->ki_flags & IOCB_NOWAIT) {
>> +			if (filemap_range_has_page(mapping, pos, end)) {
>> +				ret = -EAGAIN;
>> +				goto out_free_dio;
>> +			}
>> +			flags |= IOMAP_NOWAIT;
>> +		}
>> +
>>  		flags |= IOMAP_WRITE;
>>  		dio->flags |= IOMAP_DIO_WRITE;
>>  
>> @@ -478,14 +493,6 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>>  			dio->flags |= IOMAP_DIO_WRITE_FUA;
>>  	}
>>  
>> -	if (iocb->ki_flags & IOCB_NOWAIT) {
>> -		if (filemap_range_has_page(mapping, pos, end)) {
>> -			ret = -EAGAIN;
>> -			goto out_free_dio;
>> -		}
>> -		flags |= IOMAP_NOWAIT;
>> -	}
> 
> looking at this I really hate the scheme with the potential racyness
> and duplicated page looksups.

Me too

> Why can't we pass a nonblock flag to filemap_write_and_wait_range
> and invalidate_inode_pages2_range that makes them return -EAGAIN
> when they would block to clean this whole mess up?

We could, but that's a _lot_ of surgery. I'd rather live with the
slight race for now instead of teaching writepages, page laundering,
etc about IOCB_NOWAIT.

I do think that's a worthy long term goal, but we dio read situation
is bad enough that it warrants a quicker fix.

-- 
Jens Axboe