[Drbd-dev] main.c comments

[Drbd-dev] main.c comments

[David Butterfield]

These three comments about main.c are the end of what I have on DRBD for now.

Three more things I noticed in drbd_main.c:

(1) Isn't the third argument to module_param_named() supposed to be the type of
    the second argument?  (But seems to require it as a single token??)

(2) The cast avoids a compiler warning about signed/unsigned comparison.

(3) q->queue_lock is needed by blk_queue_flag_set(), even if !defined(blk_queue_plugged),
    so I would move its initialization outside the #ifdef blk_queue_plugged.

diff --git a/drbd/drbd_main.c b/drbd/drbd_main.c
index 4204deff..69245c57 100644
--- a/drbd/drbd_main.c
+++ b/drbd/drbd_main.c
@@ -146,7 +153,7 @@ const struct kernel_param_ops param_ops_drbd_protocol_version = {
 #endif

 unsigned int drbd_protocol_version_min = PRO_VERSION_MIN;
-module_param_named(protocol_version_min, drbd_protocol_version_min, drbd_protocol_version, 0644);
+module_param_named(protocol_version_min, drbd_protocol_version_min, uint, 0644);


 /* in 2.6.x, our device mapping and config info contains our virtual gendisks
@@ -1831,7 +1840,7 @@ static void dcbp_set_start(struct p_compressed_bm *p, int set)
 static void dcbp_set_pad_bits(struct p_compressed_bm *p, int n)
 {
        BUG_ON(n & ~0x7);
-       p->encoding = (p->encoding & (~0x7 << 4)) | (n << 4);
+       p->encoding = (p->encoding & ((unsigned)~0x7 << 4)) | (n << 4);
 }

 static int fill_bitmap_rle_bits(struct drbd_peer_device *peer_device,
@@ -3760,8 +3769,9 @@ enum drbd_ret_code drbd_create_device(struct drbd_config_context *adm_ctx, unsig
 #ifdef COMPAT_HAVE_BLK_QUEUE_MERGE_BVEC
        blk_queue_merge_bvec(q, drbd_merge_bvec);
 #endif
+       q->queue_lock = &resource->req_lock; /* used by blk_queue_flag_set() */
 #ifdef blk_queue_plugged
-       q->queue_lock = &resource->req_lock; /* needed since we use */
+       /* needed since we use */
        /* plugging on a queue, that actually has no requests! */
        q->unplug_fn = drbd_unplug_fn;
 #endif

Re: [Drbd-dev] main.c comments

[Lars Ellenberg]

On Wed, Jun 05, 2019 at 10:15:23AM -0600, David Butterfield wrote:
> These three comments about main.c are the end of what I have on DRBD for now.
> 
> Three more things I noticed in drbd_main.c:
> 
> (1) Isn't the third argument to module_param_named() supposed to be the type of
>     the second argument?  (But seems to require it as a single token??)

Yes and no ;-)
the type of the second argument in this case is a drbd_protocol_version,
so by naming that as third argument, we point to the "conversion and
validation functions" for that type.

See param_ops_drbd_protocol_version

again, "details depend on the kernel version"...

> (2) The cast avoids a compiler warning about signed/unsigned comparison.

As long as our kernel compiles complete without warnings, "boring".

> (3) q->queue_lock is needed by blk_queue_flag_set(), even if !defined(blk_queue_plugged),
>     so I would move its initialization outside the #ifdef blk_queue_plugged.

No. This is compat code, and needs to be that way.
See how the q->queue_lock evolved over time in the kernel,
used to be a pointer, without implicit initialization,
then was changed to implicit initialization "sometimes"
to an embeded struct, then was changed to implicit initialization
always to that embeded struct, then was changed to become that embeded
struct itself.

> diff --git a/drbd/drbd_main.c b/drbd/drbd_main.c
> index 4204deff..69245c57 100644
> --- a/drbd/drbd_main.c
> +++ b/drbd/drbd_main.c
> @@ -146,7 +153,7 @@ const struct kernel_param_ops param_ops_drbd_protocol_version = {
>  #endif
> 
>  unsigned int drbd_protocol_version_min = PRO_VERSION_MIN;
> -module_param_named(protocol_version_min, drbd_protocol_version_min, drbd_protocol_version, 0644);
> +module_param_named(protocol_version_min, drbd_protocol_version_min, uint, 0644);
> 
> 
>  /* in 2.6.x, our device mapping and config info contains our virtual gendisks
> @@ -1831,7 +1840,7 @@ static void dcbp_set_start(struct p_compressed_bm *p, int set)
>  static void dcbp_set_pad_bits(struct p_compressed_bm *p, int n)
>  {
>         BUG_ON(n & ~0x7);
> -       p->encoding = (p->encoding & (~0x7 << 4)) | (n << 4);
> +       p->encoding = (p->encoding & ((unsigned)~0x7 << 4)) | (n << 4);
>  }
> 
>  static int fill_bitmap_rle_bits(struct drbd_peer_device *peer_device,
> @@ -3760,8 +3769,9 @@ enum drbd_ret_code drbd_create_device(struct drbd_config_context *adm_ctx, unsig
>  #ifdef COMPAT_HAVE_BLK_QUEUE_MERGE_BVEC
>         blk_queue_merge_bvec(q, drbd_merge_bvec);
>  #endif
> +       q->queue_lock = &resource->req_lock; /* used by blk_queue_flag_set() */
>  #ifdef blk_queue_plugged
> -       q->queue_lock = &resource->req_lock; /* needed since we use */
> +       /* needed since we use */
>         /* plugging on a queue, that actually has no requests! */
>         q->unplug_fn = drbd_unplug_fn;
>  #endif

Re: [Drbd-dev] main.c comments

[David Butterfield]

>> (1) Isn't the third argument to module_param_named() supposed to be the type of
>>     the second argument?  (But seems to require it as a single token??)
> 
> Yes and no ;-)
> the type of the second argument in this case is a drbd_protocol_version,
> so by naming that as third argument, we point to the "conversion and
> validation functions" for that type.

Aha!  Thanks.  All I was doing was comparing the types at compile-time
and not finding a match.

> again, "details depend on the kernel version"...

Yeah, I'm pretending to be 2.6.32, the minimum supported by DRBD's kernel
compatibility, to minimize the amount of kernel function I have to emulate.

>> (2) The cast avoids a compiler warning about signed/unsigned comparison.
> 
> As long as our kernel compiles complete without warnings, "boring".

OK, I'll set my own warnings back down to the same ones the kernel enables.

For warnings that happen a lot I already disabled them, but there were
three warnings that each was reported only once in 51,444 lines of DRBD
code -- I had figured those conditions were being maintained and not
supposed to be there, so those are the ones I mentioned.

David Butterfield

Re: [Drbd-dev] q->queue_lock initialization

[David Butterfield]

Regarding initialization of q->queue_lock:

Apparently the DRBD source requires that
    !defined(COMPAT_HAVE_BLK_QUEUE_FLAG_SET)  implies  defined(blk_queue_plugged)

So I suppose that must be true in all Linux kernel versions at or above 2.6.32.

But I did not implement or #define blk_queue_plugged, so the initialization of
q->queue_lock at drbd_main.c:3773 did not happen, so the spin_lock_irqsave() in the
compatibility version of blk_queue_flag_set:1942 would use the NULL q->queue_lock.

So I will look into defining blk_queue_plugged in my ~2.6.32 environment.
Thanks for your help understanding this.

From drbd_nl.c:
1937 #ifndef COMPAT_HAVE_BLK_QUEUE_FLAG_SET
1938 static void blk_queue_flag_set(unsigned int flag, struct request_queue *q)
1939 {
1940         unsigned long flags;
1941
1942         spin_lock_irqsave(q->queue_lock, flags);
1943         queue_flag_set(flag, q);
1944         spin_unlock_irqrestore(q->queue_lock, flags);
1945 }

From drbd_main.c:
3772 #ifdef blk_queue_plugged
3773         q->queue_lock = &resource->req_lock; /* needed since we use */
3774         /* plugging on a queue, that actually has no requests! */
3775         q->unplug_fn = drbd_unplug_fn;
3776 #endif

drbdsetup attach leads to spin_lock(NULL):

/usermode_compat/src/usermode_lib.h:1098:19: runtime error: member access within null pointer of type 'struct spinlock_t'
Thread 5 "netlink_recv" received signal SIGSEGV, Segmentation fault.
(gdb)
(gdb) bt
#0  __GI___pthread_mutex_trylock (mutex=0x8) at ../nptl/pthread_mutex_trylock.c:41
#1  0x0000555555c2994a in _spin_lock_try (lock=0x0, whence=0x555555e99859 "drbd_nl.c:1951") at usermode_compat/src/usermode_lib.h:1098
#2  0x0000555555c2a070 in _spin_lock (lock=0x0, whence=0x555555e99859 "drbd_nl.c:1951") at usermode_compat/src/usermode_lib.h:1125
#3  0x0000555555c60f73 in blk_queue_flag_clear (flag=4294967295, q=0x7fff6400c800) at drbd_nl.c:1951
#4  0x0000555555c613c9 in decide_on_discard_support (device=0x7fff6401c400, q=0x7fff6400c800, b=0x5555562374c0, discard_zeroes_if_aligned=true) at drbd_nl.c:1987
#5  0x0000555555c6196d in drbd_setup_queue_param (device=0x7fff6401c400, bdev=0x7fff6405d400, max_bio_size=1048576, o=0x0) at drbd_nl.c:2115
#6  0x0000555555c62194 in drbd_reconsider_queue_parameters (device=0x7fff6401c400, bdev=0x7fff6405d400, o=0x0) at drbd_nl.c:2143
#7  0x0000555555c710b0 in drbd_adm_attach (skb=0x7fff64037d40, info=0x7ffff5626460) at drbd_nl.c:3042
#8  0x0000555555e33586 in genl_rcv_msg (skb=0x7fff64037d40, nlh=0x7fff6405f000) at UMC_genl.c:566
#9  0x0000555555e33cfa in netlink_rcv_skb (skb=0x7fff64037d40, cb=0x555555e32e99 <genl_rcv_msg>) at UMC_genl.c:618
#10 0x0000555555e33ef9 in genl_rcv (skb=0x7fff64037d40) at UMC_genl.c:639
#11 0x0000555555e2d78b in on_netlink_recv (sk=0x555556233098, len=0) at usermode_lib.c:994
#12 0x0000555555e2b9dc in UMC_sock_recv_event (env=0x555556233000, events=1, err=0) at usermode_lib.c:792
#13 0x00007ffff7f5eec3 in callback_deliver (err=0, arg=1, cb=0x5555562357c0) at mte_event_task.c:128
#14 event_poll_deliver (nevents=<optimized out>, events=<optimized out>) at mte_event_task.c:254
#15 event_task_loop (event_task=event_task@entry=0x555556234e00) at mte_event_task.c:741
#16 0x00007ffff7f62d0f in mte_event_task_run (event_task=0x555556234e00) at mte_event_task.c:860
#17 0x0000555555e2276f in UMC_irqthread_fn (v_irqthread=0x555556233a00) at usermode_lib.c:187
#18 0x00007ffff7f4cc05 in sys_thread_fn (env=<optimized out>) at mte_service.c:145
#19 0x00007ffff757d182 in start_thread (arg=<optimized out>) at pthread_create.c:486
#20 0x00007ffff74a6b1f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Nice to be able to use valgrind and gdb and libubsan!