All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/2] iomap: Add a page_prepare callback
@ 2019-04-25 16:09 ` Andreas Gruenbacher
  0 siblings, 0 replies; 14+ messages in thread
From: Andreas Gruenbacher @ 2019-04-25 16:09 UTC (permalink / raw)
  To: cluster-devel, Christoph Hellwig
  Cc: Bob Peterson, Jan Kara, Dave Chinner, Ross Lagerwall, Mark Syms,
	Edwin Török, linux-fsdevel, linux-mm,
	Andreas Gruenbacher

Move the page_done callback into a separate iomap_page_ops structure and
add a page_prepare calback to be called before a page is written to.  In
gfs2, we'll want to start a transaction in page_prepare and end it in
page_done, and other filesystems that implement data journaling will
require the same kind of mechanism.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/iomap.c            | 22 ++++++++++++++++++----
 include/linux/iomap.h | 18 +++++++++++++-----
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 97cb9d486a7d..667a822ecb7d 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -665,6 +665,7 @@ static int
 iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
 		struct page **pagep, struct iomap *iomap)
 {
+	const struct iomap_page_ops *page_ops = iomap->page_ops;
 	pgoff_t index = pos >> PAGE_SHIFT;
 	struct page *page;
 	int status = 0;
@@ -674,9 +675,17 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
 	if (fatal_signal_pending(current))
 		return -EINTR;
 
+	if (page_ops) {
+		status = page_ops->page_prepare(inode, pos, len, iomap);
+		if (status)
+			return status;
+	}
+
 	page = grab_cache_page_write_begin(inode->i_mapping, index, flags);
-	if (!page)
-		return -ENOMEM;
+	if (!page) {
+		status = -ENOMEM;
+		goto no_page;
+	}
 
 	if (iomap->type == IOMAP_INLINE)
 		iomap_read_inline_data(inode, page, iomap);
@@ -684,12 +693,16 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
 		status = __block_write_begin_int(page, pos, len, NULL, iomap);
 	else
 		status = __iomap_write_begin(inode, pos, len, page, iomap);
+
 	if (unlikely(status)) {
 		unlock_page(page);
 		put_page(page);
 		page = NULL;
 
 		iomap_write_failed(inode, pos, len);
+no_page:
+		if (page_ops)
+			page_ops->page_done(inode, pos, 0, NULL, iomap);
 	}
 
 	*pagep = page;
@@ -769,6 +782,7 @@ static int
 iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
 		unsigned copied, struct page *page, struct iomap *iomap)
 {
+	const struct iomap_page_ops *page_ops = iomap->page_ops;
 	int ret;
 
 	if (iomap->type == IOMAP_INLINE) {
@@ -780,8 +794,8 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
 		ret = __iomap_write_end(inode, pos, len, copied, page, iomap);
 	}
 
-	if (iomap->page_done)
-		iomap->page_done(inode, pos, copied, page, iomap);
+	if (page_ops)
+		page_ops->page_done(inode, pos, copied, page, iomap);
 
 	if (ret < len)
 		iomap_write_failed(inode, pos, len);
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 0fefb5455bda..fd65f27d300e 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -53,6 +53,8 @@ struct vm_fault;
  */
 #define IOMAP_NULL_ADDR -1ULL	/* addr is not valid */
 
+struct iomap_page_ops;
+
 struct iomap {
 	u64			addr; /* disk offset of mapping, bytes */
 	loff_t			offset;	/* file offset of mapping, bytes */
@@ -63,12 +65,18 @@ struct iomap {
 	struct dax_device	*dax_dev; /* dax_dev for dax operations */
 	void			*inline_data;
 	void			*private; /* filesystem private */
+	const struct iomap_page_ops *page_ops;
+};
 
-	/*
-	 * Called when finished processing a page in the mapping returned in
-	 * this iomap.  At least for now this is only supported in the buffered
-	 * write path.
-	 */
+/*
+ * Called before / after processing a page in the mapping returned in this
+ * iomap.  At least for now, this is only supported in the buffered write path.
+ * When page_prepare returns 0, page_done is called as well
+ * (possibly with page == NULL).
+ */
+struct iomap_page_ops {
+	int (*page_prepare)(struct inode *inode, loff_t pos, unsigned len,
+			struct iomap *iomap);
 	void (*page_done)(struct inode *inode, loff_t pos, unsigned copied,
 			struct page *page, struct iomap *iomap);
 };
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Cluster-devel] [PATCH v3 1/2] iomap: Add a page_prepare callback
@ 2019-04-25 16:09 ` Andreas Gruenbacher
  0 siblings, 0 replies; 14+ messages in thread
From: Andreas Gruenbacher @ 2019-04-25 16:09 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Move the page_done callback into a separate iomap_page_ops structure and
add a page_prepare calback to be called before a page is written to.  In
gfs2, we'll want to start a transaction in page_prepare and end it in
page_done, and other filesystems that implement data journaling will
require the same kind of mechanism.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/iomap.c            | 22 ++++++++++++++++++----
 include/linux/iomap.h | 18 +++++++++++++-----
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 97cb9d486a7d..667a822ecb7d 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -665,6 +665,7 @@ static int
 iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
 		struct page **pagep, struct iomap *iomap)
 {
+	const struct iomap_page_ops *page_ops = iomap->page_ops;
 	pgoff_t index = pos >> PAGE_SHIFT;
 	struct page *page;
 	int status = 0;
@@ -674,9 +675,17 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
 	if (fatal_signal_pending(current))
 		return -EINTR;
 
+	if (page_ops) {
+		status = page_ops->page_prepare(inode, pos, len, iomap);
+		if (status)
+			return status;
+	}
+
 	page = grab_cache_page_write_begin(inode->i_mapping, index, flags);
-	if (!page)
-		return -ENOMEM;
+	if (!page) {
+		status = -ENOMEM;
+		goto no_page;
+	}
 
 	if (iomap->type == IOMAP_INLINE)
 		iomap_read_inline_data(inode, page, iomap);
@@ -684,12 +693,16 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
 		status = __block_write_begin_int(page, pos, len, NULL, iomap);
 	else
 		status = __iomap_write_begin(inode, pos, len, page, iomap);
+
 	if (unlikely(status)) {
 		unlock_page(page);
 		put_page(page);
 		page = NULL;
 
 		iomap_write_failed(inode, pos, len);
+no_page:
+		if (page_ops)
+			page_ops->page_done(inode, pos, 0, NULL, iomap);
 	}
 
 	*pagep = page;
@@ -769,6 +782,7 @@ static int
 iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
 		unsigned copied, struct page *page, struct iomap *iomap)
 {
+	const struct iomap_page_ops *page_ops = iomap->page_ops;
 	int ret;
 
 	if (iomap->type == IOMAP_INLINE) {
@@ -780,8 +794,8 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
 		ret = __iomap_write_end(inode, pos, len, copied, page, iomap);
 	}
 
-	if (iomap->page_done)
-		iomap->page_done(inode, pos, copied, page, iomap);
+	if (page_ops)
+		page_ops->page_done(inode, pos, copied, page, iomap);
 
 	if (ret < len)
 		iomap_write_failed(inode, pos, len);
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 0fefb5455bda..fd65f27d300e 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -53,6 +53,8 @@ struct vm_fault;
  */
 #define IOMAP_NULL_ADDR -1ULL	/* addr is not valid */
 
+struct iomap_page_ops;
+
 struct iomap {
 	u64			addr; /* disk offset of mapping, bytes */
 	loff_t			offset;	/* file offset of mapping, bytes */
@@ -63,12 +65,18 @@ struct iomap {
 	struct dax_device	*dax_dev; /* dax_dev for dax operations */
 	void			*inline_data;
 	void			*private; /* filesystem private */
+	const struct iomap_page_ops *page_ops;
+};
 
-	/*
-	 * Called when finished processing a page in the mapping returned in
-	 * this iomap.  At least for now this is only supported in the buffered
-	 * write path.
-	 */
+/*
+ * Called before / after processing a page in the mapping returned in this
+ * iomap.  At least for now, this is only supported in the buffered write path.
+ * When page_prepare returns 0, page_done is called as well
+ * (possibly with page == NULL).
+ */
+struct iomap_page_ops {
+	int (*page_prepare)(struct inode *inode, loff_t pos, unsigned len,
+			struct iomap *iomap);
 	void (*page_done)(struct inode *inode, loff_t pos, unsigned copied,
 			struct page *page, struct iomap *iomap);
 };
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 2/2] gfs2: Fix iomap write page reclaim deadlock
  2019-04-25 16:09 ` [Cluster-devel] " Andreas Gruenbacher
@ 2019-04-25 16:09   ` Andreas Gruenbacher
  -1 siblings, 0 replies; 14+ messages in thread
From: Andreas Gruenbacher @ 2019-04-25 16:09 UTC (permalink / raw)
  To: cluster-devel, Christoph Hellwig
  Cc: Bob Peterson, Jan Kara, Dave Chinner, Ross Lagerwall, Mark Syms,
	Edwin Török, linux-fsdevel, linux-mm,
	Andreas Gruenbacher

Since commit 64bc06bb32ee ("gfs2: iomap buffered write support"), gfs2 is doing
buffered writes by starting a transaction in iomap_begin, writing a range of
pages, and ending that transaction in iomap_end.  This approach suffers from
two problems:

  (1) Any allocations necessary for the write are done in iomap_begin, so when
  the data aren't journaled, there is no need for keeping the transaction open
  until iomap_end.

  (2) Transactions keep the gfs2 log flush lock held.  When
  iomap_file_buffered_write calls balance_dirty_pages, this can end up calling
  gfs2_write_inode, which will try to flush the log.  This requires taking the
  log flush lock which is already held, resulting in a deadlock.

Fix both of these issues by not keeping transactions open from iomap_begin to
iomap_end.  Instead, start a small transaction in page_prepare and end it in
page_done when necessary.

Reported-by: Edwin Török <edvin.torok@citrix.com>
Fixes: 64bc06bb32ee ("gfs2: iomap buffered write support")
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/aops.c |  14 +++++--
 fs/gfs2/bmap.c | 101 ++++++++++++++++++++++++++++---------------------
 2 files changed, 67 insertions(+), 48 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 05dd78f4b2b3..6210d4429d84 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -649,7 +649,7 @@ static int gfs2_readpages(struct file *file, struct address_space *mapping,
  */
 void adjust_fs_space(struct inode *inode)
 {
-	struct gfs2_sbd *sdp = inode->i_sb->s_fs_info;
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	struct gfs2_inode *m_ip = GFS2_I(sdp->sd_statfs_inode);
 	struct gfs2_inode *l_ip = GFS2_I(sdp->sd_sc_inode);
 	struct gfs2_statfs_change_host *m_sc = &sdp->sd_statfs_master;
@@ -657,10 +657,13 @@ void adjust_fs_space(struct inode *inode)
 	struct buffer_head *m_bh, *l_bh;
 	u64 fs_total, new_free;
 
+	if (gfs2_trans_begin(sdp, 2 * RES_STATFS, 0) != 0)
+		return;
+
 	/* Total up the file system space, according to the latest rindex. */
 	fs_total = gfs2_ri_total(sdp);
 	if (gfs2_meta_inode_buffer(m_ip, &m_bh) != 0)
-		return;
+		goto out;
 
 	spin_lock(&sdp->sd_statfs_spin);
 	gfs2_statfs_change_in(m_sc, m_bh->b_data +
@@ -675,11 +678,14 @@ void adjust_fs_space(struct inode *inode)
 	gfs2_statfs_change(sdp, new_free, new_free, 0);
 
 	if (gfs2_meta_inode_buffer(l_ip, &l_bh) != 0)
-		goto out;
+		goto out2;
 	update_statfs(sdp, m_bh, l_bh);
 	brelse(l_bh);
-out:
+out2:
 	brelse(m_bh);
+out:
+	sdp->sd_rindex_uptodate = 0;
+	gfs2_trans_end(sdp);
 }
 
 /**
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 5da4ca9041c0..2fae3f4f5db6 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -991,15 +991,31 @@ static void gfs2_write_unlock(struct inode *inode)
 	gfs2_glock_dq_uninit(&ip->i_gh);
 }
 
-static void gfs2_iomap_journaled_page_done(struct inode *inode, loff_t pos,
-				unsigned copied, struct page *page,
-				struct iomap *iomap)
+static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos,
+				   unsigned len, struct iomap *iomap)
+{
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
+
+	return gfs2_trans_begin(sdp, RES_DINODE + (len >> inode->i_blkbits), 0);
+}
+
+static void gfs2_iomap_page_done(struct inode *inode, loff_t pos,
+				 unsigned copied, struct page *page,
+				 struct iomap *iomap)
 {
 	struct gfs2_inode *ip = GFS2_I(inode);
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
 
-	gfs2_page_add_databufs(ip, page, offset_in_page(pos), copied);
+	if (page && !gfs2_is_stuffed(ip))
+		gfs2_page_add_databufs(ip, page, offset_in_page(pos), copied);
+	gfs2_trans_end(sdp);
 }
 
+const struct iomap_page_ops gfs2_iomap_page_ops = {
+	.page_prepare = gfs2_iomap_page_prepare,
+	.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,
@@ -1052,32 +1068,46 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 	if (alloc_required)
 		rblocks += gfs2_rg_blocks(ip, data_blocks + ind_blocks);
 
-	ret = gfs2_trans_begin(sdp, rblocks, iomap->length >> inode->i_blkbits);
-	if (ret)
-		goto out_trans_fail;
+	if (unstuff || iomap->type == IOMAP_HOLE) {
+		struct gfs2_trans *tr;
 
-	if (unstuff) {
-		ret = gfs2_unstuff_dinode(ip, NULL);
-		if (ret)
-			goto out_trans_end;
-		release_metapath(mp);
-		ret = gfs2_iomap_get(inode, iomap->offset, iomap->length,
-				     flags, iomap, mp);
+		ret = gfs2_trans_begin(sdp, rblocks,
+				       iomap->length >> inode->i_blkbits);
 		if (ret)
-			goto out_trans_end;
-	}
+			goto out_trans_fail;
 
-	if (iomap->type == IOMAP_HOLE) {
-		ret = gfs2_iomap_alloc(inode, iomap, flags, mp);
-		if (ret) {
-			gfs2_trans_end(sdp);
-			gfs2_inplace_release(ip);
-			punch_hole(ip, iomap->offset, iomap->length);
-			goto out_qunlock;
+		if (unstuff) {
+			ret = gfs2_unstuff_dinode(ip, NULL);
+			if (ret)
+				goto out_trans_end;
+			release_metapath(mp);
+			ret = gfs2_iomap_get(inode, iomap->offset,
+					     iomap->length, flags, iomap, mp);
+			if (ret)
+				goto out_trans_end;
 		}
+
+		if (iomap->type == IOMAP_HOLE) {
+			ret = gfs2_iomap_alloc(inode, iomap, flags, mp);
+			if (ret) {
+				gfs2_trans_end(sdp);
+				gfs2_inplace_release(ip);
+				punch_hole(ip, iomap->offset, iomap->length);
+				goto out_qunlock;
+			}
+		}
+
+		tr = current->journal_info;
+		if (tr->tr_num_buf_new)
+			__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
+		else
+			gfs2_trans_add_meta(ip->i_gl, mp->mp_bh[0]);
+
+		gfs2_trans_end(sdp);
 	}
-	if (!gfs2_is_stuffed(ip) && gfs2_is_jdata(ip))
-		iomap->page_done = gfs2_iomap_journaled_page_done;
+
+	if (gfs2_is_stuffed(ip) || gfs2_is_jdata(ip))
+		iomap->page_ops = &gfs2_iomap_page_ops;
 	return 0;
 
 out_trans_end:
@@ -1116,10 +1146,6 @@ static int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 		    iomap->type != IOMAP_MAPPED)
 			ret = -ENOTBLK;
 	}
-	if (!ret) {
-		get_bh(mp.mp_bh[0]);
-		iomap->private = mp.mp_bh[0];
-	}
 	release_metapath(&mp);
 	trace_gfs2_iomap_end(ip, iomap, ret);
 	return ret;
@@ -1130,27 +1156,16 @@ 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);
-	struct gfs2_trans *tr = current->journal_info;
-	struct buffer_head *dibh = iomap->private;
 
 	if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) != IOMAP_WRITE)
 		goto out;
 
-	if (iomap->type != IOMAP_INLINE) {
+	if (!gfs2_is_stuffed(ip))
 		gfs2_ordered_add_inode(ip);
 
-		if (tr->tr_num_buf_new)
-			__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
-		else
-			gfs2_trans_add_meta(ip->i_gl, dibh);
-	}
-
-	if (inode == sdp->sd_rindex) {
+	if (inode == sdp->sd_rindex)
 		adjust_fs_space(inode);
-		sdp->sd_rindex_uptodate = 0;
-	}
 
-	gfs2_trans_end(sdp);
 	gfs2_inplace_release(ip);
 
 	if (length != written && (iomap->flags & IOMAP_F_NEW)) {
@@ -1170,8 +1185,6 @@ static int gfs2_iomap_end(struct inode *inode, loff_t pos, loff_t length,
 	gfs2_write_unlock(inode);
 
 out:
-	if (dibh)
-		brelse(dibh);
 	return 0;
 }
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Cluster-devel] [PATCH v3 2/2] gfs2: Fix iomap write page reclaim deadlock
@ 2019-04-25 16:09   ` Andreas Gruenbacher
  0 siblings, 0 replies; 14+ messages in thread
From: Andreas Gruenbacher @ 2019-04-25 16:09 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Since commit 64bc06bb32ee ("gfs2: iomap buffered write support"), gfs2 is doing
buffered writes by starting a transaction in iomap_begin, writing a range of
pages, and ending that transaction in iomap_end.  This approach suffers from
two problems:

  (1) Any allocations necessary for the write are done in iomap_begin, so when
  the data aren't journaled, there is no need for keeping the transaction open
  until iomap_end.

  (2) Transactions keep the gfs2 log flush lock held.  When
  iomap_file_buffered_write calls balance_dirty_pages, this can end up calling
  gfs2_write_inode, which will try to flush the log.  This requires taking the
  log flush lock which is already held, resulting in a deadlock.

Fix both of these issues by not keeping transactions open from iomap_begin to
iomap_end.  Instead, start a small transaction in page_prepare and end it in
page_done when necessary.

Reported-by: Edwin T?r?k <edvin.torok@citrix.com>
Fixes: 64bc06bb32ee ("gfs2: iomap buffered write support")
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/aops.c |  14 +++++--
 fs/gfs2/bmap.c | 101 ++++++++++++++++++++++++++++---------------------
 2 files changed, 67 insertions(+), 48 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 05dd78f4b2b3..6210d4429d84 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -649,7 +649,7 @@ static int gfs2_readpages(struct file *file, struct address_space *mapping,
  */
 void adjust_fs_space(struct inode *inode)
 {
-	struct gfs2_sbd *sdp = inode->i_sb->s_fs_info;
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	struct gfs2_inode *m_ip = GFS2_I(sdp->sd_statfs_inode);
 	struct gfs2_inode *l_ip = GFS2_I(sdp->sd_sc_inode);
 	struct gfs2_statfs_change_host *m_sc = &sdp->sd_statfs_master;
@@ -657,10 +657,13 @@ void adjust_fs_space(struct inode *inode)
 	struct buffer_head *m_bh, *l_bh;
 	u64 fs_total, new_free;
 
+	if (gfs2_trans_begin(sdp, 2 * RES_STATFS, 0) != 0)
+		return;
+
 	/* Total up the file system space, according to the latest rindex. */
 	fs_total = gfs2_ri_total(sdp);
 	if (gfs2_meta_inode_buffer(m_ip, &m_bh) != 0)
-		return;
+		goto out;
 
 	spin_lock(&sdp->sd_statfs_spin);
 	gfs2_statfs_change_in(m_sc, m_bh->b_data +
@@ -675,11 +678,14 @@ void adjust_fs_space(struct inode *inode)
 	gfs2_statfs_change(sdp, new_free, new_free, 0);
 
 	if (gfs2_meta_inode_buffer(l_ip, &l_bh) != 0)
-		goto out;
+		goto out2;
 	update_statfs(sdp, m_bh, l_bh);
 	brelse(l_bh);
-out:
+out2:
 	brelse(m_bh);
+out:
+	sdp->sd_rindex_uptodate = 0;
+	gfs2_trans_end(sdp);
 }
 
 /**
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 5da4ca9041c0..2fae3f4f5db6 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -991,15 +991,31 @@ static void gfs2_write_unlock(struct inode *inode)
 	gfs2_glock_dq_uninit(&ip->i_gh);
 }
 
-static void gfs2_iomap_journaled_page_done(struct inode *inode, loff_t pos,
-				unsigned copied, struct page *page,
-				struct iomap *iomap)
+static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos,
+				   unsigned len, struct iomap *iomap)
+{
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
+
+	return gfs2_trans_begin(sdp, RES_DINODE + (len >> inode->i_blkbits), 0);
+}
+
+static void gfs2_iomap_page_done(struct inode *inode, loff_t pos,
+				 unsigned copied, struct page *page,
+				 struct iomap *iomap)
 {
 	struct gfs2_inode *ip = GFS2_I(inode);
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
 
-	gfs2_page_add_databufs(ip, page, offset_in_page(pos), copied);
+	if (page && !gfs2_is_stuffed(ip))
+		gfs2_page_add_databufs(ip, page, offset_in_page(pos), copied);
+	gfs2_trans_end(sdp);
 }
 
+const struct iomap_page_ops gfs2_iomap_page_ops = {
+	.page_prepare = gfs2_iomap_page_prepare,
+	.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,
@@ -1052,32 +1068,46 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
 	if (alloc_required)
 		rblocks += gfs2_rg_blocks(ip, data_blocks + ind_blocks);
 
-	ret = gfs2_trans_begin(sdp, rblocks, iomap->length >> inode->i_blkbits);
-	if (ret)
-		goto out_trans_fail;
+	if (unstuff || iomap->type == IOMAP_HOLE) {
+		struct gfs2_trans *tr;
 
-	if (unstuff) {
-		ret = gfs2_unstuff_dinode(ip, NULL);
-		if (ret)
-			goto out_trans_end;
-		release_metapath(mp);
-		ret = gfs2_iomap_get(inode, iomap->offset, iomap->length,
-				     flags, iomap, mp);
+		ret = gfs2_trans_begin(sdp, rblocks,
+				       iomap->length >> inode->i_blkbits);
 		if (ret)
-			goto out_trans_end;
-	}
+			goto out_trans_fail;
 
-	if (iomap->type == IOMAP_HOLE) {
-		ret = gfs2_iomap_alloc(inode, iomap, flags, mp);
-		if (ret) {
-			gfs2_trans_end(sdp);
-			gfs2_inplace_release(ip);
-			punch_hole(ip, iomap->offset, iomap->length);
-			goto out_qunlock;
+		if (unstuff) {
+			ret = gfs2_unstuff_dinode(ip, NULL);
+			if (ret)
+				goto out_trans_end;
+			release_metapath(mp);
+			ret = gfs2_iomap_get(inode, iomap->offset,
+					     iomap->length, flags, iomap, mp);
+			if (ret)
+				goto out_trans_end;
 		}
+
+		if (iomap->type == IOMAP_HOLE) {
+			ret = gfs2_iomap_alloc(inode, iomap, flags, mp);
+			if (ret) {
+				gfs2_trans_end(sdp);
+				gfs2_inplace_release(ip);
+				punch_hole(ip, iomap->offset, iomap->length);
+				goto out_qunlock;
+			}
+		}
+
+		tr = current->journal_info;
+		if (tr->tr_num_buf_new)
+			__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
+		else
+			gfs2_trans_add_meta(ip->i_gl, mp->mp_bh[0]);
+
+		gfs2_trans_end(sdp);
 	}
-	if (!gfs2_is_stuffed(ip) && gfs2_is_jdata(ip))
-		iomap->page_done = gfs2_iomap_journaled_page_done;
+
+	if (gfs2_is_stuffed(ip) || gfs2_is_jdata(ip))
+		iomap->page_ops = &gfs2_iomap_page_ops;
 	return 0;
 
 out_trans_end:
@@ -1116,10 +1146,6 @@ static int gfs2_iomap_begin(struct inode *inode, loff_t pos, loff_t length,
 		    iomap->type != IOMAP_MAPPED)
 			ret = -ENOTBLK;
 	}
-	if (!ret) {
-		get_bh(mp.mp_bh[0]);
-		iomap->private = mp.mp_bh[0];
-	}
 	release_metapath(&mp);
 	trace_gfs2_iomap_end(ip, iomap, ret);
 	return ret;
@@ -1130,27 +1156,16 @@ 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);
-	struct gfs2_trans *tr = current->journal_info;
-	struct buffer_head *dibh = iomap->private;
 
 	if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) != IOMAP_WRITE)
 		goto out;
 
-	if (iomap->type != IOMAP_INLINE) {
+	if (!gfs2_is_stuffed(ip))
 		gfs2_ordered_add_inode(ip);
 
-		if (tr->tr_num_buf_new)
-			__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
-		else
-			gfs2_trans_add_meta(ip->i_gl, dibh);
-	}
-
-	if (inode == sdp->sd_rindex) {
+	if (inode == sdp->sd_rindex)
 		adjust_fs_space(inode);
-		sdp->sd_rindex_uptodate = 0;
-	}
 
-	gfs2_trans_end(sdp);
 	gfs2_inplace_release(ip);
 
 	if (length != written && (iomap->flags & IOMAP_F_NEW)) {
@@ -1170,8 +1185,6 @@ static int gfs2_iomap_end(struct inode *inode, loff_t pos, loff_t length,
 	gfs2_write_unlock(inode);
 
 out:
-	if (dibh)
-		brelse(dibh);
 	return 0;
 }
 
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/2] iomap: Add a page_prepare callback
  2019-04-25 16:09 ` [Cluster-devel] " Andreas Gruenbacher
@ 2019-04-25 19:09   ` kbuild test robot
  -1 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2019-04-25 19:09 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: kbuild-all, cluster-devel, Christoph Hellwig, Bob Peterson,
	Jan Kara, Dave Chinner, Ross Lagerwall, Mark Syms,
	Edwin Török, linux-fsdevel, linux-mm,
	Andreas Gruenbacher

[-- Attachment #1: Type: text/plain, Size: 8442 bytes --]

Hi Andreas,

I love your patch! Yet something to improve:

[auto build test ERROR on gfs2/for-next]
[also build test ERROR on v5.1-rc6 next-20190424]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andreas-Gruenbacher/iomap-Add-a-page_prepare-callback/20190426-020018
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

Note: the linux-review/Andreas-Gruenbacher/iomap-Add-a-page_prepare-callback/20190426-020018 HEAD 9167204805d5e926444d03b3fa3ac1d1bc899a8a builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   fs/gfs2/bmap.c: In function 'gfs2_iomap_begin_write':
>> fs/gfs2/bmap.c:1080:10: error: 'struct iomap' has no member named 'page_done'; did you mean 'page_ops'?
      iomap->page_done = gfs2_iomap_journaled_page_done;
             ^~~~~~~~~
             page_ops

vim +1080 fs/gfs2/bmap.c

64bc06bb Andreas Gruenbacher 2018-06-24  1002  
64bc06bb Andreas Gruenbacher 2018-06-24  1003  static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
64bc06bb Andreas Gruenbacher 2018-06-24  1004  				  loff_t length, unsigned flags,
c26b5aa8 Andreas Gruenbacher 2018-11-11  1005  				  struct iomap *iomap,
c26b5aa8 Andreas Gruenbacher 2018-11-11  1006  				  struct metapath *mp)
64bc06bb Andreas Gruenbacher 2018-06-24  1007  {
64bc06bb Andreas Gruenbacher 2018-06-24  1008  	struct gfs2_inode *ip = GFS2_I(inode);
64bc06bb Andreas Gruenbacher 2018-06-24  1009  	struct gfs2_sbd *sdp = GFS2_SB(inode);
64bc06bb Andreas Gruenbacher 2018-06-24  1010  	unsigned int data_blocks = 0, ind_blocks = 0, rblocks;
64bc06bb Andreas Gruenbacher 2018-06-24  1011  	bool unstuff, alloc_required;
628e366d Andreas Gruenbacher 2018-06-04  1012  	int ret;
628e366d Andreas Gruenbacher 2018-06-04  1013  
64bc06bb Andreas Gruenbacher 2018-06-24  1014  	ret = gfs2_write_lock(inode);
64bc06bb Andreas Gruenbacher 2018-06-24  1015  	if (ret)
64bc06bb Andreas Gruenbacher 2018-06-24  1016  		return ret;
64bc06bb Andreas Gruenbacher 2018-06-24  1017  
64bc06bb Andreas Gruenbacher 2018-06-24  1018  	unstuff = gfs2_is_stuffed(ip) &&
64bc06bb Andreas Gruenbacher 2018-06-24  1019  		  pos + length > gfs2_max_stuffed_size(ip);
64bc06bb Andreas Gruenbacher 2018-06-24  1020  
c26b5aa8 Andreas Gruenbacher 2018-11-11  1021  	ret = gfs2_iomap_get(inode, pos, length, flags, iomap, mp);
64bc06bb Andreas Gruenbacher 2018-06-24  1022  	if (ret)
c26b5aa8 Andreas Gruenbacher 2018-11-11  1023  		goto out_unlock;
64bc06bb Andreas Gruenbacher 2018-06-24  1024  
64bc06bb Andreas Gruenbacher 2018-06-24  1025  	alloc_required = unstuff || iomap->type == IOMAP_HOLE;
64bc06bb Andreas Gruenbacher 2018-06-24  1026  
64bc06bb Andreas Gruenbacher 2018-06-24  1027  	if (alloc_required || gfs2_is_jdata(ip))
64bc06bb Andreas Gruenbacher 2018-06-24  1028  		gfs2_write_calc_reserv(ip, iomap->length, &data_blocks,
64bc06bb Andreas Gruenbacher 2018-06-24  1029  				       &ind_blocks);
64bc06bb Andreas Gruenbacher 2018-06-24  1030  
64bc06bb Andreas Gruenbacher 2018-06-24  1031  	if (alloc_required) {
64bc06bb Andreas Gruenbacher 2018-06-24  1032  		struct gfs2_alloc_parms ap = {
64bc06bb Andreas Gruenbacher 2018-06-24  1033  			.target = data_blocks + ind_blocks
64bc06bb Andreas Gruenbacher 2018-06-24  1034  		};
64bc06bb Andreas Gruenbacher 2018-06-24  1035  
64bc06bb Andreas Gruenbacher 2018-06-24  1036  		ret = gfs2_quota_lock_check(ip, &ap);
64bc06bb Andreas Gruenbacher 2018-06-24  1037  		if (ret)
c26b5aa8 Andreas Gruenbacher 2018-11-11  1038  			goto out_unlock;
64bc06bb Andreas Gruenbacher 2018-06-24  1039  
64bc06bb Andreas Gruenbacher 2018-06-24  1040  		ret = gfs2_inplace_reserve(ip, &ap);
64bc06bb Andreas Gruenbacher 2018-06-24  1041  		if (ret)
64bc06bb Andreas Gruenbacher 2018-06-24  1042  			goto out_qunlock;
64bc06bb Andreas Gruenbacher 2018-06-24  1043  	}
64bc06bb Andreas Gruenbacher 2018-06-24  1044  
64bc06bb Andreas Gruenbacher 2018-06-24  1045  	rblocks = RES_DINODE + ind_blocks;
64bc06bb Andreas Gruenbacher 2018-06-24  1046  	if (gfs2_is_jdata(ip))
64bc06bb Andreas Gruenbacher 2018-06-24  1047  		rblocks += data_blocks;
64bc06bb Andreas Gruenbacher 2018-06-24  1048  	if (ind_blocks || data_blocks)
64bc06bb Andreas Gruenbacher 2018-06-24  1049  		rblocks += RES_STATFS + RES_QUOTA;
64bc06bb Andreas Gruenbacher 2018-06-24  1050  	if (inode == sdp->sd_rindex)
64bc06bb Andreas Gruenbacher 2018-06-24  1051  		rblocks += 2 * RES_STATFS;
64bc06bb Andreas Gruenbacher 2018-06-24  1052  	if (alloc_required)
64bc06bb Andreas Gruenbacher 2018-06-24  1053  		rblocks += gfs2_rg_blocks(ip, data_blocks + ind_blocks);
64bc06bb Andreas Gruenbacher 2018-06-24  1054  
64bc06bb Andreas Gruenbacher 2018-06-24  1055  	ret = gfs2_trans_begin(sdp, rblocks, iomap->length >> inode->i_blkbits);
64bc06bb Andreas Gruenbacher 2018-06-24  1056  	if (ret)
64bc06bb Andreas Gruenbacher 2018-06-24  1057  		goto out_trans_fail;
64bc06bb Andreas Gruenbacher 2018-06-24  1058  
64bc06bb Andreas Gruenbacher 2018-06-24  1059  	if (unstuff) {
64bc06bb Andreas Gruenbacher 2018-06-24  1060  		ret = gfs2_unstuff_dinode(ip, NULL);
64bc06bb Andreas Gruenbacher 2018-06-24  1061  		if (ret)
64bc06bb Andreas Gruenbacher 2018-06-24  1062  			goto out_trans_end;
c26b5aa8 Andreas Gruenbacher 2018-11-11  1063  		release_metapath(mp);
64bc06bb Andreas Gruenbacher 2018-06-24  1064  		ret = gfs2_iomap_get(inode, iomap->offset, iomap->length,
c26b5aa8 Andreas Gruenbacher 2018-11-11  1065  				     flags, iomap, mp);
64bc06bb Andreas Gruenbacher 2018-06-24  1066  		if (ret)
64bc06bb Andreas Gruenbacher 2018-06-24  1067  			goto out_trans_end;
64bc06bb Andreas Gruenbacher 2018-06-24  1068  	}
64bc06bb Andreas Gruenbacher 2018-06-24  1069  
64bc06bb Andreas Gruenbacher 2018-06-24  1070  	if (iomap->type == IOMAP_HOLE) {
c26b5aa8 Andreas Gruenbacher 2018-11-11  1071  		ret = gfs2_iomap_alloc(inode, iomap, flags, mp);
64bc06bb Andreas Gruenbacher 2018-06-24  1072  		if (ret) {
64bc06bb Andreas Gruenbacher 2018-06-24  1073  			gfs2_trans_end(sdp);
64bc06bb Andreas Gruenbacher 2018-06-24  1074  			gfs2_inplace_release(ip);
64bc06bb Andreas Gruenbacher 2018-06-24  1075  			punch_hole(ip, iomap->offset, iomap->length);
64bc06bb Andreas Gruenbacher 2018-06-24  1076  			goto out_qunlock;
64bc06bb Andreas Gruenbacher 2018-06-24  1077  		}
64bc06bb Andreas Gruenbacher 2018-06-24  1078  	}
fee5150c Andreas Gruenbacher 2018-10-10  1079  	if (!gfs2_is_stuffed(ip) && gfs2_is_jdata(ip))
64bc06bb Andreas Gruenbacher 2018-06-24 @1080  		iomap->page_done = gfs2_iomap_journaled_page_done;
64bc06bb Andreas Gruenbacher 2018-06-24  1081  	return 0;
64bc06bb Andreas Gruenbacher 2018-06-24  1082  
64bc06bb Andreas Gruenbacher 2018-06-24  1083  out_trans_end:
64bc06bb Andreas Gruenbacher 2018-06-24  1084  	gfs2_trans_end(sdp);
64bc06bb Andreas Gruenbacher 2018-06-24  1085  out_trans_fail:
64bc06bb Andreas Gruenbacher 2018-06-24  1086  	if (alloc_required)
64bc06bb Andreas Gruenbacher 2018-06-24  1087  		gfs2_inplace_release(ip);
64bc06bb Andreas Gruenbacher 2018-06-24  1088  out_qunlock:
64bc06bb Andreas Gruenbacher 2018-06-24  1089  	if (alloc_required)
64bc06bb Andreas Gruenbacher 2018-06-24  1090  		gfs2_quota_unlock(ip);
c26b5aa8 Andreas Gruenbacher 2018-11-11  1091  out_unlock:
64bc06bb Andreas Gruenbacher 2018-06-24  1092  	gfs2_write_unlock(inode);
64bc06bb Andreas Gruenbacher 2018-06-24  1093  	return ret;
64bc06bb Andreas Gruenbacher 2018-06-24  1094  }
64bc06bb Andreas Gruenbacher 2018-06-24  1095  

:::::: The code at line 1080 was first introduced by commit
:::::: 64bc06bb32ee9cf458f432097113c8b495d75757 gfs2: iomap buffered write support

:::::: TO: Andreas Gruenbacher <agruenba@redhat.com>
:::::: CC: Andreas Gruenbacher <agruenba@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56073 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [PATCH v3 1/2] iomap: Add a page_prepare callback
@ 2019-04-25 19:09   ` kbuild test robot
  0 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2019-04-25 19:09 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Hi Andreas,

I love your patch! Yet something to improve:

[auto build test ERROR on gfs2/for-next]
[also build test ERROR on v5.1-rc6 next-20190424]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andreas-Gruenbacher/iomap-Add-a-page_prepare-callback/20190426-020018
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

Note: the linux-review/Andreas-Gruenbacher/iomap-Add-a-page_prepare-callback/20190426-020018 HEAD 9167204805d5e926444d03b3fa3ac1d1bc899a8a builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   fs/gfs2/bmap.c: In function 'gfs2_iomap_begin_write':
>> fs/gfs2/bmap.c:1080:10: error: 'struct iomap' has no member named 'page_done'; did you mean 'page_ops'?
      iomap->page_done = gfs2_iomap_journaled_page_done;
             ^~~~~~~~~
             page_ops

vim +1080 fs/gfs2/bmap.c

64bc06bb Andreas Gruenbacher 2018-06-24  1002  
64bc06bb Andreas Gruenbacher 2018-06-24  1003  static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
64bc06bb Andreas Gruenbacher 2018-06-24  1004  				  loff_t length, unsigned flags,
c26b5aa8 Andreas Gruenbacher 2018-11-11  1005  				  struct iomap *iomap,
c26b5aa8 Andreas Gruenbacher 2018-11-11  1006  				  struct metapath *mp)
64bc06bb Andreas Gruenbacher 2018-06-24  1007  {
64bc06bb Andreas Gruenbacher 2018-06-24  1008  	struct gfs2_inode *ip = GFS2_I(inode);
64bc06bb Andreas Gruenbacher 2018-06-24  1009  	struct gfs2_sbd *sdp = GFS2_SB(inode);
64bc06bb Andreas Gruenbacher 2018-06-24  1010  	unsigned int data_blocks = 0, ind_blocks = 0, rblocks;
64bc06bb Andreas Gruenbacher 2018-06-24  1011  	bool unstuff, alloc_required;
628e366d Andreas Gruenbacher 2018-06-04  1012  	int ret;
628e366d Andreas Gruenbacher 2018-06-04  1013  
64bc06bb Andreas Gruenbacher 2018-06-24  1014  	ret = gfs2_write_lock(inode);
64bc06bb Andreas Gruenbacher 2018-06-24  1015  	if (ret)
64bc06bb Andreas Gruenbacher 2018-06-24  1016  		return ret;
64bc06bb Andreas Gruenbacher 2018-06-24  1017  
64bc06bb Andreas Gruenbacher 2018-06-24  1018  	unstuff = gfs2_is_stuffed(ip) &&
64bc06bb Andreas Gruenbacher 2018-06-24  1019  		  pos + length > gfs2_max_stuffed_size(ip);
64bc06bb Andreas Gruenbacher 2018-06-24  1020  
c26b5aa8 Andreas Gruenbacher 2018-11-11  1021  	ret = gfs2_iomap_get(inode, pos, length, flags, iomap, mp);
64bc06bb Andreas Gruenbacher 2018-06-24  1022  	if (ret)
c26b5aa8 Andreas Gruenbacher 2018-11-11  1023  		goto out_unlock;
64bc06bb Andreas Gruenbacher 2018-06-24  1024  
64bc06bb Andreas Gruenbacher 2018-06-24  1025  	alloc_required = unstuff || iomap->type == IOMAP_HOLE;
64bc06bb Andreas Gruenbacher 2018-06-24  1026  
64bc06bb Andreas Gruenbacher 2018-06-24  1027  	if (alloc_required || gfs2_is_jdata(ip))
64bc06bb Andreas Gruenbacher 2018-06-24  1028  		gfs2_write_calc_reserv(ip, iomap->length, &data_blocks,
64bc06bb Andreas Gruenbacher 2018-06-24  1029  				       &ind_blocks);
64bc06bb Andreas Gruenbacher 2018-06-24  1030  
64bc06bb Andreas Gruenbacher 2018-06-24  1031  	if (alloc_required) {
64bc06bb Andreas Gruenbacher 2018-06-24  1032  		struct gfs2_alloc_parms ap = {
64bc06bb Andreas Gruenbacher 2018-06-24  1033  			.target = data_blocks + ind_blocks
64bc06bb Andreas Gruenbacher 2018-06-24  1034  		};
64bc06bb Andreas Gruenbacher 2018-06-24  1035  
64bc06bb Andreas Gruenbacher 2018-06-24  1036  		ret = gfs2_quota_lock_check(ip, &ap);
64bc06bb Andreas Gruenbacher 2018-06-24  1037  		if (ret)
c26b5aa8 Andreas Gruenbacher 2018-11-11  1038  			goto out_unlock;
64bc06bb Andreas Gruenbacher 2018-06-24  1039  
64bc06bb Andreas Gruenbacher 2018-06-24  1040  		ret = gfs2_inplace_reserve(ip, &ap);
64bc06bb Andreas Gruenbacher 2018-06-24  1041  		if (ret)
64bc06bb Andreas Gruenbacher 2018-06-24  1042  			goto out_qunlock;
64bc06bb Andreas Gruenbacher 2018-06-24  1043  	}
64bc06bb Andreas Gruenbacher 2018-06-24  1044  
64bc06bb Andreas Gruenbacher 2018-06-24  1045  	rblocks = RES_DINODE + ind_blocks;
64bc06bb Andreas Gruenbacher 2018-06-24  1046  	if (gfs2_is_jdata(ip))
64bc06bb Andreas Gruenbacher 2018-06-24  1047  		rblocks += data_blocks;
64bc06bb Andreas Gruenbacher 2018-06-24  1048  	if (ind_blocks || data_blocks)
64bc06bb Andreas Gruenbacher 2018-06-24  1049  		rblocks += RES_STATFS + RES_QUOTA;
64bc06bb Andreas Gruenbacher 2018-06-24  1050  	if (inode == sdp->sd_rindex)
64bc06bb Andreas Gruenbacher 2018-06-24  1051  		rblocks += 2 * RES_STATFS;
64bc06bb Andreas Gruenbacher 2018-06-24  1052  	if (alloc_required)
64bc06bb Andreas Gruenbacher 2018-06-24  1053  		rblocks += gfs2_rg_blocks(ip, data_blocks + ind_blocks);
64bc06bb Andreas Gruenbacher 2018-06-24  1054  
64bc06bb Andreas Gruenbacher 2018-06-24  1055  	ret = gfs2_trans_begin(sdp, rblocks, iomap->length >> inode->i_blkbits);
64bc06bb Andreas Gruenbacher 2018-06-24  1056  	if (ret)
64bc06bb Andreas Gruenbacher 2018-06-24  1057  		goto out_trans_fail;
64bc06bb Andreas Gruenbacher 2018-06-24  1058  
64bc06bb Andreas Gruenbacher 2018-06-24  1059  	if (unstuff) {
64bc06bb Andreas Gruenbacher 2018-06-24  1060  		ret = gfs2_unstuff_dinode(ip, NULL);
64bc06bb Andreas Gruenbacher 2018-06-24  1061  		if (ret)
64bc06bb Andreas Gruenbacher 2018-06-24  1062  			goto out_trans_end;
c26b5aa8 Andreas Gruenbacher 2018-11-11  1063  		release_metapath(mp);
64bc06bb Andreas Gruenbacher 2018-06-24  1064  		ret = gfs2_iomap_get(inode, iomap->offset, iomap->length,
c26b5aa8 Andreas Gruenbacher 2018-11-11  1065  				     flags, iomap, mp);
64bc06bb Andreas Gruenbacher 2018-06-24  1066  		if (ret)
64bc06bb Andreas Gruenbacher 2018-06-24  1067  			goto out_trans_end;
64bc06bb Andreas Gruenbacher 2018-06-24  1068  	}
64bc06bb Andreas Gruenbacher 2018-06-24  1069  
64bc06bb Andreas Gruenbacher 2018-06-24  1070  	if (iomap->type == IOMAP_HOLE) {
c26b5aa8 Andreas Gruenbacher 2018-11-11  1071  		ret = gfs2_iomap_alloc(inode, iomap, flags, mp);
64bc06bb Andreas Gruenbacher 2018-06-24  1072  		if (ret) {
64bc06bb Andreas Gruenbacher 2018-06-24  1073  			gfs2_trans_end(sdp);
64bc06bb Andreas Gruenbacher 2018-06-24  1074  			gfs2_inplace_release(ip);
64bc06bb Andreas Gruenbacher 2018-06-24  1075  			punch_hole(ip, iomap->offset, iomap->length);
64bc06bb Andreas Gruenbacher 2018-06-24  1076  			goto out_qunlock;
64bc06bb Andreas Gruenbacher 2018-06-24  1077  		}
64bc06bb Andreas Gruenbacher 2018-06-24  1078  	}
fee5150c Andreas Gruenbacher 2018-10-10  1079  	if (!gfs2_is_stuffed(ip) && gfs2_is_jdata(ip))
64bc06bb Andreas Gruenbacher 2018-06-24 @1080  		iomap->page_done = gfs2_iomap_journaled_page_done;
64bc06bb Andreas Gruenbacher 2018-06-24  1081  	return 0;
64bc06bb Andreas Gruenbacher 2018-06-24  1082  
64bc06bb Andreas Gruenbacher 2018-06-24  1083  out_trans_end:
64bc06bb Andreas Gruenbacher 2018-06-24  1084  	gfs2_trans_end(sdp);
64bc06bb Andreas Gruenbacher 2018-06-24  1085  out_trans_fail:
64bc06bb Andreas Gruenbacher 2018-06-24  1086  	if (alloc_required)
64bc06bb Andreas Gruenbacher 2018-06-24  1087  		gfs2_inplace_release(ip);
64bc06bb Andreas Gruenbacher 2018-06-24  1088  out_qunlock:
64bc06bb Andreas Gruenbacher 2018-06-24  1089  	if (alloc_required)
64bc06bb Andreas Gruenbacher 2018-06-24  1090  		gfs2_quota_unlock(ip);
c26b5aa8 Andreas Gruenbacher 2018-11-11  1091  out_unlock:
64bc06bb Andreas Gruenbacher 2018-06-24  1092  	gfs2_write_unlock(inode);
64bc06bb Andreas Gruenbacher 2018-06-24  1093  	return ret;
64bc06bb Andreas Gruenbacher 2018-06-24  1094  }
64bc06bb Andreas Gruenbacher 2018-06-24  1095  

:::::: The code at line 1080 was first introduced by commit
:::::: 64bc06bb32ee9cf458f432097113c8b495d75757 gfs2: iomap buffered write support

:::::: TO: Andreas Gruenbacher <agruenba@redhat.com>
:::::: CC: Andreas Gruenbacher <agruenba@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 56073 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20190426/351b554e/attachment.gz>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] gfs2: Fix iomap write page reclaim deadlock
  2019-04-25 16:09   ` [Cluster-devel] " Andreas Gruenbacher
@ 2019-04-25 21:01     ` kbuild test robot
  -1 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2019-04-25 21:01 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: kbuild-all, cluster-devel, Christoph Hellwig, Bob Peterson,
	Jan Kara, Dave Chinner, Ross Lagerwall, Mark Syms,
	Edwin Török, linux-fsdevel, linux-mm,
	Andreas Gruenbacher

Hi Andreas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on gfs2/for-next]
[also build test WARNING on v5.1-rc6 next-20190424]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andreas-Gruenbacher/iomap-Add-a-page_prepare-callback/20190426-020018
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git for-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> fs/gfs2/bmap.c:1014:29: sparse: sparse: symbol 'gfs2_iomap_page_ops' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [PATCH v3 2/2] gfs2: Fix iomap write page reclaim deadlock
@ 2019-04-25 21:01     ` kbuild test robot
  0 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2019-04-25 21:01 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Hi Andreas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on gfs2/for-next]
[also build test WARNING on v5.1-rc6 next-20190424]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andreas-Gruenbacher/iomap-Add-a-page_prepare-callback/20190426-020018
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git for-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> fs/gfs2/bmap.c:1014:29: sparse: sparse: symbol 'gfs2_iomap_page_ops' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH] gfs2: gfs2_iomap_page_ops can be static
  2019-04-25 16:09   ` [Cluster-devel] " Andreas Gruenbacher
@ 2019-04-25 21:01     ` kbuild test robot
  -1 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2019-04-25 21:01 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: kbuild-all, cluster-devel, Christoph Hellwig, Bob Peterson,
	Jan Kara, Dave Chinner, Ross Lagerwall, Mark Syms,
	Edwin Török, linux-fsdevel, linux-mm,
	Andreas Gruenbacher


Fixes: 9167204805d5 ("gfs2: Fix iomap write page reclaim deadlock")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
 bmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 2fae3f4..27c82f4 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1011,7 +1011,7 @@ static void gfs2_iomap_page_done(struct inode *inode, loff_t pos,
 	gfs2_trans_end(sdp);
 }
 
-const struct iomap_page_ops gfs2_iomap_page_ops = {
+static const struct iomap_page_ops gfs2_iomap_page_ops = {
 	.page_prepare = gfs2_iomap_page_prepare,
 	.page_done = gfs2_iomap_page_done,
 };

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Cluster-devel] [RFC PATCH] gfs2: gfs2_iomap_page_ops can be static
@ 2019-04-25 21:01     ` kbuild test robot
  0 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2019-04-25 21:01 UTC (permalink / raw)
  To: cluster-devel.redhat.com


Fixes: 9167204805d5 ("gfs2: Fix iomap write page reclaim deadlock")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
 bmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 2fae3f4..27c82f4 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1011,7 +1011,7 @@ static void gfs2_iomap_page_done(struct inode *inode, loff_t pos,
 	gfs2_trans_end(sdp);
 }
 
-const struct iomap_page_ops gfs2_iomap_page_ops = {
+static const struct iomap_page_ops gfs2_iomap_page_ops = {
 	.page_prepare = gfs2_iomap_page_prepare,
 	.page_done = gfs2_iomap_page_done,
 };



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/2] iomap: Add a page_prepare callback
  2019-04-25 16:09 ` [Cluster-devel] " Andreas Gruenbacher
@ 2019-04-26  8:30   ` Jan Kara
  -1 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2019-04-26  8:30 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: cluster-devel, Christoph Hellwig, Bob Peterson, Jan Kara,
	Dave Chinner, Ross Lagerwall, Mark Syms, Edwin Török,
	linux-fsdevel, linux-mm

On Thu 25-04-19 18:09:12, Andreas Gruenbacher wrote:
> Move the page_done callback into a separate iomap_page_ops structure and
> add a page_prepare calback to be called before a page is written to.  In
> gfs2, we'll want to start a transaction in page_prepare and end it in
> page_done, and other filesystems that implement data journaling will
> require the same kind of mechanism.

...

> @@ -674,9 +675,17 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
>  	if (fatal_signal_pending(current))
>  		return -EINTR;
>  
> +	if (page_ops) {
> +		status = page_ops->page_prepare(inode, pos, len, iomap);
> +		if (status)
> +			return status;
> +	}
> +

Looks OK for now I guess, although I'm not sure if later some fs won't need
to get hold of the actual page in ->page_prepare() and then we will need to
switch to ->page_prepare() returning the page to use. But let's leave that
for a time when such fs wants to use iomap.

> @@ -780,8 +794,8 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
>  		ret = __iomap_write_end(inode, pos, len, copied, page, iomap);
>  	}
>  
> -	if (iomap->page_done)
> -		iomap->page_done(inode, pos, copied, page, iomap);
> +	if (page_ops)
> +		page_ops->page_done(inode, pos, copied, page, iomap);

Looking at the code now, this is actually flawed (preexisting problem):
__iomap_write_end or generic_write_end() will release the page reference
and so you cannot just pass it to ->page_done(). That is a potential
use-after-free...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [PATCH v3 1/2] iomap: Add a page_prepare callback
@ 2019-04-26  8:30   ` Jan Kara
  0 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2019-04-26  8:30 UTC (permalink / raw)
  To: cluster-devel.redhat.com

On Thu 25-04-19 18:09:12, Andreas Gruenbacher wrote:
> Move the page_done callback into a separate iomap_page_ops structure and
> add a page_prepare calback to be called before a page is written to.  In
> gfs2, we'll want to start a transaction in page_prepare and end it in
> page_done, and other filesystems that implement data journaling will
> require the same kind of mechanism.

...

> @@ -674,9 +675,17 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
>  	if (fatal_signal_pending(current))
>  		return -EINTR;
>  
> +	if (page_ops) {
> +		status = page_ops->page_prepare(inode, pos, len, iomap);
> +		if (status)
> +			return status;
> +	}
> +

Looks OK for now I guess, although I'm not sure if later some fs won't need
to get hold of the actual page in ->page_prepare() and then we will need to
switch to ->page_prepare() returning the page to use. But let's leave that
for a time when such fs wants to use iomap.

> @@ -780,8 +794,8 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
>  		ret = __iomap_write_end(inode, pos, len, copied, page, iomap);
>  	}
>  
> -	if (iomap->page_done)
> -		iomap->page_done(inode, pos, copied, page, iomap);
> +	if (page_ops)
> +		page_ops->page_done(inode, pos, copied, page, iomap);

Looking at the code now, this is actually flawed (preexisting problem):
__iomap_write_end or generic_write_end() will release the page reference
and so you cannot just pass it to ->page_done(). That is a potential
use-after-free...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/2] iomap: Add a page_prepare callback
  2019-04-26  8:30   ` [Cluster-devel] " Jan Kara
@ 2019-04-26 13:11     ` Andreas Gruenbacher
  -1 siblings, 0 replies; 14+ messages in thread
From: Andreas Gruenbacher @ 2019-04-26 13:11 UTC (permalink / raw)
  To: Jan Kara
  Cc: cluster-devel, Christoph Hellwig, Bob Peterson, Dave Chinner,
	Ross Lagerwall, Mark Syms, Edwin Török, linux-fsdevel,
	linux-mm

On Fri, 26 Apr 2019 at 10:30, Jan Kara <jack@suse.cz> wrote:
>
> On Thu 25-04-19 18:09:12, Andreas Gruenbacher wrote:
> > Move the page_done callback into a separate iomap_page_ops structure and
> > add a page_prepare calback to be called before a page is written to.  In
> > gfs2, we'll want to start a transaction in page_prepare and end it in
> > page_done, and other filesystems that implement data journaling will
> > require the same kind of mechanism.
>
> ...
>
> > @@ -674,9 +675,17 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
> >       if (fatal_signal_pending(current))
> >               return -EINTR;
> >
> > +     if (page_ops) {
> > +             status = page_ops->page_prepare(inode, pos, len, iomap);
> > +             if (status)
> > +                     return status;
> > +     }
> > +
>
> Looks OK for now I guess, although I'm not sure if later some fs won't need
> to get hold of the actual page in ->page_prepare() and then we will need to
> switch to ->page_prepare() returning the page to use. But let's leave that
> for a time when such fs wants to use iomap.

Alright.

> > @@ -780,8 +794,8 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
> >               ret = __iomap_write_end(inode, pos, len, copied, page, iomap);
> >       }
> >
> > -     if (iomap->page_done)
> > -             iomap->page_done(inode, pos, copied, page, iomap);
> > +     if (page_ops)
> > +             page_ops->page_done(inode, pos, copied, page, iomap);
>
> Looking at the code now, this is actually flawed (preexisting problem):
> __iomap_write_end or generic_write_end() will release the page reference
> and so you cannot just pass it to ->page_done(). That is a potential
> use-after-free...

Ouch. I'm sending a fix.

Thanks,
Andreas

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [PATCH v3 1/2] iomap: Add a page_prepare callback
@ 2019-04-26 13:11     ` Andreas Gruenbacher
  0 siblings, 0 replies; 14+ messages in thread
From: Andreas Gruenbacher @ 2019-04-26 13:11 UTC (permalink / raw)
  To: cluster-devel.redhat.com

On Fri, 26 Apr 2019 at 10:30, Jan Kara <jack@suse.cz> wrote:
>
> On Thu 25-04-19 18:09:12, Andreas Gruenbacher wrote:
> > Move the page_done callback into a separate iomap_page_ops structure and
> > add a page_prepare calback to be called before a page is written to.  In
> > gfs2, we'll want to start a transaction in page_prepare and end it in
> > page_done, and other filesystems that implement data journaling will
> > require the same kind of mechanism.
>
> ...
>
> > @@ -674,9 +675,17 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
> >       if (fatal_signal_pending(current))
> >               return -EINTR;
> >
> > +     if (page_ops) {
> > +             status = page_ops->page_prepare(inode, pos, len, iomap);
> > +             if (status)
> > +                     return status;
> > +     }
> > +
>
> Looks OK for now I guess, although I'm not sure if later some fs won't need
> to get hold of the actual page in ->page_prepare() and then we will need to
> switch to ->page_prepare() returning the page to use. But let's leave that
> for a time when such fs wants to use iomap.

Alright.

> > @@ -780,8 +794,8 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
> >               ret = __iomap_write_end(inode, pos, len, copied, page, iomap);
> >       }
> >
> > -     if (iomap->page_done)
> > -             iomap->page_done(inode, pos, copied, page, iomap);
> > +     if (page_ops)
> > +             page_ops->page_done(inode, pos, copied, page, iomap);
>
> Looking at the code now, this is actually flawed (preexisting problem):
> __iomap_write_end or generic_write_end() will release the page reference
> and so you cannot just pass it to ->page_done(). That is a potential
> use-after-free...

Ouch. I'm sending a fix.

Thanks,
Andreas



^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-04-26 13:11 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-25 16:09 [PATCH v3 1/2] iomap: Add a page_prepare callback Andreas Gruenbacher
2019-04-25 16:09 ` [Cluster-devel] " Andreas Gruenbacher
2019-04-25 16:09 ` [PATCH v3 2/2] gfs2: Fix iomap write page reclaim deadlock Andreas Gruenbacher
2019-04-25 16:09   ` [Cluster-devel] " Andreas Gruenbacher
2019-04-25 21:01   ` kbuild test robot
2019-04-25 21:01     ` [Cluster-devel] " kbuild test robot
2019-04-25 21:01   ` [RFC PATCH] gfs2: gfs2_iomap_page_ops can be static kbuild test robot
2019-04-25 21:01     ` [Cluster-devel] " kbuild test robot
2019-04-25 19:09 ` [PATCH v3 1/2] iomap: Add a page_prepare callback kbuild test robot
2019-04-25 19:09   ` [Cluster-devel] " kbuild test robot
2019-04-26  8:30 ` Jan Kara
2019-04-26  8:30   ` [Cluster-devel] " Jan Kara
2019-04-26 13:11   ` Andreas Gruenbacher
2019-04-26 13:11     ` [Cluster-devel] " Andreas Gruenbacher

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.