From: Eric Sandeen <sandeen@sandeen.net>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 07/12] libfrog: fix bitmap return values
Date: Wed, 22 May 2019 11:23:00 -0500 [thread overview]
Message-ID: <30afb811-c136-24f2-5c15-71c3641ef78f@sandeen.net> (raw)
In-Reply-To: <155839424514.68606.14562327454853103352.stgit@magnolia>
On 5/20/19 6:17 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Fix the return types of non-predicate bitmap functions to return the
> usual negative error codes instead of the "moveon" boolean.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> include/bitmap.h | 8 +++--
> libfrog/bitmap.c | 86 ++++++++++++++++++++++++++----------------------------
> repair/rmap.c | 18 ++++++++---
> scrub/phase6.c | 18 ++++-------
> 4 files changed, 65 insertions(+), 65 deletions(-)
Sorry for the drama on signs.
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>
> diff --git a/include/bitmap.h b/include/bitmap.h
> index e29a4335..99a2fb23 100644
> --- a/include/bitmap.h
> +++ b/include/bitmap.h
> @@ -11,11 +11,11 @@ struct bitmap {
> struct avl64tree_desc *bt_tree;
> };
>
> -bool bitmap_init(struct bitmap **bmap);
> +int bitmap_init(struct bitmap **bmap);
> void bitmap_free(struct bitmap **bmap);
> -bool bitmap_set(struct bitmap *bmap, uint64_t start, uint64_t length);
> -bool bitmap_iterate(struct bitmap *bmap,
> - bool (*fn)(uint64_t, uint64_t, void *), void *arg);
> +int bitmap_set(struct bitmap *bmap, uint64_t start, uint64_t length);
> +int bitmap_iterate(struct bitmap *bmap, int (*fn)(uint64_t, uint64_t, void *),
> + void *arg);
> bool bitmap_test(struct bitmap *bmap, uint64_t start,
> uint64_t len);
> bool bitmap_empty(struct bitmap *bmap);
> diff --git a/libfrog/bitmap.c b/libfrog/bitmap.c
> index 450ebe0a..4dafc4c9 100644
> --- a/libfrog/bitmap.c
> +++ b/libfrog/bitmap.c
> @@ -66,7 +66,7 @@ static struct avl64ops bitmap_ops = {
> };
>
> /* Initialize a bitmap. */
> -bool
> +int
> bitmap_init(
> struct bitmap **bmapp)
> {
> @@ -74,18 +74,18 @@ bitmap_init(
>
> bmap = calloc(1, sizeof(struct bitmap));
> if (!bmap)
> - return false;
> + return -ENOMEM;
> bmap->bt_tree = malloc(sizeof(struct avl64tree_desc));
> if (!bmap->bt_tree) {
> free(bmap);
> - return false;
> + return -ENOMEM;
> }
>
> pthread_mutex_init(&bmap->bt_lock, NULL);
> avl64_init_tree(bmap->bt_tree, &bitmap_ops);
> *bmapp = bmap;
>
> - return true;
> + return 0;
> }
>
> /* Free a bitmap. */
> @@ -127,8 +127,31 @@ bitmap_node_init(
> return ext;
> }
>
> +/* Create a new bitmap node and insert it. */
> +static inline int
> +__bitmap_insert(
> + struct bitmap *bmap,
> + uint64_t start,
> + uint64_t length)
> +{
> + struct bitmap_node *ext;
> + struct avl64node *node;
> +
> + ext = bitmap_node_init(start, length);
> + if (!ext)
> + return -ENOMEM;
> +
> + node = avl64_insert(bmap->bt_tree, &ext->btn_node);
> + if (node == NULL) {
> + free(ext);
> + return -EEXIST;
> + }
> +
> + return 0;
> +}
> +
> /* Set a region of bits (locked). */
> -static bool
> +static int
> __bitmap_set(
> struct bitmap *bmap,
> uint64_t start,
> @@ -142,28 +165,14 @@ __bitmap_set(
> struct bitmap_node *ext;
> uint64_t new_start;
> uint64_t new_length;
> - struct avl64node *node;
> - bool res = true;
>
> /* Find any existing nodes adjacent or within that range. */
> avl64_findranges(bmap->bt_tree, start - 1, start + length + 1,
> &firstn, &lastn);
>
> /* Nothing, just insert a new extent. */
> - if (firstn == NULL && lastn == NULL) {
> - ext = bitmap_node_init(start, length);
> - if (!ext)
> - return false;
> -
> - node = avl64_insert(bmap->bt_tree, &ext->btn_node);
> - if (node == NULL) {
> - free(ext);
> - errno = EEXIST;
> - return false;
> - }
> -
> - return true;
> - }
> + if (firstn == NULL && lastn == NULL)
> + return __bitmap_insert(bmap, start, length);
>
> assert(firstn != NULL && lastn != NULL);
> new_start = start;
> @@ -175,7 +184,7 @@ __bitmap_set(
> /* Bail if the new extent is contained within an old one. */
> if (ext->btn_start <= start &&
> ext->btn_start + ext->btn_length >= start + length)
> - return res;
> + return 0;
>
> /* Check for overlapping and adjacent extents. */
> if (ext->btn_start + ext->btn_length >= start ||
> @@ -195,28 +204,17 @@ __bitmap_set(
> }
> }
>
> - ext = bitmap_node_init(new_start, new_length);
> - if (!ext)
> - return false;
> -
> - node = avl64_insert(bmap->bt_tree, &ext->btn_node);
> - if (node == NULL) {
> - free(ext);
> - errno = EEXIST;
> - return false;
> - }
> -
> - return res;
> + return __bitmap_insert(bmap, new_start, new_length);
> }
>
> /* Set a region of bits. */
> -bool
> +int
> bitmap_set(
> struct bitmap *bmap,
> uint64_t start,
> uint64_t length)
> {
> - bool res;
> + int res;
>
> pthread_mutex_lock(&bmap->bt_lock);
> res = __bitmap_set(bmap, start, length);
> @@ -308,26 +306,26 @@ bitmap_clear(
>
> #ifdef DEBUG
> /* Iterate the set regions of this bitmap. */
> -bool
> +int
> bitmap_iterate(
> struct bitmap *bmap,
> - bool (*fn)(uint64_t, uint64_t, void *),
> + int (*fn)(uint64_t, uint64_t, void *),
> void *arg)
> {
> struct avl64node *node;
> struct bitmap_node *ext;
> - bool moveon = true;
> + int error = 0;
>
> pthread_mutex_lock(&bmap->bt_lock);
> avl_for_each(bmap->bt_tree, node) {
> ext = container_of(node, struct bitmap_node, btn_node);
> - moveon = fn(ext->btn_start, ext->btn_length, arg);
> - if (!moveon)
> + error = fn(ext->btn_start, ext->btn_length, arg);
> + if (error)
> break;
> }
> pthread_mutex_unlock(&bmap->bt_lock);
>
> - return moveon;
> + return error;
> }
> #endif
>
> @@ -372,14 +370,14 @@ bitmap_empty(
> }
>
> #ifdef DEBUG
> -static bool
> +static int
> bitmap_dump_fn(
> uint64_t startblock,
> uint64_t blockcount,
> void *arg)
> {
> printf("%"PRIu64":%"PRIu64"\n", startblock, blockcount);
> - return true;
> + return 0;
> }
>
> /* Dump bitmap. */
> diff --git a/repair/rmap.c b/repair/rmap.c
> index 19cceca3..47828a06 100644
> --- a/repair/rmap.c
> +++ b/repair/rmap.c
> @@ -490,16 +490,22 @@ rmap_store_ag_btree_rec(
> error = init_slab_cursor(ag_rmap->ar_raw_rmaps, rmap_compare, &rm_cur);
> if (error)
> goto err;
> - if (!bitmap_init(&own_ag_bitmap)) {
> - error = -ENOMEM;
> + error = -bitmap_init(&own_ag_bitmap);
> + if (error)
> goto err_slab;
> - }
> while ((rm_rec = pop_slab_cursor(rm_cur)) != NULL) {
> if (rm_rec->rm_owner != XFS_RMAP_OWN_AG)
> continue;
> - if (!bitmap_set(own_ag_bitmap, rm_rec->rm_startblock,
> - rm_rec->rm_blockcount)) {
> - error = EFSCORRUPTED;
> + error = -bitmap_set(own_ag_bitmap, rm_rec->rm_startblock,
> + rm_rec->rm_blockcount);
> + if (error) {
> + /*
> + * If this range is already set, then the incore rmap
> + * records for the AG free space btrees overlap and
> + * we're toast because that is not allowed.
> + */
> + if (error == EEXIST)
> + error = EFSCORRUPTED;
> goto err_slab;
> }
> }
> diff --git a/scrub/phase6.c b/scrub/phase6.c
> index 4b25f3bb..66e6451c 100644
> --- a/scrub/phase6.c
> +++ b/scrub/phase6.c
> @@ -341,7 +341,6 @@ xfs_check_rmap_ioerr(
> struct media_verify_state *vs = arg;
> struct bitmap *tree;
> dev_t dev;
> - bool moveon;
>
> dev = xfs_disk_to_dev(ctx, disk);
>
> @@ -356,8 +355,8 @@ xfs_check_rmap_ioerr(
> else
> tree = NULL;
> if (tree) {
> - moveon = bitmap_set(tree, start, length);
> - if (!moveon)
> + errno = -bitmap_set(tree, start, length);
> + if (errno)
> str_errno(ctx, ctx->mntpoint);
> }
>
> @@ -454,16 +453,16 @@ xfs_scan_blocks(
> struct scrub_ctx *ctx)
> {
> struct media_verify_state vs = { NULL };
> - bool moveon;
> + bool moveon = false;
>
> - moveon = bitmap_init(&vs.d_bad);
> - if (!moveon) {
> + errno = -bitmap_init(&vs.d_bad);
> + if (errno) {
> str_errno(ctx, ctx->mntpoint);
> goto out;
> }
>
> - moveon = bitmap_init(&vs.r_bad);
> - if (!moveon) {
> + errno = -bitmap_init(&vs.r_bad);
> + if (errno) {
> str_errno(ctx, ctx->mntpoint);
> goto out_dbad;
> }
> @@ -472,7 +471,6 @@ xfs_scan_blocks(
> ctx->geo.blocksize, xfs_check_rmap_ioerr,
> scrub_nproc(ctx));
> if (!vs.rvp_data) {
> - moveon = false;
> str_info(ctx, ctx->mntpoint,
> _("Could not create data device media verifier."));
> goto out_rbad;
> @@ -482,7 +480,6 @@ _("Could not create data device media verifier."));
> ctx->geo.blocksize, xfs_check_rmap_ioerr,
> scrub_nproc(ctx));
> if (!vs.rvp_log) {
> - moveon = false;
> str_info(ctx, ctx->mntpoint,
> _("Could not create log device media verifier."));
> goto out_datapool;
> @@ -493,7 +490,6 @@ _("Could not create data device media verifier."));
> ctx->geo.blocksize, xfs_check_rmap_ioerr,
> scrub_nproc(ctx));
> if (!vs.rvp_realtime) {
> - moveon = false;
> str_info(ctx, ctx->mntpoint,
> _("Could not create realtime device media verifier."));
> goto out_logpool;
>
next prev parent reply other threads:[~2019-05-22 16:23 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-20 23:16 [PATCH 00/12] xfsprogs-5.1: fix various problems Darrick J. Wong
2019-05-20 23:16 ` [PATCH 01/12] libxfs: fix attr include mess Darrick J. Wong
2019-05-21 16:30 ` Eric Sandeen
2019-05-20 23:16 ` [PATCH 02/12] libxfs: set m_finobt_nores when initializing library Darrick J. Wong
2019-05-21 16:33 ` Eric Sandeen
2019-05-20 23:17 ` [PATCH 03/12] libxfs: refactor online geometry queries Darrick J. Wong
2019-05-21 16:38 ` Eric Sandeen
2019-05-21 16:58 ` Darrick J. Wong
2019-05-20 23:17 ` [PATCH 04/12] libxfs: refactor open-coded bulkstat calls Darrick J. Wong
2019-05-20 23:17 ` [PATCH 05/12] libxfs: refactor open-coded INUMBERS calls Darrick J. Wong
2019-05-20 23:17 ` [PATCH 06/12] misc: remove all use of xfs_fsop_geom_t Darrick J. Wong
2019-05-21 16:43 ` Eric Sandeen
2019-05-21 16:58 ` Darrick J. Wong
2019-05-20 23:17 ` [PATCH 07/12] libfrog: fix bitmap return values Darrick J. Wong
2019-05-21 16:54 ` Eric Sandeen
2019-05-21 17:01 ` Darrick J. Wong
2019-05-21 18:59 ` Eric Sandeen
2019-05-21 19:19 ` Christoph Hellwig
2019-05-21 19:20 ` Eric Sandeen
2019-05-21 19:28 ` Christoph Hellwig
2019-05-21 19:33 ` Eric Sandeen
2019-05-22 16:23 ` Eric Sandeen [this message]
2019-05-20 23:17 ` [PATCH 08/12] xfs_repair: refactor namecheck functions Darrick J. Wong
2019-05-21 19:16 ` Eric Sandeen
2019-05-20 23:17 ` [PATCH 09/12] xfs_scrub: fix background-mode sleep throttling Darrick J. Wong
2019-05-21 19:18 ` Eric Sandeen
2019-05-20 23:17 ` [PATCH 10/12] mkfs: allow setting dax flag on root directory Darrick J. Wong
2019-05-21 19:19 ` Eric Sandeen
2019-05-20 23:17 ` [PATCH 11/12] mkfs: validate start and end of aligned logs Darrick J. Wong
2019-05-21 19:24 ` Eric Sandeen
2019-05-22 16:42 ` Darrick J. Wong
2019-05-20 23:18 ` [PATCH 12/12] mkfs: enable reflink by default Darrick J. Wong
2019-05-21 19:27 ` Eric Sandeen
2019-05-21 19:30 ` [PATCH 12/12 V2] " Eric Sandeen
2019-05-22 16:44 ` Darrick J. Wong
2019-05-22 16:46 ` Eric Sandeen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=30afb811-c136-24f2-5c15-71c3641ef78f@sandeen.net \
--to=sandeen@sandeen.net \
--cc=darrick.wong@oracle.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.