[Cluster-devel] Make "gfs2: implement gfs2_block_zero_range using iomap_zero_range" work properly

[Cluster-devel] Make "gfs2: implement gfs2_block_zero_range using iomap_zero_range" work properly

[Andreas Gruenbacher]

Hi Christoph,

these two patches should make your iomap_zero_range cleanup work as
intended.  I've pushed that here:

git://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git for-next.cleanups2

For further testing, I'll rebase "gfs2: use iomap for buffered I/O in
ordered and writeback mode" on top of that.

Thanks,
Andreas

Andreas Gruenbacher (2):
  gfs2: gfs2_iomap_begin cleanup
  gfs2: Add support for IOMAP_ZERO

 fs/gfs2/bmap.c | 64 ++++++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

-- 
2.20.1

[Cluster-devel] [PATCH 1/2] gfs2: gfs2_iomap_begin cleanup

[Andreas Gruenbacher]

Turn function gfs2_iomap_begin_write into something all iomap functions
will go through (and rename it to __gfs2_iomap_begin).  This will
simplify adding support for IOMAP_ZERO.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/bmap.c | 53 +++++++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 3b761a0ba6ab..e2c58c6a1276 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1016,10 +1016,9 @@ static const struct iomap_page_ops gfs2_iomap_page_ops = {
 	.page_done = gfs2_iomap_page_done,
 };
 
-static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
-				  loff_t length, unsigned flags,
-				  struct iomap *iomap,
-				  struct metapath *mp)
+static int __gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
+			      unsigned flags, struct iomap *iomap,
+			      struct metapath *mp)
 {
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
@@ -1027,17 +1026,23 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 	bool unstuff, alloc_required;
 	int ret;
 
-	ret = gfs2_write_lock(inode);
-	if (ret)
+	iomap->flags |= IOMAP_F_BUFFER_HEAD;
+
+	ret = gfs2_iomap_get(inode, pos, length, flags, iomap, mp);
+	if (ret || !(flags & IOMAP_WRITE))
 		return ret;
+	if (flags & IOMAP_DIRECT) {
+		/*
+		 * Silently fall back to buffered I/O for stuffed files or if
+		 * we've got a hole (see gfs2_file_direct_write).
+		 */
+		if (iomap->type != IOMAP_MAPPED)
+			return -ENOTBLK;
+		return 0;
+	}
 
 	unstuff = gfs2_is_stuffed(ip) &&
 		  pos + length > gfs2_max_stuffed_size(ip);
-
-	ret = gfs2_iomap_get(inode, pos, length, flags, iomap, mp);
-	if (ret)
-		goto out_unlock;
-
 	alloc_required = unstuff || iomap->type == IOMAP_HOLE;
 
 	if (alloc_required || gfs2_is_jdata(ip))
@@ -1051,7 +1056,7 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 
 		ret = gfs2_quota_lock_check(ip, &ap);
 		if (ret)
-			goto out_unlock;
+			return ret;
 
 		ret = gfs2_inplace_reserve(ip, &ap);
 		if (ret)
@@ -1118,8 +1123,6 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 out_qunlock:
 	if (alloc_required)
 		gfs2_quota_unlock(ip);
-out_unlock:
-	gfs2_write_unlock(inode);
 	return ret;
 }
 
@@ -1130,23 +1133,19 @@ static int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 	struct metapath mp = { .mp_aheight = 1, };
 	int ret;
 
-	iomap->flags |= IOMAP_F_BUFFER_HEAD;
-
 	trace_gfs2_iomap_start(ip, pos, length, flags);
 	if ((flags & IOMAP_WRITE) && !(flags & IOMAP_DIRECT)) {
-		ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap, &mp);
+		ret = gfs2_write_lock(inode);
+		if (ret)
+			goto out;
+		ret = __gfs2_iomap_begin(inode, pos, length, flags, iomap, &mp);
+		if (ret)
+			gfs2_write_unlock(inode);
 	} else {
-		ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp);
-
-		/*
-		 * Silently fall back to buffered I/O for stuffed files or if
-		 * we've hot a hole (see gfs2_file_direct_write).
-		 */
-		if ((flags & IOMAP_WRITE) && (flags & IOMAP_DIRECT) &&
-		    iomap->type != IOMAP_MAPPED)
-			ret = -ENOTBLK;
+		ret = __gfs2_iomap_begin(inode, pos, length, flags, iomap, &mp);
 	}
 	release_metapath(&mp);
+out:
 	trace_gfs2_iomap_end(ip, iomap, ret);
 	return ret;
 }
@@ -1157,7 +1156,7 @@ static int gfs2_iomap_end(struct inode *inode, loff_t pos, loff_t length,
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 
-	if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) != IOMAP_WRITE)
+	if (!(flags & IOMAP_WRITE) || (flags & IOMAP_DIRECT))
 		goto out;
 
 	if (!gfs2_is_stuffed(ip))
-- 
2.20.1

[Cluster-devel] [PATCH 2/2] gfs2: Add support for IOMAP_ZERO

[Andreas Gruenbacher]

Add support for the IOMAP_ZERO flag, which indicates a request to write
zeroes into a file without filling holes.  Used by iomap_zero_range.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/bmap.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index e2c58c6a1276..aabd78a1ae59 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1029,7 +1029,7 @@ static int __gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 	iomap->flags |= IOMAP_F_BUFFER_HEAD;
 
 	ret = gfs2_iomap_get(inode, pos, length, flags, iomap, mp);
-	if (ret || !(flags & IOMAP_WRITE))
+	if (ret || !(flags & (IOMAP_WRITE | IOMAP_ZERO)))
 		return ret;
 	if (flags & IOMAP_DIRECT) {
 		/*
@@ -1040,6 +1040,10 @@ static int __gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 			return -ENOTBLK;
 		return 0;
 	}
+	if (flags & IOMAP_ZERO) {
+		if (iomap->type == IOMAP_HOLE)
+			return 0;
+	}
 
 	unstuff = gfs2_is_stuffed(ip) &&
 		  pos + length > gfs2_max_stuffed_size(ip);
@@ -1156,8 +1160,12 @@ static int gfs2_iomap_end(struct inode *inode, loff_t pos, loff_t length,
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 
-	if (!(flags & IOMAP_WRITE) || (flags & IOMAP_DIRECT))
+	if (!(flags & (IOMAP_WRITE | IOMAP_ZERO)) || (flags & IOMAP_DIRECT))
 		goto out;
+	if (flags & IOMAP_ZERO) {
+		if (iomap->type == IOMAP_HOLE)
+			goto out;
+	}
 
 	if (!gfs2_is_stuffed(ip))
 		gfs2_ordered_add_inode(ip);
@@ -1181,7 +1189,8 @@ static int gfs2_iomap_end(struct inode *inode, loff_t pos, loff_t length,
 
 	if (ip->i_qadata && ip->i_qadata->qa_qd_num)
 		gfs2_quota_unlock(ip);
-	gfs2_write_unlock(inode);
+	if (flags & IOMAP_WRITE)
+		gfs2_write_unlock(inode);
 
 out:
 	return 0;
-- 
2.20.1

[Cluster-devel] [PATCH 2/2] gfs2: Add support for IOMAP_ZERO

[Christoph Hellwig]

Don't we actually need the write lock for zeroing as well?  Also
I think the alloc helper would be nice to keep, and a little use
of switch might clean things up.  How about something like this
instead?

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index f42718dd292f..8f5a25f507c3 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1027,17 +1027,8 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 	bool unstuff, alloc_required;
 	int ret;
 
-	ret = gfs2_write_lock(inode);
-	if (ret)
-		return ret;
-
 	unstuff = gfs2_is_stuffed(ip) &&
 		  pos + length > gfs2_max_stuffed_size(ip);
-
-	ret = gfs2_iomap_get(inode, pos, length, flags, iomap, mp);
-	if (ret)
-		goto out_unlock;
-
 	alloc_required = unstuff || iomap->type == IOMAP_HOLE;
 
 	if (alloc_required || gfs2_is_jdata(ip))
@@ -1051,7 +1042,7 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 
 		ret = gfs2_quota_lock_check(ip, &ap);
 		if (ret)
-			goto out_unlock;
+			return ret;
 
 		ret = gfs2_inplace_reserve(ip, &ap);
 		if (ret)
@@ -1118,11 +1109,23 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 out_qunlock:
 	if (alloc_required)
 		gfs2_quota_unlock(ip);
-out_unlock:
-	gfs2_write_unlock(inode);
 	return ret;
 }
 
+static inline bool gfs2_iomap_need_write_lock(unsigned flags)
+{
+	switch (flags & (IOMAP_WRITE | IOMAP_ZERO)) {
+	case IOMAP_WRITE:
+		if (flags & IOMAP_DIRECT)
+			return false;
+		return true;
+	case IOMAP_ZERO:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 			    unsigned flags, struct iomap *iomap)
 {
@@ -1133,20 +1136,48 @@ static int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 	iomap->flags |= IOMAP_F_BUFFER_HEAD;
 
 	trace_gfs2_iomap_start(ip, pos, length, flags);
-	if ((flags & IOMAP_WRITE) && !(flags & IOMAP_DIRECT)) {
-		ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap, &mp);
-	} else {
-		ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp);
 
-		/*
-		 * Silently fall back to buffered I/O for stuffed files or if
-		 * we've hot a hole (see gfs2_file_direct_write).
-		 */
-		if ((flags & IOMAP_WRITE) && (flags & IOMAP_DIRECT) &&
-		    iomap->type != IOMAP_MAPPED)
-			ret = -ENOTBLK;
+	if (gfs2_iomap_need_write_lock(flags)) {
+		ret = gfs2_write_lock(inode);
+		if (ret)
+			goto out;
 	}
+
+	ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp);
+	if (ret)
+		goto out_unlock;
+
+	switch (flags & (IOMAP_WRITE | IOMAP_ZERO)) {
+	case 0:
+		goto out_unlock;
+	case IOMAP_WRITE:
+		if (flags & IOMAP_DIRECT) {
+			/*
+			 * Silently fall back to buffered I/O for stuffed files
+			 * or if we've got a hole (see gfs2_file_direct_write).
+			 */
+			if (iomap->type != IOMAP_MAPPED)
+				ret = -ENOTBLK;
+			goto out_unlock;
+		}
+		break;
+	case IOMAP_ZERO:
+		if (iomap->type == IOMAP_HOLE)
+			goto out_unlock;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		ret = -EIO;
+		goto out_unlock;
+	}
+
+	ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap, &mp);
+
+out_unlock:
+	if (ret && gfs2_iomap_need_write_lock(flags))
+		gfs2_write_unlock(inode);
 	release_metapath(&mp);
+out:
 	trace_gfs2_iomap_end(ip, iomap, ret);
 	return ret;
 }
@@ -1157,8 +1188,21 @@ static int gfs2_iomap_end(struct inode *inode, loff_t pos, loff_t length,
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 
-	if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) != IOMAP_WRITE)
-		goto out;
+	switch (flags & (IOMAP_WRITE | IOMAP_ZERO)) {
+	case 0:
+		return 0;
+	case IOMAP_WRITE:
+		if (flags & IOMAP_DIRECT)
+			return 0;
+		break;
+	case IOMAP_ZERO:
+		if (iomap->type == IOMAP_HOLE)
+			return 0;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		return -EIO;
+	}
 
 	if (!gfs2_is_stuffed(ip))
 		gfs2_ordered_add_inode(ip);
@@ -1183,8 +1227,6 @@ static int gfs2_iomap_end(struct inode *inode, loff_t pos, loff_t length,
 	if (ip->i_qadata && ip->i_qadata->qa_qd_num)
 		gfs2_quota_unlock(ip);
 	gfs2_write_unlock(inode);
-
-out:
 	return 0;
 }
 

[Cluster-devel] [PATCH 2/2] gfs2: Add support for IOMAP_ZERO

[Andreas Gruenbacher]

On Mon, 8 Jul 2019 at 19:06, Christoph Hellwig <hch@lst.de> wrote:
> Don't we actually need the write lock for zeroing as well?

Hmm. For iomap_zero_range, we don't need the
iov_iter_fault_in_readable / iov_iter_copy_from_user_atomic logic from
iomap_write_actor, so we take the write lock outside of
iomap_zero_range. I just now noticed that xfs behaves differently
there; that's an ugly difference.

>  Also
> I think the alloc helper would be nice to keep, and a little use
> of switch might clean things up.  How about something like this
> instead?
>
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index f42718dd292f..8f5a25f507c3 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -1027,17 +1027,8 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
>         bool unstuff, alloc_required;
>         int ret;
>
> -       ret = gfs2_write_lock(inode);
> -       if (ret)
> -               return ret;
> -
>         unstuff = gfs2_is_stuffed(ip) &&
>                   pos + length > gfs2_max_stuffed_size(ip);
> -
> -       ret = gfs2_iomap_get(inode, pos, length, flags, iomap, mp);
> -       if (ret)
> -               goto out_unlock;
> -
>         alloc_required = unstuff || iomap->type == IOMAP_HOLE;
>
>         if (alloc_required || gfs2_is_jdata(ip))
> @@ -1051,7 +1042,7 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
>
>                 ret = gfs2_quota_lock_check(ip, &ap);
>                 if (ret)
> -                       goto out_unlock;
> +                       return ret;
>
>                 ret = gfs2_inplace_reserve(ip, &ap);
>                 if (ret)
> @@ -1118,11 +1109,23 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
>  out_qunlock:
>         if (alloc_required)
>                 gfs2_quota_unlock(ip);
> -out_unlock:
> -       gfs2_write_unlock(inode);
>         return ret;
>  }
>
> +static inline bool gfs2_iomap_need_write_lock(unsigned flags)
> +{
> +       switch (flags & (IOMAP_WRITE | IOMAP_ZERO)) {
> +       case IOMAP_WRITE:
> +               if (flags & IOMAP_DIRECT)
> +                       return false;
> +               return true;
> +       case IOMAP_ZERO:
> +               return true;
> +       default:
> +               return false;
> +       }
> +}
> +
>  static int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
>                             unsigned flags, struct iomap *iomap)
>  {
> @@ -1133,20 +1136,48 @@ static int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
>         iomap->flags |= IOMAP_F_BUFFER_HEAD;
>
>         trace_gfs2_iomap_start(ip, pos, length, flags);
> -       if ((flags & IOMAP_WRITE) && !(flags & IOMAP_DIRECT)) {
> -               ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap, &mp);
> -       } else {
> -               ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp);
>
> -               /*
> -                * Silently fall back to buffered I/O for stuffed files or if
> -                * we've hot a hole (see gfs2_file_direct_write).
> -                */
> -               if ((flags & IOMAP_WRITE) && (flags & IOMAP_DIRECT) &&
> -                   iomap->type != IOMAP_MAPPED)
> -                       ret = -ENOTBLK;
> +       if (gfs2_iomap_need_write_lock(flags)) {
> +               ret = gfs2_write_lock(inode);
> +               if (ret)
> +                       goto out;
>         }
> +
> +       ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp);
> +       if (ret)
> +               goto out_unlock;
> +
> +       switch (flags & (IOMAP_WRITE | IOMAP_ZERO)) {
> +       case 0:
> +               goto out_unlock;
> +       case IOMAP_WRITE:
> +               if (flags & IOMAP_DIRECT) {
> +                       /*
> +                        * Silently fall back to buffered I/O for stuffed files
> +                        * or if we've got a hole (see gfs2_file_direct_write).
> +                        */
> +                       if (iomap->type != IOMAP_MAPPED)
> +                               ret = -ENOTBLK;
> +                       goto out_unlock;
> +               }
> +               break;
> +       case IOMAP_ZERO:
> +               if (iomap->type == IOMAP_HOLE)
> +                       goto out_unlock;
> +               break;
> +       default:
> +               WARN_ON_ONCE(1);
> +               ret = -EIO;
> +               goto out_unlock;
> +       }
> +
> +       ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap, &mp);
> +
> +out_unlock:
> +       if (ret && gfs2_iomap_need_write_lock(flags))
> +               gfs2_write_unlock(inode);
>         release_metapath(&mp);
> +out:
>         trace_gfs2_iomap_end(ip, iomap, ret);
>         return ret;
>  }
> @@ -1157,8 +1188,21 @@ static int gfs2_iomap_end(struct inode *inode, loff_t pos, loff_t length,
>         struct gfs2_inode *ip = GFS2_I(inode);
>         struct gfs2_sbd *sdp = GFS2_SB(inode);
>
> -       if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) != IOMAP_WRITE)
> -               goto out;
> +       switch (flags & (IOMAP_WRITE | IOMAP_ZERO)) {
> +       case 0:
> +               return 0;
> +       case IOMAP_WRITE:
> +               if (flags & IOMAP_DIRECT)
> +                       return 0;
> +               break;
> +       case IOMAP_ZERO:
> +               if (iomap->type == IOMAP_HOLE)
> +                       return 0;
> +               break;
> +       default:
> +               WARN_ON_ONCE(1);
> +               return -EIO;
> +       }
>
>         if (!gfs2_is_stuffed(ip))
>                 gfs2_ordered_add_inode(ip);
> @@ -1183,8 +1227,6 @@ static int gfs2_iomap_end(struct inode *inode, loff_t pos, loff_t length,
>         if (ip->i_qadata && ip->i_qadata->qa_qd_num)
>                 gfs2_quota_unlock(ip);
>         gfs2_write_unlock(inode);
> -
> -out:
>         return 0;
>  }
>
>