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.

fs/ocfs2/aops.c +27 -0

@@ -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;
+		}

Quoted 10 lines · On Mon, 18 Dec 2017 12:06:21 +0000 Changwei Ge <[email protected]> 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 ;)>

Quoted 33 lines

fs/ocfs2/aops.c +27 -0

@@ -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;
+		}

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb *iocb, · line 2453

Quoted 10 lines

fs/ocfs2/aops.c +10 -0

+
+		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...

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +2 -2

 	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)

_

Quoted 8 lines

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +0 -2

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

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb · line 2474

Quoted 4 lines

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +2 -1

+static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
 {
-	int ret = 0;
+	bool ret = false;

I have a different opinion here. If we change return value from int to bool, the error returned by ocfs2_get_clusters cannot be reflected. So I'd prefer the original version.

Quoted 8 lines · On Tue, 19 Dec 2017 09:24:17 +0800 Joseph Qi <[email protected]> wrote:

Quoted 8 lines

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +0 -2

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

Quoted 8 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb · line 2474

Quoted 4 lines

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +2 -1

+static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
 {
-	int ret = 0;
+	bool ret = false;

I have a different opinion here. If we change return value from int to bool, the error returned by ocfs2_get_clusters cannot be reflected. So I'd prefer the original version.

But that error code is not used?

Quoted 19 lines · Andrew Morton

Quoted 8 lines · On Tue, 19 Dec 2017 09:24:17 +0800 Joseph Qi <[email protected]> wrote:

Quoted 8 lines

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +0 -2

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

Quoted 8 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb · line 2474

Quoted 4 lines

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +2 -1

+static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
  {
-	int ret = 0;
+	bool ret = false;

I have a different opinion here. If we change return value from int to bool, the error returned by ocfs2_get_clusters cannot be reflected. So I'd prefer the original version.

Quoted 2 lines

But that error code is not used?

IMO, since ocfs2_get_clusters will get io involved, the caller shouldn't ignore its error.

Something like:
ret = ocfs2_check_range_for_holes();

if (ret < 0) {
	mlog_errno(ret);
	goto out;
}

Then check if append dio feature has been enabled as well as write range and hole.

Thanks,
Joseph

Quoted 13 lines · Andrew Morton

Quoted 10 lines · On Mon, 18 Dec 2017 12:06:21 +0000 Changwei Ge <[email protected]> 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.

Quoted 2 lines

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

Also suggest rearrange the description:)

Quoted 61 lines

Quoted 34 lines

fs/ocfs2/aops.c +27 -0

@@ -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;
+		}

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb *iocb, · line 2453

Quoted 10 lines

fs/ocfs2/aops.c +10 -0

+
+		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...

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb · line 2474

Quoted 12 lines

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +2 -3

@@ -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;

I have a different opinion here. If we change return value from int to bool, the error returned by ocfs2_get_clusters cannot be reflected. So I'd prefer the original version.

Thanks,
Joseph

Quoted 16 lines

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +2 -2

  	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)

_

Quoted 10 lines · On Mon, 18 Dec 2017 12:06:21 +0000 Changwei Ge <[email protected]> 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 ;)

Quoted 34 lines

fs/ocfs2/aops.c +27 -0

@@ -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;
+		}

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb *iocb, · line 2453

Quoted 10 lines

fs/ocfs2/aops.c +10 -0

+
+		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...

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +2 -3

@@ -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;

fs/ocfs2/aops.c renamed from fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix +0 -0

  		if (extent_len > clusters)

_

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 <[email protected]>

---
  fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
  1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +38 -0

index d151632..a982cf6 100644
@@ -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;

fs/ocfs2/aops.c +0 -0

  		return 0;
 
  	if (iov_iter_rw(iter) == READ)

Hi Changwei,

Quoted 10 lines · Changwei Ge

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'?

Quoted 57 lines

Signed-off-by Changwei Ge <[email protected]>

---
   fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
   1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +38 -0

index d151632..a982cf6 100644
@@ -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;
 

fs/ocfs2/aops.c static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) · line 2473

Quoted 7 lines

fs/ocfs2/aops.c +4 -2

   	/* 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

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

   		return 0;
 
   	if (iov_iter_rw(iter) == READ)

Hi Jun,

Quoted 13 lines · piaojun

Hi Changwei,

Quoted 10 lines · Changwei Ge

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.

Quoted 1 line

1. Do you mean that filling hole can't go with dio when append-dio is disabled?

Yes, direct IO will fall back to buffer IO with _append-dio_ disabled.

Quoted 2 lines

2. Is your checking-hole just for 'append-dio' or for 'all-common-dio'?

Just for append-dio

Quoted 57 lines

Quoted 57 lines

Signed-off-by Changwei Ge <[email protected]>

---
   fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
   1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +38 -0

index d151632..a982cf6 100644
@@ -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;
 

Quoted 8 lines

fs/ocfs2/aops.c static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) · line 2473

Quoted 7 lines

fs/ocfs2/aops.c +4 -2

   	/* 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?

Accept this point.

Thanks,
Changwei

Quoted 8 lines

thanks,
Jun

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

   		return 0;
 
   	if (iov_iter_rw(iter) == READ)

.

Hi Changwei,

Quoted 16 lines · Changwei Ge

Hi Jun,

Quoted 13 lines · piaojun

Hi Changwei,

Quoted 10 lines · Changwei Ge

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.

Quoted 3 lines

Quoted 1 line

1. Do you mean that filling hole can't go with dio when append-dio is disabled?

Yes, direct IO will fall back to buffer IO with _append-dio_ disabled.

Why does dio need fall back to buffer-io when append-dio disabled? Could 'common-dio' on file hole go through direct io process? If not, could you please point out the necessity.

Quoted 6 lines

Quoted 3 lines

2. Is your checking-hole just for 'append-dio' or for 'all-common-dio'?

Just for append-dio

If your patch is just for 'append-dio', I wonder that will have impact on 'common-dio'.

thanks,
Jun

Quoted 82 lines

Quoted 57 lines

Quoted 57 lines

Signed-off-by Changwei Ge <[email protected]>

---
    fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
    1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +38 -0

index d151632..a982cf6 100644
@@ -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;
 

Quoted 8 lines

fs/ocfs2/aops.c static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) · line 2473

Quoted 7 lines

fs/ocfs2/aops.c +4 -2

    	/* 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?

Accept this point.

Thanks,
Changwei

Quoted 8 lines

thanks,
Jun

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

    		return 0;
 
    	if (iov_iter_rw(iter) == READ)

.

Quoted 23 lines · piaojun

Hi Changwei,

Quoted 19 lines · Changwei Ge

Hi Jun,

Quoted 13 lines · piaojun

Hi Changwei,

Quoted 10 lines · Changwei Ge

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.

Quoted 1 line

1. Do you mean that filling hole can't go with dio when append-dio is disabled?

Yes, direct IO will fall back to buffer IO with _append-dio_ disabled.

Quoted 3 lines

Why does dio need fall back to buffer-io when append-dio disabled? Could 'common-dio' on file hole go through direct io process? If not, could you please point out the necessity.

Hi Jun,

The intention to make dio fall back to buffer io is to provide *an option* to users, which is more stable and even fast.

From my perspective, current ocfs2 dio implementation especially around space allocation during
doing dio still needs more test and improvements.

Whether to make ocfs2 fall back to buffer io is up to ocfs2 users through toggling append-dio feature. It rather makes ocfs2 configurable and flexible.

Besides, do you still remember John's report about dio crash weeks ago?

I managed to reproduce this issue, so for now, I don't trust dio related code one hundred percents. So if something bad happens to dio writing with space allocation, we can still make ocfs2 fall back to buffer io It's an option not a mandatory action.

Besides, append-dio feature is key to whether to allocate space with dio writing.
So writing to file hole and enlarging file(appending file) should have the same reflection to append-dio feature. :)

Thanks,
Changwei

Quoted 96 lines

Quoted 6 lines

Quoted 3 lines

2. Is your checking-hole just for 'append-dio' or for 'all-common-dio'?

Just for append-dio

If your patch is just for 'append-dio', I wonder that will have impact on 'common-dio'.

thanks,
Jun

Quoted 82 lines

Quoted 57 lines

Quoted 57 lines

Signed-off-by Changwei Ge <[email protected]>

---
    fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
    1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +38 -0

index d151632..a982cf6 100644
@@ -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;
 

Quoted 8 lines

fs/ocfs2/aops.c static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) · line 2473

Quoted 7 lines

fs/ocfs2/aops.c +4 -2

    	/* 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?

Accept this point.

Thanks,
Changwei

Quoted 8 lines

thanks,
Jun

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

    		return 0;
 
    	if (iov_iter_rw(iter) == READ)

.

Hi Changwei,

Quoted 29 lines · Changwei Ge

Quoted 19 lines · piaojun

Hi Changwei,

Quoted 16 lines · Changwei Ge

Hi Jun,

Quoted 13 lines · piaojun

Hi Changwei,

Quoted 10 lines · Changwei Ge

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.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

1. Do you mean that filling hole can't go with dio when append-dio is disabled?

Yes, direct IO will fall back to buffer IO with _append-dio_ disabled.

Why does dio need fall back to buffer-io when append-dio disabled? Could 'common-dio' on file hole go through direct io process? If not, could you please point out the necessity.

Hi Jun,

Quoted 2 lines

The intention to make dio fall back to buffer io is to provide *an option* to users, which is more stable and even fast.

More memory will be consumed for some important user cases. Like if ocfs2 is using to store vm system image file, the file is highly sparse but never extended. If fall back buffer io, more memory will be consumed by page cache in dom0, that will cause less VM running on dom0 and performance issue.

Quoted 8 lines

From my perspective, current ocfs2 dio implementation especially around space allocation during
doing dio still needs more test and improvements.

Whether to make ocfs2 fall back to buffer io is up to ocfs2 users through toggling append-dio feature. It rather makes ocfs2 configurable and flexible.

Besides, do you still remember John's report about dio crash weeks ago?

Looks like this is a workaround, why not fix the bug directly? If with this, people may disable append-dio by default to avoid dio issues. That will make it never stable. But it is a useful feature.

Thanks,
Junxiao.

Quoted 109 lines

I managed to reproduce this issue, so for now, I don't trust dio related code one hundred percents. So if something bad happens to dio writing with space allocation, we can still make ocfs2 fall back to buffer io It's an option not a mandatory action.

Besides, append-dio feature is key to whether to allocate space with dio writing.
So writing to file hole and enlarging file(appending file) should have the same reflection to append-dio feature. :)

Thanks,
Changwei

Quoted 96 lines

Quoted 6 lines

Quoted 3 lines

2. Is your checking-hole just for 'append-dio' or for 'all-common-dio'?

Just for append-dio

If your patch is just for 'append-dio', I wonder that will have impact on 'common-dio'.

thanks,
Jun

Quoted 82 lines

Quoted 57 lines

Quoted 57 lines

Signed-off-by Changwei Ge <[email protected]>

---
     fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
     1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +38 -0

index d151632..a982cf6 100644
@@ -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;
 

Quoted 8 lines

fs/ocfs2/aops.c static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) · line 2473

Quoted 7 lines

fs/ocfs2/aops.c +4 -2

     	/* 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?

Accept this point.

Thanks,
Changwei

Quoted 8 lines

thanks,
Jun

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

     		return 0;
 
     	if (iov_iter_rw(iter) == READ)

.

Hi Junxiao,

Quoted 32 lines · Junxiao Bi

Hi Changwei,

Quoted 29 lines · Changwei Ge

Quoted 19 lines · piaojun

Hi Changwei,

Quoted 16 lines · Changwei Ge

Hi Jun,

Quoted 13 lines · piaojun

Hi Changwei,

Quoted 10 lines · Changwei Ge

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.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

1. Do you mean that filling hole can't go with dio when append-dio is disabled?

Yes, direct IO will fall back to buffer IO with _append-dio_ disabled.

Why does dio need fall back to buffer-io when append-dio disabled? Could 'common-dio' on file hole go through direct io process? If not, could you please point out the necessity.

Hi Jun,

Quoted 7 lines

Quoted 2 lines

The intention to make dio fall back to buffer io is to provide *an option* to users, which is more stable and even fast.

More memory will be consumed for some important user cases. Like if ocfs2 is using to store vm system image file, the file is highly sparse but never extended. If fall back buffer io, more memory will be consumed by page cache in dom0, that will cause less VM running on dom0 and performance issue.

I admit your point above makes scene.
But, AFAIK, major ocfs2 crash issues comes from direct IO part, especially when file is extremely sparse. So for the worst case, virtual machine even can't run due to crash again and again.

I think the most benefit DIO brings to VM is that data can be transferred to LUN as soon as possible, thus no data could be missed.

Quoted 5 lines

Quoted 5 lines

From my perspective, current ocfs2 dio implementation especially around space allocation during
doing dio still needs more test and improvements.

Quoted 7 lines

Quoted 4 lines

Whether to make ocfs2 fall back to buffer io is up to ocfs2 users through toggling append-dio feature. It rather makes ocfs2 configurable and flexible.

Besides, do you still remember John's report about dio crash weeks ago?

Looks like this is a workaround, why not fix the bug directly? If with this, people may disable append-dio by default to avoid dio issues. That will make it never stable. But it is a useful feature.

Arguably, this patch just provides an extra option to users. It's up to ocfs2 users how to use ocfs2 for their business. I think we should not limit ocfs2 users.

Moreover, I agree that direct IO is a useful feature, but it is not mature enough yet.
We have to improve it, however, it needs time. I suppose we still have a short or long journey until that. So before that we could provide a backup way.
This may look like kind of workaround, but I prefer to call it an extra option.

Thanks,
Changwei

Quoted 115 lines

Thanks,
Junxiao.

Quoted 109 lines

I managed to reproduce this issue, so for now, I don't trust dio related code one hundred percents. So if something bad happens to dio writing with space allocation, we can still make ocfs2 fall back to buffer io It's an option not a mandatory action.

Besides, append-dio feature is key to whether to allocate space with dio writing.
So writing to file hole and enlarging file(appending file) should have the same reflection to append-dio feature. :)

Thanks,
Changwei

Quoted 96 lines

Quoted 6 lines

Quoted 3 lines

2. Is your checking-hole just for 'append-dio' or for 'all-common-dio'?

Just for append-dio

If your patch is just for 'append-dio', I wonder that will have impact on 'common-dio'.

thanks,
Jun

Quoted 82 lines

Quoted 57 lines

Quoted 57 lines

Signed-off-by Changwei Ge <[email protected]>

---
     fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
     1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +38 -0

index d151632..a982cf6 100644
@@ -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;
 

Quoted 8 lines

fs/ocfs2/aops.c static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) · line 2473

Quoted 7 lines

fs/ocfs2/aops.c +4 -2

     	/* 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?

Accept this point.

Thanks,
Changwei

Quoted 8 lines

thanks,
Jun

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

     		return 0;
 
     	if (iov_iter_rw(iter) == READ)

.

Quoted 43 lines · Changwei Ge

Hi Junxiao,

Quoted 39 lines · Junxiao Bi

Hi Changwei,

Quoted 24 lines · Changwei Ge

Quoted 23 lines · piaojun

Hi Changwei,

Quoted 19 lines · Changwei Ge

Hi Jun,

Quoted 13 lines · piaojun

Hi Changwei,

Quoted 10 lines · Changwei Ge

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.

Quoted 1 line

1. Do you mean that filling hole can't go with dio when append-dio is disabled?

Yes, direct IO will fall back to buffer IO with _append-dio_ disabled.

Quoted 7 lines

Quoted 3 lines

Why does dio need fall back to buffer-io when append-dio disabled? Could 'common-dio' on file hole go through direct io process? If not, could you please point out the necessity.

Hi Jun,

The intention to make dio fall back to buffer io is to provide *an option* to users, which is more stable and even fast.

More memory will be consumed for some important user cases. Like if ocfs2 is using to store vm system image file, the file is highly sparse but never extended. If fall back buffer io, more memory will be consumed by page cache in dom0, that will cause less VM running on dom0 and performance issue.

Quoted 3 lines

I admit your point above makes scene.
But, AFAIK, major ocfs2 crash issues comes from direct IO part, especially when file is extremely sparse. So for the worst case, virtual machine even can't run due to crash again and again.

Can you please point out where those crash were? I would like take a look at them. I run ocfs2-test for mainline sometimes, and never find a dio crash. As i know, Eric also run the test regularly, he also didn't have a dio crash report.

Quoted 4 lines

I think the most benefit DIO brings to VM is that data can be transferred to LUN as soon as possible, thus no data could be missed.

Right, another benefit.

Thanks,
Junxiao.

Quoted 140 lines

Quoted 5 lines

Quoted 5 lines

From my perspective, current ocfs2 dio implementation especially around space allocation during
doing dio still needs more test and improvements.

Quoted 7 lines

Quoted 4 lines

Whether to make ocfs2 fall back to buffer io is up to ocfs2 users through toggling append-dio feature. It rather makes ocfs2 configurable and flexible.

Besides, do you still remember John's report about dio crash weeks ago?

Looks like this is a workaround, why not fix the bug directly? If with this, people may disable append-dio by default to avoid dio issues. That will make it never stable. But it is a useful feature.

Arguably, this patch just provides an extra option to users. It's up to ocfs2 users how to use ocfs2 for their business. I think we should not limit ocfs2 users.

Moreover, I agree that direct IO is a useful feature, but it is not mature enough yet.
We have to improve it, however, it needs time. I suppose we still have a short or long journey until that. So before that we could provide a backup way.
This may look like kind of workaround, but I prefer to call it an extra option.

Thanks,
Changwei

Quoted 115 lines

Thanks,
Junxiao.

Quoted 109 lines

I managed to reproduce this issue, so for now, I don't trust dio related code one hundred percents. So if something bad happens to dio writing with space allocation, we can still make ocfs2 fall back to buffer io It's an option not a mandatory action.

Besides, append-dio feature is key to whether to allocate space with dio writing.
So writing to file hole and enlarging file(appending file) should have the same reflection to append-dio feature. :)

Thanks,
Changwei

Quoted 96 lines

Quoted 6 lines

Quoted 3 lines

2. Is your checking-hole just for 'append-dio' or for 'all-common-dio'?

Just for append-dio

If your patch is just for 'append-dio', I wonder that will have impact on 'common-dio'.

thanks,
Jun

Quoted 82 lines

Quoted 57 lines

Quoted 57 lines

Signed-off-by Changwei Ge <[email protected]>

---
      fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
      1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +38 -0

index d151632..a982cf6 100644
@@ -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;
 

Quoted 8 lines

fs/ocfs2/aops.c static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) · line 2473

Quoted 7 lines

fs/ocfs2/aops.c +4 -2

      	/* 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?

Accept this point.

Thanks,
Changwei

Quoted 8 lines

thanks,
Jun

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

      		return 0;
 
      	if (iov_iter_rw(iter) == READ)

.

Quoted 44 lines · Junxiao Bi

Quoted 43 lines · Changwei Ge

Hi Junxiao,

Quoted 39 lines · Junxiao Bi

Hi Changwei,

Quoted 24 lines · Changwei Ge

Quoted 23 lines · piaojun

Hi Changwei,

Quoted 19 lines · Changwei Ge

Hi Jun,

Quoted 13 lines · piaojun

Hi Changwei,

Quoted 10 lines · Changwei Ge

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.

Quoted 1 line

1. Do you mean that filling hole can't go with dio when append-dio is disabled?

Yes, direct IO will fall back to buffer IO with _append-dio_ disabled.

Quoted 7 lines

Quoted 3 lines

Why does dio need fall back to buffer-io when append-dio disabled? Could 'common-dio' on file hole go through direct io process? If not, could you please point out the necessity.

Hi Jun,

The intention to make dio fall back to buffer io is to provide *an option* to users, which is more stable and even fast.

More memory will be consumed for some important user cases. Like if ocfs2 is using to store vm system image file, the file is highly sparse but never extended. If fall back buffer io, more memory will be consumed by page cache in dom0, that will cause less VM running on dom0 and performance issue.

Quoted 7 lines

Quoted 3 lines

I admit your point above makes scene.
But, AFAIK, major ocfs2 crash issues comes from direct IO part, especially when file is extremely sparse. So for the worst case, virtual machine even can't run due to crash again and again.

Can you please point out where those crash were? I would like take a look at them. I run ocfs2-test for mainline sometimes, and never find a dio crash. As i know, Eric also run the test regularly, he also didn't have a dio crash report.

Actually, we are running ocfs2-test, too.
I think why it can't detect some issue is that the target file is not sparse enough.

Weeks ago, John report a dio crash issue and provide a reproducer.

I manage to reproduce it easily. Although, Alex has provided a way to fix it, but I think it has something smells.

Also, his patch didn't handle journal related operation well. Moreover, it will bring extra several times of read for a single time of write.

I was planning to improve his patch, but it can't be very elegant. So If I have to,otherwise I want to figure out the root cause for the dio crash issue.

So we are still investigating the root cause why metadata needed is not estimated accurately.

It will be better if you can also take a glance at the dio crash issue John reported.

For now we already have some clues that extents estimated are enough actually but merged and declaimed during marking extents written. We still need more test on it.

Thanks,
Changwei

Quoted 150 lines

Quoted 4 lines

I think the most benefit DIO brings to VM is that data can be transferred to LUN as soon as possible, thus no data could be missed.

Right, another benefit.

Thanks,
Junxiao.

Quoted 140 lines

Quoted 5 lines

Quoted 5 lines

From my perspective, current ocfs2 dio implementation especially around space allocation during
doing dio still needs more test and improvements.

Quoted 7 lines

Quoted 4 lines

Whether to make ocfs2 fall back to buffer io is up to ocfs2 users through toggling append-dio feature. It rather makes ocfs2 configurable and flexible.

Besides, do you still remember John's report about dio crash weeks ago?

Looks like this is a workaround, why not fix the bug directly? If with this, people may disable append-dio by default to avoid dio issues. That will make it never stable. But it is a useful feature.

Arguably, this patch just provides an extra option to users. It's up to ocfs2 users how to use ocfs2 for their business. I think we should not limit ocfs2 users.

Moreover, I agree that direct IO is a useful feature, but it is not mature enough yet.
We have to improve it, however, it needs time. I suppose we still have a short or long journey until that. So before that we could provide a backup way.
This may look like kind of workaround, but I prefer to call it an extra option.

Thanks,
Changwei

Quoted 115 lines

Thanks,
Junxiao.

Quoted 109 lines

I managed to reproduce this issue, so for now, I don't trust dio related code one hundred percents. So if something bad happens to dio writing with space allocation, we can still make ocfs2 fall back to buffer io It's an option not a mandatory action.

Besides, append-dio feature is key to whether to allocate space with dio writing.
So writing to file hole and enlarging file(appending file) should have the same reflection to append-dio feature. :)

Thanks,
Changwei

Quoted 96 lines

Quoted 6 lines

Quoted 3 lines

2. Is your checking-hole just for 'append-dio' or for 'all-common-dio'?

Just for append-dio

If your patch is just for 'append-dio', I wonder that will have impact on 'common-dio'.

thanks,
Jun

Quoted 82 lines

Quoted 57 lines

Quoted 57 lines

Signed-off-by Changwei Ge <[email protected]>

---
      fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
      1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +38 -0

index d151632..a982cf6 100644
@@ -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;
 

Quoted 8 lines

fs/ocfs2/aops.c static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) · line 2473

Quoted 7 lines

fs/ocfs2/aops.c +4 -2

      	/* 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?

Accept this point.

Thanks,
Changwei

Quoted 8 lines

thanks,
Jun

Quoted 4 lines

fs/ocfs2/aops.c +0 -0

      		return 0;
 
      	if (iov_iter_rw(iter) == READ)

.

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

.

Quoted 1 line

Quoted 1 line

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 <[email protected]>

---
  fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
  1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +0 -0

index d151632..a982cf6 100644

fs/ocfs2/aops.c +28 -2

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

Signature · 6 lines

2.7.4

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com 
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Hi Changwei,

Quoted 27 lines

Quoted 1 line

Quoted 1 line

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 <[email protected]>

---
   fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
   1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +0 -0

index d151632..a982cf6 100644
@@ -2414,6 +2414,44 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
   	return ret;
   }
 

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb *iocb, · line 2420

Quoted 4 lines

fs/ocfs2/aops.c +4 -0

+/*
+ * 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?

Quoted 4 lines

fs/ocfs2/aops.c +4 -0

+static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
+				       size_t count)
+{
+	int ret = 0;

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb *iocb, · line 2430

Quoted 6 lines

fs/ocfs2/aops.c +6 -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.

Quoted 49 lines

fs/ocfs2/aops.c +28 -2

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

Signature · 6 lines

2.7.4

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Quoted 4 lines

Quoted 4 lines

Quoted 5 lines · Gang He

Hi Changwei,

Quoted 2 lines

Quoted 1 line

Quoted 1 line

Before ocfs2 supporting allocating clusters while doing append-dio, all

append

Quoted 25 lines

Quoted 25 lines

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 <[email protected]>

---
   fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
   1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +0 -0

index d151632..a982cf6 100644
@@ -2414,6 +2414,44 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
   	return ret;
   }
 

Quoted 5 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb *iocb, · line 2420

Quoted 4 lines

fs/ocfs2/aops.c +4 -0

+/*
+ * 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.

Quoted 1 line

Unwritten extents will not bring any block allocation, my understanding is

right here?

Quoted 1 line

Hi Gang,
For now, I think your comment here makes scene.
Unwritten cluster should not make dio fall back to buffer io. Actually, I copied this function snippet from earlier version of ocfs2 :)

Quoted 11 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb *iocb, · line 2430

Quoted 10 lines

fs/ocfs2/aops.c +10 -0

+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.

Quoted 1 line

Then, if there is not any more block need to be allocated, we should make

the check return true.

I suppose current code in ocfs2_direct_IO already checks this for us.

Thanks,
Changwei

Quoted 51 lines

Quoted 49 lines

fs/ocfs2/aops.c +28 -2

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

Signature · 6 lines

2.7.4

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com 
https://oss.oracle.com/mailman/listinfo/ocfs2-devel 

Quoted 63 lines

Quoted 4 lines

Quoted 4 lines

Quoted 5 lines · Gang He

Hi Changwei,

Quoted 2 lines

Quoted 1 line

Quoted 1 line

Before ocfs2 supporting allocating clusters while doing append-dio, all

append

Quoted 25 lines

Quoted 25 lines

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 <[email protected]>

---
    fs/ocfs2/aops.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
    1 file changed, 42 insertions(+), 2 deletions(-)

fs/ocfs2/aops.c +0 -0

index d151632..a982cf6 100644
@@ -2414,6 +2414,44 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
    	return ret;
    }
 

Quoted 5 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb *iocb, · line 2420

Quoted 4 lines

fs/ocfs2/aops.c +4 -0

+/*
+ * 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.

Quoted 1 line

Unwritten extents will not bring any block allocation, my understanding is

right here?

Quoted 1 line

Hi Gang,
For now, I think your comment here makes scene.
Unwritten cluster should not make dio fall back to buffer io. Actually, I copied this function snippet from earlier version of ocfs2 :)

Quoted 11 lines

fs/ocfs2/aops.c static int ocfs2_dio_end_io(struct kiocb *iocb, · line 2430

Quoted 10 lines

fs/ocfs2/aops.c +10 -0

+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.

Quoted 4 lines

Quoted 1 line

Then, if there is not any more block need to be allocated, we should make

the check return true.

I suppose current code in ocfs2_direct_IO already checks this for us.

Hi Changwei, please take a look again.
My means, if the user try to do di-write a inline-data inode,
the inode has not any cluster, but still can write in inline-data area. like the similar cases, maybe you need to check, make sure all the cases can work well.

Thanks
Gang

Quoted 55 lines

Thanks,
Changwei

Quoted 51 lines

Quoted 49 lines

fs/ocfs2/aops.c +28 -2

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

Signature · 6 lines

2.7.4

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel