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

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.

Signed-off-by: Changwei Ge <ge.changwei at h3c.com>
---
  fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
  1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index d151632..a982cf6 100644
--- 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;
+}
+
  static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
  {
  	struct file *file = iocb->ki_filp;
@@ -2429,8 +2467,10 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct
iov_iter *iter)
  		return 0;
  
  	/* Fallback to buffered I/O if we do not support append dio. */
-	if (iocb->ki_pos + iter->count > i_size_read(inode) &&
-	    !ocfs2_supports_append_dio(osb))
+	if (!ocfs2_supports_append_dio(osb) &&
+			(iocb->ki_pos + iter->count > i_size_read(inode) ||
+			 ocfs2_check_range_for_holes(inode, iocb->ki_pos,
+						     iter->count)))
  		return 0;
  
  	if (iov_iter_rw(iter) == READ)
-- 
2.7.4

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)
_

Hi Changwei,

On 2017/12/18 20:06, Changwei Ge 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.
> 
1. Do you mean that filling hole can't go with dio when append-dio is
disabled?
2. Is your checking-hole just for 'append-dio' or for
'all-common-dio'?
> Signed-off-by: Changwei Ge <ge.changwei at h3c.com>
> ---
>   fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index d151632..a982cf6 100644
> --- 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;
> +}
> +
>   static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>   {
>   	struct file *file = iocb->ki_filp;
> @@ -2429,8 +2467,10 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb,
struct iov_iter *iter)
>   		return 0;
>   
>   	/* Fallback to buffered I/O if we do not support append dio. */
> -	if (iocb->ki_pos + iter->count > i_size_read(inode) &&
> -	    !ocfs2_supports_append_dio(osb))
> +	if (!ocfs2_supports_append_dio(osb) &&
> +			(iocb->ki_pos + iter->count > i_size_read(inode) ||
> +			 ocfs2_check_range_for_holes(inode, iocb->ki_pos,
> +						     iter->count)))
we should check error here, right?

thanks,
Jun
>   		return 0;
>   
>   	if (iov_iter_rw(iter) == READ)
>

Hi Changwei,

>>> 
> 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.
> 
> Signed-off-by: Changwei Ge <ge.changwei at h3c.com>
> ---
>   fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index d151632..a982cf6 100644
> --- 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).
> + */
Why do we also need to look for unwritten extents here? unwritten extents means,
the clusters have been allocated, but have not been written yet.
Unwritten extents will not bring any block allocation, my understanding is right
here?
> +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;
Please consider data-inline case, if in data-inline case, we maybe need not to
allocate more cluster.
Then, if there is not any more block need to be allocated, we should make the
check return true.
> +
> +	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;
> +}
> +
>   static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>   {
>   	struct file *file = iocb->ki_filp;
> @@ -2429,8 +2467,10 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, 
> struct iov_iter *iter)
>   		return 0;
>   
>   	/* Fallback to buffered I/O if we do not support append dio. */
> -	if (iocb->ki_pos + iter->count > i_size_read(inode) &&
> -	    !ocfs2_supports_append_dio(osb))
> +	if (!ocfs2_supports_append_dio(osb) &&
> +			(iocb->ki_pos + iter->count > i_size_read(inode) ||
> +			 ocfs2_check_range_for_holes(inode, iocb->ki_pos,
> +						     iter->count)))
>   		return 0;
>   
>   	if (iov_iter_rw(iter) == READ)
> -- 
> 2.7.4
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com 
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel