Merge ~rafaeldtinoco/ubuntu/+source/ndctl:lp1811785-disco into ubuntu/+source/ndctl:ubuntu/disco-devel

Proposed by Rafael David Tinoco
Status: Rejected
Rejected by: Christian Ehrhardt 
Proposed branch: ~rafaeldtinoco/ubuntu/+source/ndctl:lp1811785-disco
Merge into: ubuntu/+source/ndctl:ubuntu/disco-devel
Diff against target: 132 lines (+101/-1)
4 files modified
debian/changelog (+6/-0)
debian/control (+2/-1)
debian/patches/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch (+92/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Rafael David Tinoco (community) Needs Fixing
Christian Ehrhardt  (community) Needs Fixing
Review via email: mp+375929@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

An XML file for a KVM guest (example) can be found here:

https://bugs.launchpad.net/ubuntu/+source/ndctl/+bug/1853506/comments/2

And an example on how to test ndctl can be found here:

https://bugs.launchpad.net/ubuntu/+source/ndctl/+bug/1853506/comments/1

Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

I think the fix itself is good, but a few things need cleanup.

changelog: we usually name the patch before the colon:
so instead
     * ndctl/init-labels: Fix label slot accounting per UEFI 2.7 (LP: #1811785)
maybe:
     * d/p/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch: Fix label slot accounting per UEFI 2.7 (LP: #1811785)

Furthermore the bug should get a proper SRU Template before sponsoring it into -unapproved.

review: Needs Fixing
Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

I'm removing it from canonical-server view based on:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1811785/comments/21

and

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1811785/comments/22

If end-user chooses to continue the SRU, I'll get back to this.

review: Needs Fixing
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

No activity on this one, setting rejected to get off our overview.
Please revive if needed.

Unmerged commits

9d84733... by Rafael David Tinoco

update-maintainer

ca31b1a... by Rafael David Tinoco

changelog

862b738... by Rafael David Tinoco

 * ndctl/init-labels: Fix label slot accounting per UEFI 2.7 (LP: #1811785)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 393bc2e..89168f6 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,9 @@
6+ndctl (63-1.3ubuntu1) disco; urgency=medium
7+
8+ * d/p/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch (LP: #1811785)
9+
10+ -- Rafael David Tinoco <rafaeldtinoco@ubuntu.com> Sat, 23 Nov 2019 00:32:03 +0000
11+
12 ndctl (63-1.3) unstable; urgency=medium
13
14 * Non-maintainer upload.
15diff --git a/debian/control b/debian/control
16index e8cc763..c5cfbaf 100644
17--- a/debian/control
18+++ b/debian/control
19@@ -1,5 +1,6 @@
20 Source: ndctl
21-Maintainer: Breno Leitao <leitao@debian.org>
22+Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
23+XSBC-Original-Maintainer: Breno Leitao <leitao@debian.org>
24 Section: libs
25 Priority: optional
26 Standards-Version: 4.2.1
27diff --git a/debian/patches/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch b/debian/patches/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch
28new file mode 100644
29index 0000000..ef7f00f
30--- /dev/null
31+++ b/debian/patches/ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch
32@@ -0,0 +1,92 @@
33+From 9c6aae5635ebe8d0ca56a3ed949eef88639cb1b5 Mon Sep 17 00:00:00 2001
34+From: Dan Williams <dan.j.williams@intel.com>
35+Date: Mon, 14 Jan 2019 19:51:45 -0800
36+Subject: [PATCH] ndctl/init-labels: Fix label slot accounting per UEFI 2.7
37+
38+Quoting from Linux kernel commit 9e694d9c18dd "libnvdimm, label: change
39+nvdimm_num_label_slots per UEFI 2.7":
40+
41+ sizeof_namespace_index() fails when NVDIMM devices have the minimum
42+ 1024 bytes label storage area. nvdimm_num_label_slots() returns 3
43+ slots while the area is only big enough for 2 slots.
44+
45+ Change nvdimm_num_label_slots() to calculate a number of label slots
46+ according to UEFI 2.7 spec.
47+
48+Without this fix attempts to initialize labels on a small (1K) label
49+area results in the following:
50+
51+libndctl: sizeof_namespace_index: nmem2: label area (1024) too small to host (128 byte) labels
52+libndctl: sizeof_namespace_index: nmem2: label area (1024) too small to host (256 byte) labels
53+
54+Based on an original patch by Toshi Kani
55+Fixes: bdaec95463ca ("ndctl: introduce ndctl_dimm_{validate_labels,init_labels}")
56+Reported-by: Sujith Pandel <sujith_pandel@dell.com>
57+Link: https://github.com/pmem/ndctl/issues/78
58+Signed-off-by: Dan Williams <dan.j.williams@intel.com>
59+Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
60+---
61+ ndctl/lib/dimm.c | 35 +++++++++++++++++++++++++----------
62+ 1 file changed, 25 insertions(+), 10 deletions(-)
63+
64+diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
65+index 5e41734..634999f 100644
66+--- a/ndctl/lib/dimm.c
67++++ b/ndctl/lib/dimm.c
68+@@ -66,9 +66,27 @@ static unsigned int sizeof_namespace_label(struct nvdimm_data *ndd)
69+ return ndctl_dimm_sizeof_namespace_label(to_dimm(ndd));
70+ }
71+
72++static size_t __sizeof_namespace_index(u32 nslot)
73++{
74++ return ALIGN(sizeof(struct namespace_index) + DIV_ROUND_UP(nslot, 8),
75++ NSINDEX_ALIGN);
76++}
77++
78++static int __nvdimm_num_label_slots(struct nvdimm_data *ndd,
79++ size_t index_size)
80++{
81++ return (ndd->config_size - index_size * 2) /
82++ sizeof_namespace_label(ndd);
83++}
84++
85+ static int nvdimm_num_label_slots(struct nvdimm_data *ndd)
86+ {
87+- return ndd->config_size / (sizeof_namespace_label(ndd) + 1);
88++ u32 tmp_nslot, n;
89++
90++ tmp_nslot = ndd->config_size / sizeof_namespace_label(ndd);
91++ n = __sizeof_namespace_index(tmp_nslot) / NSINDEX_ALIGN;
92++
93++ return __nvdimm_num_label_slots(ndd, NSINDEX_ALIGN * n);
94+ }
95+
96+ static unsigned int sizeof_namespace_index(struct nvdimm_data *ndd)
97+@@ -78,18 +96,15 @@ static unsigned int sizeof_namespace_index(struct nvdimm_data *ndd)
98+ struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
99+
100+ /*
101+- * The minimum index space is 512 bytes, with that amount of
102+- * index we can describe ~1400 labels which is less than a byte
103+- * of overhead per label. Round up to a byte of overhead per
104+- * label and determine the size of the index region. Yes, this
105+- * starts to waste space at larger config_sizes, but it's
106+- * unlikely we'll ever see anything but 128K.
107++ * Per UEFI 2.7, the minimum size of the Label Storage Area is
108++ * large enough to hold 2 index blocks and 2 labels. The
109++ * minimum index block size is 256 bytes, and the minimum label
110++ * size is 256 bytes.
111+ */
112+ nslot = nvdimm_num_label_slots(ndd);
113+ space = ndd->config_size - nslot * sizeof_namespace_label(ndd);
114+- size = ALIGN(sizeof(struct namespace_index) + DIV_ROUND_UP(nslot, 8),
115+- NSINDEX_ALIGN) * 2;
116+- if (size <= space)
117++ size = __sizeof_namespace_index(nslot) * 2;
118++ if (size <= space && nslot >= 2)
119+ return size / 2;
120+
121+ err(ctx, "%s: label area (%ld) too small to host (%d byte) labels\n",
122+--
123+2.24.0
124+
125diff --git a/debian/patches/series b/debian/patches/series
126index fc2b1e8..d835c28 100644
127--- a/debian/patches/series
128+++ b/debian/patches/series
129@@ -1,2 +1,3 @@
130 avoid_modprobe_test.patch
131 include_errno.h.patch
132+ndctl-init-labels-Fix-label-slot-accounting-per-UEFI.patch

Subscribers

People subscribed via source and target branches