diff mbox series

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

Message ID 20210209023008.76263-4-axboe@kernel.dk (mailing list archive)
State New
Headers show
Series Improve IOCB_NOWAIT O_DIRECT reads | expand

Commit Message

Jens Axboe Feb. 9, 2021, 2:30 a.m. UTC
For reads, use the better variant of checking for the need to call
filemap_write_and_wait_range() when doing O_DIRECT. This avoids falling
back to the slow path for IOCB_NOWAIT, if there are no pages to wait for
(or write out).

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/iomap/direct-io.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

Comments

Christoph Hellwig Feb. 9, 2021, 7:51 a.m. UTC | #1
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.

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?
Jens Axboe Feb. 9, 2021, 2:29 p.m. UTC | #2
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.
diff mbox series

Patch

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 933f234d5bec..5b111d1635ab 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -458,9 +458,24 @@  __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		if (pos >= dio->i_size)
 			goto out_free_dio;
 
+		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;
-	}
-
 	ret = filemap_write_and_wait_range(mapping, pos, end);
 	if (ret)
 		goto out_free_dio;