Re: [PATCH 1/6] iomap: Convert wait_for_completion to flags

[Dave Chinner <david@fromorbit.com>]

On Mon, Jun 22, 2020 at 10:24:52AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Convert wait_for_completion boolean to flags so we can pass more flags
> to iomap_dio_rw()
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/ext4/file.c        | 11 +++++++++--
>  fs/gfs2/file.c        |  7 ++++---
>  fs/iomap/direct-io.c  |  3 ++-
>  fs/xfs/xfs_file.c     | 10 ++++++----
>  fs/zonefs/super.c     |  8 ++++++--
>  include/linux/iomap.h |  9 ++++++++-
>  6 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 2a01e31a032c..d20120c4d833 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -53,6 +53,7 @@ static ssize_t ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  {
>  	ssize_t ret;
>  	struct inode *inode = file_inode(iocb->ki_filp);
> +	int flags = 0;
>  
>  	if (iocb->ki_flags & IOCB_NOWAIT) {
>  		if (!inode_trylock_shared(inode))
> @@ -74,8 +75,11 @@ static ssize_t ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  		return generic_file_read_iter(iocb, to);
>  	}
>  
> +	if (is_sync_kiocb(iocb))
> +		flags |= IOMAP_DIOF_WAIT_FOR_COMPLETION;

The name of the flag conflates implementation with intent. "wait for
completion" is the implementation, "synchronous IO" is the intent.

Can you name this <namespace>_SYNCIO, please? Read further below for
comments on the flag namespace issues...

>  		ext4_journal_stop(handle);
>  	}
>  
> +	if (is_sync_kiocb(iocb) || unaligned_io || extend)
> +		flags |= IOMAP_DIOF_WAIT_FOR_COMPLETION;

Then stuff like this is self documenting:

	if (any of this is true)
		IO needs to be issued synchronously

> @@ -767,6 +767,7 @@ static ssize_t gfs2_file_direct_read(struct kiocb *iocb, struct iov_iter *to)
>  	size_t count = iov_iter_count(to);
>  	struct gfs2_holder gh;
>  	ssize_t ret;
> +	int flags = is_sync_kiocb(iocb) ? IOMAP_DIOF_WAIT_FOR_COMPLETION : 0;
>  
>  	if (!count)
>  		return 0; /* skip atime */
> @@ -777,7 +778,7 @@ static ssize_t gfs2_file_direct_read(struct kiocb *iocb, struct iov_iter *to)
>  		goto out_uninit;
>  
>  	ret = iomap_dio_rw(iocb, to, &gfs2_iomap_ops, NULL,
> -			   is_sync_kiocb(iocb));
> +			   flags);

Why do we need a new flags variable here, but not for other
conversions that are identical? 

Hmmm - you use 3 different methods of calculating flags to pass
to iomap_dio_rw() in this patchset. Can you pick one method and use
it for all the code? e.g. make all the code look like this:

	int	flags = 0;


	....
	if (is_sync_kiocb(iocb)
		flags |= IOMAP_DIOF_SYNCIO;
	ret = iomap_dio_rw(....., flags);
	....

So the setting of the flags is right next to the iomap_dio_rw()
call and we don't have to go searching for them?


> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index ec7b78e6feca..7ed857196a39 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -405,7 +405,7 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
>  ssize_t
>  iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  		const struct iomap_ops *ops, const struct iomap_dio_ops *dops,
> -		bool wait_for_completion)
> +		int dio_flags)
>  {
>  	struct address_space *mapping = iocb->ki_filp->f_mapping;
>  	struct inode *inode = file_inode(iocb->ki_filp);
> @@ -415,6 +415,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  	unsigned int flags = IOMAP_DIRECT;
>  	struct blk_plug plug;
>  	struct iomap_dio *dio;
> +	bool wait_for_completion = !!(dio_flags & IOMAP_DIOF_WAIT_FOR_COMPLETION);

1. the compiler will squash (x & y) down to a boolean state
correctly without needing to add double negatives.

2. I don't like variable names shadowing core kernel API functions
(i.e. wait_for_completion()). Especially as this has nothign to do
with the completion API...

> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 4d1d3c3469e9..f6230446b08d 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -255,9 +255,16 @@ struct iomap_dio_ops {
>  			struct bio *bio, loff_t file_offset);
>  };
>  
> +/*
> + * Flags to pass iomap_dio_rw()
> + */
> +
> +/* Wait for completion of DIO */
> +#define IOMAP_DIOF_WAIT_FOR_COMPLETION 		0x1

Hmmm. Namespace issues. We already have a IOMAP_DIO_* flags defined
for passing to ->end_io. It's going to be confusing having a set of
flags with almost exactly the namespace but with an "F" for flags
and no indication which iomap operation the flags actually belong to.

This is simples, though:

#define IOMAP_DIO_RWF_SYNCIO		(1 << 0)

And it might also be worthwhile renaming the ->endio flags to:

#define IOMAP_DIO_ENDIO_UNWRITTEN	(1 << 0)
#define IOMAP_DIO_ENDIO_COW		(1 << 1)

So there's no confusion there either.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com