On Fri, Jul 23, 2021 at 06:00:03AM +0100, Christoph Hellwig wrote:
> On Thu, Jul 22, 2021 at 06:52:33PM -0700, Eric Biggers wrote:
> > I am trying to do this, but unfortunately I don't see a way to make it work
> > correctly in all cases.
> >
> > The main problem is that when iomap_dio_rw() returns an error (other than
> > -EIOCBQUEUED), there is no way to know whether ->end_io() has been called or
> > not. This is because iomap_dio_rw() can fail either early, before "starting"
> > the I/O (in which case ->end_io() won't have been called), or later, after
> > "starting" the I/O (in which case ->end_io() will have been called). Note that
> > this can't be worked around by checking whether the iov_iter has been advanced
> > or not, since a failure could occur between "starting" the I/O and the iov_iter
> > being advanced for the first time.
> >
> > Would you be receptive to adding a ->begin_io() callback to struct iomap_dio_ops
> > in order to allow filesystems to maintain counters like this?
>
> I think we can triviall fix this by using the slightly lower level
> __iomap_dio_rw API. Incremental patch to my previous one below:
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 4fed90cc1462..11844bd0cb7a 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -4243,6 +4243,7 @@ static ssize_t f2fs_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
> struct f2fs_inode_info *fi = F2FS_I(inode);
> const loff_t pos = iocb->ki_pos;
> const size_t count = iov_iter_count(to);
> + struct iomap_dio *dio;
> ssize_t ret;
>
> if (count == 0)
> @@ -4260,8 +4261,13 @@ static ssize_t f2fs_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
> }
>
> inc_page_count(F2FS_I_SB(inode), F2FS_DIO_READ);
> - ret = iomap_dio_rw(iocb, to, &f2fs_iomap_ops, &f2fs_iomap_dio_ops, 0);
> -
> + dio = __iomap_dio_rw(iocb, to, &f2fs_iomap_ops, &f2fs_iomap_dio_ops, 0);
> + if (IS_ERR_OR_NULL(dio)) {
> + dec_page_count(F2FS_I_SB(inode), F2FS_DIO_READ);
> + ret = PTR_ERR_OR_ZERO(dio);
> + } else {
> + ret = iomap_dio_complete(dio);
> + }
> up_read(&fi->i_gc_rwsem[READ]);
>
> file_accessed(file);
> @@ -4271,8 +4277,6 @@ static ssize_t f2fs_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
> else if (ret == -EIOCBQUEUED)
> f2fs_update_iostat(F2FS_I_SB(inode), APP_DIRECT_READ_IO,
> count - iov_iter_count(to));
> - else
> - dec_page_count(F2FS_I_SB(inode), F2FS_DIO_READ);
> out:
> trace_f2fs_direct_IO_exit(inode, pos, count, READ, ret);
> return ret;
I wouldn't call it trivial, but yes that seems to work (after fixing it to
handle EIOCBQUEUED correctly). Take a look at the v2 I've sent out. Thanks!
- Eric
