[Ocfs2-devel] [PATCH] ocfs2: fall back to buffer IO when append dio is disabled with file hole existing

[Andrew Morton]

On Mon, 18 Dec 2017 12:06:21 +0000 Changwei Ge <ge.changwei at h3c.com> wrote:

> Before ocfs2 supporting allocating clusters while doing append-dio, all append
> dio will fall back to buffer io to allocate clusters firstly. Also, when it
> steps on a file hole, it will fall back to buffer io, too. But for current
> code, writing to file hole will leverage dio to allocate clusters. This is not
> right, since whether append-io is enabled tells the capability whether ocfs2 can
> allocate space while doing dio.
> So introduce file hole check function back into ocfs2.
> Once ocfs2 is doing dio upon a file hole with append-dio disabled, it will fall
> back to buffer IO to allocate clusters.
> 

hm, that's a bit hard to understand.  Hopefully reviewers will know
what's going on ;)

> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -2414,6 +2414,44 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
>   	return ret;
>   }
>   
> +/*
> + * Will look for holes and unwritten extents in the range starting at
> + * pos for count bytes (inclusive).
> + */
> +static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
> +				       size_t count)
> +{
> +	int ret = 0;
> +	unsigned int extent_flags;
> +	u32 cpos, clusters, extent_len, phys_cpos;
> +	struct super_block *sb = inode->i_sb;
> +
> +	cpos = pos >> OCFS2_SB(sb)->s_clustersize_bits;
> +	clusters = ocfs2_clusters_for_bytes(sb, pos + count) - cpos;
> +
> +	while (clusters) {
> +		ret = ocfs2_get_clusters(inode, cpos, &phys_cpos, &extent_len,
> +					 &extent_flags);
> +		if (ret < 0) {
> +			mlog_errno(ret);
> +			goto out;
> +		}
> +
> +		if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) {
> +			ret = 1;
> +			break;
> +		}
> +
> +		if (extent_len > clusters)
> +			extent_len = clusters;
> +
> +		clusters -= extent_len;
> +		cpos += extent_len;
> +	}
> +out:
> +	return ret;
> +}

A few thoughts:

- a function which does "check_foo" isn't well named.  Because the
  reader cannot determine whether the return value means "foo is true"
  or "foo is false".

  So a better name for this function is ocfs2_range_has_holes(), so
  the reader immediately understand what its return value *means".

  Also a bool return value is more logical.

- Mixing "goto out" with "break" as above is a bit odd.

So...


--- a/fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix
+++ a/fs/ocfs2/aops.c
@@ -2469,10 +2469,9 @@ static int ocfs2_dio_end_io(struct kiocb
  * Will look for holes and unwritten extents in the range starting at
  * pos for count bytes (inclusive).
  */
-static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
-				       size_t count)
+static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
 {
-	int ret = 0;
+	bool ret = false;
 	unsigned int extent_flags;
 	u32 cpos, clusters, extent_len, phys_cpos;
 	struct super_block *sb = inode->i_sb;
@@ -2489,8 +2488,8 @@ static int ocfs2_check_range_for_holes(s
 		}
 
 		if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) {
-			ret = 1;
-			break;
+			ret = true;
+			goto out;
 		}
 
 		if (extent_len > clusters)
_