* [Cluster-devel] Make "gfs2: implement gfs2_block_zero_range using iomap_zero_range" work properly @ 2019-07-05 23:56 Andreas Gruenbacher 2019-07-05 23:56 ` [Cluster-devel] [PATCH 1/2] gfs2: gfs2_iomap_begin cleanup Andreas Gruenbacher 2019-07-05 23:56 ` [Cluster-devel] [PATCH 2/2] gfs2: Add support for IOMAP_ZERO Andreas Gruenbacher 0 siblings, 2 replies; 5+ messages in thread From: Andreas Gruenbacher @ 2019-07-05 23:56 UTC (permalink / raw) To: cluster-devel.redhat.com 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Cluster-devel] [PATCH 1/2] gfs2: gfs2_iomap_begin cleanup 2019-07-05 23:56 [Cluster-devel] Make "gfs2: implement gfs2_block_zero_range using iomap_zero_range" work properly Andreas Gruenbacher @ 2019-07-05 23:56 ` Andreas Gruenbacher 2019-07-05 23:56 ` [Cluster-devel] [PATCH 2/2] gfs2: Add support for IOMAP_ZERO Andreas Gruenbacher 1 sibling, 0 replies; 5+ messages in thread From: Andreas Gruenbacher @ 2019-07-05 23:56 UTC (permalink / raw) To: cluster-devel.redhat.com 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Cluster-devel] [PATCH 2/2] gfs2: Add support for IOMAP_ZERO 2019-07-05 23:56 [Cluster-devel] Make "gfs2: implement gfs2_block_zero_range using iomap_zero_range" work properly Andreas Gruenbacher 2019-07-05 23:56 ` [Cluster-devel] [PATCH 1/2] gfs2: gfs2_iomap_begin cleanup Andreas Gruenbacher @ 2019-07-05 23:56 ` Andreas Gruenbacher 2019-07-08 17:06 ` Christoph Hellwig 1 sibling, 1 reply; 5+ messages in thread From: Andreas Gruenbacher @ 2019-07-05 23:56 UTC (permalink / raw) To: cluster-devel.redhat.com 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Cluster-devel] [PATCH 2/2] gfs2: Add support for IOMAP_ZERO 2019-07-05 23:56 ` [Cluster-devel] [PATCH 2/2] gfs2: Add support for IOMAP_ZERO Andreas Gruenbacher @ 2019-07-08 17:06 ` Christoph Hellwig 2019-07-08 18:07 ` Andreas Gruenbacher 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2019-07-08 17:06 UTC (permalink / raw) To: cluster-devel.redhat.com 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; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Cluster-devel] [PATCH 2/2] gfs2: Add support for IOMAP_ZERO 2019-07-08 17:06 ` Christoph Hellwig @ 2019-07-08 18:07 ` Andreas Gruenbacher 0 siblings, 0 replies; 5+ messages in thread From: Andreas Gruenbacher @ 2019-07-08 18:07 UTC (permalink / raw) To: cluster-devel.redhat.com 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; > } > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-08 18:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-07-05 23:56 [Cluster-devel] Make "gfs2: implement gfs2_block_zero_range using iomap_zero_range" work properly Andreas Gruenbacher 2019-07-05 23:56 ` [Cluster-devel] [PATCH 1/2] gfs2: gfs2_iomap_begin cleanup Andreas Gruenbacher 2019-07-05 23:56 ` [Cluster-devel] [PATCH 2/2] gfs2: Add support for IOMAP_ZERO Andreas Gruenbacher 2019-07-08 17:06 ` Christoph Hellwig 2019-07-08 18:07 ` Andreas Gruenbacher
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).