Re: [Drbd-dev] [PATCH 28/42] drbd: switch to proc_create_single
On Wed, May 16, 2018 at 11:43:32AM +0200, Christoph Hellwig wrote: > And stop messing with try_module_get on THIS_MODULE, which doesn't make > any sense here. The idea was to increase module count on /proc/drbd access. If someone holds /proc/drbd open, previously rmmod would "succeed" in starting the unload, but then block on remove_proc_entry, leading to a situation where the lsmod does not show drbd anymore, but /proc/drbd being still there (but no longer accessible). I'd rather have rmmod fail up front in this case. And try_module_get() seemed most appropriate. Lars
Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in nl messages
On Tue, May 10, 2016 at 11:39:49AM -0400, David Miller wrote: > From: Lars Ellenberg <lars.ellenb...@linbit.com> > Date: Tue, 10 May 2016 11:40:23 +0200 excuse me for reordering the original: > Anyways, back to the topic, can you please just relent and come to > some kind of agreement about the fix for this alignment bug? I thought we did? I'm fine with the "v3", it even carries my signed-of-by. Whether or not Nicholas wants to prefix those headers with drbd_, I don't really care. > This is taking a very long time and patches are just rotting in > patchwork with no resolution. Why would Nicholas asked how to go about DRBD, I suggested to use 0 as a padding attribute, and after taking a detour, he did. All good. Rest of original: > > If we introduce a new config option, > > we have to add it to the config scanner (one line), > > define min, max, default and scale (four short defines), > > and add it to the netlink definition here (one line). > > Done, rest of the code is generated, > > both on the kernel side, > > and on the drbd-utils side used to talk to the kernel. > > We found that to be very convenient. > > But it entirely misses the core design point of netlink. > > Sender and receive _DO NOT_ need to coordinate at all. That's the > whole point. So tightly coupling such coordination is going to run > you into all kinds of problems. > > When implemented properly, the sender can emit whatever attributes it > knows about and can generate, and the receive scans the attributes one > by one and picks out the ones it understands and processes them. > > If you go against this model > then you have no clean way to We don't. We extend (not violate) that model, so the sender *may* indicate to the recipient that for some particular attribute, the sender would rather have an "I don't understand this" return than a silent ignore. And that we can indicate in the definition of the attributes which ones are required to make a message meaningful. > extend things whilst allowing existing software to continue working. *that* is exactly why we use netlink, and why we do things with it the way we do. Actually I think what we are doing there is, comparatively, "elegant". You obviously don't have to agree. I could discuss this in more detail, but I assume you are not really interested, at least not here and now. Thanks, Lars
Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in nl messages
On Tue, May 10, 2016 at 11:09:53AM +0200, Nicolas Dichtel wrote:
> Le 09/05/2016 15:15, Lars Ellenberg a écrit :
> > On Mon, May 09, 2016 at 11:40:20AM +0200, Nicolas Dichtel wrote:
> [snip]
> >> Maybe prefixing genl_magic_func.h and genl_magic_struct.h by 'drbd_'
> >> could be interesting so that new module won't use it. What is your
> >> opinion?
> >
> > This was supposed to not be DRBD specific. But it might even still
> > need some massaging before it was truly generic. And obviously,
> > it does not meet the taste of genetlink folks, to say the least :(
> Yes, this file is not generic and netlink APIs are never defined like this.
> These tons of macro complexifies the code too much. It's overengineering for
> what purpose?
If we introduce a new config option,
we have to add it to the config scanner (one line),
define min, max, default and scale (four short defines),
and add it to the netlink definition here (one line).
Done, rest of the code is generated,
both on the kernel side,
and on the drbd-utils side used to talk to the kernel.
We found that to be very convenient.
> Small examples:
> - the drbd netlink API is not exported via uapi (I wonder how apps using this
>API get it)
There used to be a time where there was no "uapi".
(I wonder how apps ever worked back then).
> - v2 of the patch is nacked because adding a new attribute may break existing
No.
But because the "new" attributes you chose have not been new,
but already used (though not yet merged back into mainline yet).
(Which you did not realize, and had no obvious way of knowing.
Could have been fixed.).
And because your patch introduced useless new members to the structs.
(Could also have been fixed).
And because I did not see any use defining that many new "padding attributes"
for no reason, where the obvious (to me) choice was to use 0, and you
did not even try to explain why that would have been a bad choice.
>apps (in networking code, a lot of new attributes are added in each
> version)
> - it's not possible to grep to show the definition of an attribute ('git grep
>-w T_bits_total' returns only 1 line)
Opencoded, it would return 2.
;-)
Is this going somewhere?
Cheers,
Lars
Re: [PATCH net-next v3] block/drbd: align properly u64 in nl messages
On Mon, May 09, 2016 at 11:40:20AM +0200, Nicolas Dichtel wrote: > The attribute 0 is never used in drbd, so let's use it as pad attribute > in netlink messages. This minimizes the patch. > > Note that this patch is only compile-tested. > > Signed-off-by: Nicolas Dichtel <nicolas.dich...@6wind.com> > Signed-off-by: Lars Ellenberg <lars.ellenb...@linbit.com> > --- > > v2 -> v3: > use 0 as padattr instead of adding new attributes Thanks. > v1 -> v2: > rework the patch to handle all cases > > Maybe prefixing genl_magic_func.h and genl_magic_struct.h by 'drbd_' > could be interesting so that new module won't use it. What is your > opinion? This was supposed to not be DRBD specific. But it might even still need some massaging before it was truly generic. And obviously, it does not meet the taste of genetlink folks, to say the least :( I don't care either way. Lars Ellenberg
Re: [Drbd-dev] [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()
On Wed, May 04, 2016 at 02:49:00PM +0200, Nicolas Dichtel wrote: > Le 04/05/2016 11:05, Lars Ellenberg a écrit : > [snip] > > We don't have an "alignment problem" there, btw. > > Last time I checked, we did work fine without this alignment magic, > > we already take care of that, yes, even on affected architectures. > The code adds several consecutive u64 attributes. The nl attribute header is 4 > bytes, thus the full attribute length is 12 bytes. If the first u64 is aligned > on 8 (nla_data()), the next one is not aligned on 8: it starts 12 bytes (8 > (u64) > + 4 (nl attr hdr)) after the previous u64. Yes. Which in our case is not a problem. But I don't object to the padding per se, if that is how things "should be". I try to understand why you so much object to using 0 as pad. Lars
Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()
On Tue, May 03, 2016 at 12:05:56PM -0400, David Miller wrote: > From: Lars Ellenberg <lars.ellenb...@linbit.com> > Date: Tue, 3 May 2016 12:06:44 +0200 > > > Please just NOT use an additional "field", > > but always use 0 to pad. > > You can't, it doesn't work. I did, and it *did* work. At least, it appeared to. I'm not talking about every user of netlink out there. That I don't know. But specifically for DRBD netlink, from what my experiments tell me, it works just fine. > We are adding a new field to every netlink protocol family that has > this alignment problem. We don't have an "alignment problem" there, btw. Last time I checked, we did work fine without this alignment magic, we already take care of that, yes, even on affected architectures. On Tue, May 03, 2016 at 12:06:52PM -0400, David Miller wrote: > From: Lars Ellenberg <lars.ellenb...@linbit.com> > Date: Tue, 3 May 2016 12:06:44 +0200 > > > Whereas using some arbitrary value will be wrong, > > and will needlessly break userland. > > It cannot break userland. It can, if those tags have been used already. There is DRBD out-of-tree as well, it usually is ahead of in-tree DRBD. But yes, I could obviously check and assign and reserve some not-yet-used tag to all of them. I don't see why, though, given that 0 (appearently) works fine. Can you elaborate why and how that does not work? Lars
Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()
On Tue, May 03, 2016 at 11:39:18AM +0200, Nicolas Dichtel wrote:
> Two new handlers have been defined in genl_magic_ headers:
> - __field2: the corresponding nla_put() function (nla_put_flag()) takes
> only two args
> - __field4: the corresponding nla_put() function (nla_put_u64_64bit())
> takes four args
>
> __field2 allows us to define __unspec_field for padding attribute.
> __field4 allows us to update the definition of __u64_field: the pad
> attribute should now be specified.
Please just NOT use an additional "field",
but always use 0 to pad.
Patch is much shorter as well, see below.
Attribute type "0" is not used,
and will never be of semantic value,
but always be ignored in the DRBD netlink family.
Whereas using some arbitrary value will be wrong,
and will needlessly break userland.
Thanks,
Lars Ellenberg
diff --git a/include/linux/genl_magic_struct.h
b/include/linux/genl_magic_struct.h
index eecd19b..6270a56 100644
--- a/include/linux/genl_magic_struct.h
+++ b/include/linux/genl_magic_struct.h
@@ -62,6 +62,11 @@ extern void CONCAT_(GENL_MAGIC_FAMILY,
_genl_unregister)(void);
/* MAGIC helpers {{{2 */
+static inline int nla_put_u64_0pad(struct sk_buff *skb, int attrtype, u64
value)
+{
+ return nla_put_64bit(skb, attrtype, sizeof(u64), , 0);
+}
+
/* possible field types */
#define __flg_field(attr_nr, attr_flag, name) \
__field(attr_nr, attr_flag, name, NLA_U8, char, \
@@ -80,7 +85,7 @@ extern void CONCAT_(GENL_MAGIC_FAMILY,
_genl_unregister)(void);
nla_get_u32, nla_put_u32, true)
#define __u64_field(attr_nr, attr_flag, name) \
__field(attr_nr, attr_flag, name, NLA_U64, __u64, \
- nla_get_u64, nla_put_u64, false)
+ nla_get_u64, nla_put_u64_0pad, false)
#define __str_field(attr_nr, attr_flag, name, maxlen) \
__array(attr_nr, attr_flag, name, NLA_NUL_STRING, char, maxlen, \
nla_strlcpy, nla_put, false)
diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index 1fd1dcc..206cc76 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -3633,14 +3633,14 @@ static int nla_put_status_info(struct sk_buff *skb,
struct drbd_device *device,
goto nla_put_failure;
if (nla_put_u32(skb, T_sib_reason, sib ? sib->sib_reason :
SIB_GET_STATUS_REPLY) ||
nla_put_u32(skb, T_current_state, device->state.i) ||
- nla_put_u64(skb, T_ed_uuid, device->ed_uuid) ||
- nla_put_u64(skb, T_capacity, drbd_get_capacity(device->this_bdev))
||
- nla_put_u64(skb, T_send_cnt, device->send_cnt) ||
- nla_put_u64(skb, T_recv_cnt, device->recv_cnt) ||
- nla_put_u64(skb, T_read_cnt, device->read_cnt) ||
- nla_put_u64(skb, T_writ_cnt, device->writ_cnt) ||
- nla_put_u64(skb, T_al_writ_cnt, device->al_writ_cnt) ||
- nla_put_u64(skb, T_bm_writ_cnt, device->bm_writ_cnt) ||
+ nla_put_u64_0pad(skb, T_ed_uuid, device->ed_uuid) ||
+ nla_put_u64_0pad(skb, T_capacity,
drbd_get_capacity(device->this_bdev)) ||
+ nla_put_u64_0pad(skb, T_send_cnt, device->send_cnt) ||
+ nla_put_u64_0pad(skb, T_recv_cnt, device->recv_cnt) ||
+ nla_put_u64_0pad(skb, T_read_cnt, device->read_cnt) ||
+ nla_put_u64_0pad(skb, T_writ_cnt, device->writ_cnt) ||
+ nla_put_u64_0pad(skb, T_al_writ_cnt, device->al_writ_cnt) ||
+ nla_put_u64_0pad(skb, T_bm_writ_cnt, device->bm_writ_cnt) ||
nla_put_u32(skb, T_ap_bio_cnt, atomic_read(>ap_bio_cnt)) ||
nla_put_u32(skb, T_ap_pending_cnt,
atomic_read(>ap_pending_cnt)) ||
nla_put_u32(skb, T_rs_pending_cnt,
atomic_read(>rs_pending_cnt)))
@@ -3657,13 +3657,13 @@ static int nla_put_status_info(struct sk_buff *skb,
struct drbd_device *device,
goto nla_put_failure;
if (nla_put_u32(skb, T_disk_flags, device->ldev->md.flags) ||
- nla_put_u64(skb, T_bits_total, drbd_bm_bits(device)) ||
- nla_put_u64(skb, T_bits_oos, drbd_bm_total_weight(device)))
+ nla_put_u64_0pad(skb, T_bits_total, drbd_bm_bits(device)) ||
+ nla_put_u64_0pad(skb, T_bits_oos,
drbd_bm_total_weight(device)))
goto nla_put_failure;
if (C_SYNC_SOURCE <= device->state.conn &&
C_PAUSED_SYNC_T >= device->state.conn) {
- if (nla_put_u64(skb, T_bits_rs_total, device->rs_total)
||
- nla_put_u64(skb, T_bits_rs_failed,
device->rs_failed))
+ if (nla_put_u64_0pad(skb, T_bits_rs_total,
device->rs_total) ||
+ nla_put_u64_0pad(skb, T_bits_rs_failed,
device->rs_failed))
goto nla_put_failure;
}
}
Re: [Drbd-dev] [PATCH net-next 0/8] netlink: align attributes when needed (patchset #3)
On Tue, Apr 26, 2016 at 01:54:27PM +0200, Lars Ellenberg wrote:
> On Tue, Apr 26, 2016 at 10:06:10AM +0200, Nicolas Dichtel wrote:
> >
> > This is the continuation (series #3) of the work done to align netlink
> > attributes when these attributes contain some 64-bit fields.
> >
> > It's the last patchset from what I've seen.
> >
> > The last user of nla_put_u64() is block/drbd. This module does not use
> > standard netlink API (see all the stuff in include/linux/genl_magic_struct.h
> > and include/linux/genl_magic_func.h). I didn't modify it because it's seems
> > hard to do it whithout testing and fully understanding the context
>
> Something like this should just work.
> + * @attrtype: attribute type
> + * @value: numeric value
> + */
> +static inline int nla_put_u64_64bit_unspec(struct sk_buff *skb, int attrtype,
> + u64 value)
> +{
> + return nla_put_64bit(skb, attrtype, sizeof(u64), , NLA_UNSPEC);
Ok, I confused attribute and policy type there for a sec.
Anyways, 0 works just fine,
all our nested attribute enums are != 0,
because nla_parse skips type 0.
Lars
Re: [PATCH net-next 0/8] netlink: align attributes when needed (patchset #3)
On Tue, Apr 26, 2016 at 10:06:10AM +0200, Nicolas Dichtel wrote:
>
> This is the continuation (series #3) of the work done to align netlink
> attributes when these attributes contain some 64-bit fields.
>
> It's the last patchset from what I've seen.
>
> The last user of nla_put_u64() is block/drbd. This module does not use
> standard netlink API (see all the stuff in include/linux/genl_magic_struct.h
> and include/linux/genl_magic_func.h). I didn't modify it because it's seems
> hard to do it whithout testing and fully understanding the context
Something like this should just work.
diff --git a/include/linux/genl_magic_struct.h
b/include/linux/genl_magic_struct.h
index eecd19b..5715dac 100644
--- a/include/linux/genl_magic_struct.h
+++ b/include/linux/genl_magic_struct.h
@@ -80,7 +80,7 @@ extern void CONCAT_(GENL_MAGIC_FAMILY,
_genl_unregister)(void);
nla_get_u32, nla_put_u32, true)
#define __u64_field(attr_nr, attr_flag, name) \
__field(attr_nr, attr_flag, name, NLA_U64, __u64, \
- nla_get_u64, nla_put_u64, false)
+ nla_get_u64, nla_put_u64_64bit_unspec, false)
#define __str_field(attr_nr, attr_flag, name, maxlen) \
__array(attr_nr, attr_flag, name, NLA_NUL_STRING, char, maxlen, \
nla_strlcpy, nla_put, false)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index e589cb3..38ba770 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -871,6 +871,18 @@ static inline int nla_put_u64_64bit(struct sk_buff *skb,
int attrtype,
}
/**
+ * nla_put_u64_64bit_unspec - nla_put_u64_64bit() with padattr = 0
+ * @skb: socket buffer to add attribute to
+ * @attrtype: attribute type
+ * @value: numeric value
+ */
+static inline int nla_put_u64_64bit_unspec(struct sk_buff *skb, int attrtype,
+ u64 value)
+{
+ return nla_put_64bit(skb, attrtype, sizeof(u64), , NLA_UNSPEC);
+}
+
+/**
* nla_put_be64 - Add a __be64 netlink attribute to a socket buffer and align
it
* @skb: socket buffer to add attribute to
* @attrtype: attribute type
> (for
> example, why include/linux/drbd_genl.h is not part of uapi?).
> Any thoughts?
probably should be.
Thanks,
Lars
