From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andreas Gruenbacher Date: Tue, 24 Jul 2018 14:59:03 +0200 Subject: [Cluster-devel] [PATCH v2] gfs2: Fix gfs2_testbit to use clone bitmaps In-Reply-To: <805116413.50533265.1531513153795.JavaMail.zimbra@redhat.com> References: <1726844332.50526668.1531512206762.JavaMail.zimbra@redhat.com> <805116413.50533265.1531513153795.JavaMail.zimbra@redhat.com> Message-ID: List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Bob, On 13 July 2018 at 22:19, Bob Peterson wrote: > Hi, > > The previous version of my patch had a bug. Apparently function > gfs2_check_blk_type does need to check the non-clone bitmap. The other > two callers need to check the clone bitmap, if available. > So this version adds a parameter to specify. Also, I got rid of > function gfs2_get_block_type() in favor of calling it directly > from its only caller, gfs2_check_blk_type. This simplifies the code. could you please add some documentation about how those clone bitmaps are supposed to work? It's really hard to guess that from the code alone. > Bob Peterson > --- > Function gfs2_testbit is called in three places. Two of those places, > gfs2_alloc_extent and gfs2_unaligned_extlen, should be using the > clone bitmaps, not the "real" bitmaps. Function gfs2_unaligned_extlen > is used by the block reservations scheme to determine the length of > an extent of free blocks. Before this patch, it wasn't using the > clone bitmap, which means soon-to-be-freed blocks will be treated > as free blocks for the purposes of an extent, and that's clearly > wrong. > > This patch adds a new parameter to gfs2_testbit to indicate whether > or not the clone bitmaps should be used (if available). > > Signed-off-by: Bob Peterson > --- > fs/gfs2/rgrp.c | 45 ++++++++++++++++++++------------------------- > 1 file changed, 20 insertions(+), 25 deletions(-) > > diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c > index 3821b3ab04fa..7e22918d32d6 100644 > --- a/fs/gfs2/rgrp.c > +++ b/fs/gfs2/rgrp.c > @@ -123,17 +123,27 @@ static inline void gfs2_setbit(const struct gfs2_rbm *rbm, bool do_clone, > /** > * gfs2_testbit - test a bit in the bitmaps > * @rbm: The bit to test > + * @use_clone: If true, test the clone bitmap, not the official bitmap. > + * > + * Some callers, like gfs2_unaligned_extlen, need to test the clone bitmaps, > + * not the "real" bitmaps, because we can't reserve blocks if they're still > + * considered metadata in the journal. > * > * Returns: The two bit block state of the requested bit > */ > > -static inline u8 gfs2_testbit(const struct gfs2_rbm *rbm) > +static inline u8 gfs2_testbit(const struct gfs2_rbm *rbm, bool use_clone) > { > struct gfs2_bitmap *bi = rbm_bi(rbm); > - const u8 *buffer = bi->bi_bh->b_data + bi->bi_offset; > + const u8 *buffer; > const u8 *byte; > unsigned int bit; > > + if (use_clone && bi->bi_clone) > + buffer = bi->bi_clone; > + else > + buffer = bi->bi_bh->b_data; > + buffer += bi->bi_offset; > byte = buffer + (rbm->offset / GFS2_NBBY); > bit = (rbm->offset % GFS2_NBBY) * GFS2_BIT_SIZE; > > @@ -322,7 +332,7 @@ static bool gfs2_unaligned_extlen(struct gfs2_rbm *rbm, u32 n_unaligned, u32 *le > u8 res; > > for (n = 0; n < n_unaligned; n++) { > - res = gfs2_testbit(rbm); > + res = gfs2_testbit(rbm, true); > if (res != GFS2_BLKST_FREE) > return true; > (*len)--; > @@ -2142,26 +2152,6 @@ void gfs2_inplace_release(struct gfs2_inode *ip) > gfs2_glock_dq_uninit(&rs->rs_rgd_gh); > } > > -/** > - * gfs2_get_block_type - Check a block in a RG is of given type > - * @rgd: the resource group holding the block > - * @block: the block number > - * > - * Returns: The block type (GFS2_BLKST_*) > - */ > - > -static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block) > -{ > - struct gfs2_rbm rbm = { .rgd = rgd, }; > - int ret; > - > - ret = gfs2_rbm_from_block(&rbm, block); > - WARN_ON_ONCE(ret != 0); > - > - return gfs2_testbit(&rbm); > -} > - > - > /** > * gfs2_alloc_extent - allocate an extent from a given bitmap > * @rbm: the resource group information > @@ -2186,7 +2176,7 @@ static void gfs2_alloc_extent(const struct gfs2_rbm *rbm, bool dinode, > block++; > while (*n < elen) { > ret = gfs2_rbm_from_block(&pos, block); > - if (ret || gfs2_testbit(&pos) != GFS2_BLKST_FREE) > + if (ret || gfs2_testbit(&pos, 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); > @@ -2543,6 +2533,7 @@ int gfs2_check_blk_type(struct gfs2_sbd *sdp, u64 no_addr, unsigned int type) > { > struct gfs2_rgrpd *rgd; > struct gfs2_holder rgd_gh; > + struct gfs2_rbm rbm; > int error = -EINVAL; > > rgd = gfs2_blk2rgrpd(sdp, no_addr, 1); > @@ -2553,7 +2544,11 @@ int gfs2_check_blk_type(struct gfs2_sbd *sdp, u64 no_addr, unsigned int type) > if (error) > goto fail; > > - if (gfs2_get_block_type(rgd, no_addr) != type) > + rbm.rgd = rgd; > + error = gfs2_rbm_from_block(&rbm, no_addr); > + WARN_ON_ONCE(error != 0); > + > + if (gfs2_testbit(&rbm, false) != type) > error = -ESTALE; > > gfs2_glock_dq_uninit(&rgd_gh); > Thanks, Andreas