Skip to content

Commit

Permalink
drbd: describe bitmap locking for bulk operation in finer detail
Browse files Browse the repository at this point in the history 
Now that we do no longer in-place endian-swap the bitmap, we allow
selected bitmap operations (testing bits, sometimes even settting bits)
during some bulk operations.

This caused us to hit a lot of FIXME asserts similar to
	FIXME asender in drbd_bm_count_bits,
	bitmap locked for 'write from resync_finished' by worker
Which now is nonsense: looking at the bitmap is perfectly legal
as long as it is not being resized.

This cosmetic patch defines some flags to describe expectations in finer
detail, so the asserts in e.g. bm_change_bits_to() can be skipped if
appropriate.

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com>
  • Loading branch information
Lars Ellenberg authored and Philipp Reisner committed Mar 10, 2011
1 parent 62b0da3 commit 20ceb2b
Show file tree
Hide file tree
Showing 5 changed files with 115 additions and 63 deletions.
48 changes: 20 additions & 28 deletions drivers/block/drbd/drbd_bitmap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,26 +104,16 @@ struct drbd_bitmap {

wait_queue_head_t bm_io_wait; /* used to serialize IO of single pages */

unsigned long bm_flags;
enum bm_flag bm_flags;

/* debugging aid, in case we are still racy somewhere */
char *bm_why;
struct task_struct *bm_task;
};

/* definition of bits in bm_flags */
#define BM_LOCKED 0
// #define BM_MD_IO_ERROR 1 unused now.
#define BM_P_VMALLOCED 2

static int __bm_change_bits_to(struct drbd_conf *mdev, const unsigned long s,
unsigned long e, int val, const enum km_type km);

static int bm_is_locked(struct drbd_bitmap *b)
{
return test_bit(BM_LOCKED, &b->bm_flags);
}

#define bm_print_lock_info(m) __bm_print_lock_info(m, __func__)
static void __bm_print_lock_info(struct drbd_conf *mdev, const char *func)
{
Expand All @@ -140,7 +130,7 @@ static void __bm_print_lock_info(struct drbd_conf *mdev, const char *func)
b->bm_task == mdev->worker.task ? "worker" : "?");
}

void drbd_bm_lock(struct drbd_conf *mdev, char *why)
void drbd_bm_lock(struct drbd_conf *mdev, char *why, enum bm_flag flags)
{
struct drbd_bitmap *b = mdev->bitmap;
int trylock_failed;
Expand All @@ -163,8 +153,9 @@ void drbd_bm_lock(struct drbd_conf *mdev, char *why)
b->bm_task == mdev->worker.task ? "worker" : "?");
mutex_lock(&b->bm_change);
}
if (__test_and_set_bit(BM_LOCKED, &b->bm_flags))
if (BM_LOCKED_MASK & b->bm_flags)
dev_err(DEV, "FIXME bitmap already locked in bm_lock\n");
b->bm_flags |= flags & BM_LOCKED_MASK;

b->bm_why = why;
b->bm_task = current;
Expand All @@ -178,9 +169,10 @@ void drbd_bm_unlock(struct drbd_conf *mdev)
return;
}

if (!__test_and_clear_bit(BM_LOCKED, &mdev->bitmap->bm_flags))
if (!(BM_LOCKED_MASK & mdev->bitmap->bm_flags))
dev_err(DEV, "FIXME bitmap not locked in bm_unlock\n");

b->bm_flags &= ~BM_LOCKED_MASK;
b->bm_why = NULL;
b->bm_task = NULL;
mutex_unlock(&b->bm_change);
Expand Down Expand Up @@ -421,9 +413,9 @@ static struct page **bm_realloc_pages(struct drbd_bitmap *b, unsigned long want)
}

if (vmalloced)
set_bit(BM_P_VMALLOCED, &b->bm_flags);
b->bm_flags |= BM_P_VMALLOCED;
else
clear_bit(BM_P_VMALLOCED, &b->bm_flags);
b->bm_flags &= ~BM_P_VMALLOCED;

return new_pages;
}
Expand Down Expand Up @@ -460,7 +452,7 @@ void drbd_bm_cleanup(struct drbd_conf *mdev)
{
ERR_IF (!mdev->bitmap) return;
bm_free_pages(mdev->bitmap->bm_pages, mdev->bitmap->bm_number_of_pages);
bm_vk_free(mdev->bitmap->bm_pages, test_bit(BM_P_VMALLOCED, &mdev->bitmap->bm_flags));
bm_vk_free(mdev->bitmap->bm_pages, (BM_P_VMALLOCED & mdev->bitmap->bm_flags));
kfree(mdev->bitmap);
mdev->bitmap = NULL;
}
Expand Down Expand Up @@ -623,15 +615,15 @@ int drbd_bm_resize(struct drbd_conf *mdev, sector_t capacity, int set_new_bits)

ERR_IF(!b) return -ENOMEM;

drbd_bm_lock(mdev, "resize");
drbd_bm_lock(mdev, "resize", BM_LOCKED_MASK);

dev_info(DEV, "drbd_bm_resize called with capacity == %llu\n",
(unsigned long long)capacity);

if (capacity == b->bm_dev_capacity)
goto out;

opages_vmalloced = test_bit(BM_P_VMALLOCED, &b->bm_flags);
opages_vmalloced = (BM_P_VMALLOCED & b->bm_flags);

if (capacity == 0) {
spin_lock_irq(&b->bm_lock);
Expand Down Expand Up @@ -1030,7 +1022,7 @@ static int bm_rw(struct drbd_conf *mdev, int rw, unsigned lazy_writeout_upper_id
* as we submit copies of pages anyways.
*/
if (!ctx.flags)
WARN_ON(!bm_is_locked(b));
WARN_ON(!(BM_LOCKED_MASK & b->bm_flags));

num_pages = b->bm_number_of_pages;

Expand Down Expand Up @@ -1220,7 +1212,7 @@ static unsigned long bm_find_next(struct drbd_conf *mdev,
ERR_IF(!b->bm_pages) return i;

spin_lock_irq(&b->bm_lock);
if (bm_is_locked(b))
if (BM_DONT_TEST & b->bm_flags)
bm_print_lock_info(mdev);

i = __bm_find_next(mdev, bm_fo, find_zero_bit, KM_IRQ1);
Expand All @@ -1246,13 +1238,13 @@ unsigned long drbd_bm_find_next_zero(struct drbd_conf *mdev, unsigned long bm_fo
* you must take drbd_bm_lock() first */
unsigned long _drbd_bm_find_next(struct drbd_conf *mdev, unsigned long bm_fo)
{
/* WARN_ON(!bm_is_locked(mdev)); */
/* WARN_ON(!(BM_DONT_SET & mdev->b->bm_flags)); */
return __bm_find_next(mdev, bm_fo, 0, KM_USER1);
}

unsigned long _drbd_bm_find_next_zero(struct drbd_conf *mdev, unsigned long bm_fo)
{
/* WARN_ON(!bm_is_locked(mdev)); */
/* WARN_ON(!(BM_DONT_SET & mdev->b->bm_flags)); */
return __bm_find_next(mdev, bm_fo, 1, KM_USER1);
}

Expand Down Expand Up @@ -1322,7 +1314,7 @@ static int bm_change_bits_to(struct drbd_conf *mdev, const unsigned long s,
ERR_IF(!b->bm_pages) return 0;

spin_lock_irqsave(&b->bm_lock, flags);
if (bm_is_locked(b))
if ((val ? BM_DONT_SET : BM_DONT_CLEAR) & b->bm_flags)
bm_print_lock_info(mdev);

c = __bm_change_bits_to(mdev, s, e, val, KM_IRQ1);
Expand Down Expand Up @@ -1439,7 +1431,7 @@ int drbd_bm_test_bit(struct drbd_conf *mdev, const unsigned long bitnr)
ERR_IF(!b->bm_pages) return 0;

spin_lock_irqsave(&b->bm_lock, flags);
if (bm_is_locked(b))
if (BM_DONT_TEST & b->bm_flags)
bm_print_lock_info(mdev);
if (bitnr < b->bm_bits) {
p_addr = bm_map_pidx(b, bm_bit_to_page_idx(b, bitnr));
Expand Down Expand Up @@ -1474,7 +1466,7 @@ int drbd_bm_count_bits(struct drbd_conf *mdev, const unsigned long s, const unsi
ERR_IF(!b->bm_pages) return 1;

spin_lock_irqsave(&b->bm_lock, flags);
if (bm_is_locked(b))
if (BM_DONT_TEST & b->bm_flags)
bm_print_lock_info(mdev);
for (bitnr = s; bitnr <= e; bitnr++) {
unsigned int idx = bm_bit_to_page_idx(b, bitnr);
Expand Down Expand Up @@ -1522,7 +1514,7 @@ int drbd_bm_e_weight(struct drbd_conf *mdev, unsigned long enr)
ERR_IF(!b->bm_pages) return 0;

spin_lock_irqsave(&b->bm_lock, flags);
if (bm_is_locked(b))
if (BM_DONT_TEST & b->bm_flags)
bm_print_lock_info(mdev);

s = S2W(enr);
Expand Down Expand Up @@ -1555,7 +1547,7 @@ unsigned long drbd_bm_ALe_set_all(struct drbd_conf *mdev, unsigned long al_enr)
ERR_IF(!b->bm_pages) return 0;

spin_lock_irq(&b->bm_lock);
if (bm_is_locked(b))
if (BM_DONT_SET & b->bm_flags)
bm_print_lock_info(mdev);
weight = b->bm_set;

Expand Down
36 changes: 32 additions & 4 deletions drivers/block/drbd/drbd_int.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -855,6 +855,32 @@ enum {

struct drbd_bitmap; /* opaque for drbd_conf */

/* definition of bits in bm_flags to be used in drbd_bm_lock
* and drbd_bitmap_io and friends. */
enum bm_flag {
/* do we need to kfree, or vfree bm_pages? */
BM_P_VMALLOCED = 0x10000, /* internal use only, will be masked out */

/* currently locked for bulk operation */
BM_LOCKED_MASK = 0x7,

/* in detail, that is: */
BM_DONT_CLEAR = 0x1,
BM_DONT_SET = 0x2,
BM_DONT_TEST = 0x4,

/* (test bit, count bit) allowed (common case) */
BM_LOCKED_TEST_ALLOWED = 0x3,

/* testing bits, as well as setting new bits allowed, but clearing bits
* would be unexpected. Used during bitmap receive. Setting new bits
* requires sending of "out-of-sync" information, though. */
BM_LOCKED_SET_ALLOWED = 0x1,

/* clear is not expected while bitmap is locked for bulk operation */
};


/* TODO sort members for performance
* MAYBE group them further */

Expand Down Expand Up @@ -920,6 +946,7 @@ struct drbd_md_io {
struct bm_io_work {
struct drbd_work w;
char *why;
enum bm_flag flags;
int (*io_fn)(struct drbd_conf *mdev);
void (*done)(struct drbd_conf *mdev, int rv);
};
Expand Down Expand Up @@ -1242,7 +1269,6 @@ extern void drbd_free_bc(struct drbd_backing_dev *ldev);
extern void drbd_mdev_cleanup(struct drbd_conf *mdev);
void drbd_print_uuids(struct drbd_conf *mdev, const char *text);

/* drbd_meta-data.c (still in drbd_main.c) */
extern void drbd_md_sync(struct drbd_conf *mdev);
extern int drbd_md_read(struct drbd_conf *mdev, struct drbd_backing_dev *bdev);
extern void drbd_uuid_set(struct drbd_conf *mdev, int idx, u64 val) __must_hold(local);
Expand All @@ -1263,10 +1289,12 @@ extern void drbd_md_mark_dirty_(struct drbd_conf *mdev,
extern void drbd_queue_bitmap_io(struct drbd_conf *mdev,
int (*io_fn)(struct drbd_conf *),
void (*done)(struct drbd_conf *, int),
char *why);
char *why, enum bm_flag flags);
extern int drbd_bitmap_io(struct drbd_conf *mdev,
int (*io_fn)(struct drbd_conf *),
char *why, enum bm_flag flags);
extern int drbd_bmio_set_n_write(struct drbd_conf *mdev);
extern int drbd_bmio_clear_n_write(struct drbd_conf *mdev);
extern int drbd_bitmap_io(struct drbd_conf *mdev, int (*io_fn)(struct drbd_conf *), char *why);
extern void drbd_go_diskless(struct drbd_conf *mdev);
extern void drbd_ldev_destroy(struct drbd_conf *mdev);

Expand Down Expand Up @@ -1452,7 +1480,7 @@ extern void drbd_bm_merge_lel(struct drbd_conf *mdev, size_t offset,
extern void drbd_bm_get_lel(struct drbd_conf *mdev, size_t offset,
size_t number, unsigned long *buffer);

extern void drbd_bm_lock(struct drbd_conf *mdev, char *why);
extern void drbd_bm_lock(struct drbd_conf *mdev, char *why, enum bm_flag flags);
extern void drbd_bm_unlock(struct drbd_conf *mdev);
/* drbd_main.c */

Expand Down
Loading

0 comments on commit 20ceb2b

Please sign in to comment.