Re: [PATCH 6/9] f2fs: implement iomap operations

[Eric Biggers <ebiggers@kernel.org>]

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