[Cluster-devel] [PATCH 00/11] Read bitmap buffers on demand

[Cluster-devel] [PATCH 00/11] Read bitmap buffers on demand

[Andreas Gruenbacher]

Here's a bunch of patches that have accumulated here lately.  The most
interesting one is likely "gfs2: Read bitmap buffers on demand" which
should make filesystems with large resource groups more efficient.

Thanks,
Andreas

Andreas Gruenbacher (11):
  gfs2: Fix setting GBF_FULL in gfs2_rbm_find
  gfs2: Rename minext => minlen
  gfs2: Don't use struct gfs2_rbm in struct gfs2_extent
  gfs2: Minor gfs2_free_extlen cleanup
  gfs2: Only use struct gfs2_rbm for bitmap manipulations
  gfs2: Minor gfs2_adjust_reservation and gfs2_alloc_extent cleanups
  gfs2: Minor gfs2_rgrp_send_discards cleanup
  gfs2: Add buffer head to struct gfs2_rgrpd
  gfs2: Read bitmap buffers on demand
  gfs2: Clean up assertion, consistency check, and error reporting
    functions
  gfs2: Skip gfs2_metatype_check for cached buffers

 fs/gfs2/bmap.c       |   2 +-
 fs/gfs2/incore.h     |  37 +--
 fs/gfs2/lops.c       |   3 +-
 fs/gfs2/meta_io.c    |  10 +-
 fs/gfs2/rgrp.c       | 672 +++++++++++++++++++++++++------------------
 fs/gfs2/rgrp.h       |   3 +-
 fs/gfs2/trace_gfs2.h |  10 +-
 fs/gfs2/trans.h      |   2 +-
 fs/gfs2/util.c       | 128 ++++-----
 fs/gfs2/util.h       |  85 ++----
 10 files changed, 509 insertions(+), 443 deletions(-)

-- 
2.20.1

[Cluster-devel] [PATCH 01/11] gfs2: Fix setting GBF_FULL in gfs2_rbm_find

[Andreas Gruenbacher]

In gfs2_rbm_find, set the GBF_FULL flag whenever there are no free
blocks in an entire bitmap.  That requires that gfs2_bitfit scans the
entire bitmap (offset == 0), but doesn't depend on initial_offset.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/rgrp.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 831d7cb5a49c4..cb87560c7bcbc 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1731,7 +1731,6 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minext,
 {
 	struct buffer_head *bh;
 	int initial_bii;
-	u32 initial_offset;
 	int first_bii = rbm->bii;
 	u32 first_offset = rbm->offset;
 	u32 offset;
@@ -1761,10 +1760,12 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minext,
 		WARN_ON(!buffer_uptodate(bh));
 		if (state != GFS2_BLKST_UNLINKED && bi->bi_clone)
 			buffer = bi->bi_clone + bi->bi_offset;
-		initial_offset = rbm->offset;
 		offset = gfs2_bitfit(buffer, bi->bi_bytes, rbm->offset, state);
-		if (offset == BFITNOENT)
-			goto bitmap_full;
+		if (offset == BFITNOENT) {
+			if (state == GFS2_BLKST_FREE && rbm->offset == 0)
+				set_bit(GBF_FULL, &bi->bi_flags);
+			goto next_bitmap;
+		}
 		rbm->offset = offset;
 		if (ip == NULL)
 			return 0;
@@ -1787,10 +1788,6 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minext,
 		}
 		return ret;
 
-bitmap_full:	/* Mark bitmap as full and fall through */
-		if ((state == GFS2_BLKST_FREE) && initial_offset == 0)
-			set_bit(GBF_FULL, &bi->bi_flags);
-
 next_bitmap:	/* Find next bitmap in the rgrp */
 		rbm->offset = 0;
 		rbm->bii++;
-- 
2.20.1

[Cluster-devel] [PATCH 02/11] gfs2: Rename minext => minlen

[Andreas Gruenbacher]

Rename the minext parameters which represent extent lengths to minlen to
avoid confusion with maxext, which are actual extents.

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

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index cb87560c7bcbc..cfef8cc5fa155 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -73,7 +73,7 @@ static const char valid_change[16] = {
 	        1, 0, 0, 0
 };
 
-static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minext,
+static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minlen,
 			 const struct gfs2_inode *ip, bool nowrap);
 
 
@@ -1318,7 +1318,8 @@ void gfs2_rgrp_go_unlock(struct gfs2_holder *gh)
 
 int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
 			     struct buffer_head *bh,
-			     const struct gfs2_bitmap *bi, unsigned minlen, u64 *ptrimmed)
+			     const struct gfs2_bitmap *bi, unsigned minlen,
+			     u64 *ptrimmed)
 {
 	struct super_block *sb = sdp->sd_vfs;
 	u64 blk;
@@ -1653,7 +1654,7 @@ static u64 gfs2_next_unreserved_block(struct gfs2_rgrpd *rgd, u64 block,
  * gfs2_reservation_check_and_update - Check for reservations during block alloc
  * @rbm: The current position in the resource group
  * @ip: The inode for which we are searching for blocks
- * @minext: The minimum extent length
+ * @minlen: The minimum extent length
  * @maxext: A pointer to the maximum extent structure
  *
  * This checks the current position in the rgrp to see whether there is
@@ -1667,7 +1668,7 @@ static u64 gfs2_next_unreserved_block(struct gfs2_rgrpd *rgd, u64 block,
 
 static int gfs2_reservation_check_and_update(struct gfs2_rbm *rbm,
 					     const struct gfs2_inode *ip,
-					     u32 minext,
+					     u32 minlen,
 					     struct gfs2_extent *maxext)
 {
 	u64 block = gfs2_rbm_to_block(rbm);
@@ -1679,8 +1680,8 @@ static int gfs2_reservation_check_and_update(struct gfs2_rbm *rbm,
 	 * If we have a minimum extent length, then skip over any extent
 	 * which is less than the min extent length in size.
 	 */
-	if (minext) {
-		extlen = gfs2_free_extlen(rbm, minext);
+	if (minlen) {
+		extlen = gfs2_free_extlen(rbm, minlen);
 		if (extlen <= maxext->len)
 			goto fail;
 	}
@@ -1691,7 +1692,7 @@ static int gfs2_reservation_check_and_update(struct gfs2_rbm *rbm,
 	 */
 	nblock = gfs2_next_unreserved_block(rbm->rgd, block, extlen, ip);
 	if (nblock == block) {
-		if (!minext || extlen >= minext)
+		if (!minlen || extlen >= minlen)
 			return 0;
 
 		if (extlen > maxext->len) {
@@ -1711,7 +1712,7 @@ static int gfs2_reservation_check_and_update(struct gfs2_rbm *rbm,
  * gfs2_rbm_find - Look for blocks of a particular state
  * @rbm: Value/result starting position and final position
  * @state: The state which we want to find
- * @minext: Pointer to the requested extent length (NULL for a single block)
+ * @minlen: Pointer to the requested extent length (NULL for a single block)
  *          This is updated to be the actual reservation size.
  * @ip: If set, check for reservations
  * @nowrap: Stop looking at the end of the rgrp, rather than wrapping
@@ -1726,7 +1727,7 @@ static int gfs2_reservation_check_and_update(struct gfs2_rbm *rbm,
  * Returns: 0 on success, -ENOSPC if there is no block of the requested state
  */
 
-static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minext,
+static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minlen,
 			 const struct gfs2_inode *ip, bool nowrap)
 {
 	struct buffer_head *bh;
@@ -1772,7 +1773,7 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minext,
 
 		initial_bii = rbm->bii;
 		ret = gfs2_reservation_check_and_update(rbm, ip,
-							minext ? *minext : 0,
+							minlen ? *minlen : 0,
 							&maxext);
 		if (ret == 0)
 			return 0;
@@ -1802,21 +1803,21 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minext,
 			break;
 	}
 
-	if (minext == NULL || state != GFS2_BLKST_FREE)
+	if (minlen == NULL || state != GFS2_BLKST_FREE)
 		return -ENOSPC;
 
 	/* If the extent was too small, and it's smaller than the smallest
 	   to have failed before, remember for future reference that it's
 	   useless to search this rgrp again for this amount or more. */
 	if ((first_offset == 0) && (first_bii == 0) &&
-	    (*minext < rbm->rgd->rd_extfail_pt))
-		rbm->rgd->rd_extfail_pt = *minext;
+	    (*minlen < rbm->rgd->rd_extfail_pt))
+		rbm->rgd->rd_extfail_pt = *minlen;
 
 	/* If the maximum extent we found is big enough to fulfill the
 	   minimum requirements, use it anyway. */
 	if (maxext.len) {
 		*rbm = maxext.rbm;
-		*minext = maxext.len;
+		*minlen = maxext.len;
 		return 0;
 	}
 
-- 
2.20.1

[Cluster-devel] [PATCH 03/11] gfs2: Don't use struct gfs2_rbm in struct gfs2_extent

[Andreas Gruenbacher]

Don't use struct gfs2_rbm in struct gfs2_extent: the extent will always
be in the same resource group, so there is no need to track the resource
group separately.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/rgrp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index cfef8cc5fa155..2ce46e372b020 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -61,7 +61,8 @@
  */
 
 struct gfs2_extent {
-	struct gfs2_rbm rbm;
+	int bii;
+	u32 offset;
 	u32 len;
 };
 
@@ -1696,8 +1697,9 @@ static int gfs2_reservation_check_and_update(struct gfs2_rbm *rbm,
 			return 0;
 
 		if (extlen > maxext->len) {
+			maxext->bii = rbm->bii;
+			maxext->offset = rbm->offset;
 			maxext->len = extlen;
-			maxext->rbm = *rbm;
 		}
 fail:
 		nblock = block + extlen;
@@ -1740,7 +1742,7 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minlen,
 	int iters = rbm->rgd->rd_length;
 	int ret;
 	struct gfs2_bitmap *bi;
-	struct gfs2_extent maxext = { .rbm.rgd = rbm->rgd, };
+	struct gfs2_extent maxext = { };
 
 	/* If we are not starting at the beginning of a bitmap, then we
 	 * need to add one to the bitmap count to ensure that we search
@@ -1816,7 +1818,8 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minlen,
 	/* If the maximum extent we found is big enough to fulfill the
 	   minimum requirements, use it anyway. */
 	if (maxext.len) {
-		*rbm = maxext.rbm;
+		rbm->bii = maxext.bii;
+		rbm->offset = maxext.offset;
 		*minlen = maxext.len;
 		return 0;
 	}
-- 
2.20.1

[Cluster-devel] [PATCH 04/11] gfs2: Minor gfs2_free_extlen cleanup

[Andreas Gruenbacher]

Limit the n_unaligned argument of gfs2_unaligned_extlen to the number of
blocks to scan so that the len argument can never underrun.

Slightly simplify the unaligned block logic in gfs2_free_extlen.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/rgrp.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 2ce46e372b020..f06755dae951e 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -335,8 +335,6 @@ static bool gfs2_unaligned_extlen(struct gfs2_rbm *rbm, u32 n_unaligned, u32 *le
 		if (res != GFS2_BLKST_FREE)
 			return true;
 		(*len)--;
-		if (*len == 0)
-			return true;
 		if (gfs2_rbm_incr(rbm))
 			return true;
 	}
@@ -362,7 +360,6 @@ static bool gfs2_unaligned_extlen(struct gfs2_rbm *rbm, u32 n_unaligned, u32 *le
 static u32 gfs2_free_extlen(const struct gfs2_rbm *rrbm, u32 len)
 {
 	struct gfs2_rbm rbm = *rrbm;
-	u32 n_unaligned = rbm.offset & 3;
 	u32 size = len;
 	u32 bytes;
 	u32 chunk_size;
@@ -370,11 +367,14 @@ static u32 gfs2_free_extlen(const struct gfs2_rbm *rrbm, u32 len)
 	u64 block;
 	struct gfs2_bitmap *bi;
 
-	if (n_unaligned &&
-	    gfs2_unaligned_extlen(&rbm, 4 - n_unaligned, &len))
-		goto out;
+	/* Deal with unaligned bits at the front */
+	if (rbm.offset & 3) {
+		u32 n_unaligned = min(4 - (rbm.offset & 3), len);
+
+		if (gfs2_unaligned_extlen(&rbm, n_unaligned, &len))
+			goto out;
+	}
 
-	n_unaligned = len & 3;
 	/* Start is now byte aligned */
 	while (len > 3) {
 		bi = rbm_bi(&rbm);
@@ -392,20 +392,16 @@ static u32 gfs2_free_extlen(const struct gfs2_rbm *rrbm, u32 len)
 		BUG_ON(len < chunk_size);
 		len -= chunk_size;
 		block = gfs2_rbm_to_block(&rbm);
-		if (gfs2_rbm_from_block(&rbm, block + chunk_size)) {
-			n_unaligned = 0;
-			break;
-		}
-		if (ptr) {
-			n_unaligned = 3;
+		if (gfs2_rbm_from_block(&rbm, block + chunk_size))
+			goto out;
+		if (ptr)
 			break;
-		}
-		n_unaligned = len & 3;
 	}
 
 	/* Deal with any bits left over@the end */
-	if (n_unaligned)
-		gfs2_unaligned_extlen(&rbm, n_unaligned, &len);
+	if (len)
+		gfs2_unaligned_extlen(&rbm, len, &len);
+
 out:
 	return size - len;
 }
-- 
2.20.1

[Cluster-devel] [PATCH 05/11] gfs2: Only use struct gfs2_rbm for bitmap manipulations

[Andreas Gruenbacher]

GFS2 uses struct gfs2_rbm to represent a filesystem block number as a
bit position within a resource group.  This representation is used in
the bitmap manipulation code to prevent excessive conversions between
block numbers and bit positions, but also in struct gfs2_blkreserv which
is part of struct gfs2_inode, to mark the start of a reservation.  In
the inode, the bit position representation makes less sense: first, the
start position is used as a block number about as often as a bit
position; second, the bit position representation makes the code
unnecessarily complicated and difficult to read.

Change struct gfs2_blkreserv to represent the start of a reservation as
a block number instead of a bit position.  This requires keeping track
of the resource group in gfs2_blkreserv separately.  With that, struct
gfs2_rbm can be moved to rgrp.c.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/bmap.c       |   2 +-
 fs/gfs2/incore.h     |  30 +--------
 fs/gfs2/rgrp.c       | 153 +++++++++++++++++++++++++------------------
 fs/gfs2/trace_gfs2.h |  10 +--
 fs/gfs2/trans.h      |   2 +-
 5 files changed, 100 insertions(+), 97 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 02b2646d84b3a..e338182f74efd 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1498,7 +1498,7 @@ static int sweep_bh_for_rgrps(struct gfs2_inode *ip, struct gfs2_holder *rd_gh,
 
 			/* Must be done with the rgrp glock held: */
 			if (gfs2_rs_active(&ip->i_res) &&
-			    rgd == ip->i_res.rs_rbm.rgd)
+			    rgd == ip->i_res.rs_rgd)
 				gfs2_rs_deltree(&ip->i_res);
 		}
 
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index e10e0b0a7cd58..1dad2c54e7d8a 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -124,31 +124,6 @@ struct gfs2_rgrpd {
 	struct rb_root rd_rstree;       /* multi-block reservation tree */
 };
 
-struct gfs2_rbm {
-	struct gfs2_rgrpd *rgd;
-	u32 offset;		/* The offset is bitmap relative */
-	int bii;		/* Bitmap index */
-};
-
-static inline struct gfs2_bitmap *rbm_bi(const struct gfs2_rbm *rbm)
-{
-	return rbm->rgd->rd_bits + rbm->bii;
-}
-
-static inline u64 gfs2_rbm_to_block(const struct gfs2_rbm *rbm)
-{
-	BUG_ON(rbm->offset >= rbm->rgd->rd_data);
-	return rbm->rgd->rd_data0 + (rbm_bi(rbm)->bi_start * GFS2_NBBY) +
-		rbm->offset;
-}
-
-static inline bool gfs2_rbm_eq(const struct gfs2_rbm *rbm1,
-			       const struct gfs2_rbm *rbm2)
-{
-	return (rbm1->rgd == rbm2->rgd) && (rbm1->bii == rbm2->bii) &&
-	       (rbm1->offset == rbm2->offset);
-}
-
 enum gfs2_state_bits {
 	BH_Pinned = BH_PrivateStart,
 	BH_Escaped = BH_PrivateStart + 1,
@@ -308,8 +283,9 @@ struct gfs2_qadata { /* quota allocation data */
 */
 
 struct gfs2_blkreserv {
-	struct rb_node rs_node;       /* link to other block reservations */
-	struct gfs2_rbm rs_rbm;       /* Start of reservation */
+	struct rb_node rs_node;       /* node within rd_rstree */
+	struct gfs2_rgrpd *rs_rgd;
+	u64 rs_start;		      /* start of reservation */
 	u32 rs_free;                  /* how many blocks are still free */
 };
 
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index f06755dae951e..14ffa86b6b1c5 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -60,6 +60,12 @@
  * 3 = Used (metadata)
  */
 
+struct gfs2_rbm {
+	struct gfs2_rgrpd *rgd;
+	u32 offset;		/* The offset is bitmap relative */
+	int bii;		/* Bitmap index */
+};
+
 struct gfs2_extent {
 	int bii;
 	u32 offset;
@@ -74,6 +80,18 @@ static const char valid_change[16] = {
 	        1, 0, 0, 0
 };
 
+static inline struct gfs2_bitmap *rbm_bi(const struct gfs2_rbm *rbm)
+{
+	return rbm->rgd->rd_bits + rbm->bii;
+}
+
+static inline u64 gfs2_rbm_to_block(const struct gfs2_rbm *rbm)
+{
+	BUG_ON(rbm->offset >= rbm->rgd->rd_data);
+	return rbm->rgd->rd_data0 + (rbm_bi(rbm)->bi_start * GFS2_NBBY) +
+		rbm->offset;
+}
+
 static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minlen,
 			 const struct gfs2_inode *ip, bool nowrap);
 
@@ -189,7 +207,7 @@ static inline u64 gfs2_bit_search(const __le64 *ptr, u64 mask, u8 state)
 
 /**
  * rs_cmp - multi-block reservation range compare
- * @blk: absolute file system block number of the new reservation
+ * @start: start of the new reservation
  * @len: number of blocks in the new reservation
  * @rs: existing reservation to compare against
  *
@@ -197,13 +215,11 @@ static inline u64 gfs2_bit_search(const __le64 *ptr, u64 mask, u8 state)
  *         -1 if the block range is before the start of the reservation
  *          0 if the block range overlaps with the reservation
  */
-static inline int rs_cmp(u64 blk, u32 len, struct gfs2_blkreserv *rs)
+static inline int rs_cmp(u64 start, u32 len, struct gfs2_blkreserv *rs)
 {
-	u64 startblk = gfs2_rbm_to_block(&rs->rs_rbm);
-
-	if (blk >= startblk + rs->rs_free)
+	if (start >= rs->rs_start + rs->rs_free)
 		return 1;
-	if (blk + len - 1 < startblk)
+	if (rs->rs_start >= start + len)
 		return -1;
 	return 0;
 }
@@ -316,13 +332,28 @@ static bool gfs2_rbm_incr(struct gfs2_rbm *rbm)
 	return false;
 }
 
+static struct gfs2_bitmap *gfs2_block_to_bitmap(struct gfs2_rgrpd *rgd,
+						u64 block)
+{
+	unsigned int delta = (sizeof(struct gfs2_rgrp) -
+			      sizeof(struct gfs2_meta_header)) * GFS2_NBBY;
+	unsigned int rblock, bii;
+
+	if (!rgrp_contains_block(rgd, block))
+		return NULL;
+	rblock = block - rgd->rd_data0;
+	bii = (rblock + delta) / rgd->rd_sbd->sd_blocks_per_bitmap;
+	return rgd->rd_bits + bii;
+}
+
 /**
  * gfs2_unaligned_extlen - Look for free blocks which are not byte aligned
  * @rbm: Position to search (value/result)
  * @n_unaligned: Number of unaligned blocks to check
  * @len: Decremented for each block found (terminate on zero)
  *
- * Returns: true if a non-free block is encountered
+ * Returns: true if a non-free block is encountered or the end of the resource
+ *	    group is reached.
  */
 
 static bool gfs2_unaligned_extlen(struct gfs2_rbm *rbm, u32 n_unaligned, u32 *len)
@@ -614,10 +645,10 @@ static void dump_rs(struct seq_file *seq, const struct gfs2_blkreserv *rs)
 {
 	struct gfs2_inode *ip = container_of(rs, struct gfs2_inode, i_res);
 
-	gfs2_print_dbg(seq, "  B: n:%llu s:%llu b:%u f:%u\n",
+	gfs2_print_dbg(seq, "  B: n:%llu s:%llu f:%u\n",
 		       (unsigned long long)ip->i_no_addr,
-		       (unsigned long long)gfs2_rbm_to_block(&rs->rs_rbm),
-		       rs->rs_rbm.offset, rs->rs_free);
+		       (unsigned long long)rs->rs_start,
+		       rs->rs_free);
 }
 
 /**
@@ -632,30 +663,28 @@ static void __rs_deltree(struct gfs2_blkreserv *rs)
 	if (!gfs2_rs_active(rs))
 		return;
 
-	rgd = rs->rs_rbm.rgd;
+	rgd = rs->rs_rgd;
 	trace_gfs2_rs(rs, TRACE_RS_TREEDEL);
 	rb_erase(&rs->rs_node, &rgd->rd_rstree);
 	RB_CLEAR_NODE(&rs->rs_node);
 
 	if (rs->rs_free) {
-		u64 last_block = gfs2_rbm_to_block(&rs->rs_rbm) +
-				 rs->rs_free - 1;
-		struct gfs2_rbm last_rbm = { .rgd = rs->rs_rbm.rgd, };
+		u64 last_block = rs->rs_start + rs->rs_free - 1;
 		struct gfs2_bitmap *start, *last;
 
 		/* return reserved blocks to the rgrp */
-		BUG_ON(rs->rs_rbm.rgd->rd_reserved < rs->rs_free);
-		rs->rs_rbm.rgd->rd_reserved -= rs->rs_free;
+		BUG_ON(rs->rs_rgd->rd_reserved < rs->rs_free);
+		rs->rs_rgd->rd_reserved -= rs->rs_free;
 		/* The rgrp extent failure point is likely not to increase;
 		   it will only do so if the freed blocks are somehow
 		   contiguous with a span of free blocks that follows. Still,
 		   it will force the number to be recalculated later. */
 		rgd->rd_extfail_pt += rs->rs_free;
 		rs->rs_free = 0;
-		if (gfs2_rbm_from_block(&last_rbm, last_block))
+		start = gfs2_block_to_bitmap(rgd, rs->rs_start);
+		last = gfs2_block_to_bitmap(rgd, last_block);
+		if (!start || !last)
 			return;
-		start = rbm_bi(&rs->rs_rbm);
-		last = rbm_bi(&last_rbm);
 		do
 			clear_bit(GBF_FULL, &start->bi_flags);
 		while (start++ != last);
@@ -671,7 +700,7 @@ void gfs2_rs_deltree(struct gfs2_blkreserv *rs)
 {
 	struct gfs2_rgrpd *rgd;
 
-	rgd = rs->rs_rbm.rgd;
+	rgd = rs->rs_rgd;
 	if (rgd) {
 		spin_lock(&rgd->rd_rsspin);
 		__rs_deltree(rs);
@@ -1490,8 +1519,7 @@ static void rs_insert(struct gfs2_inode *ip)
 	struct rb_node **newn, *parent = NULL;
 	int rc;
 	struct gfs2_blkreserv *rs = &ip->i_res;
-	struct gfs2_rgrpd *rgd = rs->rs_rbm.rgd;
-	u64 fsblock = gfs2_rbm_to_block(&rs->rs_rbm);
+	struct gfs2_rgrpd *rgd = rs->rs_rgd;
 
 	BUG_ON(gfs2_rs_active(rs));
 
@@ -1502,7 +1530,7 @@ static void rs_insert(struct gfs2_inode *ip)
 			rb_entry(*newn, struct gfs2_blkreserv, rs_node);
 
 		parent = *newn;
-		rc = rs_cmp(fsblock, rs->rs_free, cur);
+		rc = rs_cmp(rs->rs_start, rs->rs_free, cur);
 		if (rc > 0)
 			newn = &((*newn)->rb_right);
 		else if (rc < 0)
@@ -1590,7 +1618,7 @@ static void rg_mblk_search(struct gfs2_rgrpd *rgd, struct gfs2_inode *ip,
 
 	ret = gfs2_rbm_find(&rbm, GFS2_BLKST_FREE, &extlen, ip, true);
 	if (ret == 0) {
-		rs->rs_rbm = rbm;
+		rs->rs_start = gfs2_rbm_to_block(&rbm);
 		rs->rs_free = extlen;
 		rs_insert(ip);
 	} else {
@@ -1635,7 +1663,7 @@ static u64 gfs2_next_unreserved_block(struct gfs2_rgrpd *rgd, u64 block,
 
 	if (n) {
 		while ((rs_cmp(block, length, rs) == 0) && (&ip->i_res != rs)) {
-			block = gfs2_rbm_to_block(&rs->rs_rbm) + rs->rs_free;
+			block = rs->rs_start + rs->rs_free;
 			n = n->rb_right;
 			if (n == NULL)
 				break;
@@ -1975,7 +2003,7 @@ static bool gfs2_rgrp_used_recently(const struct gfs2_blkreserv *rs,
 	u64 tdiff;
 
 	tdiff = ktime_to_ns(ktime_sub(ktime_get_real(),
-                            rs->rs_rbm.rgd->rd_gl->gl_dstamp));
+                            rs->rs_rgd->rd_gl->gl_dstamp));
 
 	return tdiff > (msecs * 1000 * 1000);
 }
@@ -2053,45 +2081,45 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, struct gfs2_alloc_parms *ap)
 	if (gfs2_assert_warn(sdp, ap->target))
 		return -EINVAL;
 	if (gfs2_rs_active(rs)) {
-		begin = rs->rs_rbm.rgd;
-	} else if (rs->rs_rbm.rgd &&
-		   rgrp_contains_block(rs->rs_rbm.rgd, ip->i_goal)) {
-		begin = rs->rs_rbm.rgd;
+		begin = rs->rs_rgd;
+	} else if (rs->rs_rgd &&
+		   rgrp_contains_block(rs->rs_rgd, ip->i_goal)) {
+		begin = rs->rs_rgd;
 	} else {
 		check_and_update_goal(ip);
-		rs->rs_rbm.rgd = begin = gfs2_blk2rgrpd(sdp, ip->i_goal, 1);
+		rs->rs_rgd = begin = gfs2_blk2rgrpd(sdp, ip->i_goal, 1);
 	}
 	if (S_ISDIR(ip->i_inode.i_mode) && (ap->aflags & GFS2_AF_ORLOV))
 		skip = gfs2_orlov_skip(ip);
-	if (rs->rs_rbm.rgd == NULL)
+	if (rs->rs_rgd == NULL)
 		return -EBADSLT;
 
 	while (loops < 3) {
 		rg_locked = 1;
 
-		if (!gfs2_glock_is_locked_by_me(rs->rs_rbm.rgd->rd_gl)) {
+		if (!gfs2_glock_is_locked_by_me(rs->rs_rgd->rd_gl)) {
 			rg_locked = 0;
 			if (skip && skip--)
 				goto next_rgrp;
 			if (!gfs2_rs_active(rs)) {
 				if (loops == 0 &&
-				    !fast_to_acquire(rs->rs_rbm.rgd))
+				    !fast_to_acquire(rs->rs_rgd))
 					goto next_rgrp;
 				if ((loops < 2) &&
 				    gfs2_rgrp_used_recently(rs, 1000) &&
-				    gfs2_rgrp_congested(rs->rs_rbm.rgd, loops))
+				    gfs2_rgrp_congested(rs->rs_rgd, loops))
 					goto next_rgrp;
 			}
-			error = gfs2_glock_nq_init(rs->rs_rbm.rgd->rd_gl,
+			error = gfs2_glock_nq_init(rs->rs_rgd->rd_gl,
 						   LM_ST_EXCLUSIVE, flags,
 						   &ip->i_rgd_gh);
 			if (unlikely(error))
 				return error;
 			if (!gfs2_rs_active(rs) && (loops < 2) &&
-			    gfs2_rgrp_congested(rs->rs_rbm.rgd, loops))
+			    gfs2_rgrp_congested(rs->rs_rgd, loops))
 				goto skip_rgrp;
 			if (sdp->sd_args.ar_rgrplvb) {
-				error = update_rgrp_lvb(rs->rs_rbm.rgd);
+				error = update_rgrp_lvb(rs->rs_rgd);
 				if (unlikely(error)) {
 					gfs2_glock_dq_uninit(&ip->i_rgd_gh);
 					return error;
@@ -2100,24 +2128,24 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, struct gfs2_alloc_parms *ap)
 		}
 
 		/* Skip unusable resource groups */
-		if ((rs->rs_rbm.rgd->rd_flags & (GFS2_RGF_NOALLOC |
+		if ((rs->rs_rgd->rd_flags & (GFS2_RGF_NOALLOC |
 						 GFS2_RDF_ERROR)) ||
-		    (loops == 0 && ap->target > rs->rs_rbm.rgd->rd_extfail_pt))
+		    (loops == 0 && ap->target > rs->rs_rgd->rd_extfail_pt))
 			goto skip_rgrp;
 
 		if (sdp->sd_args.ar_rgrplvb)
-			gfs2_rgrp_bh_get(rs->rs_rbm.rgd);
+			gfs2_rgrp_bh_get(rs->rs_rgd);
 
 		/* Get a reservation if we don't already have one */
 		if (!gfs2_rs_active(rs))
-			rg_mblk_search(rs->rs_rbm.rgd, ip, ap);
+			rg_mblk_search(rs->rs_rgd, ip, ap);
 
 		/* Skip rgrps when we can't get a reservation on first pass */
 		if (!gfs2_rs_active(rs) && (loops < 1))
 			goto check_rgrp;
 
 		/* If rgrp has enough free space, use it */
-		free_blocks = rgd_free(rs->rs_rbm.rgd, rs);
+		free_blocks = rgd_free(rs->rs_rgd, rs);
 		if (free_blocks >= ap->target ||
 		    (loops == 2 && ap->min_target &&
 		     free_blocks >= ap->min_target)) {
@@ -2126,8 +2154,8 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, struct gfs2_alloc_parms *ap)
 		}
 check_rgrp:
 		/* Check for unlinked inodes which can be reclaimed */
-		if (rs->rs_rbm.rgd->rd_flags & GFS2_RDF_CHECK)
-			try_rgrp_unlink(rs->rs_rbm.rgd, &last_unlinked,
+		if (rs->rs_rgd->rd_flags & GFS2_RDF_CHECK)
+			try_rgrp_unlink(rs->rs_rgd, &last_unlinked,
 					ip->i_no_addr);
 skip_rgrp:
 		/* Drop reservation, if we couldn't use reserved rgrp */
@@ -2139,7 +2167,7 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, struct gfs2_alloc_parms *ap)
 			gfs2_glock_dq_uninit(&ip->i_rgd_gh);
 next_rgrp:
 		/* Find the next rgrp, and continue looking */
-		if (gfs2_select_rgrp(&rs->rs_rbm.rgd, begin))
+		if (gfs2_select_rgrp(&rs->rs_rgd, begin))
 			continue;
 		if (skip)
 			continue;
@@ -2307,20 +2335,21 @@ static void gfs2_adjust_reservation(struct gfs2_inode *ip,
 {
 	struct gfs2_blkreserv *rs = &ip->i_res;
 	struct gfs2_rgrpd *rgd = rbm->rgd;
-	unsigned rlen;
-	u64 block;
-	int ret;
 
 	spin_lock(&rgd->rd_rsspin);
 	if (gfs2_rs_active(rs)) {
-		if (gfs2_rbm_eq(&rs->rs_rbm, rbm)) {
-			block = gfs2_rbm_to_block(rbm);
-			ret = gfs2_rbm_from_block(&rs->rs_rbm, block + len);
+		u64 start = gfs2_rbm_to_block(rbm);
+
+		if (rs->rs_start == start) {
+			unsigned int rlen;
+
+			rs->rs_start += len;
 			rlen = min(rs->rs_free, len);
 			rs->rs_free -= rlen;
 			rgd->rd_reserved -= rlen;
 			trace_gfs2_rs(rs, TRACE_RS_CLAIM);
-			if (rs->rs_free && !ret)
+			if (rs->rs_start < rgd->rd_data0 + rgd->rd_data &&
+			    rs->rs_free)
 				goto out;
 			/* We used up our block reservation, so we should
 			   reserve more blocks next time. */
@@ -2349,15 +2378,13 @@ static void gfs2_set_alloc_start(struct gfs2_rbm *rbm,
 	u64 goal;
 
 	if (gfs2_rs_active(&ip->i_res)) {
-		*rbm = ip->i_res.rs_rbm;
-		return;
+		goal = ip->i_res.rs_start;
+	} else {
+		if (!dinode && rgrp_contains_block(rbm->rgd, ip->i_goal))
+			goal = ip->i_goal;
+		else
+			goal = rbm->rgd->rd_last_alloc + rbm->rgd->rd_data0;
 	}
-
-	if (!dinode && rgrp_contains_block(rbm->rgd, ip->i_goal))
-		goal = ip->i_goal;
-	else
-		goal = rbm->rgd->rd_last_alloc + rbm->rgd->rd_data0;
-
 	if (WARN_ON_ONCE(gfs2_rbm_from_block(rbm, goal))) {
 		rbm->bii = 0;
 		rbm->offset = 0;
@@ -2380,7 +2407,7 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *nblocks,
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct buffer_head *dibh;
-	struct gfs2_rbm rbm = { .rgd = ip->i_res.rs_rbm.rgd, };
+	struct gfs2_rbm rbm = { .rgd = ip->i_res.rs_rgd, };
 	unsigned int ndata;
 	u64 block; /* block, within the file system scope */
 	int error;
@@ -2612,7 +2639,7 @@ void gfs2_rlist_add(struct gfs2_inode *ip, struct gfs2_rgrp_list *rlist,
 			return;
 		rgd = gfs2_blk2rgrpd(sdp, block, 1);
 	} else {
-		rgd = ip->i_res.rs_rbm.rgd;
+		rgd = ip->i_res.rs_rgd;
 		if (!rgd || !rgrp_contains_block(rgd, block))
 			rgd = gfs2_blk2rgrpd(sdp, block, 1);
 	}
diff --git a/fs/gfs2/trace_gfs2.h b/fs/gfs2/trace_gfs2.h
index e0025258107a8..7586c7629497f 100644
--- a/fs/gfs2/trace_gfs2.h
+++ b/fs/gfs2/trace_gfs2.h
@@ -602,13 +602,13 @@ TRACE_EVENT(gfs2_rs,
 	),
 
 	TP_fast_assign(
-		__entry->dev		= rs->rs_rbm.rgd->rd_sbd->sd_vfs->s_dev;
-		__entry->rd_addr	= rs->rs_rbm.rgd->rd_addr;
-		__entry->rd_free_clone	= rs->rs_rbm.rgd->rd_free_clone;
-		__entry->rd_reserved	= rs->rs_rbm.rgd->rd_reserved;
+		__entry->dev		= rs->rs_rgd->rd_sbd->sd_vfs->s_dev;
+		__entry->rd_addr	= rs->rs_rgd->rd_addr;
+		__entry->rd_free_clone	= rs->rs_rgd->rd_free_clone;
+		__entry->rd_reserved	= rs->rs_rgd->rd_reserved;
 		__entry->inum		= container_of(rs, struct gfs2_inode,
 						       i_res)->i_no_addr;
-		__entry->start		= gfs2_rbm_to_block(&rs->rs_rbm);
+		__entry->start		= rs->rs_start;
 		__entry->free		= rs->rs_free;
 		__entry->func		= func;
 	),
diff --git a/fs/gfs2/trans.h b/fs/gfs2/trans.h
index ad70087d05971..a27078f98e7a9 100644
--- a/fs/gfs2/trans.h
+++ b/fs/gfs2/trans.h
@@ -30,7 +30,7 @@ struct gfs2_glock;
  * block, or all of the blocks in the rg, whichever is smaller */
 static inline unsigned int gfs2_rg_blocks(const struct gfs2_inode *ip, unsigned requested)
 {
-	struct gfs2_rgrpd *rgd = ip->i_res.rs_rbm.rgd;
+	struct gfs2_rgrpd *rgd = ip->i_res.rs_rgd;
 
 	if (requested < rgd->rd_length)
 		return requested + 1;
-- 
2.20.1

[Cluster-devel] [PATCH 06/11] gfs2: Minor gfs2_adjust_reservation and gfs2_alloc_extent cleanups

[Andreas Gruenbacher]

Pass the resource group and starting position to gfs2_adjust_reservation
directly instead of passing an rbm.  With this change, gfs2_alloc_blocks
no longer needs the rbm after calling gfs2_alloc_extent, so
gfs2_alloc_extent can modify the rbm instead of creating a copy.

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

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 14ffa86b6b1c5..7a93568fdc709 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -2207,34 +2207,32 @@ void gfs2_inplace_release(struct gfs2_inode *ip)
 
 /**
  * gfs2_alloc_extent - allocate an extent from a given bitmap
- * @rbm: the resource group information
+ * @rbm: The position in the resource group
  * @dinode: TRUE if the first block we allocate is for a dinode
  * @n: The extent length (value/result)
  *
+ * Side effects:
+ * - Modifies rbm.
+ *
  * Add the bitmap buffer to the transaction.
  * Set the found bits to @new_state to change block's allocation state.
  */
-static void gfs2_alloc_extent(const struct gfs2_rbm *rbm, bool dinode,
+static void gfs2_alloc_extent(struct gfs2_rbm *rbm, bool dinode,
 			     unsigned int *n)
 {
-	struct gfs2_rbm pos = { .rgd = rbm->rgd, };
 	const unsigned int elen = *n;
-	u64 block;
 	int ret;
 
 	*n = 1;
-	block = gfs2_rbm_to_block(rbm);
 	gfs2_trans_add_meta(rbm->rgd->rd_gl, rbm_bi(rbm)->bi_bh);
 	gfs2_setbit(rbm, true, dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
-	block++;
 	while (*n < elen) {
-		ret = gfs2_rbm_from_block(&pos, block);
-		if (ret || gfs2_testbit(&pos, true) != GFS2_BLKST_FREE)
+		ret = gfs2_rbm_incr(rbm);
+		if (ret || gfs2_testbit(rbm, true) != GFS2_BLKST_FREE)
 			break;
-		gfs2_trans_add_meta(pos.rgd->rd_gl, rbm_bi(&pos)->bi_bh);
-		gfs2_setbit(&pos, true, GFS2_BLKST_USED);
+		gfs2_trans_add_meta(rbm->rgd->rd_gl, rbm_bi(rbm)->bi_bh);
+		gfs2_setbit(rbm, true, GFS2_BLKST_USED);
 		(*n)++;
-		block++;
 	}
 }
 
@@ -2322,7 +2320,8 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
 /**
  * gfs2_adjust_reservation - Adjust (or remove) a reservation after allocation
  * @ip: The inode we have just allocated blocks for
- * @rbm: The start of the allocated blocks
+ * @rgd: The resource group
+ * @start: The start of the reservation
  * @len: The extent length
  *
  * Adjusts a reservation after an allocation has taken place. If the
@@ -2331,15 +2330,13 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
  */
 
 static void gfs2_adjust_reservation(struct gfs2_inode *ip,
-				    const struct gfs2_rbm *rbm, unsigned len)
+				    struct gfs2_rgrpd *rgd,
+				    u64 start, unsigned len)
 {
 	struct gfs2_blkreserv *rs = &ip->i_res;
-	struct gfs2_rgrpd *rgd = rbm->rgd;
 
 	spin_lock(&rgd->rd_rsspin);
 	if (gfs2_rs_active(rs)) {
-		u64 start = gfs2_rbm_to_block(rbm);
-
 		if (rs->rs_start == start) {
 			unsigned int rlen;
 
@@ -2429,11 +2426,11 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *nblocks,
 		goto rgrp_error;
 	}
 
-	gfs2_alloc_extent(&rbm, dinode, nblocks);
 	block = gfs2_rbm_to_block(&rbm);
+	gfs2_alloc_extent(&rbm, dinode, nblocks);
 	rbm.rgd->rd_last_alloc = block - rbm.rgd->rd_data0;
 	if (gfs2_rs_active(&ip->i_res))
-		gfs2_adjust_reservation(ip, &rbm, *nblocks);
+		gfs2_adjust_reservation(ip, rbm.rgd, block, *nblocks);
 	ndata = *nblocks;
 	if (dinode)
 		ndata--;
-- 
2.20.1

[Cluster-devel] [PATCH 07/11] gfs2: Minor gfs2_rgrp_send_discards cleanup

[Andreas Gruenbacher]

Pass the resource group and bitmap index to gfs2_rgrp_send_discards
separately: one of the next patches will remove bi_bh from struct
gfs2_bitmap, so we'll need to get it from the resource group in the
future.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/lops.c |  3 ++-
 fs/gfs2/rgrp.c | 11 +++++------
 fs/gfs2/rgrp.h |  3 ++-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index 94dcab655bc02..2db478768b229 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -81,7 +81,8 @@ static void maybe_release_space(struct gfs2_bufdata *bd)
 	if (bi->bi_clone == NULL)
 		return;
 	if (sdp->sd_args.ar_discard)
-		gfs2_rgrp_send_discards(sdp, rgd->rd_data0, bd->bd_bh, bi, 1, NULL);
+		gfs2_rgrp_send_discards(sdp, rgd->rd_data0, bd->bd_bh, rgd,
+					index, 1, NULL);
 	memcpy(bi->bi_clone + bi->bi_offset,
 	       bd->bd_bh->b_data + bi->bi_offset, bi->bi_bytes);
 	clear_bit(GBF_FULL, &bi->bi_flags);
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 7a93568fdc709..a86358afb33e7 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1343,10 +1343,10 @@ void gfs2_rgrp_go_unlock(struct gfs2_holder *gh)
 }
 
 int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
-			     struct buffer_head *bh,
-			     const struct gfs2_bitmap *bi, unsigned minlen,
-			     u64 *ptrimmed)
+			    struct buffer_head *bh, struct gfs2_rgrpd *rgd,
+			    unsigned int bii, unsigned minlen, u64 *ptrimmed)
 {
+	struct gfs2_bitmap *bi = rgd->rd_bits + bii;
 	struct super_block *sb = sdp->sd_vfs;
 	u64 blk;
 	sector_t start = 0;
@@ -1472,10 +1472,9 @@ int gfs2_fitrim(struct file *filp, void __user *argp)
 		if (!(rgd->rd_flags & GFS2_RGF_TRIMMED)) {
 			/* Trim each bitmap in the rgrp */
 			for (x = 0; x < rgd->rd_length; x++) {
-				struct gfs2_bitmap *bi = rgd->rd_bits + x;
 				ret = gfs2_rgrp_send_discards(sdp,
-						rgd->rd_data0, NULL, bi, minlen,
-						&amt);
+						rgd->rd_data0, NULL, rgd, x,
+						minlen, &amt);
 				if (ret) {
 					gfs2_glock_dq_uninit(&gh);
 					goto out;
diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
index 499079a9dbbed..4af09a548c358 100644
--- a/fs/gfs2/rgrp.h
+++ b/fs/gfs2/rgrp.h
@@ -75,7 +75,8 @@ extern u64 gfs2_ri_total(struct gfs2_sbd *sdp);
 extern void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_glock *gl);
 extern int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
 				   struct buffer_head *bh,
-				   const struct gfs2_bitmap *bi, unsigned minlen, u64 *ptrimmed);
+				   struct gfs2_rgrpd *rgd, unsigned int bii,
+				   unsigned minlen, u64 *ptrimmed);
 extern int gfs2_fitrim(struct file *filp, void __user *argp);
 
 /* This is how to tell if a reservation is in the rgrp tree: */
-- 
2.20.1

[Cluster-devel] [PATCH 08/11] gfs2: Add buffer head to struct gfs2_rgrpd

[Andreas Gruenbacher]

Prepare for removing the buffer head from struct gfs2_bitmap by adding a
buffer head to struct gfs2_rgrpd for the resource group header.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/incore.h |  1 +
 fs/gfs2/rgrp.c   | 46 +++++++++++++++++++++++++++-------------------
 2 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index 1dad2c54e7d8a..51eb41484e9af 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -99,6 +99,7 @@ struct gfs2_bitmap {
 struct gfs2_rgrpd {
 	struct rb_node rd_node;		/* Link with superblock */
 	struct gfs2_glock *rd_gl;	/* Glock for this rgrp */
+	struct buffer_head *rd_bh;
 	u64 rd_addr;			/* grp block disk address */
 	u64 rd_data0;			/* first data location */
 	u32 rd_length;			/* length of rgrp header in fs blocks */
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index a86358afb33e7..339d6b064f1fc 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1139,7 +1139,7 @@ static void gfs2_rgrp_out(struct gfs2_rgrpd *rgd, void *buf)
 static int gfs2_rgrp_lvb_valid(struct gfs2_rgrpd *rgd)
 {
 	struct gfs2_rgrp_lvb *rgl = rgd->rd_rgl;
-	struct gfs2_rgrp *str = (struct gfs2_rgrp *)rgd->rd_bits[0].bi_bh->b_data;
+	struct gfs2_rgrp *str = (struct gfs2_rgrp *)rgd->rd_bh->b_data;
 	int valid = 1;
 
 	if (rgl->rl_flags != str->rg_flags) {
@@ -1212,12 +1212,20 @@ static int gfs2_rgrp_bh_get(struct gfs2_rgrpd *rgd)
 	struct gfs2_glock *gl = rgd->rd_gl;
 	unsigned int length = rgd->rd_length;
 	struct gfs2_bitmap *bi;
-	unsigned int x, y;
+	unsigned int x = 0, y;
 	int error;
 
-	if (rgd->rd_bits[0].bi_bh != NULL)
+	if (rgd->rd_bh != NULL)
 		return 0;
 
+	error = gfs2_meta_read(gl, rgd->rd_addr, DIO_WAIT, 0, &rgd->rd_bh);
+	if (error)
+		return error;
+	if (gfs2_metatype_check(sdp, rgd->rd_bh, GFS2_METATYPE_RG)) {
+		error = -EIO;
+		goto fail;
+	}
+
 	for (x = 0; x < length; x++) {
 		bi = rgd->rd_bits + x;
 		error = gfs2_meta_read(gl, rgd->rd_addr + x, 0, 0, &bi->bi_bh);
@@ -1240,7 +1248,7 @@ static int gfs2_rgrp_bh_get(struct gfs2_rgrpd *rgd)
 	if (!(rgd->rd_flags & GFS2_RDF_UPTODATE)) {
 		for (x = 0; x < length; x++)
 			clear_bit(GBF_FULL, &rgd->rd_bits[x].bi_flags);
-		gfs2_rgrp_in(rgd, (rgd->rd_bits[0].bi_bh)->b_data);
+		gfs2_rgrp_in(rgd, rgd->rd_bh->b_data);
 		rgd->rd_flags |= (GFS2_RDF_UPTODATE | GFS2_RDF_CHECK);
 		rgd->rd_free_clone = rgd->rd_free;
 		/* max out the rgrp allocation failure point */
@@ -1248,8 +1256,7 @@ static int gfs2_rgrp_bh_get(struct gfs2_rgrpd *rgd)
 	}
 	if (cpu_to_be32(GFS2_MAGIC) != rgd->rd_rgl->rl_magic) {
 		rgd->rd_rgl->rl_unlinked = cpu_to_be32(count_unlinked(rgd));
-		gfs2_rgrp_ondisk2lvb(rgd->rd_rgl,
-				     rgd->rd_bits[0].bi_bh->b_data);
+		gfs2_rgrp_ondisk2lvb(rgd->rd_rgl, rgd->rd_bh->b_data);
 	}
 	else if (sdp->sd_args.ar_rgrplvb) {
 		if (!gfs2_rgrp_lvb_valid(rgd)){
@@ -1269,6 +1276,8 @@ static int gfs2_rgrp_bh_get(struct gfs2_rgrpd *rgd)
 		bi->bi_bh = NULL;
 		gfs2_assert_warn(sdp, !bi->bi_clone);
 	}
+	brelse(rgd->rd_bh);
+	rgd->rd_bh = NULL;
 
 	return error;
 }
@@ -1323,7 +1332,8 @@ void gfs2_rgrp_brelse(struct gfs2_rgrpd *rgd)
 			bi->bi_bh = NULL;
 		}
 	}
-
+	brelse(rgd->rd_bh);
+	rgd->rd_bh = NULL;
 }
 
 /**
@@ -1423,7 +1433,6 @@ int gfs2_fitrim(struct file *filp, void __user *argp)
 	struct inode *inode = file_inode(filp);
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	struct request_queue *q = bdev_get_queue(sdp->sd_vfs->s_bdev);
-	struct buffer_head *bh;
 	struct gfs2_rgrpd *rgd;
 	struct gfs2_rgrpd *rgd_end;
 	struct gfs2_holder gh;
@@ -1485,10 +1494,9 @@ int gfs2_fitrim(struct file *filp, void __user *argp)
 			/* Mark rgrp as having been trimmed */
 			ret = gfs2_trans_begin(sdp, RES_RG_HDR, 0);
 			if (ret == 0) {
-				bh = rgd->rd_bits[0].bi_bh;
 				rgd->rd_flags |= GFS2_RGF_TRIMMED;
-				gfs2_trans_add_meta(rgd->rd_gl, bh);
-				gfs2_rgrp_out(rgd, bh->b_data);
+				gfs2_trans_add_meta(rgd->rd_gl, rgd->rd_bh);
+				gfs2_rgrp_out(rgd, rgd->rd_bh->b_data);
 				gfs2_trans_end(sdp);
 			}
 		}
@@ -2459,8 +2467,8 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *nblocks,
 			*generation = rbm.rgd->rd_igeneration++;
 	}
 
-	gfs2_trans_add_meta(rbm.rgd->rd_gl, rbm.rgd->rd_bits[0].bi_bh);
-	gfs2_rgrp_out(rbm.rgd, rbm.rgd->rd_bits[0].bi_bh->b_data);
+	gfs2_trans_add_meta(rbm.rgd->rd_gl, rbm.rgd->rd_bh);
+	gfs2_rgrp_out(rbm.rgd, rbm.rgd->rd_bh->b_data);
 
 	gfs2_statfs_change(sdp, 0, -(s64)*nblocks, dinode ? 1 : 0);
 	if (dinode)
@@ -2498,8 +2506,8 @@ void __gfs2_free_blocks(struct gfs2_inode *ip, struct gfs2_rgrpd *rgd,
 	trace_gfs2_block_alloc(ip, rgd, bstart, blen, GFS2_BLKST_FREE);
 	rgd->rd_free += blen;
 	rgd->rd_flags &= ~GFS2_RGF_TRIMMED;
-	gfs2_trans_add_meta(rgd->rd_gl, rgd->rd_bits[0].bi_bh);
-	gfs2_rgrp_out(rgd, rgd->rd_bits[0].bi_bh->b_data);
+	gfs2_trans_add_meta(rgd->rd_gl, rgd->rd_bh);
+	gfs2_rgrp_out(rgd, rgd->rd_bh->b_data);
 
 	/* Directories keep their data in the metadata address space */
 	if (meta || ip->i_depth)
@@ -2537,8 +2545,8 @@ void gfs2_unlink_di(struct inode *inode)
 		return;
 	rgblk_free(sdp, rgd, blkno, 1, GFS2_BLKST_UNLINKED);
 	trace_gfs2_block_alloc(ip, rgd, blkno, 1, GFS2_BLKST_UNLINKED);
-	gfs2_trans_add_meta(rgd->rd_gl, rgd->rd_bits[0].bi_bh);
-	gfs2_rgrp_out(rgd, rgd->rd_bits[0].bi_bh->b_data);
+	gfs2_trans_add_meta(rgd->rd_gl, rgd->rd_bh);
+	gfs2_rgrp_out(rgd, rgd->rd_bh->b_data);
 	be32_add_cpu(&rgd->rd_rgl->rl_unlinked, 1);
 }
 
@@ -2552,8 +2560,8 @@ void gfs2_free_di(struct gfs2_rgrpd *rgd, struct gfs2_inode *ip)
 	rgd->rd_dinodes--;
 	rgd->rd_free++;
 
-	gfs2_trans_add_meta(rgd->rd_gl, rgd->rd_bits[0].bi_bh);
-	gfs2_rgrp_out(rgd, rgd->rd_bits[0].bi_bh->b_data);
+	gfs2_trans_add_meta(rgd->rd_gl, rgd->rd_bh);
+	gfs2_rgrp_out(rgd, rgd->rd_bh->b_data);
 	be32_add_cpu(&rgd->rd_rgl->rl_unlinked, -1);
 
 	gfs2_statfs_change(sdp, 0, +1, -1);
-- 
2.20.1

[Cluster-devel] [PATCH 09/11] gfs2: Read bitmap buffers on demand

[Andreas Gruenbacher]

Before this patch, when locking a resource group, gfs2 would read in the
resource group header and all the bitmap buffers of the resource group.
Those buffers would then be locked into memory until the resource group
is unlocked, which will happen when the filesystem is unmounted or when
transferring the resource group lock to another node, but not due to
memory pressure.  Larger resource groups lock more buffers into memory,
and cause more unnecessary I/O when resource group locks are transferred
between nodes.

With this patch, when locking a resource group, only the resource group
header is read in.  The other bitmap buffers (the resource group header
contains part of the bitmap) are only read in on demand.

It would probably make sense to also only read in the resource group
header on demand, when the resource group is modified, but since the
header contains the number of free blocks in the resource group, there
is a higher incentive to keep the header locked in memory after that.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/incore.h |   1 -
 fs/gfs2/rgrp.c   | 382 ++++++++++++++++++++++++++++++-----------------
 2 files changed, 241 insertions(+), 142 deletions(-)

diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index 51eb41484e9af..d39c26b950121 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -87,7 +87,6 @@ struct gfs2_log_operations {
  * be reallocated in that same transaction.
  */
 struct gfs2_bitmap {
-	struct buffer_head *bi_bh;
 	char *bi_clone;
 	unsigned long bi_flags;
 	u32 bi_offset;
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 339d6b064f1fc..503ea6f18ed74 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -62,6 +62,7 @@
 
 struct gfs2_rbm {
 	struct gfs2_rgrpd *rgd;
+	struct buffer_head *bh;
 	u32 offset;		/* The offset is bitmap relative */
 	int bii;		/* Bitmap index */
 };
@@ -112,8 +113,8 @@ static inline void gfs2_setbit(const struct gfs2_rbm *rbm, bool do_clone,
 	unsigned int buflen = bi->bi_bytes;
 	const unsigned int bit = (rbm->offset % GFS2_NBBY) * GFS2_BIT_SIZE;
 
-	byte1 = bi->bi_bh->b_data + bi->bi_offset + (rbm->offset / GFS2_NBBY);
-	end = bi->bi_bh->b_data + bi->bi_offset + buflen;
+	byte1 = rbm->bh->b_data + bi->bi_offset + (rbm->offset / GFS2_NBBY);
+	end = rbm->bh->b_data + bi->bi_offset + buflen;
 
 	BUG_ON(byte1 >= end);
 
@@ -126,7 +127,7 @@ static inline void gfs2_setbit(const struct gfs2_rbm *rbm, bool do_clone,
 			rbm->offset, cur_state, new_state);
 		fs_warn(sdp, "rgrp=0x%llx bi_start=0x%x biblk: 0x%llx\n",
 			(unsigned long long)rbm->rgd->rd_addr, bi->bi_start,
-			(unsigned long long)bi->bi_bh->b_blocknr);
+			(unsigned long long)rbm->bh->b_blocknr);
 		fs_warn(sdp, "bi_offset=0x%x bi_bytes=0x%x block=0x%llx\n",
 			bi->bi_offset, bi->bi_bytes,
 			(unsigned long long)gfs2_rbm_to_block(rbm));
@@ -164,7 +165,7 @@ static inline u8 gfs2_testbit(const struct gfs2_rbm *rbm, bool use_clone)
 	if (use_clone && bi->bi_clone)
 		buffer = bi->bi_clone;
 	else
-		buffer = bi->bi_bh->b_data;
+		buffer = rbm->bh->b_data;
 	buffer += bi->bi_offset;
 	byte = buffer + (rbm->offset / GFS2_NBBY);
 	bit = (rbm->offset % GFS2_NBBY) * GFS2_BIT_SIZE;
@@ -276,62 +277,152 @@ static u32 gfs2_bitfit(const u8 *buf, const unsigned int len,
 }
 
 /**
- * gfs2_rbm_from_block - Set the rbm based upon rgd and block number
- * @rbm: The rbm with rgd already set correctly
+ * __gfs2_rbm_get - Get the buffer head of @rbm
+ * @rbm: The rbm
+ *
+ * Get the buffer head of the bitmap block the rbm points at.
+ *
+ * Returns: 0 on success, or an error code
+ */
+static int __gfs2_rbm_get(struct gfs2_rbm *rbm) {
+	struct gfs2_rgrpd *rgd = rbm->rgd;
+	struct buffer_head *bh;
+	int error;
+
+	if (rbm->bii == 0) {
+		rbm->bh = rgd->rd_bh;
+		return 0;
+	}
+
+	/* FIXME: Might want to do read-ahead here. */
+	error = gfs2_meta_read(rgd->rd_gl, rgd->rd_addr + rbm->bii,
+			       DIO_WAIT, 0, &bh);
+	if (error)
+		goto out;
+	if (gfs2_metatype_check(rgd->rd_sbd, bh, GFS2_METATYPE_RB)) {
+		brelse(bh);
+		error = -EIO;
+		goto out;
+	}
+	rbm->bh = bh;
+out:
+	return error;
+}
+
+/**
+ * gfs2_rbm_get - Set up @rbm to point at @block
+ * @rbm: The rbm
+ * @rgd: The resource group of @block
  * @block: The block number (filesystem relative)
  *
- * This sets the bi and offset members of an rbm based on a
- * resource group and a filesystem relative block number. The
- * resource group must be set in the rbm on entry, the bi and
- * offset members will be set by this function.
+ * This sets the bii and offset of an rbm based on a resource group and a
+ * filesystem relative block number.  The rbm may hold a reference to the
+ * bitmap buffer it points to, and must be put with gfs2_rbm_put.
  *
  * Returns: 0 on success, or an error code
  */
 
-static int gfs2_rbm_from_block(struct gfs2_rbm *rbm, u64 block)
+static int gfs2_rbm_get(struct gfs2_rbm *rbm, struct gfs2_rgrpd *rgd, u64 block)
 {
-	if (!rgrp_contains_block(rbm->rgd, block))
+	if (!rgrp_contains_block(rgd, block))
 		return -E2BIG;
+
+	rbm->rgd = rgd;
 	rbm->bii = 0;
-	rbm->offset = block - rbm->rgd->rd_data0;
+	rbm->offset = block - rgd->rd_data0;
 	/* Check if the block is within the first block */
-	if (rbm->offset < rbm_bi(rbm)->bi_blocks)
-		return 0;
+	if (rbm->offset >= rgd->rd_bits[0].bi_blocks) {
+		u32 blocks_per_bitmap = rgd->rd_sbd->sd_blocks_per_bitmap;
 
-	/* Adjust for the size diff between gfs2_meta_header and gfs2_rgrp */
-	rbm->offset += (sizeof(struct gfs2_rgrp) -
-			sizeof(struct gfs2_meta_header)) * GFS2_NBBY;
-	rbm->bii = rbm->offset / rbm->rgd->rd_sbd->sd_blocks_per_bitmap;
-	rbm->offset -= rbm->bii * rbm->rgd->rd_sbd->sd_blocks_per_bitmap;
-	return 0;
+		/* Adjust for the size diff between gfs2_meta_header and gfs2_rgrp */
+		rbm->offset += (sizeof(struct gfs2_rgrp) -
+				sizeof(struct gfs2_meta_header)) * GFS2_NBBY;
+		rbm->bii = rbm->offset / blocks_per_bitmap;
+		rbm->offset = rbm->offset % blocks_per_bitmap;
+	}
+	return __gfs2_rbm_get(rbm);
 }
 
 /**
- * gfs2_rbm_incr - increment an rbm structure
- * @rbm: The rbm with rgd already set correctly
+ * gfs2_rbm_put - Put an rbm
+ * @rbm: The rbm
  *
- * This function takes an existing rbm structure and increments it to the next
- * viable block offset.
+ * If @rbm holds a reference on a bitmap buffer, drop that reference.
+ */
+static void gfs2_rbm_put(struct gfs2_rbm *rbm)
+{
+	if (rbm->bh != rbm->rgd->rd_bh)
+		brelse(rbm->bh);
+	rbm->bh = NULL;
+}
+
+/**
+ * gfs2_rbm_seek - Seek to a specific bitmap and offset
+ * @rbm: The rbm
+ * @bii: The bitmap index
+ * @offset: Offset within the bitmap
  *
- * Returns: If incrementing the offset would cause the rbm to go past the
- *          end of the rgrp, true is returned, otherwise false.
+ * Seek to a specific bitmap and offset in @rbm.
  *
+ * Returns: 0 on success, or an error code
  */
+static int gfs2_rbm_seek(struct gfs2_rbm *rbm, unsigned int bii, u32 offset)
+{
+	rbm->offset = offset;
+	if (rbm->bii == bii)
+		return 0;
+	gfs2_rbm_put(rbm);
+	rbm->bii = bii;
+	return __gfs2_rbm_get(rbm);
+}
 
-static bool gfs2_rbm_incr(struct gfs2_rbm *rbm)
+/**
+ * gfs2_rbm_skip - seek ahead in an rbm structure
+ * @rbm: The rbm
+ * @skip: The number of bytes to seek ahead
+ *
+ * Returns: If incrementing the offset would cause the rbm to go past the
+ *          end of the rgrp, 1 is returned.  Otherwise, 0 or an error code is
+ *          returned.
+ */
+static int gfs2_rbm_skip(struct gfs2_rbm *rbm, unsigned int skip)
 {
-	if (rbm->offset + 1 < rbm_bi(rbm)->bi_blocks) { /* in the same bitmap */
-		rbm->offset++;
-		return false;
+	struct gfs2_bitmap *bi = &rbm->rgd->rd_bits[rbm->bii];
+	u32 rest = bi->bi_blocks - rbm->offset;
+	int bii;
+
+	if (skip < rest) {
+		/* in the same bitmap */
+		rbm->offset += skip;
+		return 0;
 	}
-	if (rbm->bii == rbm->rgd->rd_length - 1) /* at the last bitmap */
-		return true;
 
-	rbm->offset = 0;
-	rbm->bii++;
-	return false;
+	skip -= rest;
+	for (bii = rbm->bii + 1, bi++;
+	     bii < rbm->rgd->rd_length;
+	     bii++, bi++) {
+		if (skip < bi->bi_blocks)
+			return gfs2_rbm_seek(rbm, bii, skip);
+		skip -= bi->bi_blocks;
+	}
+	return 1;
 }
 
+/**
+ * gfs2_rbm_clone - Make a copy of an rbm
+ * @to: The rbm to initialize
+ * @from: The rbm to copy from
+ *
+ * Make an independent copy of @from.  The copy must be put with gfs2_rbm_put.
+ */
+static void gfs2_rbm_clone(struct gfs2_rbm *to, const struct gfs2_rbm *from)
+{
+	memcpy(to, from, sizeof(*to));
+	if (to->bh != to->rgd->rd_bh)
+		get_bh(to->bh);
+}
+
+
 static struct gfs2_bitmap *gfs2_block_to_bitmap(struct gfs2_rgrpd *rgd,
 						u64 block)
 {
@@ -366,7 +457,7 @@ static bool gfs2_unaligned_extlen(struct gfs2_rbm *rbm, u32 n_unaligned, u32 *le
 		if (res != GFS2_BLKST_FREE)
 			return true;
 		(*len)--;
-		if (gfs2_rbm_incr(rbm))
+		if (gfs2_rbm_skip(rbm, 1))
 			return true;
 	}
 
@@ -390,14 +481,15 @@ static bool gfs2_unaligned_extlen(struct gfs2_rbm *rbm, u32 n_unaligned, u32 *le
 
 static u32 gfs2_free_extlen(const struct gfs2_rbm *rrbm, u32 len)
 {
-	struct gfs2_rbm rbm = *rrbm;
+	struct gfs2_rbm rbm;
 	u32 size = len;
 	u32 bytes;
 	u32 chunk_size;
 	u8 *ptr, *start, *end;
-	u64 block;
 	struct gfs2_bitmap *bi;
 
+	gfs2_rbm_clone(&rbm, rrbm);
+
 	/* Deal with unaligned bits at the front */
 	if (rbm.offset & 3) {
 		u32 n_unaligned = min(4 - (rbm.offset & 3), len);
@@ -409,7 +501,7 @@ static u32 gfs2_free_extlen(const struct gfs2_rbm *rrbm, u32 len)
 	/* Start is now byte aligned */
 	while (len > 3) {
 		bi = rbm_bi(&rbm);
-		start = bi->bi_bh->b_data;
+		start = rbm.bh->b_data;
 		if (bi->bi_clone)
 			start = bi->bi_clone;
 		start += bi->bi_offset;
@@ -422,8 +514,7 @@ static u32 gfs2_free_extlen(const struct gfs2_rbm *rrbm, u32 len)
 		chunk_size *= GFS2_NBBY;
 		BUG_ON(len < chunk_size);
 		len -= chunk_size;
-		block = gfs2_rbm_to_block(&rbm);
-		if (gfs2_rbm_from_block(&rbm, block + chunk_size))
+		if (gfs2_rbm_skip(&rbm, chunk_size))
 			goto out;
 		if (ptr)
 			break;
@@ -434,6 +525,7 @@ static u32 gfs2_free_extlen(const struct gfs2_rbm *rrbm, u32 len)
 		gfs2_unaligned_extlen(&rbm, len, &len);
 
 out:
+	gfs2_rbm_put(&rbm);
 	return size - len;
 }
 
@@ -480,6 +572,7 @@ static u32 gfs2_bitcount(struct gfs2_rgrpd *rgd, const u8 *buffer,
 void gfs2_rgrp_verify(struct gfs2_rgrpd *rgd)
 {
 	struct gfs2_sbd *sdp = rgd->rd_sbd;
+	struct gfs2_rbm rbm;
 	struct gfs2_bitmap *bi = NULL;
 	u32 length = rgd->rd_length;
 	u32 count[4], tmp;
@@ -487,16 +580,23 @@ void gfs2_rgrp_verify(struct gfs2_rgrpd *rgd)
 
 	memset(count, 0, 4 * sizeof(u32));
 
+	if (WARN_ON_ONCE(gfs2_rbm_get(&rbm, rgd, rgd->rd_data0)))
+		return;
+
 	/* Count # blocks in each of 4 possible allocation states */
 	for (buf = 0; buf < length; buf++) {
 		bi = rgd->rd_bits + buf;
 		for (x = 0; x < 4; x++)
 			count[x] += gfs2_bitcount(rgd,
-						  bi->bi_bh->b_data +
+						  rbm.bh->b_data +
 						  bi->bi_offset,
 						  bi->bi_bytes, x);
+		if (WARN_ON_ONCE(gfs2_rbm_skip(&rbm, bi->bi_blocks) < 0))
+			return;
 	}
 
+	gfs2_rbm_put(&rbm);
+
 	if (count[0] != rgd->rd_free) {
 		if (gfs2_consist_rgrpd(rgd))
 			fs_err(sdp, "free data mismatch:  %u != %u\n",
@@ -1173,15 +1273,18 @@ static int gfs2_rgrp_lvb_valid(struct gfs2_rgrpd *rgd)
 
 static u32 count_unlinked(struct gfs2_rgrpd *rgd)
 {
+	struct gfs2_rbm rbm;
 	struct gfs2_bitmap *bi;
 	const u32 length = rgd->rd_length;
 	const u8 *buffer = NULL;
 	u32 i, goal, count = 0;
 
+	if (WARN_ON_ONCE(gfs2_rbm_get(&rbm, rgd, rgd->rd_data0)))
+		goto out;
 	for (i = 0, bi = rgd->rd_bits; i < length; i++, bi++) {
 		goal = 0;
-		buffer = bi->bi_bh->b_data + bi->bi_offset;
-		WARN_ON(!buffer_uptodate(bi->bi_bh));
+		buffer = rbm.bh->b_data + bi->bi_offset;
+		WARN_ON(!buffer_uptodate(rbm.bh));
 		while (goal < bi->bi_blocks) {
 			goal = gfs2_bitfit(buffer, bi->bi_bytes, goal,
 					   GFS2_BLKST_UNLINKED);
@@ -1190,18 +1293,23 @@ static u32 count_unlinked(struct gfs2_rgrpd *rgd)
 			count++;
 			goal++;
 		}
+
+		if (WARN_ON_ONCE(gfs2_rbm_skip(&rbm, bi->bi_blocks) < 0))
+			break;
 	}
+	gfs2_rbm_put(&rbm);
 
+out:
 	return count;
 }
 
 
 /**
- * gfs2_rgrp_bh_get - Read in a RG's header and bitmaps
+ * gfs2_rgrp_bh_get - Read in a RG's header
  * @rgd: the struct gfs2_rgrpd describing the RG to read in
  *
- * Read in all of a Resource Group's header and bitmap blocks.
- * Caller must eventually call gfs2_rgrp_brelse() to free the bitmaps.
+ * Read in a Resource Group's header.
+ * Caller must eventually call gfs2_rgrp_brelse.
  *
  * Returns: errno
  */
@@ -1211,8 +1319,7 @@ static int gfs2_rgrp_bh_get(struct gfs2_rgrpd *rgd)
 	struct gfs2_sbd *sdp = rgd->rd_sbd;
 	struct gfs2_glock *gl = rgd->rd_gl;
 	unsigned int length = rgd->rd_length;
-	struct gfs2_bitmap *bi;
-	unsigned int x = 0, y;
+	unsigned int x;
 	int error;
 
 	if (rgd->rd_bh != NULL)
@@ -1226,25 +1333,6 @@ static int gfs2_rgrp_bh_get(struct gfs2_rgrpd *rgd)
 		goto fail;
 	}
 
-	for (x = 0; x < length; x++) {
-		bi = rgd->rd_bits + x;
-		error = gfs2_meta_read(gl, rgd->rd_addr + x, 0, 0, &bi->bi_bh);
-		if (error)
-			goto fail;
-	}
-
-	for (y = length; y--;) {
-		bi = rgd->rd_bits + y;
-		error = gfs2_meta_wait(sdp, bi->bi_bh);
-		if (error)
-			goto fail;
-		if (gfs2_metatype_check(sdp, bi->bi_bh, y ? GFS2_METATYPE_RB :
-					      GFS2_METATYPE_RG)) {
-			error = -EIO;
-			goto fail;
-		}
-	}
-
 	if (!(rgd->rd_flags & GFS2_RDF_UPTODATE)) {
 		for (x = 0; x < length; x++)
 			clear_bit(GBF_FULL, &rgd->rd_bits[x].bi_flags);
@@ -1270,12 +1358,6 @@ static int gfs2_rgrp_bh_get(struct gfs2_rgrpd *rgd)
 	return 0;
 
 fail:
-	while (x--) {
-		bi = rgd->rd_bits + x;
-		brelse(bi->bi_bh);
-		bi->bi_bh = NULL;
-		gfs2_assert_warn(sdp, !bi->bi_clone);
-	}
 	brelse(rgd->rd_bh);
 	rgd->rd_bh = NULL;
 
@@ -1316,22 +1398,13 @@ int gfs2_rgrp_go_lock(struct gfs2_holder *gh)
 }
 
 /**
- * gfs2_rgrp_brelse - Release RG bitmaps read in with gfs2_rgrp_bh_get()
+ * gfs2_rgrp_brelse - Release RG header read in with gfs2_rgrp_bh_get()
  * @rgd: The resource group
  *
  */
 
 void gfs2_rgrp_brelse(struct gfs2_rgrpd *rgd)
 {
-	int x, length = rgd->rd_length;
-
-	for (x = 0; x < length; x++) {
-		struct gfs2_bitmap *bi = rgd->rd_bits + x;
-		if (bi->bi_bh) {
-			brelse(bi->bi_bh);
-			bi->bi_bh = NULL;
-		}
-	}
 	brelse(rgd->rd_bh);
 	rgd->rd_bh = NULL;
 }
@@ -1356,7 +1429,11 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
 			    struct buffer_head *bh, struct gfs2_rgrpd *rgd,
 			    unsigned int bii, unsigned minlen, u64 *ptrimmed)
 {
-	struct gfs2_bitmap *bi = rgd->rd_bits + bii;
+	struct gfs2_rbm rbm = {
+		.rgd = rgd,
+		.bii = bii,
+	};
+	struct gfs2_bitmap *bi;
 	struct super_block *sb = sdp->sd_vfs;
 	u64 blk;
 	sector_t start = 0;
@@ -1366,8 +1443,13 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
 	u32 trimmed = 0;
 	u8 diff;
 
+	rv = __gfs2_rbm_get(&rbm);
+	if (rv)
+		goto fail;
+	bi = rbm_bi(&rbm);
+
 	for (x = 0; x < bi->bi_bytes; x++) {
-		const u8 *clone = bi->bi_clone ? bi->bi_clone : bi->bi_bh->b_data;
+		const u8 *clone = bi->bi_clone ? bi->bi_clone : rbm.bh->b_data;
 		clone += bi->bi_offset;
 		clone += x;
 		if (bh) {
@@ -1411,13 +1493,16 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
 	}
 	if (ptrimmed)
 		*ptrimmed = trimmed;
-	return 0;
+
+out:
+	gfs2_rbm_put(&rbm);
+	return rv;
 
 fail:
 	if (sdp->sd_args.ar_discard)
 		fs_warn(sdp, "error %d on discard request, turning discards off for this filesystem\n", rv);
 	sdp->sd_args.ar_discard = 0;
-	return -EIO;
+	goto out;
 }
 
 /**
@@ -1597,7 +1682,7 @@ static inline u32 rgd_free(struct gfs2_rgrpd *rgd, struct gfs2_blkreserv *rs)
 static void rg_mblk_search(struct gfs2_rgrpd *rgd, struct gfs2_inode *ip,
 			   const struct gfs2_alloc_parms *ap)
 {
-	struct gfs2_rbm rbm = { .rgd = rgd, };
+	struct gfs2_rbm rbm;
 	u64 goal;
 	struct gfs2_blkreserv *rs = &ip->i_res;
 	u32 extlen;
@@ -1620,7 +1705,7 @@ static void rg_mblk_search(struct gfs2_rgrpd *rgd, struct gfs2_inode *ip,
 	else
 		goal = rgd->rd_last_alloc + rgd->rd_data0;
 
-	if (WARN_ON(gfs2_rbm_from_block(&rbm, goal)))
+	if (WARN_ON(gfs2_rbm_get(&rbm, rgd, goal)))
 		return;
 
 	ret = gfs2_rbm_find(&rbm, GFS2_BLKST_FREE, &extlen, ip, true);
@@ -1632,6 +1717,8 @@ static void rg_mblk_search(struct gfs2_rgrpd *rgd, struct gfs2_inode *ip,
 		if (goal == rgd->rd_last_alloc + rgd->rd_data0)
 			rgd->rd_last_alloc = 0;
 	}
+
+	gfs2_rbm_put(&rbm);
 }
 
 /**
@@ -1735,9 +1822,11 @@ static int gfs2_reservation_check_and_update(struct gfs2_rbm *rbm,
 fail:
 		nblock = block + extlen;
 	}
-	ret = gfs2_rbm_from_block(rbm, nblock);
+	ret = gfs2_rbm_skip(rbm, nblock - block);
 	if (ret < 0)
 		return ret;
+	if (ret > 0)
+		return -E2BIG;
 	return 1;
 }
 
@@ -1764,7 +1853,7 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minlen,
 			 const struct gfs2_inode *ip, bool nowrap)
 {
 	struct buffer_head *bh;
-	int initial_bii;
+	int initial_bii, bii;
 	int first_bii = rbm->bii;
 	u32 first_offset = rbm->offset;
 	u32 offset;
@@ -1789,7 +1878,7 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minlen,
 		    (state == GFS2_BLKST_FREE))
 			goto next_bitmap;
 
-		bh = bi->bi_bh;
+		bh = rbm->bh;
 		buffer = bh->b_data + bi->bi_offset;
 		WARN_ON(!buffer_uptodate(bh));
 		if (state != GFS2_BLKST_UNLINKED && bi->bi_clone)
@@ -1811,25 +1900,26 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minlen,
 		if (ret == 0)
 			return 0;
 		if (ret > 0) {
-			n += (rbm->bii - initial_bii);
+			n += rbm->bii - initial_bii;
 			goto next_iter;
 		}
 		if (ret == -E2BIG) {
 			n += rbm->bii - initial_bii;
-			rbm->bii = 0;
-			rbm->offset = 0;
-			goto res_covered_end_of_rgrp;
+			bii = 0;
+			goto this_bitmap;
 		}
 		return ret;
 
 next_bitmap:	/* Find next bitmap in the rgrp */
-		rbm->offset = 0;
-		rbm->bii++;
-		if (rbm->bii == rbm->rgd->rd_length)
-			rbm->bii = 0;
-res_covered_end_of_rgrp:
-		if ((rbm->bii == 0) && nowrap)
+		bii = rbm->bii + 1;
+		if (bii == rbm->rgd->rd_length)
+			bii = 0;
+this_bitmap:
+		if (bii == 0 && nowrap)
 			break;
+		ret = gfs2_rbm_seek(rbm, bii, 0);
+		if (ret)
+			return ret;
 		n++;
 next_iter:
 		if (n >= iters)
@@ -1849,10 +1939,8 @@ static int gfs2_rbm_find(struct gfs2_rbm *rbm, u8 state, u32 *minlen,
 	/* If the maximum extent we found is big enough to fulfill the
 	   minimum requirements, use it anyway. */
 	if (maxext.len) {
-		rbm->bii = maxext.bii;
-		rbm->offset = maxext.offset;
 		*minlen = maxext.len;
-		return 0;
+		return gfs2_rbm_seek(rbm, maxext.bii, maxext.offset);
 	}
 
 	return -ENOSPC;
@@ -1876,7 +1964,11 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
 	struct gfs2_inode *ip;
 	int error;
 	int found = 0;
-	struct gfs2_rbm rbm = { .rgd = rgd, .bii = 0, .offset = 0 };
+	struct gfs2_rbm rbm;
+
+	error = gfs2_rbm_get(&rbm, rgd, rgd->rd_data0);
+	if (error)
+		return;
 
 	while (1) {
 		down_write(&sdp->sd_log_flush_lock);
@@ -1889,17 +1981,15 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
 			break;
 
 		block = gfs2_rbm_to_block(&rbm);
-		if (gfs2_rbm_from_block(&rbm, block + 1))
-			break;
 		if (*last_unlinked != NO_BLOCK && block <= *last_unlinked)
-			continue;
+			goto next;
 		if (block == skip)
-			continue;
+			goto next;
 		*last_unlinked = block;
 
 		error = gfs2_glock_get(sdp, block, &gfs2_iopen_glops, CREATE, &gl);
 		if (error)
-			continue;
+			goto next;
 
 		/* If the inode is already in cache, we can ignore it here
 		 * because the existing inode disposal code will deal with
@@ -1917,11 +2007,17 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
 
 		/* Limit reclaim to sensible number of tasks */
 		if (found > NR_CPUS)
-			return;
+			goto out;
+
+next:
+		if (gfs2_rbm_skip(&rbm, 1))
+			break;
 	}
 
 	rgd->rd_flags &= ~GFS2_RDF_CHECK;
-	return;
+
+out:
+	gfs2_rbm_put(&rbm);
 }
 
 /**
@@ -2231,13 +2327,13 @@ static void gfs2_alloc_extent(struct gfs2_rbm *rbm, bool dinode,
 	int ret;
 
 	*n = 1;
-	gfs2_trans_add_meta(rbm->rgd->rd_gl, rbm_bi(rbm)->bi_bh);
+	gfs2_trans_add_meta(rbm->rgd->rd_gl, rbm->bh);
 	gfs2_setbit(rbm, true, dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
 	while (*n < elen) {
-		ret = gfs2_rbm_incr(rbm);
+		ret = gfs2_rbm_skip(rbm, 1);
 		if (ret || gfs2_testbit(rbm, true) != GFS2_BLKST_FREE)
 			break;
-		gfs2_trans_add_meta(rbm->rgd->rd_gl, rbm_bi(rbm)->bi_bh);
+		gfs2_trans_add_meta(rbm->rgd->rd_gl, rbm->bh);
 		gfs2_setbit(rbm, true, GFS2_BLKST_USED);
 		(*n)++;
 	}
@@ -2258,25 +2354,27 @@ static void rgblk_free(struct gfs2_sbd *sdp, struct gfs2_rgrpd *rgd,
 	struct gfs2_rbm rbm;
 	struct gfs2_bitmap *bi, *bi_prev = NULL;
 
-	rbm.rgd = rgd;
-	if (WARN_ON_ONCE(gfs2_rbm_from_block(&rbm, bstart)))
+	if (WARN_ON_ONCE(gfs2_rbm_get(&rbm, rgd, bstart)))
 		return;
+
 	while (blen--) {
 		bi = rbm_bi(&rbm);
 		if (bi != bi_prev) {
 			if (!bi->bi_clone) {
-				bi->bi_clone = kmalloc(bi->bi_bh->b_size,
+				bi->bi_clone = kmalloc(rbm.bh->b_size,
 						      GFP_NOFS | __GFP_NOFAIL);
 				memcpy(bi->bi_clone + bi->bi_offset,
-				       bi->bi_bh->b_data + bi->bi_offset,
+				       rbm.bh->b_data + bi->bi_offset,
 				       bi->bi_bytes);
 			}
-			gfs2_trans_add_meta(rbm.rgd->rd_gl, bi->bi_bh);
+			gfs2_trans_add_meta(rbm.rgd->rd_gl, rbm.bh);
 			bi_prev = bi;
 		}
 		gfs2_setbit(&rbm, false, new_state);
-		gfs2_rbm_incr(&rbm);
+		gfs2_rbm_skip(&rbm, 1);
 	}
+
+	gfs2_rbm_put(&rbm);
 }
 
 /**
@@ -2376,22 +2474,17 @@ static void gfs2_adjust_reservation(struct gfs2_inode *ip,
  * inode's goal block or the last allocation point in the rgrp.
  */
 
-static void gfs2_set_alloc_start(struct gfs2_rbm *rbm,
-				 const struct gfs2_inode *ip, bool dinode)
+static int gfs2_alloc_goal(const struct gfs2_inode *ip, bool dinode)
 {
-	u64 goal;
+	struct gfs2_rgrpd *rgd = ip->i_res.rs_rgd;
 
 	if (gfs2_rs_active(&ip->i_res)) {
-		goal = ip->i_res.rs_start;
+		return ip->i_res.rs_start;
 	} else {
-		if (!dinode && rgrp_contains_block(rbm->rgd, ip->i_goal))
-			goal = ip->i_goal;
+		if (!dinode && rgrp_contains_block(rgd, ip->i_goal))
+			return ip->i_goal;
 		else
-			goal = rbm->rgd->rd_last_alloc + rbm->rgd->rd_data0;
-	}
-	if (WARN_ON_ONCE(gfs2_rbm_from_block(rbm, goal))) {
-		rbm->bii = 0;
-		rbm->offset = 0;
+			return rgd->rd_last_alloc + rgd->rd_data0;
 	}
 }
 
@@ -2411,16 +2504,21 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *nblocks,
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct buffer_head *dibh;
-	struct gfs2_rbm rbm = { .rgd = ip->i_res.rs_rgd, };
+	struct gfs2_rbm rbm;
 	unsigned int ndata;
+	u64 goal;
 	u64 block; /* block, within the file system scope */
 	int error;
 
-	gfs2_set_alloc_start(&rbm, ip, dinode);
+	goal = gfs2_alloc_goal(ip, dinode);
+	if (WARN_ON_ONCE(gfs2_rbm_get(&rbm, ip->i_res.rs_rgd, goal)))
+		return -EIO;
 	error = gfs2_rbm_find(&rbm, GFS2_BLKST_FREE, NULL, ip, false);
-
 	if (error == -ENOSPC) {
-		gfs2_set_alloc_start(&rbm, ip, dinode);
+		gfs2_rbm_put(&rbm);
+		goal = gfs2_alloc_goal(ip, dinode);
+		if (WARN_ON_ONCE(gfs2_rbm_get(&rbm, ip->i_res.rs_rgd, goal)))
+			return -EIO;
 		error = gfs2_rbm_find(&rbm, GFS2_BLKST_FREE, NULL, NULL, false);
 	}
 
@@ -2480,10 +2578,12 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *nblocks,
 	trace_gfs2_block_alloc(ip, rbm.rgd, block, *nblocks,
 			       dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
 	*bn = block;
+	gfs2_rbm_put(&rbm);
 	return 0;
 
 rgrp_error:
 	gfs2_rgrp_error(rbm.rgd);
+	gfs2_rbm_put(&rbm);
 	return -EIO;
 }
 
@@ -2596,8 +2696,7 @@ int gfs2_check_blk_type(struct gfs2_sbd *sdp, u64 no_addr, unsigned int type)
 	if (error)
 		goto fail;
 
-	rbm.rgd = rgd;
-	error = gfs2_rbm_from_block(&rbm, no_addr);
+	error = gfs2_rbm_get(&rbm, rgd, no_addr);
 	if (WARN_ON_ONCE(error))
 		goto fail;
 
@@ -2605,6 +2704,7 @@ int gfs2_check_blk_type(struct gfs2_sbd *sdp, u64 no_addr, unsigned int type)
 		error = -ESTALE;
 
 	gfs2_glock_dq_uninit(&rgd_gh);
+	gfs2_rbm_put(&rbm);
 fail:
 	return error;
 }
-- 
2.20.1

[Cluster-devel] [PATCH 09/11] gfs2: Read bitmap buffers on demand

[Bob Peterson]

----- Original Message -----
> Before this patch, when locking a resource group, gfs2 would read in the
> resource group header and all the bitmap buffers of the resource group.
> Those buffers would then be locked into memory until the resource group
> is unlocked, which will happen when the filesystem is unmounted or when
> transferring the resource group lock to another node, but not due to
> memory pressure.  Larger resource groups lock more buffers into memory,
> and cause more unnecessary I/O when resource group locks are transferred
> between nodes.
> 
> With this patch, when locking a resource group, only the resource group
> header is read in.  The other bitmap buffers (the resource group header
> contains part of the bitmap) are only read in on demand.
> 
> It would probably make sense to also only read in the resource group
> header on demand, when the resource group is modified, but since the
> header contains the number of free blocks in the resource group, there
> is a higher incentive to keep the header locked in memory after that.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---

Hi,

This patch looks good. However, I'm concerned about its performance,
at least until we get some bitmap read-ahead in place.
(see "/* FIXME: Might want to do read-ahead here. */")

In particular, I'm concerned that it does the exact opposite of this
performance enhancement:

https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/fs/gfs2?id=39b0f1e9290880a6c905f639e7db6b646e302a4f

So we need to test that 2GB rgrps (33 bitmap blocks) that are fragmented
don't have a disproportionate slowdown compared to, say, 128MB rgrps
because we end up spending so much time fetching pages out of page cache,
only to find them inadequate and doing brelse() again. And by fragmented,
I mean all 33 bitmap blocks have a small number of free blocks, thus
disqualifying them from having "full" status but still having a large
enough number of free blocks that we might consider searching them.
Some of the patches we did after that one may have mitigated the problem
to some extent, but we need to be sure we're not regressing performance.

Regards,

Bob Peterson

[Cluster-devel] [PATCH 09/11] gfs2: Read bitmap buffers on demand

[Andreas Gruenbacher]

Bob,

On Mon, 14 Jan 2019 at 18:12, Bob Peterson <rpeterso@redhat.com> wrote:
> ----- Original Message -----
> > Before this patch, when locking a resource group, gfs2 would read in the
> > resource group header and all the bitmap buffers of the resource group.
> > Those buffers would then be locked into memory until the resource group
> > is unlocked, which will happen when the filesystem is unmounted or when
> > transferring the resource group lock to another node, but not due to
> > memory pressure.  Larger resource groups lock more buffers into memory,
> > and cause more unnecessary I/O when resource group locks are transferred
> > between nodes.
> >
> > With this patch, when locking a resource group, only the resource group
> > header is read in.  The other bitmap buffers (the resource group header
> > contains part of the bitmap) are only read in on demand.
> >
> > It would probably make sense to also only read in the resource group
> > header on demand, when the resource group is modified, but since the
> > header contains the number of free blocks in the resource group, there
> > is a higher incentive to keep the header locked in memory after that.
> >
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > ---
>
> Hi,
>
> This patch looks good. However, I'm concerned about its performance,
> at least until we get some bitmap read-ahead in place.
> (see "/* FIXME: Might want to do read-ahead here. */")
>
> In particular, I'm concerned that it does the exact opposite of this
> performance enhancement:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/fs/gfs2?id=39b0f1e9290880a6c905f639e7db6b646e302a4f
>
> So we need to test that 2GB rgrps (33 bitmap blocks) that are fragmented
> don't have a disproportionate slowdown compared to, say, 128MB rgrps
> because we end up spending so much time fetching pages out of page cache,
> only to find them inadequate and doing brelse() again. And by fragmented,
> I mean all 33 bitmap blocks have a small number of free blocks, thus
> disqualifying them from having "full" status but still having a large
> enough number of free blocks that we might consider searching them.
> Some of the patches we did after that one may have mitigated the problem
> to some extent, but we need to be sure we're not regressing performance.

Yes, there are a few ways this patch can still be tuned depending on
the performance impact. This should speed up "normal" workloads by
requiring fewer reads from disk when acquiring a resource group glock,
and less memory is pinned for filesystem metadata, but we do end up
with more page cache lookups during allocations, and depending on
read-ahead, possibly more smaller reads at inconvenient times.

rd_last_alloc should help limit the performance impact when bitmaps
become mostly full. (Forms of rd_last_alloc have been in gfs2 since
the mainline merge.)

One way to further reduce the number of page cache lookups would be to
cache the buffer head of the last allocation, but it's unclear if this
is even an issue, so let's do some performance testing first.

Thanks,
Andreas

[Cluster-devel] [PATCH 09/11] gfs2: Read bitmap buffers on demand

[Andreas Gruenbacher]

On Mon, 14 Jan 2019 at 19:41, Andreas Gruenbacher <agruenba@redhat.com> wrote:
>
> Bob,
>
> On Mon, 14 Jan 2019 at 18:12, Bob Peterson <rpeterso@redhat.com> wrote:
> > ----- Original Message -----
> > > Before this patch, when locking a resource group, gfs2 would read in the
> > > resource group header and all the bitmap buffers of the resource group.
> > > Those buffers would then be locked into memory until the resource group
> > > is unlocked, which will happen when the filesystem is unmounted or when
> > > transferring the resource group lock to another node, but not due to
> > > memory pressure.  Larger resource groups lock more buffers into memory,
> > > and cause more unnecessary I/O when resource group locks are transferred
> > > between nodes.
> > >
> > > With this patch, when locking a resource group, only the resource group
> > > header is read in.  The other bitmap buffers (the resource group header
> > > contains part of the bitmap) are only read in on demand.
> > >
> > > It would probably make sense to also only read in the resource group
> > > header on demand, when the resource group is modified, but since the
> > > header contains the number of free blocks in the resource group, there
> > > is a higher incentive to keep the header locked in memory after that.
> > >
> > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > > ---
> >
> > Hi,
> >
> > This patch looks good. However, I'm concerned about its performance,
> > at least until we get some bitmap read-ahead in place.
> > (see "/* FIXME: Might want to do read-ahead here. */")
> >
> > In particular, I'm concerned that it does the exact opposite of this
> > performance enhancement:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/fs/gfs2?id=39b0f1e9290880a6c905f639e7db6b646e302a4f
> >
> > So we need to test that 2GB rgrps (33 bitmap blocks) that are fragmented
> > don't have a disproportionate slowdown compared to, say, 128MB rgrps
> > because we end up spending so much time fetching pages out of page cache,
> > only to find them inadequate and doing brelse() again. And by fragmented,
> > I mean all 33 bitmap blocks have a small number of free blocks, thus
> > disqualifying them from having "full" status but still having a large
> > enough number of free blocks that we might consider searching them.
> > Some of the patches we did after that one may have mitigated the problem
> > to some extent, but we need to be sure we're not regressing performance.
>
> Yes, there are a few ways this patch can still be tuned depending on
> the performance impact. This should speed up "normal" workloads by
> requiring fewer reads from disk when acquiring a resource group glock,
> and less memory is pinned for filesystem metadata, but we do end up
> with more page cache lookups during allocations, and depending on
> read-ahead, possibly more smaller reads at inconvenient times.
>
> rd_last_alloc should help limit the performance impact when bitmaps
> become mostly full. (Forms of rd_last_alloc have been in gfs2 since
> the mainline merge.)

Hmm, rd_extfail_pt actually, not rd_last_alloc, and that predates the
optimization in commit 39b0f1e929 by less than two years.

> One way to further reduce the number of page cache lookups would be to
> cache the buffer head of the last allocation, but it's unclear if this
> is even an issue, so let's do some performance testing first.

Andreas

[Cluster-devel] [PATCH 10/11] gfs2: Clean up assertion, consistency check, and error reporting functions

[Andreas Gruenbacher]

Instead of passing the __func__, __FILE__, and __LINE__ pre-processor macros to
each of those functions, print the location of the caller via:

  printk(%pS", (void *)_RET_IP_).

This gives enough context information to locate where in the code an error
occurred, and reduces the code size by about 2 percent.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/util.c | 107 ++++++++++++++++++++-----------------------------
 fs/gfs2/util.h |  71 +++++++++-----------------------
 2 files changed, 62 insertions(+), 116 deletions(-)

diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
index 0a814ccac41d2..b4c72bb799052 100644
--- a/fs/gfs2/util.c
+++ b/fs/gfs2/util.c
@@ -88,14 +88,12 @@ int gfs2_lm_withdraw(struct gfs2_sbd *sdp, const char *fmt, ...)
  *          -2 if it was already withdrawn
  */
 
-int gfs2_assert_withdraw_i(struct gfs2_sbd *sdp, char *assertion,
-			   const char *function, char *file, unsigned int line)
+int gfs2_assert_withdraw_i(struct gfs2_sbd *sdp, char *assertion)
 {
 	int me;
 	me = gfs2_lm_withdraw(sdp,
-			      "fatal: assertion \"%s\" failed\n"
-			      "   function = %s, file = %s, line = %u\n",
-			      assertion, function, file, line);
+			      "fatal: assertion \"%s\" failed at %pS\n",
+			      assertion, (void *)_RET_IP_);
 	dump_stack();
 	return (me) ? -1 : -2;
 }
@@ -106,8 +104,7 @@ int gfs2_assert_withdraw_i(struct gfs2_sbd *sdp, char *assertion,
  *          -2 if we didn't
  */
 
-int gfs2_assert_warn_i(struct gfs2_sbd *sdp, char *assertion,
-		       const char *function, char *file, unsigned int line)
+int gfs2_assert_warn_i(struct gfs2_sbd *sdp, char *assertion)
 {
 	if (time_before(jiffies,
 			sdp->sd_last_warning +
@@ -115,8 +112,8 @@ int gfs2_assert_warn_i(struct gfs2_sbd *sdp, char *assertion,
 		return -2;
 
 	if (sdp->sd_args.ar_errors == GFS2_ERRORS_WITHDRAW)
-		fs_warn(sdp, "warning: assertion \"%s\" failed at function = %s, file = %s, line = %u\n",
-			assertion, function, file, line);
+		fs_warn(sdp, "warning: assertion \"%s\" failed at %pS\n",
+			assertion, (void *)_RET_IP_);
 
 	if (sdp->sd_args.ar_debug)
 		BUG();
@@ -124,10 +121,8 @@ int gfs2_assert_warn_i(struct gfs2_sbd *sdp, char *assertion,
 		dump_stack();
 
 	if (sdp->sd_args.ar_errors == GFS2_ERRORS_PANIC)
-		panic("GFS2: fsid=%s: warning: assertion \"%s\" failed\n"
-		      "GFS2: fsid=%s:   function = %s, file = %s, line = %u\n",
-		      sdp->sd_fsname, assertion,
-		      sdp->sd_fsname, function, file, line);
+		panic("GFS2: fsid=%s: warning: assertion \"%s\" failed at %pS\n",
+		      sdp->sd_fsname, assertion, (void *)_RET_IP_);
 
 	sdp->sd_last_warning = jiffies;
 
@@ -135,61 +130,56 @@ int gfs2_assert_warn_i(struct gfs2_sbd *sdp, char *assertion,
 }
 
 /**
- * gfs2_consist_i - Flag a filesystem consistency error and withdraw
+ * gfs2_consist - Flag a filesystem consistency error and withdraw
  * Returns: -1 if this call withdrew the machine,
  *          0 if it was already withdrawn
  */
 
-int gfs2_consist_i(struct gfs2_sbd *sdp, int cluster_wide, const char *function,
-		   char *file, unsigned int line)
+int gfs2_consist(struct gfs2_sbd *sdp)
 {
 	int rv;
 	rv = gfs2_lm_withdraw(sdp,
-			      "fatal: filesystem consistency error - function = %s, file = %s, line = %u\n",
-			      function, file, line);
+			      "fatal: filesystem consistency error at %pS\n",
+			      (void *)_RET_IP_);
 	return rv;
 }
 
 /**
- * gfs2_consist_inode_i - Flag an inode consistency error and withdraw
+ * gfs2_consist_inode - Flag an inode consistency error and withdraw
  * Returns: -1 if this call withdrew the machine,
  *          0 if it was already withdrawn
  */
 
-int gfs2_consist_inode_i(struct gfs2_inode *ip, int cluster_wide,
-			 const char *function, char *file, unsigned int line)
+int gfs2_consist_inode(struct gfs2_inode *ip)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	int rv;
 	rv = gfs2_lm_withdraw(sdp,
-			      "fatal: filesystem consistency error\n"
-			      "  inode = %llu %llu\n"
-			      "  function = %s, file = %s, line = %u\n",
+			      "fatal: filesystem consistency error at %pS\n"
+			      "  inode = %llu %llu\n",
+			      (void *)_RET_IP_,
 			      (unsigned long long)ip->i_no_formal_ino,
-			      (unsigned long long)ip->i_no_addr,
-			      function, file, line);
+			      (unsigned long long)ip->i_no_addr);
 	return rv;
 }
 
 /**
- * gfs2_consist_rgrpd_i - Flag a RG consistency error and withdraw
+ * gfs2_consist_rgrpd - Flag a RG consistency error and withdraw
  * Returns: -1 if this call withdrew the machine,
  *          0 if it was already withdrawn
  */
 
-int gfs2_consist_rgrpd_i(struct gfs2_rgrpd *rgd, int cluster_wide,
-			 const char *function, char *file, unsigned int line)
+int gfs2_consist_rgrpd(struct gfs2_rgrpd *rgd)
 {
 	struct gfs2_sbd *sdp = rgd->rd_sbd;
 	int rv;
 
 	gfs2_rgrp_dump(NULL, rgd->rd_gl);
 	rv = gfs2_lm_withdraw(sdp,
-			      "fatal: filesystem consistency error\n"
-			      "  RG = %llu\n"
-			      "  function = %s, file = %s, line = %u\n",
-			      (unsigned long long)rgd->rd_addr,
-			      function, file, line);
+			      "fatal: filesystem consistency error at %pS\n"
+			      "  RG = %llu\n",
+			      (void *)_RET_IP_,
+			      (unsigned long long)rgd->rd_addr);
 	return rv;
 }
 
@@ -200,16 +190,14 @@ int gfs2_consist_rgrpd_i(struct gfs2_rgrpd *rgd, int cluster_wide,
  */
 
 int gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh,
-		       const char *type, const char *function, char *file,
-		       unsigned int line)
+		       const char *type, void *caller)
 {
 	int me;
 	me = gfs2_lm_withdraw(sdp,
-			      "fatal: invalid metadata block\n"
-			      "  bh = %llu (%s)\n"
-			      "  function = %s, file = %s, line = %u\n",
-			      (unsigned long long)bh->b_blocknr, type,
-			      function, file, line);
+			      "fatal: invalid metadata block at %pS\n"
+			      "  bh = %llu (%s)\n",
+			      caller,
+			      (unsigned long long)bh->b_blocknr, type);
 	return (me) ? -1 : -2;
 }
 
@@ -220,53 +208,46 @@ int gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh,
  */
 
 int gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh,
-			   u16 type, u16 t, const char *function,
-			   char *file, unsigned int line)
+			   u16 type, u16 t, void *caller)
 {
 	int me;
 	me = gfs2_lm_withdraw(sdp,
-			      "fatal: invalid metadata block\n"
-			      "  bh = %llu (type: exp=%u, found=%u)\n"
-			      "  function = %s, file = %s, line = %u\n",
-			      (unsigned long long)bh->b_blocknr, type, t,
-			      function, file, line);
+			      "fatal: invalid metadata block at %pS\n"
+			      "  bh = %llu (type: exp=%u, found=%u)\n",
+			      caller,
+			      (unsigned long long)bh->b_blocknr, type, t);
 	return (me) ? -1 : -2;
 }
 
 /**
- * gfs2_io_error_i - Flag an I/O error and withdraw
+ * gfs2_io_error - Flag an I/O error and withdraw
  * Returns: -1 if this call withdrew the machine,
  *          0 if it was already withdrawn
  */
 
-int gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function, char *file,
-		    unsigned int line)
+int gfs2_io_error(struct gfs2_sbd *sdp)
 {
 	int rv;
 	rv = gfs2_lm_withdraw(sdp,
-			      "fatal: I/O error\n"
-			      "  function = %s, file = %s, line = %u\n",
-			      function, file, line);
+			      "fatal: I/O error at %pS\n",
+			      (void *)_RET_IP_);
 	return rv;
 }
 
 /**
- * gfs2_io_error_bh_i - Flag a buffer I/O error
+ * gfs2_io_error_bh - Flag a buffer I/O error
  * @withdraw: withdraw the filesystem
  */
 
-void gfs2_io_error_bh_i(struct gfs2_sbd *sdp, struct buffer_head *bh,
-			const char *function, char *file, unsigned int line,
+void __gfs2_io_error_bh(struct gfs2_sbd *sdp, struct buffer_head *bh,
 			bool withdraw)
 {
 	if (!test_bit(SDF_SHUTDOWN, &sdp->sd_flags))
 		fs_err(sdp,
-		       "fatal: I/O error\n"
-		       "  block = %llu\n"
-		       "  function = %s, file = %s, line = %u\n",
-		       (unsigned long long)bh->b_blocknr,
-		       function, file, line);
+		       "fatal: I/O error at %pS\n"
+		       "  block = %llu\n",
+		       (void *)_RET_IP_,
+		       (unsigned long long)bh->b_blocknr);
 	if (withdraw)
 		gfs2_lm_withdraw(sdp, NULL);
 }
-
diff --git a/fs/gfs2/util.h b/fs/gfs2/util.h
index 9278fecba6321..453d814c1efa3 100644
--- a/fs/gfs2/util.h
+++ b/fs/gfs2/util.h
@@ -39,46 +39,24 @@ do { \
 } while (0)
 
 
-int gfs2_assert_withdraw_i(struct gfs2_sbd *sdp, char *assertion,
-			   const char *function, char *file, unsigned int line);
+int gfs2_assert_withdraw_i(struct gfs2_sbd *sdp, char *assertion);
 
 #define gfs2_assert_withdraw(sdp, assertion) \
-((likely(assertion)) ? 0 : gfs2_assert_withdraw_i((sdp), #assertion, \
-					__func__, __FILE__, __LINE__))
+	((likely(assertion)) ? 0 : gfs2_assert_withdraw_i((sdp), #assertion))
 
 
-int gfs2_assert_warn_i(struct gfs2_sbd *sdp, char *assertion,
-		       const char *function, char *file, unsigned int line);
+int gfs2_assert_warn_i(struct gfs2_sbd *sdp, char *assertion);
 
 #define gfs2_assert_warn(sdp, assertion) \
-((likely(assertion)) ? 0 : gfs2_assert_warn_i((sdp), #assertion, \
-					__func__, __FILE__, __LINE__))
+	((likely(assertion)) ? 0 : gfs2_assert_warn_i((sdp), #assertion))
 
 
-int gfs2_consist_i(struct gfs2_sbd *sdp, int cluster_wide,
-		   const char *function, char *file, unsigned int line);
-
-#define gfs2_consist(sdp) \
-gfs2_consist_i((sdp), 0, __func__, __FILE__, __LINE__)
-
-
-int gfs2_consist_inode_i(struct gfs2_inode *ip, int cluster_wide,
-			 const char *function, char *file, unsigned int line);
-
-#define gfs2_consist_inode(ip) \
-gfs2_consist_inode_i((ip), 0, __func__, __FILE__, __LINE__)
-
-
-int gfs2_consist_rgrpd_i(struct gfs2_rgrpd *rgd, int cluster_wide,
-			 const char *function, char *file, unsigned int line);
-
-#define gfs2_consist_rgrpd(rgd) \
-gfs2_consist_rgrpd_i((rgd), 0, __func__, __FILE__, __LINE__)
-
+int gfs2_consist(struct gfs2_sbd *sdp);
+int gfs2_consist_inode(struct gfs2_inode *ip);
+int gfs2_consist_rgrpd(struct gfs2_rgrpd *rgd);
 
 int gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh,
-		       const char *type, const char *function,
-		       char *file, unsigned int line);
+		       const char *type, void *caller);
 
 static inline int gfs2_meta_check(struct gfs2_sbd *sdp,
 				    struct buffer_head *bh)
@@ -95,30 +73,23 @@ static inline int gfs2_meta_check(struct gfs2_sbd *sdp,
 
 int gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh,
 			   u16 type, u16 t,
-			   const char *function,
-			   char *file, unsigned int line);
+			   void *caller);
 
-static inline int gfs2_metatype_check_i(struct gfs2_sbd *sdp,
+static inline int gfs2_metatype_check(struct gfs2_sbd *sdp,
 					struct buffer_head *bh,
-					u16 type,
-					const char *function,
-					char *file, unsigned int line)
+					u16 type)
 {
 	struct gfs2_meta_header *mh = (struct gfs2_meta_header *)bh->b_data;
 	u32 magic = be32_to_cpu(mh->mh_magic);
 	u16 t = be32_to_cpu(mh->mh_type);
 	if (unlikely(magic != GFS2_MAGIC))
-		return gfs2_meta_check_ii(sdp, bh, "magic number", function,
-					  file, line);
+		return gfs2_meta_check_ii(sdp, bh, "magic number", (void *)_RET_IP_);
         if (unlikely(t != type))
-		return gfs2_metatype_check_ii(sdp, bh, type, t, function,
-					      file, line);
+		return gfs2_metatype_check_ii(sdp, bh, type, t,
+					      (void *)_RET_IP_);
 	return 0;
 }
 
-#define gfs2_metatype_check(sdp, bh, type) \
-gfs2_metatype_check_i((sdp), (bh), (type), __func__, __FILE__, __LINE__)
-
 static inline void gfs2_metatype_set(struct buffer_head *bh, u16 type,
 				     u16 format)
 {
@@ -129,22 +100,16 @@ static inline void gfs2_metatype_set(struct buffer_head *bh, u16 type,
 }
 
 
-int gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function,
-		    char *file, unsigned int line);
-
-#define gfs2_io_error(sdp) \
-gfs2_io_error_i((sdp), __func__, __FILE__, __LINE__);
-
+int gfs2_io_error(struct gfs2_sbd *sdp);
 
-void gfs2_io_error_bh_i(struct gfs2_sbd *sdp, struct buffer_head *bh,
-			const char *function, char *file, unsigned int line,
+void __gfs2_io_error_bh(struct gfs2_sbd *sdp, struct buffer_head *bh,
 			bool withdraw);
 
 #define gfs2_io_error_bh_wd(sdp, bh) \
-gfs2_io_error_bh_i((sdp), (bh), __func__, __FILE__, __LINE__, true);
+	__gfs2_io_error_bh((sdp), (bh), true);
 
 #define gfs2_io_error_bh(sdp, bh) \
-gfs2_io_error_bh_i((sdp), (bh), __func__, __FILE__, __LINE__, false);
+	__gfs2_io_error_bh((sdp), (bh), false);
 
 
 extern struct kmem_cache *gfs2_glock_cachep;
-- 
2.20.1

[Cluster-devel] [PATCH 10/11] gfs2: Clean up assertion, consistency check, and error reporting functions

[Bob Peterson]

----- Original Message -----
> Instead of passing the __func__, __FILE__, and __LINE__ pre-processor macros
> to
> each of those functions, print the location of the caller via:
> 
>   printk(%pS", (void *)_RET_IP_).
> 
> This gives enough context information to locate where in the code an error
> occurred, and reduces the code size by about 2 percent.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---

Hi,

Sorry, but I don't like this patch at all.

With all the different versions floating around, and further obscured by static
function optimization by the compiler, this patch just makes debugging harder.
Not impossible, but harder.

In other words, in my opinion, the benefit is not worth the cost.

Plus it has nothing to do with making the rgrp bitmaps read in on-demand.

Regards,

Bob Peterson

[Cluster-devel] [PATCH 11/11] gfs2: Skip gfs2_metatype_check for cached buffers

[Andreas Gruenbacher]

When reading in buffers from disk, set a new BH_Verify buffer head flag.  In
gfs2_metatype_check, skip the check if BH_Verify is cleared and clear the flag
when checking.  That way, we'll only check the metatype once when reading
buffers from disk, and not when the buffers were already in the page cache.

While touching this code, convert two 'be32_to_cpu(magic) == GFS2_MAGIC' checks
into 'magic == cpu_to_be32(GFS2_MAGIC)'.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/incore.h  |  5 ++++-
 fs/gfs2/meta_io.c | 10 ++++++++--
 fs/gfs2/util.c    | 31 ++++++++++++++++++++++++-------
 fs/gfs2/util.h    | 24 ++++++------------------
 4 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index d39c26b950121..40644e1207969 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -126,13 +126,16 @@ struct gfs2_rgrpd {
 
 enum gfs2_state_bits {
 	BH_Pinned = BH_PrivateStart,
-	BH_Escaped = BH_PrivateStart + 1,
+	BH_Escaped,
+	BH_Verify
 };
 
 BUFFER_FNS(Pinned, pinned)
 TAS_BUFFER_FNS(Pinned, pinned)
 BUFFER_FNS(Escaped, escaped)
 TAS_BUFFER_FNS(Escaped, escaped)
+BUFFER_FNS(Verify, verify)
+TAS_BUFFER_FNS(Verify, verify)
 
 struct gfs2_bufdata {
 	struct buffer_head *bd_bh;
diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
index be9c0bf697fe6..a111e12a5e1b6 100644
--- a/fs/gfs2/meta_io.c
+++ b/fs/gfs2/meta_io.c
@@ -268,6 +268,7 @@ int gfs2_meta_read(struct gfs2_glock *gl, u64 blkno, int flags,
 	} else {
 		bh->b_end_io = end_buffer_read_sync;
 		get_bh(bh);
+		set_buffer_verify(bh);
 		bhs[num++] = bh;
 	}
 
@@ -280,6 +281,7 @@ int gfs2_meta_read(struct gfs2_glock *gl, u64 blkno, int flags,
 			brelse(bh);
 		} else {
 			bh->b_end_io = end_buffer_read_sync;
+			set_buffer_verify(bh);
 			bhs[num++] = bh;
 		}
 	}
@@ -452,8 +454,10 @@ struct buffer_head *gfs2_meta_ra(struct gfs2_glock *gl, u64 dblock, u32 extlen)
 
 	if (buffer_uptodate(first_bh))
 		goto out;
-	if (!buffer_locked(first_bh))
+	if (!buffer_locked(first_bh)) {
+		set_buffer_verify(first_bh);
 		ll_rw_block(REQ_OP_READ, REQ_META | REQ_PRIO, 1, &first_bh);
+	}
 
 	dblock++;
 	extlen--;
@@ -461,10 +465,12 @@ struct buffer_head *gfs2_meta_ra(struct gfs2_glock *gl, u64 dblock, u32 extlen)
 	while (extlen) {
 		bh = gfs2_getbuf(gl, dblock, CREATE);
 
-		if (!buffer_uptodate(bh) && !buffer_locked(bh))
+		if (!buffer_uptodate(bh) && !buffer_locked(bh)) {
+			set_buffer_verify(bh);
 			ll_rw_block(REQ_OP_READ,
 				    REQ_RAHEAD | REQ_META | REQ_PRIO,
 				    1, &bh);
+		}
 		brelse(bh);
 		dblock++;
 		extlen--;
diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
index b4c72bb799052..2fb4e83bd3efa 100644
--- a/fs/gfs2/util.c
+++ b/fs/gfs2/util.c
@@ -202,20 +202,37 @@ int gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh,
 }
 
 /**
- * gfs2_metatype_check_ii - Flag a metadata type consistency error and withdraw
+ * gfs2_metatype_check_ii - Check the metadata type of a block
+ *
+ * Report and withdraw on inconsistencies.
+ *
  * Returns: -1 if this call withdrew the machine,
  *          -2 if it was already withdrawn
  */
 
 int gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh,
-			   u16 type, u16 t, void *caller)
+			   u16 type, void *caller)
 {
+	struct gfs2_meta_header *mh = (struct gfs2_meta_header *)bh->b_data;
 	int me;
-	me = gfs2_lm_withdraw(sdp,
-			      "fatal: invalid metadata block at %pS\n"
-			      "  bh = %llu (type: exp=%u, found=%u)\n",
-			      caller,
-			      (unsigned long long)bh->b_blocknr, type, t);
+
+	clear_buffer_verify(bh);
+
+	if (unlikely(mh->mh_magic != cpu_to_be32(GFS2_MAGIC))) {
+		me = gfs2_lm_withdraw(sdp,
+				      "fatal: invalid metadata block at %pS\n"
+				      "  bh = %llu (magic number)\n",
+				      caller,
+				      (unsigned long long)bh->b_blocknr);
+	} else if (unlikely(be32_to_cpu(mh->mh_type) != type)) {
+		me = gfs2_lm_withdraw(sdp,
+				      "fatal: invalid metadata block at %pS\n"
+				      "  bh = %llu (type: exp=%u, found=%u)\n",
+				      caller,
+				      (unsigned long long)bh->b_blocknr, type,
+				      be32_to_cpu(mh->mh_type));
+	} else
+		return 0;
 	return (me) ? -1 : -2;
 }
 
diff --git a/fs/gfs2/util.h b/fs/gfs2/util.h
index 453d814c1efa3..0432b0aedefc2 100644
--- a/fs/gfs2/util.h
+++ b/fs/gfs2/util.h
@@ -55,15 +55,11 @@ int gfs2_consist(struct gfs2_sbd *sdp);
 int gfs2_consist_inode(struct gfs2_inode *ip);
 int gfs2_consist_rgrpd(struct gfs2_rgrpd *rgd);
 
-int gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh,
-		       const char *type, void *caller);
-
 static inline int gfs2_meta_check(struct gfs2_sbd *sdp,
 				    struct buffer_head *bh)
 {
 	struct gfs2_meta_header *mh = (struct gfs2_meta_header *)bh->b_data;
-	u32 magic = be32_to_cpu(mh->mh_magic);
-	if (unlikely(magic != GFS2_MAGIC)) {
+	if (unlikely(mh->mh_magic != cpu_to_be32(GFS2_MAGIC))) {
 		fs_err(sdp, "Magic number missing at %llu\n",
 		       (unsigned long long)bh->b_blocknr);
 		return -EIO;
@@ -72,22 +68,14 @@ static inline int gfs2_meta_check(struct gfs2_sbd *sdp,
 }
 
 int gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh,
-			   u16 type, u16 t,
-			   void *caller);
+			   u16 type, void *caller);
 
 static inline int gfs2_metatype_check(struct gfs2_sbd *sdp,
-					struct buffer_head *bh,
-					u16 type)
+				      struct buffer_head *bh, u16 type)
 {
-	struct gfs2_meta_header *mh = (struct gfs2_meta_header *)bh->b_data;
-	u32 magic = be32_to_cpu(mh->mh_magic);
-	u16 t = be32_to_cpu(mh->mh_type);
-	if (unlikely(magic != GFS2_MAGIC))
-		return gfs2_meta_check_ii(sdp, bh, "magic number", (void *)_RET_IP_);
-        if (unlikely(t != type))
-		return gfs2_metatype_check_ii(sdp, bh, type, t,
-					      (void *)_RET_IP_);
-	return 0;
+	if (!buffer_verify(bh))
+		return 0;
+	return gfs2_metatype_check_ii(sdp, bh, type, (void *)_RET_IP_);
 }
 
 static inline void gfs2_metatype_set(struct buffer_head *bh, u16 type,
-- 
2.20.1

[Cluster-devel] [PATCH 11/11] gfs2: Skip gfs2_metatype_check for cached buffers

[Bob Peterson]

----- Original Message -----
> When reading in buffers from disk, set a new BH_Verify buffer head flag.  In
> gfs2_metatype_check, skip the check if BH_Verify is cleared and clear the
> flag
> when checking.  That way, we'll only check the metatype once when reading
> buffers from disk, and not when the buffers were already in the page cache.
> 
> While touching this code, convert two 'be32_to_cpu(magic) == GFS2_MAGIC'
> checks
> into 'magic == cpu_to_be32(GFS2_MAGIC)'.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
(snip)
>  static inline int gfs2_metatype_check(struct gfs2_sbd *sdp,
> -					struct buffer_head *bh,
> -					u16 type)
> +				      struct buffer_head *bh, u16 type)
>  {
> -	struct gfs2_meta_header *mh = (struct gfs2_meta_header *)bh->b_data;
> -	u32 magic = be32_to_cpu(mh->mh_magic);
> -	u16 t = be32_to_cpu(mh->mh_type);
> -	if (unlikely(magic != GFS2_MAGIC))
> -		return gfs2_meta_check_ii(sdp, bh, "magic number", (void *)_RET_IP_);
> -        if (unlikely(t != type))
> -		return gfs2_metatype_check_ii(sdp, bh, type, t,
> -					      (void *)_RET_IP_);
> -	return 0;
> +	if (!buffer_verify(bh))
> +		return 0;
> +	return gfs2_metatype_check_ii(sdp, bh, type, (void *)_RET_IP_);

May I suggest simplifying with:

if (!test_clear_buffer_verify(bh))
	return 0;

Regards,

Bob Peterson