> On Oct 26, 2021, at 2:15 PM, Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> We always go through i_size_read(), and we rarely end up needing it. Push
> the read to down where we need to check it, which avoids it for most
> cases.
>
> It looks like we can even remove this check entirely, which might be
> worth pursuing. But at least this takes it out of the hot path.
>
> Acked-by: Chris Mason <clm@xxxxxx>
> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
>
> ---
>
> I came across this and wrote the patch the other day, then Pavel pointed
> me at his original posting of a very similar patch back in August.
> Discussed it with Chris, and it sure _seems_ like this would be fine.
>
> In an attempt to move the original discussion forward, here's this
> posting.
>
I had the same concerns Dave Chinner did, but I think the i_size check inside generic_file_read_iter() is dead code at this point. Checking ki_pos against i_size was added for Btrfs:
commit 66f998f611897319b555364cefd5d6e88a205866
Author: Josef Bacik <josef@xxxxxxxxxx>
Date: Sun May 23 11:00:54 2010 -0400
fs: allow short direct-io reads to be completed via buffered IO
And we’ve switched to btrfs_file_read_iter(), which does the check the same way PavelJens have done it here.
I don’t think checking i_size before or after O_DIRECT makes the race fundamentally different. We might return a short read at different times than we did before, but we won’t be returning stale/incorrect data.
-chris
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 44b4b551e430..850920276846 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2736,9 +2736,7 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> struct file *file = iocb->ki_filp;
> struct address_space *mapping = file->f_mapping;
> struct inode *inode = mapping->host;
> - loff_t size;
>
> - size = i_size_read(inode);
> if (iocb->ki_flags & IOCB_NOWAIT) {
> if (filemap_range_needs_writeback(mapping, iocb->ki_pos,
> iocb->ki_pos + count - 1))
> @@ -2770,8 +2768,9 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> * the rest of the read. Buffered reads will not work for
> * DAX files, so don't bother trying.
> */
> - if (retval < 0 || !count || iocb->ki_pos >= size ||
> - IS_DAX(inode))
> + if (retval < 0 || !count || IS_DAX(inode))
> + return retval;
> + if (iocb->ki_pos >= i_size_read(inode))
> return retval;
> }
>
