* [mdadm PATCH 2/4] systemd/mdadm-last-resort: use ConditionPathExists instead of Conflicts
From: NeilBrown @ 2017-04-20 2:40 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <149265560315.31004.3851231165281498425.stgit@noble>
Commit cec72c071bbe ("systemd/mdadm-last-resort: add Conflicts to .service file.")
added a 'Conflicts' directive to the mdadm-last-resort@.service file in
the hope that this would make sure the service didn't run after the device
was active, even if the timer managed to get started, which is possible in
race conditions.
This seemed to work is testing, but it isn't clear why, and it is known
to cause problems.
If systemd happens to know that the mentioned device is a dependency of a
mount point, the Conflicts can unmount that mountpoint, which is certainly
not wanted.
So remove the "Conflicts" and instead use
ConditionPathExists=!/sys/devices/virtual/block/%i/md/sync_action
The "sync_action" file exists for any array which requires last-resort
handling, and only appears when the array is activated. So it is safe
to rely on it to determine if the last-resort is really needed.
Fixes: cec72c071bbe ("systemd/mdadm-last-resort: add Conflicts to .service file.")
Signed-off-by: NeilBrown <neilb@suse.com>
---
systemd/mdadm-last-resort@.service | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/systemd/mdadm-last-resort@.service b/systemd/mdadm-last-resort@.service
index e93d72b2b45e..f9d4d12738a3 100644
--- a/systemd/mdadm-last-resort@.service
+++ b/systemd/mdadm-last-resort@.service
@@ -1,7 +1,7 @@
[Unit]
Description=Activate md array even though degraded
DefaultDependencies=no
-Conflicts=sys-devices-virtual-block-%i.device
+ConditionPathExists=!/sys/devices/virtual/block/%i/md/sync_action
[Service]
Type=oneshot
^ permalink raw reply related
* [mdadm PATCH 0/4] Assorted mdadm patches
From: NeilBrown @ 2017-04-20 2:40 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
These are 4 unrelated patches that I've gathered
over the last few weeks.
The last one is probably the most interesting and one
that you should probably think carefully before apply
(but I hope you'll decide in favour).
When you --assemble -or --create an array, udev immediately has a look
at the new device and might act on that content. e.g. tell udisks it
can mount a filesystem, or tell mdadm that there is part of a RAID
array in here.
When you --assemble an array you want that to happen.
When you --create it, you don't.
udev cannot distinguish 'assemble' from 'create'.
This can be annoying.
The way I have found to tell udev about the distinction to create a
/run/udev/rules.d/01-mdadm.rules file which marks any newly appearing
md array as SYSTEMD_READY==0. This is created before the array, and
removed once the array exisits (and hopefully has been handled by
udev).
Most udev rules which might process a newly appearing device will
avoid doing so i SYSTEMD_READY is set to 0.
Thanks,
NeilBrown
---
NeilBrown (4):
Grow_continue_command: ensure 'content' is properly initialised.
systemd/mdadm-last-resort: use ConditionPathExists instead of Conflicts
Detail: ensure --export names are acceptable as shell variables.
Create: tell udev device is not ready when first created.
Create.c | 9 +++++++++
Detail.c | 12 +++++++++---
Grow.c | 1 +
lib.c | 22 ++++++++++++++++++++++
mdadm.h | 2 ++
systemd/mdadm-last-resort@.service | 2 +-
6 files changed, 44 insertions(+), 4 deletions(-)
--
Signature
^ permalink raw reply
* [mdadm PATCH 1/4] Grow_continue_command: ensure 'content' is properly initialised.
From: NeilBrown @ 2017-04-20 2:40 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <149265560315.31004.3851231165281498425.stgit@noble>
Grow_continue_command() call verify_reshape_position(), which assumes
that info->sys_name is initialised.
'info' in verify_reshape_position() is 'content' in Grow_continue_command().
In the st->ss->external != 0 branch of that function, sysfs_init() is called
to initialize content->sys_name.
In the st->ss->external == 0 branch, ->sys_name is not initialized so
verify_reshape_position() will not do the right thing.
Signed-off-by: NeilBrown <neilb@suse.com>
---
Grow.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/Grow.c b/Grow.c
index 15f4ed1253bf..c6967ed1c9c7 100755
--- a/Grow.c
+++ b/Grow.c
@@ -5002,6 +5002,7 @@ int Grow_continue_command(char *devname, int fd,
goto Grow_continue_command_exit;
}
content = &array;
+ sysfs_init(content, fd, NULL);
/* Need to load a superblock.
* FIXME we should really get what we need from
* sysfs
^ permalink raw reply related
* Re: [md PATCH 00/10] Simplify bio splitting and related code.
From: NeilBrown @ 2017-04-20 1:37 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20170412025157.qlodbacq2afuvq6j@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 2609 bytes --]
On Tue, Apr 11 2017, Shaohua Li wrote:
> On Wed, Apr 12, 2017 at 09:27:07AM +1000, Neil Brown wrote:
>> On Tue, Apr 11 2017, Shaohua Li wrote:
>>
>> > On Wed, Apr 05, 2017 at 02:05:50PM +1000, Neil Brown wrote:
>> >> This is part of my little project to make bio splitting
>> >> in Linux uniform and dead-lock free, in a way that will mean
>> >> that we can get rid of all the bioset threads.
>> >>
>> >> The basic approach is that when a bio needs to be split, we call
>> >> bio_split(), bio_chain() and then generic_make_request().
>> >> We then proceed to handle the remainder without further splitting.
>> >> Recent changes to generic_make_request() ensure that this will
>> >> be safe from deadlocks, providing each bioset is used only once
>> >> in the stack.
>> >>
>> >> This leads to simpler code in various places. In particular, the
>> >> splitting of bios that is needed to work around known bad blocks
>> >> is now much less complex. There is only ever one r1bio per bio.
>> >>
>> >> As you can see from
>> >> 10 files changed, 335 insertions(+), 540 deletions(-)
>> >> there is a net reduction in code.
>> >
>> > Looks good and makes code simpler, applied, thanks Neil! The patch 1 and 6 need
>> > comments in the code to explain how deadlock is avoided though. Care to send a
>> > new patch?
>>
>> It isn't clear to me what sort of comment you want, or where it should
>> go.
>> It might make sense to have a comment near bio_split() explaining how to
>> use it (i.e. explaining the pattern used in various patches here), but
>> I don't see what sort of comments would help in raid1.c or raid10.c
>> ??
>
> Both raid1.c and raid10.c have comments why we need offload the bio to
> raid1d/raid10d to avoid deadlock before, we also have comments to explain why
> we do bio_split() and then generic_make_request() before. Now these info are
> lost, so I hope we can add it back why the new way (bio_split and follow
> generic_make_request of next part) can avoid deadlock. That will be very
> helpful for others.
Those comments were needed before because of design flaws in
generic_make_request(). It make it too easy to trigger deadlocks.
With the recent changes to generic_make_request(), deadlocks are much
harder to trigger so there is less need for documentation on how to be
careful.
It would be good to have some clear documentation for bio_split()
describing how it should be used. Once I have tidied up all the users
of bio_split() and removed the bioset work queues, I plan to add that
documentation.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH 26/27] scsi: sd: Separate zeroout and discard command choices
From: Martin K. Petersen @ 2017-04-20 1:34 UTC (permalink / raw)
To: Paolo Bonzini
Cc: axboe-tSWWG44O7X1aa/9Udqfwiw, linux-raid-u79uwXL29TY76Z2rM5mHXA,
martin.petersen-QHcLZuEGTsvQT0dZR+AlfA,
snitzer-H+wXaHxf7aLQT0dZR+AlfA,
philipp.reisner-63ez5xqkn6DQT0dZR+AlfA,
linux-block-u79uwXL29TY76Z2rM5mHXA,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
linux-scsi-u79uwXL29TY76Z2rM5mHXA,
lars.ellenberg-63ez5xqkn6DQT0dZR+AlfA,
shli-DgEjT+Ai2ygdnm+yROfE0A, Christoph Hellwig,
agk-H+wXaHxf7aLQT0dZR+AlfA, drbd-dev-cunTk1MwBs8qoQakbn7OcQ
In-Reply-To: <58c3d6a6-924e-cc86-1907-a9fd02a39c0e-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Paolo,
> Should this be conditional on lbprz, lbpws, lbpws10 and max_ws_blocks?
It is intentional that things can be overridden from userland for
devices that report the "wrong" thing. We do the same for discard so
people can set up udev rules.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* (unknown),
From: hp @ 2017-04-19 20:46 UTC (permalink / raw)
To: linux-raid
[-- Attachment #1: EMAIL_7992249_linux-raid.zip --]
[-- Type: application/zip, Size: 1431 bytes --]
^ permalink raw reply
* [PATCH] Add a comment to indicate valid fallthrough
From: Khem Raj @ 2017-04-19 19:13 UTC (permalink / raw)
To: linux-raid; +Cc: Khem Raj
gcc7 warns about code with fallthroughs, this patch adds
the comment to indicate a valid fallthrough, helps gcc7
compiler warnings
This works in cross and native compilation case
Signed-off-by: Khem Raj <raj.khem@gmail.com>
---
Grow.c | 4 ++++
bitmap.c | 8 ++++++++
mdadm.c | 2 ++
super-intel.c | 1 +
util.c | 1 +
5 files changed, 16 insertions(+)
diff --git a/Grow.c b/Grow.c
index 455c5f9..27c73b1 100755
--- a/Grow.c
+++ b/Grow.c
@@ -1257,6 +1257,7 @@ char *analyse_change(char *devname, struct mdinfo *info, struct reshape *re)
switch (info->new_level) {
case 4:
delta_parity = 1;
+ /* fallthrough */
case 0:
re->level = 4;
re->before.layout = 0;
@@ -1284,10 +1285,12 @@ char *analyse_change(char *devname, struct mdinfo *info, struct reshape *re)
case 4:
info->array.layout = ALGORITHM_PARITY_N;
+ /* fallthrough */
case 5:
switch (info->new_level) {
case 0:
delta_parity = -1;
+ /* fallthrough */
case 4:
re->level = info->array.level;
re->before.data_disks = info->array.raid_disks - 1;
@@ -1343,6 +1346,7 @@ char *analyse_change(char *devname, struct mdinfo *info, struct reshape *re)
case 4:
case 5:
delta_parity = -1;
+ /* fallthrough */
case 6:
re->level = 6;
re->before.data_disks = info->array.raid_disks - 2;
diff --git a/bitmap.c b/bitmap.c
index ccedfd3..a6ff091 100644
--- a/bitmap.c
+++ b/bitmap.c
@@ -82,13 +82,21 @@ static inline int count_dirty_bits_byte(char byte, int num_bits)
switch (num_bits) { /* fall through... */
case 8: if (byte & 128) num++;
+ /* fallthrough */
case 7: if (byte & 64) num++;
+ /* fallthrough */
case 6: if (byte & 32) num++;
+ /* fallthrough */
case 5: if (byte & 16) num++;
+ /* fallthrough */
case 4: if (byte & 8) num++;
+ /* fallthrough */
case 3: if (byte & 4) num++;
+ /* fallthrough */
case 2: if (byte & 2) num++;
+ /* fallthrough */
case 1: if (byte & 1) num++;
+ /* fallthrough */
default: break;
}
diff --git a/mdadm.c b/mdadm.c
index c3a265b..2d06d3b 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -148,6 +148,7 @@ int main(int argc, char *argv[])
mode == CREATE || mode == GROW ||
mode == INCREMENTAL || mode == MANAGE)
break; /* b means bitmap */
+ /* fallthrough */
case Brief:
c.brief = 1;
continue;
@@ -828,6 +829,7 @@ int main(int argc, char *argv[])
case O(INCREMENTAL,NoDegraded):
pr_err("--no-degraded is deprecated in Incremental mode\n");
+ /* fallthrough */
case O(ASSEMBLE,NoDegraded): /* --no-degraded */
c.runstop = -1; /* --stop isn't allowed for --assemble,
* so we overload slightly */
diff --git a/super-intel.c b/super-intel.c
index 4e466ff..00a2925 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -3271,6 +3271,7 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info,
<< SECT_PER_MB_SHIFT;
}
}
+ /* fallthrough */
case MIGR_VERIFY:
/* we could emulate the checkpointing of
* 'sync_action=check' migrations, but for now
diff --git a/util.c b/util.c
index 32bd909..f2a4d19 100644
--- a/util.c
+++ b/util.c
@@ -335,6 +335,7 @@ unsigned long long parse_size(char *size)
switch (*c) {
case 'K':
c++;
+ /* fallthrough */
default:
s *= 2;
break;
--
2.12.2
^ permalink raw reply related
* Re: [PATCH] uapi: fix linux/raid/md_p.h userspace compilation error
From: Greg Thelen @ 2017-04-19 16:59 UTC (permalink / raw)
To: Artur Paszkiewicz; +Cc: Shaohua Li, linux-raid, ncroxon
In-Reply-To: <20170419084806.28405-1-artur.paszkiewicz@intel.com>
On Wed, Apr 19, 2017 at 1:48 AM, Artur Paszkiewicz
<artur.paszkiewicz@intel.com> wrote:
> Use __le32 and __le64 instead of u32 and u64.
>
> This fixes klibc build error:
> In file included from /klibc/usr/klibc/../include/sys/md.h:30:0,
> from /klibc/usr/kinit/do_mounts_md.c:19:
> /linux-next/usr/include/linux/raid/md_p.h:414:51: error: 'u32' undeclared here (not in a function)
> (PPL_HEADER_SIZE - PPL_HDR_RESERVED - 4 * sizeof(u32) - sizeof(u64))
>
> Reported-by: Greg Thelen <gthelen@google.com>
> Reported-by: Nigel Croxon <ncroxon@redhat.com>
> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Tested-by: Greg Thelen <gthelen@google.com>
My klibc builds are working again. Thanks.
^ permalink raw reply
* Re: mdadm Consistency Policy initialization
From: Jes Sorensen @ 2017-04-19 16:19 UTC (permalink / raw)
To: Artur Paszkiewicz; +Cc: linux-raid
In-Reply-To: <8c3ddfa5-2069-b2e2-40c6-e1ec14b2461f@intel.com>
On 04/19/2017 06:29 AM, Artur Paszkiewicz wrote:
> On 04/18/2017 06:50 PM, Jes Sorensen wrote:
>> Hi Artur,
>>
>> In 5308f11727b889965efe5ac0e854d197c2b51f6d you introduced struct mdinfo: enum consistency_policy, but in mdadm.c you initialize it to UnSet which isn't part of the enum.
>>
>> Is there any actual difference between CONSISTENCY_POLICY_UNKNOWN and UnSet? It seems suboptimal to mix and match within the enum like this, and if CONSISTENCY_POLICY_UNKNOWN does the job, couldn't we just initialize with that?
>
> Hi Jes,
>
> The "enum consistency_policy" and "mapping_t consistency_policies[]"
> represent values that can appear in sysfs. md/consistency_policy can be
> "unknown" when the array is inactive. On the other hand, UnSet just
> means that the --consistency-policy= parameter was not provided by the
> user. I wanted to differentiate between these two cases. If you think
> this is redundant I can change it and use CONSISTENCY_POLICY_UNKNOWN
> instead, this should be straightforward.
Hi Artur,
I would prefer to either use CONSISTENCY_POLICY_UNKNOWN or introduce a
new state within the enum so we don't cross pollute the namespace.
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH 26/27] scsi: sd: Separate zeroout and discard command choices
From: Paolo Bonzini @ 2017-04-19 14:56 UTC (permalink / raw)
To: Christoph Hellwig, axboe, martin.petersen, agk, snitzer, shli,
philipp.reisner, lars.ellenberg
Cc: linux-block, linux-scsi, drbd-dev, dm-devel, linux-raid
In-Reply-To: <20170405172125.22600-27-hch@lst.de>
On 05/04/2017 19:21, Christoph Hellwig wrote:
> +static ssize_t
> +zeroing_mode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct scsi_disk *sdkp = to_scsi_disk(dev);
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
> +
> +
> + if (!strncmp(buf, zeroing_mode[SD_ZERO_WRITE], 20))
> + sdkp->zeroing_mode = SD_ZERO_WRITE;
> + else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS], 20))
> + sdkp->zeroing_mode = SD_ZERO_WS;
> + else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS16_UNMAP], 20))
> + sdkp->zeroing_mode = SD_ZERO_WS16_UNMAP;
> + else if (!strncmp(buf, zeroing_mode[SD_ZERO_WS10_UNMAP], 20))
> + sdkp->zeroing_mode = SD_ZERO_WS10_UNMAP;
Should this be conditional on lbprz, lbpws, lbpws10 and max_ws_blocks?
Thanks,
Paolo
^ permalink raw reply
* Re: mdadm Consistency Policy initialization
From: Artur Paszkiewicz @ 2017-04-19 10:29 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <9b041a85-7d61-75c8-7fbf-857e095ab838@gmail.com>
On 04/18/2017 06:50 PM, Jes Sorensen wrote:
> Hi Artur,
>
> In 5308f11727b889965efe5ac0e854d197c2b51f6d you introduced struct mdinfo: enum consistency_policy, but in mdadm.c you initialize it to UnSet which isn't part of the enum.
>
> Is there any actual difference between CONSISTENCY_POLICY_UNKNOWN and UnSet? It seems suboptimal to mix and match within the enum like this, and if CONSISTENCY_POLICY_UNKNOWN does the job, couldn't we just initialize with that?
Hi Jes,
The "enum consistency_policy" and "mapping_t consistency_policies[]"
represent values that can appear in sysfs. md/consistency_policy can be
"unknown" when the array is inactive. On the other hand, UnSet just
means that the --consistency-policy= parameter was not provided by the
user. I wanted to differentiate between these two cases. If you think
this is redundant I can change it and use CONSISTENCY_POLICY_UNKNOWN
instead, this should be straightforward.
Thanks,
Artur
^ permalink raw reply
* [PATCH] uapi: fix linux/raid/md_p.h userspace compilation error
From: Artur Paszkiewicz @ 2017-04-19 8:48 UTC (permalink / raw)
To: shli; +Cc: linux-raid, gthelen, ncroxon, Artur Paszkiewicz
In-Reply-To: <xr93wpak0xkb.fsf@gthelen.mtv.corp.google.com>
Use __le32 and __le64 instead of u32 and u64.
This fixes klibc build error:
In file included from /klibc/usr/klibc/../include/sys/md.h:30:0,
from /klibc/usr/kinit/do_mounts_md.c:19:
/linux-next/usr/include/linux/raid/md_p.h:414:51: error: 'u32' undeclared here (not in a function)
(PPL_HEADER_SIZE - PPL_HDR_RESERVED - 4 * sizeof(u32) - sizeof(u64))
Reported-by: Greg Thelen <gthelen@google.com>
Reported-by: Nigel Croxon <ncroxon@redhat.com>
Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
include/uapi/linux/raid/md_p.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/raid/md_p.h b/include/uapi/linux/raid/md_p.h
index d9a1ead867b9..d500bd224979 100644
--- a/include/uapi/linux/raid/md_p.h
+++ b/include/uapi/linux/raid/md_p.h
@@ -411,7 +411,7 @@ struct ppl_header_entry {
#define PPL_HEADER_SIZE 4096
#define PPL_HDR_RESERVED 512
#define PPL_HDR_ENTRY_SPACE \
- (PPL_HEADER_SIZE - PPL_HDR_RESERVED - 4 * sizeof(u32) - sizeof(u64))
+ (PPL_HEADER_SIZE - PPL_HDR_RESERVED - 4 * sizeof(__le32) - sizeof(__le64))
#define PPL_HDR_MAX_ENTRIES \
(PPL_HDR_ENTRY_SPACE / sizeof(struct ppl_header_entry))
--
2.11.0
^ permalink raw reply related
* Re: Lost second disk during resync, can I recover?
From: Adam B @ 2017-04-18 18:03 UTC (permalink / raw)
To: Roman Mamedov; +Cc: linux-raid
In-Reply-To: <20170417140800.0731c206@natsu>
Thanks Roman -- that was what it took to get it recovering.. cheers
for noticing. It has bad blocks but I am able to get the data needed.
On Mon, Apr 17, 2017 at 2:08 AM, Roman Mamedov <rm@romanrm.net> wrote:
> On Sun, 16 Apr 2017 17:30:26 -0700
> Adam B <adam.backer@gmail.com> wrote:
>
>> I've got a synology ds1812+
>> Could have sworn I did raid 6, but md says raid5 (dont recall
>> personalities pre-failure)
>> Lost disk 2, was rebuilding and disk 6 crashed.
>>
>> Booting from a fresh machine, and attempting to discover the arrays
>> (md125 is the target)
>>
>> root@ds:~# mdadm --assemble --scan
>> mdadm: /dev/md/DiskStation:2 assembled from 6 drives - not enough to
>> start the array.
>> mdadm: /dev/md/1_1 has been started with 7 drives (out of 8).
>> mdadm: /dev/md/0_1 has been started with 7 drives (out of 8).
>> mdadm: /dev/md/DiskStation:2 exists - ignoring
>> mdadm: /dev/md125 assembled from 6 drives - not enough to start the array.
>> root@ds:~# cat /proc/mdstat
>> Personalities : [linear] [raid0] [raid1] [raid10] [raid6] [raid5] [raid4]
>> md126 : active raid1 sda1[0] sdh1[7] sdg1[6] sde1[4] sdd1[3] sdc1[2] sdb1[1]
>> 2490176 blocks [8/7] [UUUUU_UU]
>>
>> md127 : active raid1 sda2[0] sdb2[7] sdh2[6] sdg2[5] sde2[3] sdd2[2] sdc2[1]
>> 2097088 blocks [8/7] [UUUU_UUU]
>>
>> md2 : active raid5 sdf3[5]
>> 20478809792 blocks super 1.2 level 5, 64k chunk, algorithm 2
>> [8/1] [_____U__]
>
> From a quick glance it's not clear what is sdf3, why it's got an array of its
> own, and why later you don't try to do anything with it (didn't try re-add,
> neither list in assemble, nor examine). Isn't it that one more drive you need
> (even if partially bad) that would let you start the RAID5?
>
> Also perhaps before being able to do anything with it you will need to stop
> its "md2" array.
>
> --
> With respect,
> Roman
^ permalink raw reply
* Re: [PATCH v6 0/3] dm: boot a mapped device without an initramfs
From: Kees Cook @ 2017-04-18 17:37 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: Alasdair Kergon, Mike Snitzer, Will Drewry, dm-devel,
linux-doc@vger.kernel.org, LKML, Shaohua Li, linux-raid,
Guenter Roeck, David Zeuthen
In-Reply-To: <20170418164240.18079-1-enric.balletbo@collabora.com>
On Tue, Apr 18, 2017 at 9:42 AM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Hello,
>
> Some of these patches were send few years back, I saw that first
> version was send to this list in 2010, and after version 4 did not
> land [1]. Some days ago I resend the patches [2] and few hours later I
> noticed that one year ago was send a v5 version [3] and I was not aware.
>
> There was some discussion about v5 and during the discussion Mike Snitzer
> proposed that at least a change of the syntax is required, we're really
> interested on see this upstream as is extensively used in ChromeOS based
> devices so I'm wondering if we can restart the discussion and hopefully
> we will be able to do the modifications needed.
>
> So my first question is, apart of the change of the syntax, what more
> should be changed?
AFAIK, this was the main change needed. Change the syntax and plumb
into the ioctl interface. The discussion ended with Mike being open to
the idea, and for me to go work on it. I haven't had time to work on
it, though, so it has continued to be a locally carried patch:
https://www.redhat.com/archives/dm-devel/2016-February/msg00199.html
More recently David Zeuthen has been poking at this code, so I've
included him on CC here, in case there are new developments.
-Kees
>
> Thanks for your help,
> Enric
>
> [1] Patchwork links:
> https://patchwork.kernel.org/patch/104857/
> https://patchwork.kernel.org/patch/104856/
> https://patchwork.kernel.org/patch/104858/
>
> [2] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1375276.html
>
> [3] https://www.redhat.com/archives/dm-devel/2016-February/msg00112.html
>
>
> Brian Norris (1):
> dm: make some mapped_device functions available
>
> Will Drewry (2):
> dm: export a table+mapped device to the ioctl interface
> init: add support to directly boot to a mapped device
>
> Documentation/admin-guide/kernel-parameters.rst | 1 +
> Documentation/admin-guide/kernel-parameters.txt | 3 +
> Documentation/device-mapper/boot.txt | 65 ++++
> drivers/md/dm-ioctl.c | 36 ++
> drivers/md/dm.h | 8 -
> include/linux/device-mapper.h | 19 +
> init/Makefile | 1 +
> init/do_mounts.c | 1 +
> init/do_mounts.h | 10 +
> init/do_mounts_dm.c | 448 ++++++++++++++++++++++++
> 10 files changed, 584 insertions(+), 8 deletions(-)
> create mode 100644 Documentation/device-mapper/boot.txt
> create mode 100644 init/do_mounts_dm.c
>
> --
> 2.9.3
>
--
Kees Cook
Pixel Security
^ permalink raw reply
* mdadm Consistency Policy initialization
From: Jes Sorensen @ 2017-04-18 16:50 UTC (permalink / raw)
To: Artur Paszkiewicz; +Cc: linux-raid
Hi Artur,
In 5308f11727b889965efe5ac0e854d197c2b51f6d you introduced struct
mdinfo: enum consistency_policy, but in mdadm.c you initialize it to
UnSet which isn't part of the enum.
Is there any actual difference between CONSISTENCY_POLICY_UNKNOWN and
UnSet? It seems suboptimal to mix and match within the enum like this,
and if CONSISTENCY_POLICY_UNKNOWN does the job, couldn't we just
initialize with that?
Cheers,
Jes
^ permalink raw reply
* [PATCH v6 3/3] init: add support to directly boot to a mapped device
From: Enric Balletbo i Serra @ 2017-04-18 16:42 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, Kees Cook, Will Drewry
Cc: dm-devel, linux-doc, linux-kernel, Shaohua Li, linux-raid,
Guenter Roeck
In-Reply-To: <20170418164240.18079-1-enric.balletbo@collabora.com>
From: Will Drewry <wad@chromium.org>
Add a dm= kernel parameter modeled after the md= parameter from
do_mounts_md. It allows for device-mapper targets to be configured at
boot time for use early in the boot process (as the root device or
otherwise).
Signed-off-by: Will Drewry <wad@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
v6: setup md->queue to reflect md's type, fix memory leak on error path
v5: resurrection, multiple devices, cleanups, error reporting improvements
v4: https://patchwork.kernel.org/patch/104861/
---
Documentation/admin-guide/kernel-parameters.rst | 1 +
Documentation/admin-guide/kernel-parameters.txt | 3 +
Documentation/device-mapper/boot.txt | 65 ++++
init/Makefile | 1 +
init/do_mounts.c | 1 +
init/do_mounts.h | 10 +
init/do_mounts_dm.c | 448 ++++++++++++++++++++++++
7 files changed, 529 insertions(+)
create mode 100644 Documentation/device-mapper/boot.txt
create mode 100644 init/do_mounts_dm.c
diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index c5eae20..b1d4c39 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -91,6 +91,7 @@ parameter is applicable::
BLACKFIN Blackfin architecture is enabled.
CLK Common clock infrastructure is enabled.
CMA Contiguous Memory Area support is enabled.
+ DM Device mapper support is enabled.
DRM Direct Rendering Management support is enabled.
DYNAMIC_DEBUG Build in debug messages and enable them at runtime
EDD BIOS Enhanced Disk Drive Services (EDD) is enabled
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index fdca2f2..5b06d0f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -830,6 +830,9 @@
dis_ucode_ldr [X86] Disable the microcode loader.
+ dm= [DM] Allows early creation of a device-mapper device.
+ See Documentation/device-mapper/boot.txt.
+
dma_debug=off If the kernel is compiled with DMA_API_DEBUG support,
this option disables the debugging code at boot.
diff --git a/Documentation/device-mapper/boot.txt b/Documentation/device-mapper/boot.txt
new file mode 100644
index 0000000..1d4ac1d
--- /dev/null
+++ b/Documentation/device-mapper/boot.txt
@@ -0,0 +1,65 @@
+Boot time creation of mapped devices
+====================================
+
+It is possible to configure a device mapper device to act as the root
+device for your system in two ways.
+
+The first is to build an initial ramdisk which boots to a minimal
+userspace which configures the device, then pivot_root(8) in to it.
+
+The second is to possible when the device-mapper and any targets are
+compiled into the kernel (not a module), one or more device-mappers may
+be created and used as the root device at boot time with the parameters
+given with the boot line dm=...
+
+Multiple device-mappers can be stacked by specifying the number of
+devices. A device can have multiple tables if the the number of tables
+is specified.
+
+ <dm> ::= <num-mappers> <device-mapper>+
+ <device-mapper> ::= <head> "," <table>+
+ <head> ::= <name> <uuid> <mode> [<num-tables>]
+ <table> ::= <start> <length> <type> <options> ","
+ <mode> ::= "ro" | "rw"
+ <uuid> ::= xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx | "none"
+ <type> ::= "verity" | "bootcache" | ...
+
+Each tables line may be as normal when using the dmsetup tool except for
+two variations:
+1. Any use of commas will be interpreted as a newline
+2. Quotation marks cannot be escaped and cannot be used without
+ terminating the dm= argument.
+
+Unless renamed by udev, the device node created will be dm-0 as the
+first minor number for the device-mapper is used during early creation.
+
+The <num-tables> field is optional and assumed to be 1.
+
+Examples
+========
+An example of booting to a linear array made up of user-mode linux block
+devices:
+
+ dm="1 lroot none rw 2, 0 4096 linear 98:16 0, 4096 4096 linear 98:32 0" \
+ root=/dev/dm-0
+
+This will boot to a rw dm-linear target of 8192 sectors split across two
+block devices identified by their major:minor numbers. After boot, udev
+will rename this target to /dev/mapper/lroot (depending on the rules).
+No uuid was assigned.
+
+An example of multiple device-mappers, with the dm="..." contents shown
+here split on multiple lines for readability:
+
+ 3 vboot none ro,
+ 0 1768000 bootcache
+ device=aa55b119-2a47-8c45-946a-5ac57765011f+1
+ signature=76e9be054b15884a9fa85973e9cb274c93afadb6
+ cache_start=1768000 max_blocks=100000 size_limit=23 max_trace=20000,
+ vroot none ro,
+ 0 1740800 verity payload=254:0 hashtree=254:0 hashstart=1740800 alg=sha1
+ root_hexdigest=76e9be054b15884a9fa85973e9cb274c93afadb6
+ salt=5b3549d54d6c7a3837b9b81ed72e49463a64c03680c47835bef94d768e5646fe,
+ vram none rw 2,
+ 0 32768 linear 1:0 0,
+ 32768 32768 linear 1:1 0,
diff --git a/init/Makefile b/init/Makefile
index c4fb455..30424d7 100644
--- a/init/Makefile
+++ b/init/Makefile
@@ -20,6 +20,7 @@ mounts-y := do_mounts.o
mounts-$(CONFIG_BLK_DEV_RAM) += do_mounts_rd.o
mounts-$(CONFIG_BLK_DEV_INITRD) += do_mounts_initrd.o
mounts-$(CONFIG_BLK_DEV_MD) += do_mounts_md.o
+mounts-$(CONFIG_BLK_DEV_DM) += do_mounts_dm.o
# dependencies on generated files need to be listed explicitly
$(obj)/version.o: include/generated/compile.h
diff --git a/init/do_mounts.c b/init/do_mounts.c
index c2de510..8b9182b 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -566,6 +566,7 @@ void __init prepare_namespace(void)
wait_for_device_probe();
md_run_setup();
+ dm_run_setup();
if (saved_root_name[0]) {
root_device_name = saved_root_name;
diff --git a/init/do_mounts.h b/init/do_mounts.h
index 282d65b..4e71c29 100644
--- a/init/do_mounts.h
+++ b/init/do_mounts.h
@@ -60,3 +60,13 @@ void md_run_setup(void);
static inline void md_run_setup(void) {}
#endif
+
+#ifdef CONFIG_BLK_DEV_DM
+
+void dm_run_setup(void);
+
+#else
+
+static inline void dm_run_setup(void) {}
+
+#endif
diff --git a/init/do_mounts_dm.c b/init/do_mounts_dm.c
new file mode 100644
index 0000000..feaa6ad
--- /dev/null
+++ b/init/do_mounts_dm.c
@@ -0,0 +1,448 @@
+/*
+ * do_mounts_dm.c
+ * Copyright (C) 2010 The Chromium OS Authors <chromium-os-dev@chromium.org>
+ * Based on do_mounts_md.c
+ *
+ * This file is released under the GPLv2.
+ */
+#include <linux/async.h>
+#include <linux/ctype.h>
+#include <linux/device-mapper.h>
+#include <linux/fs.h>
+#include <linux/string.h>
+#include <linux/delay.h>
+
+#include "do_mounts.h"
+
+#define DM_MAX_DEVICES 256
+#define DM_MAX_TARGETS 256
+#define DM_MAX_NAME 32
+#define DM_MAX_UUID 129
+#define DM_NO_UUID "none"
+
+#define DM_MSG_PREFIX "init"
+#define DMERR_PARSE(fmt, args...) \
+ DMERR("failed to parse " fmt " for device %s<%lu>", args)
+
+/* Separators used for parsing the dm= argument. */
+#define DM_FIELD_SEP " "
+#define DM_LINE_SEP ","
+#define DM_ANY_SEP DM_FIELD_SEP DM_LINE_SEP
+
+/* See Documentation/device-mapper/boot.txt for dm="..." format details. */
+
+struct dm_setup_table {
+ sector_t begin;
+ sector_t length;
+ char *type;
+ char *params;
+ /* simple singly linked list */
+ struct dm_setup_table *next;
+};
+
+struct dm_device {
+ int minor;
+ int ro;
+ char name[DM_MAX_NAME];
+ char uuid[DM_MAX_UUID];
+ unsigned long num_tables;
+ struct dm_setup_table *table;
+ int table_count;
+ struct dm_device *next;
+};
+
+struct dm_option {
+ char *start;
+ char *next;
+ size_t len;
+ char delim;
+};
+
+static struct {
+ unsigned long num_devices;
+ char *str;
+} dm_setup_args __initdata;
+
+static int dm_early_setup __initdata;
+
+static int __init get_dm_option(struct dm_option *opt, const char *accept)
+{
+ char *str = opt->next;
+ char *endp;
+
+ if (!str)
+ return 0;
+
+ str = skip_spaces(str);
+ opt->start = str;
+ endp = strpbrk(str, accept);
+ if (!endp) { /* act like strchrnul */
+ opt->len = strlen(str);
+ endp = str + opt->len;
+ } else {
+ opt->len = endp - str;
+ }
+ opt->delim = *endp;
+ if (*endp == 0) {
+ /* Don't advance past the nul. */
+ opt->next = endp;
+ } else {
+ opt->next = endp + 1;
+ }
+ return opt->len != 0;
+}
+
+static int __init get_dm_option_u64(struct dm_option *opt, const char *sep,
+ unsigned long long *result)
+{
+ char buf[32];
+
+ if (!get_dm_option(opt, sep))
+ return -EINVAL;
+
+ strlcpy(buf, opt->start, min(sizeof(buf), opt->len + 1));
+ return kstrtoull(buf, 0, result);
+}
+
+static void __init dm_setup_cleanup(struct dm_device *devices)
+{
+ struct dm_device *dev = devices;
+
+ while (dev) {
+ struct dm_device *old_dev = dev;
+ struct dm_setup_table *table = dev->table;
+
+ while (table) {
+ struct dm_setup_table *old_table = table;
+
+ kfree(table->type);
+ kfree(table->params);
+ table = table->next;
+ kfree(old_table);
+ dev->table_count--;
+ }
+ WARN_ON(dev->table_count);
+ dev = dev->next;
+ kfree(old_dev);
+ }
+}
+
+static char * __init dm_parse_device(struct dm_device *dev, char *str,
+ unsigned long idx)
+{
+ struct dm_option opt;
+ size_t len;
+ unsigned long long num_tables;
+
+ /* Grab the logical name of the device to be exported to udev */
+ opt.next = str;
+ if (!get_dm_option(&opt, DM_FIELD_SEP)) {
+ DMERR_PARSE("name", "", idx);
+ goto parse_fail;
+ }
+ len = min(opt.len + 1, sizeof(dev->name));
+ strlcpy(dev->name, opt.start, len); /* includes nul */
+
+ /* Grab the UUID value or "none" */
+ if (!get_dm_option(&opt, DM_FIELD_SEP)) {
+ DMERR_PARSE("uuid", dev->name, idx);
+ goto parse_fail;
+ }
+ len = min(opt.len + 1, sizeof(dev->uuid));
+ strlcpy(dev->uuid, opt.start, len);
+
+ /* Determine if the table/device will be read only or read-write */
+ get_dm_option(&opt, DM_ANY_SEP);
+ if (!strncmp("ro", opt.start, opt.len)) {
+ dev->ro = 1;
+ } else if (!strncmp("rw", opt.start, opt.len)) {
+ dev->ro = 0;
+ } else {
+ DMERR_PARSE("table mode", dev->name, idx);
+ goto parse_fail;
+ }
+
+ /* Optional number field */
+ if (opt.delim == DM_FIELD_SEP[0]) {
+ if (get_dm_option_u64(&opt, DM_LINE_SEP, &num_tables)) {
+ DMERR_PARSE("number of tables", dev->name, idx);
+ goto parse_fail;
+ }
+ } else {
+ num_tables = 1;
+ }
+ if (num_tables > DM_MAX_TARGETS) {
+ DMERR_PARSE("too many tables (%llu > %d)", num_tables,
+ DM_MAX_TARGETS, dev->name, idx);
+ }
+ dev->num_tables = num_tables;
+
+ return opt.next;
+
+parse_fail:
+ return NULL;
+}
+
+static char * __init dm_parse_tables(struct dm_device *dev, char *str,
+ unsigned long idx)
+{
+ struct dm_option opt;
+ struct dm_setup_table **table = &dev->table;
+ unsigned long num_tables = dev->num_tables;
+ unsigned long i;
+ unsigned long long value;
+
+ /*
+ * Tables are defined as per the normal table format but with a
+ * comma as a newline separator.
+ */
+ opt.next = str;
+ for (i = 0; i < num_tables; i++) {
+ *table = kzalloc(sizeof(struct dm_setup_table), GFP_KERNEL);
+ if (!*table) {
+ DMERR_PARSE("table %lu (out of memory)", i, dev->name,
+ idx);
+ goto parse_fail;
+ }
+ dev->table_count++;
+
+ if (get_dm_option_u64(&opt, DM_FIELD_SEP, &value)) {
+ DMERR_PARSE("starting sector for table %lu", i,
+ dev->name, idx);
+ goto parse_fail;
+ }
+ (*table)->begin = value;
+
+ if (get_dm_option_u64(&opt, DM_FIELD_SEP, &value)) {
+ DMERR_PARSE("length for table %lu", i, dev->name, idx);
+ goto parse_fail;
+ }
+ (*table)->length = value;
+
+ if (get_dm_option(&opt, DM_FIELD_SEP))
+ (*table)->type = kstrndup(opt.start, opt.len,
+ GFP_KERNEL);
+ if (!((*table)->type)) {
+ DMERR_PARSE("type for table %lu", i, dev->name, idx);
+ goto parse_fail;
+ }
+ if (get_dm_option(&opt, DM_LINE_SEP))
+ (*table)->params = kstrndup(opt.start, opt.len,
+ GFP_KERNEL);
+ if (!((*table)->params)) {
+ DMERR_PARSE("params for table %lu", i, dev->name, idx);
+ goto parse_fail;
+ }
+ table = &((*table)->next);
+ }
+ DMDEBUG("tables parsed: %d", dev->table_count);
+
+ return opt.next;
+
+parse_fail:
+ return NULL;
+}
+
+static struct dm_device * __init dm_parse_args(void)
+{
+ struct dm_device *devices = NULL;
+ struct dm_device **tail = &devices;
+ struct dm_device *dev;
+ char *str = dm_setup_args.str;
+ unsigned long num_devices = dm_setup_args.num_devices;
+ unsigned long i;
+
+ if (!str)
+ return NULL;
+ for (i = 0; i < num_devices; i++) {
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ DMERR("failed to allocated memory for device %lu", i);
+ goto error;
+ }
+ *tail = dev;
+ tail = &dev->next;
+ /*
+ * devices are given minor numbers 0 - n-1
+ * in the order they are found in the arg
+ * string.
+ */
+ dev->minor = i;
+ str = dm_parse_device(dev, str, i);
+ if (!str) /* NULL indicates error in parsing, bail */
+ goto error;
+
+ str = dm_parse_tables(dev, str, i);
+ if (!str)
+ goto error;
+ }
+ return devices;
+error:
+ dm_setup_cleanup(devices);
+ return NULL;
+}
+
+/*
+ * Parse the command-line parameters given our kernel, but do not
+ * actually try to invoke the DM device now; that is handled by
+ * dm_setup_drives after the low-level disk drivers have initialised.
+ * dm format is described at the top of the file.
+ *
+ * Because dm minor numbers are assigned in assending order starting with 0,
+ * You can assume the first device is /dev/dm-0, the next device is /dev/dm-1,
+ * and so forth.
+ */
+static int __init dm_setup(char *str)
+{
+ struct dm_option opt;
+ unsigned long long num_devices;
+
+ if (!str) {
+ DMERR("setup str is NULL");
+ goto parse_fail;
+ }
+
+ DMDEBUG("Want to parse \"%s\"", str);
+ opt.next = str;
+ if (get_dm_option_u64(&opt, DM_FIELD_SEP, &num_devices))
+ goto parse_fail;
+ str = opt.next;
+ if (num_devices > DM_MAX_DEVICES) {
+ DMERR("too many devices %llu > %d", num_devices,
+ DM_MAX_DEVICES);
+ }
+ dm_setup_args.num_devices = num_devices;
+ dm_setup_args.str = str;
+
+ DMINFO("will configure %lu device%s", dm_setup_args.num_devices,
+ dm_setup_args.num_devices == 1 ? "" : "s");
+ dm_early_setup = 1;
+ return 1;
+
+parse_fail:
+ DMWARN("Invalid arguments supplied to dm=.");
+ return 0;
+}
+
+static void __init dm_setup_drives(void)
+{
+ struct mapped_device *md = NULL;
+ struct dm_table *tables = NULL;
+ struct dm_setup_table *table;
+ struct dm_device *dev;
+ char *uuid;
+ fmode_t fmode = FMODE_READ;
+ struct dm_device *devices;
+
+ devices = dm_parse_args();
+
+ for (dev = devices; dev; dev = dev->next) {
+ if (dm_create(dev->minor, &md)) {
+ DMERR("failed to create device %s", dev->name);
+ goto fail;
+ }
+ DMDEBUG("created device '%s'", dm_device_name(md));
+
+ /*
+ * In addition to flagging the table below, the disk must be
+ * set explicitly ro/rw.
+ */
+ set_disk_ro(dm_disk(md), dev->ro);
+
+ if (!dev->ro)
+ fmode |= FMODE_WRITE;
+ if (dm_table_create(&tables, fmode, dev->table_count, md)) {
+ DMERR("failed to create device %s tables", dev->name);
+ goto fail_put;
+ }
+
+ dm_lock_md_type(md);
+
+ for (table = dev->table; table; table = table->next) {
+ DMINFO("device %s adding table '%llu %llu %s %s'",
+ dev->name,
+ (unsigned long long) table->begin,
+ (unsigned long long) table->length,
+ table->type, table->params);
+ if (dm_table_add_target(tables, table->type,
+ table->begin,
+ table->length,
+ table->params)) {
+ DMERR("failed to add table to device %s",
+ dev->name);
+ goto fail_add_target;
+ }
+ }
+ if (dm_table_complete(tables)) {
+ DMERR("failed to complete device %s tables",
+ dev->name);
+ goto fail_add_target;
+ }
+
+ /* Suspend the device so that we can bind it to the tables. */
+ if (dm_suspend(md, 0)) {
+ DMERR("failed to suspend device %s pre-bind",
+ dev->name);
+ goto fail_add_target;
+ }
+
+ /* Initial table load: acquire type of table. */
+ dm_set_md_type(md, dm_table_get_type(tables));
+
+ /* Setup md->queue to reflect md's type. */
+ if (dm_setup_md_queue(md, tables)) {
+ DMERR("unable to set up device queue for new table.");
+ goto fail_add_target;
+ }
+
+ /*
+ * Bind the tables to the device. This is the only way
+ * to associate md->map with the tables and set the disk
+ * capacity directly.
+ */
+ if (dm_swap_table(md, tables)) { /* should return NULL. */
+ DMERR("failed to bind device %s to tables",
+ dev->name);
+ goto fail_add_target;
+ }
+
+ /* Finally, resume and the device should be ready. */
+ if (dm_resume(md)) {
+ DMERR("failed to resume device %s", dev->name);
+ goto fail_add_target;
+ }
+
+ /* Export the dm device via the ioctl interface */
+ if (!strcmp(DM_NO_UUID, dev->uuid))
+ uuid = NULL;
+ if (dm_ioctl_export(md, dev->name, uuid)) {
+ DMERR("failed to export device %s", dev->name);
+ goto fail_add_target;
+ }
+
+ dm_unlock_md_type(md);
+
+ DMINFO("dm-%d (%s) is ready", dev->minor, dev->name);
+ }
+ dm_setup_cleanup(devices);
+ return;
+
+fail_add_target:
+ dm_unlock_md_type(md);
+ dm_table_destroy(tables);
+fail_put:
+ dm_put(md);
+fail:
+ DMERR("starting dm-%d (%s) failed", dev->minor, dev->name);
+ dm_setup_cleanup(devices);
+}
+
+__setup("dm=", dm_setup);
+
+void __init dm_run_setup(void)
+{
+ if (!dm_early_setup)
+ return;
+ DMINFO("attempting early device configuration.");
+ dm_setup_drives();
+}
--
2.9.3
^ permalink raw reply related
* [PATCH v6 2/3] dm: export a table+mapped device to the ioctl interface
From: Enric Balletbo i Serra @ 2017-04-18 16:42 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, Kees Cook, Will Drewry
Cc: Guenter Roeck, linux-doc, linux-kernel, linux-raid, dm-devel,
Shaohua Li
In-Reply-To: <20170418164240.18079-1-enric.balletbo@collabora.com>
From: Will Drewry <wad@chromium.org>
One function is added which allows for a programmatically created
mapped device to be inserted into the dm-ioctl hash table. This binds
the device to a name and, optional, uuid which is needed by udev and
allows for userspace management of the mapped device.
Signed-off-by: Will Drewry <wad@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
v6: none
v5: resurrection
v4: https://patchwork.kernel.org/patch/104860/
---
drivers/md/dm-ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
include/linux/device-mapper.h | 6 ++++++
2 files changed, 42 insertions(+)
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 4951bf9..f4c610f 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1924,6 +1924,42 @@ void dm_interface_exit(void)
}
/**
+ * dm_ioctl_export - Permanently export a mapped device via the ioctl interface
+ * @md: Pointer to mapped_device
+ * @name: Buffer (size DM_NAME_LEN) for name
+ * @uuid: Buffer (size DM_UUID_LEN) for uuid or NULL if not desired
+ */
+int dm_ioctl_export(struct mapped_device *md, const char *name,
+ const char *uuid)
+{
+ int r = 0;
+ struct hash_cell *hc;
+
+ if (!md)
+ return -ENXIO;
+
+ /* The name and uuid can only be set once. */
+ mutex_lock(&dm_hash_cells_mutex);
+ hc = dm_get_mdptr(md);
+ mutex_unlock(&dm_hash_cells_mutex);
+ if (hc) {
+ DMERR("%s: already exported", dm_device_name(md));
+ return -ENXIO;
+ }
+
+ r = dm_hash_insert(name, uuid, md);
+ if (r) {
+ DMERR("%s: could not bind to '%s'", dm_device_name(md), name);
+ return r;
+ }
+
+ /* Let udev know we've changed. */
+ dm_kobject_uevent(md, KOBJ_CHANGE, dm_get_event_nr(md));
+
+ return r;
+}
+
+/**
* dm_copy_name_and_uuid - Copy mapped device name & uuid into supplied buffers
* @md: Pointer to mapped_device
* @name: Buffer (size DM_NAME_LEN) for name
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 9f70b21..2efac3b 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -412,6 +412,12 @@ void dm_set_mdptr(struct mapped_device *md, void *ptr);
void *dm_get_mdptr(struct mapped_device *md);
/*
+ * Export the device via the ioctl interface (uses mdptr).
+ */
+int dm_ioctl_export(struct mapped_device *md, const char *name,
+ const char *uuid);
+
+/*
* A device can still be used while suspended, but I/O is deferred.
*/
int dm_suspend(struct mapped_device *md, unsigned suspend_flags);
--
2.9.3
^ permalink raw reply related
* [PATCH v6 1/3] dm: make some mapped_device functions available
From: Enric Balletbo i Serra @ 2017-04-18 16:42 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, Kees Cook, Will Drewry
Cc: Guenter Roeck, linux-doc, Brian Norris, linux-kernel, linux-raid,
dm-devel, Shaohua Li
In-Reply-To: <20170418164240.18079-1-enric.balletbo@collabora.com>
From: Brian Norris <briannorris@chromium.org>
For init to build a mapped_device, it must hold the appropriate locks,
able to use dm_table_destroy() and the functions to get/set the md
type, so move these to the common header.
Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
v6: added more functions required
v5: first version of this specific patch in the series
---
drivers/md/dm.h | 8 --------
include/linux/device-mapper.h | 13 +++++++++++++
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index f298b01..6b501b5 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -48,7 +48,6 @@ struct dm_md_mempools;
/*-----------------------------------------------------------------
* Internal table functions.
*---------------------------------------------------------------*/
-void dm_table_destroy(struct dm_table *t);
void dm_table_event_callback(struct dm_table *t,
void (*fn)(void *), void *context);
struct dm_target *dm_table_get_target(struct dm_table *t, unsigned int index);
@@ -64,7 +63,6 @@ void dm_table_presuspend_undo_targets(struct dm_table *t);
void dm_table_postsuspend_targets(struct dm_table *t);
int dm_table_resume_targets(struct dm_table *t);
int dm_table_any_congested(struct dm_table *t, int bdi_bits);
-unsigned dm_table_get_type(struct dm_table *t);
struct target_type *dm_table_get_immutable_target_type(struct dm_table *t);
struct dm_target *dm_table_get_immutable_target(struct dm_table *t);
struct dm_target *dm_table_get_wildcard_target(struct dm_table *t);
@@ -74,14 +72,8 @@ bool dm_table_all_blk_mq_devices(struct dm_table *t);
void dm_table_free_md_mempools(struct dm_table *t);
struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
-void dm_lock_md_type(struct mapped_device *md);
-void dm_unlock_md_type(struct mapped_device *md);
-void dm_set_md_type(struct mapped_device *md, unsigned type);
-unsigned dm_get_md_type(struct mapped_device *md);
struct target_type *dm_get_immutable_target_type(struct mapped_device *md);
-int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t);
-
/*
* To check the return value from dm_table_find_target().
*/
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index b7ab76e..9f70b21 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -479,6 +479,11 @@ void dm_table_set_type(struct dm_table *t, unsigned type);
int dm_table_complete(struct dm_table *t);
/*
+ * Destroy the table when finished.
+ */
+void dm_table_destroy(struct dm_table *t);
+
+/*
* Target may require that it is never sent I/O larger than len.
*/
int __must_check dm_set_target_max_io_len(struct dm_target *ti, sector_t len);
@@ -515,6 +520,14 @@ void dm_table_run_md_queue_async(struct dm_table *t);
struct dm_table *dm_swap_table(struct mapped_device *md,
struct dm_table *t);
+unsigned dm_table_get_type(struct dm_table *t);
+
+void dm_lock_md_type(struct mapped_device *md);
+void dm_unlock_md_type(struct mapped_device *md);
+void dm_set_md_type(struct mapped_device *md, unsigned type);
+unsigned dm_get_md_type(struct mapped_device *md);
+int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t);
+
/*
* A wrapper around vmalloc.
*/
--
2.9.3
^ permalink raw reply related
* [PATCH v6 0/3] dm: boot a mapped device without an initramfs
From: Enric Balletbo i Serra @ 2017-04-18 16:42 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, Kees Cook, Will Drewry
Cc: Guenter Roeck, linux-doc, linux-kernel, linux-raid, dm-devel,
Shaohua Li
Hello,
Some of these patches were send few years back, I saw that first
version was send to this list in 2010, and after version 4 did not
land [1]. Some days ago I resend the patches [2] and few hours later I
noticed that one year ago was send a v5 version [3] and I was not aware.
There was some discussion about v5 and during the discussion Mike Snitzer
proposed that at least a change of the syntax is required, we're really
interested on see this upstream as is extensively used in ChromeOS based
devices so I'm wondering if we can restart the discussion and hopefully
we will be able to do the modifications needed.
So my first question is, apart of the change of the syntax, what more
should be changed?
Thanks for your help,
Enric
[1] Patchwork links:
https://patchwork.kernel.org/patch/104857/
https://patchwork.kernel.org/patch/104856/
https://patchwork.kernel.org/patch/104858/
[2] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1375276.html
[3] https://www.redhat.com/archives/dm-devel/2016-February/msg00112.html
Brian Norris (1):
dm: make some mapped_device functions available
Will Drewry (2):
dm: export a table+mapped device to the ioctl interface
init: add support to directly boot to a mapped device
Documentation/admin-guide/kernel-parameters.rst | 1 +
Documentation/admin-guide/kernel-parameters.txt | 3 +
Documentation/device-mapper/boot.txt | 65 ++++
drivers/md/dm-ioctl.c | 36 ++
drivers/md/dm.h | 8 -
include/linux/device-mapper.h | 19 +
init/Makefile | 1 +
init/do_mounts.c | 1 +
init/do_mounts.h | 10 +
init/do_mounts_dm.c | 448 ++++++++++++++++++++++++
10 files changed, 584 insertions(+), 8 deletions(-)
create mode 100644 Documentation/device-mapper/boot.txt
create mode 100644 init/do_mounts_dm.c
--
2.9.3
^ permalink raw reply
* Re: GET_ARRAY_INFO assumptions?
From: Jes Sorensen @ 2017-04-18 16:28 UTC (permalink / raw)
To: NeilBrown, Shaohua Li; +Cc: linux-raid
In-Reply-To: <87k26itx2v.fsf@notabene.neil.brown.name>
On 04/17/2017 07:48 PM, NeilBrown wrote:
> On Fri, Apr 14 2017, Jes Sorensen wrote:
>
>> On 04/13/2017 05:06 PM, Jes Sorensen wrote:
>>> On 04/13/2017 04:37 PM, Shaohua Li wrote:
>>>> On Thu, Apr 13, 2017 at 01:50:06PM -0400, Jes Sorensen wrote:
>>>>> Hi Neil,
>>>>>
>>>>> Looking at trying to phase out the ioctl usage, I am trying to
>>>>> introduce a
>>>>> helper for the 'is the array valid' situation.
>>>>>
>>>>> Now looking at places like Incremental.c (around like 557 in my current
>>>>> tree):
>>>>> /* 7b/ if yes, */
>>>>> /* - if number of OK devices match expected, or -R and there */
>>>>> /* are enough, */
>>>>> /* + add any bitmap file */
>>>>> /* + start the array (auto-readonly). */
>>>>>
>>>>> if (md_get_array_info(mdfd, &ainf) == 0) {
>>>>> if (c->export) {
>>>>> printf("MD_STARTED=already\n");
>>>>> } else if (c->verbose >= 0)
>>>>> pr_err("%s attached to %s which is already active.\n",
>>>>> devname, chosen_name);
>>>>> rv = 0;
>>>>> goto out_unlock;
>>>>> }
>>>>>
>>>>> I am wondering if there are any side effects/assumptions about
>>>>> GET_ARRAY_INFO that I am not considering? Basically I am making the
>>>>> assumption that if /sys/block/md<X>/md exists, the array is valid.
>>>>
>>>> what does 'valid' really mean? md<x>/md exists after a md device is
>>>> allocated,
>>>> the md device might not have any under layer disks bound yet.
>>>>
>>>>> The code in Incremental.c already deals with sysfs higher up in the
>>>>> code, so
>>>>> I guess the question is if the above test is even relevant anymore?
>>>>>
>>>>> Alternative, do we need export a new state in sysfs 'running'?
>>>>
>>>> I'd assume 'running' means the md device has a personality attached. See
>>>> array_state_show(), !running == 'clear' or 'inactive'.
>>>
>>> Good point, I guess what I am trying to figure out is what is assumed
>>> when ioctl(GET_ARRAY_INFO) returns 0 and how do we map it to sysfs?
>>
>> Looking some more at this, it may be simpler than I thought. How about
>> this approach (only compile tested):
>>
>> int md_array_active(int fd)
>> {
>> struct mdinfo *sra;
>> struct mdu_array_info_s array;
>> int ret;
>>
>> sra = sysfs_read(fd, NULL, GET_VERSION | GET_DISKS);
>> if (sra) {
>> if (!sra->array.raid_disks &&
>> !(sra->array.major_version == -1 &&
>> sra->array.minor_version == -2))
>> ret = -ENODEV;
>> else
>> ret = 0;
>>
>> free(sra);
>> } else {
>> ret = ioctl(fd, GET_ARRAY_INFO, &array);
>> }
>>
>> return !ret;
>> }
>>
>> Note 'major = -1 && minor = -2' is sysfs_read's way of saying 'external'.
>>
>> This pretty much mimics what the kernel does in the ioctl handler for
>> GET_ARRAY_INFO:
>>
>> case GET_ARRAY_INFO:
>> if (!mddev->raid_disks && !mddev->external)
>> err = -ENODEV;
>> else
>> err = get_array_info(mddev, argp);
>> goto out;
>>
>> What do you think?
>
> I think that it accurately mimics what the current code does.
> I'm not sure that is what we really want.
> For testing in Incremental.c if an array is "active" we really
> should be testing more than "raid_disks != 0".
> We should be testing, as Shaohua suggested, if
> array_state != 'clear' or 'inactive'.
> You cannot get that info through the ioctl interface, so I suppose
> I decided the current test was 'close enough'.
> If we are going to stop supported kernels that don't have (e.g.)
> array_state, then we should really fo the right thing and test
> array_state.
Neil,
That makes a lot of sense - let me look into this. There are other cases
in the code where the intention isn't 100% clear, so expect me to nag
you as I work through them :)
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH 16/22] xen-blkfront: Make use of the new sg_map helper function
From: Logan Gunthorpe @ 2017-04-18 15:59 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: linux-nvdimm@lists.01.org, Steve Wise,
linux-nvme@lists.infradead.org, target-devel@vger.kernel.org,
Sumit Semwal, devel@driverdev.osuosl.org,
rds-devel@oss.oracle.com, xen-devel, Sagi Grimberg,
linux-scsi@vger.kernel.org, Matthew Wilcox,
linux-rdma@vger.kernel.org, Christoph Hellwig, Ross Zwisler,
open-iscsi@googlegroups.com, linux-media@vger.kernel.org,
intel-gfx@lists.freedesktop.org, sparma
In-Reply-To: <20170418155020.GF12001@char.us.oracle.com>
On 18/04/17 09:50 AM, Konrad Rzeszutek Wilk wrote:
> I am not sure if you know, but you can add on each patch the respective
> maintainer via 'CC'. That way you can have certain maintainers CCed only
> on the subsystems they cover. You put it after (or before) your SoB and
> git send-email happilly picks it up.
Yes, but I've seen some maintainers complain when they receive a patch
with no context (ie. cover letter and first patch). So I chose to do it
this way. I expect in this situation, no matter what you do, someone is
going to complain about the approach chosen.
Thanks anyway for the tip.
Logan
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH 16/22] xen-blkfront: Make use of the new sg_map helper function
From: Konrad Rzeszutek Wilk @ 2017-04-18 15:50 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: linux-nvdimm@lists.01.org, Steve Wise,
linux-nvme@lists.infradead.org, target-devel@vger.kernel.org,
Sumit Semwal, devel@driverdev.osuosl.org,
rds-devel@oss.oracle.com, xen-devel, Sagi Grimberg,
linux-scsi@vger.kernel.org, Matthew Wilcox,
linux-rdma@vger.kernel.org, Christoph Hellwig,
fcoe-devel@open-fcoe.org, Ross Zwisler,
open-iscsi@googlegroups.com, linux-media@vger.kernel.org,
Ming Lin
In-Reply-To: <7930aa93-6106-e12f-ba76-e2771d4ec2dc@deltatee.com>
On Tue, Apr 18, 2017 at 09:42:20AM -0600, Logan Gunthorpe wrote:
>
>
> On 18/04/17 08:27 AM, Konrad Rzeszutek Wilk wrote:
> > Interesting that you didn't CC any of the maintainers. Could you
> > do that in the future please?
>
> Please read the cover letter. The distribution list for the patchset
> would have been way too large to cc every maintainer (even as limited as
> it was, I had mailing lists yelling at me). My plan was to get buy in
I am not sure if you know, but you can add on each patch the respective
maintainer via 'CC'. That way you can have certain maintainers CCed only
on the subsystems they cover. You put it after (or before) your SoB and
git send-email happilly picks it up.
It does mean that for every patch you have to run something like this:
$ more add_cc
#!/bin/bash
git diff HEAD^.. > /tmp/a
echo "---"
scripts/get_maintainer.pl --no-l /tmp/a | while read file
do
echo "Cc: $file"
done
Or such.
> for the first patch, get it merged and resend the rest independently to
> their respective maintainers. Of course, though, I'd be open to other
> suggestions.
>
> >>>
> >>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> >>> ---
> >>> drivers/block/xen-blkfront.c | 33 +++++++++++++++++++++++++++------
> >>> 1 file changed, 27 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> >>> index 5067a0a..7dcf41d 100644
> >>> --- a/drivers/block/xen-blkfront.c
> >>> +++ b/drivers/block/xen-blkfront.c
> >>> @@ -807,8 +807,19 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri
> >>> BUG_ON(sg->offset + sg->length > PAGE_SIZE);
> >>>
> >>> if (setup.need_copy) {
> >>> - setup.bvec_off = sg->offset;
> >>> - setup.bvec_data = kmap_atomic(sg_page(sg));
> >>> + setup.bvec_off = 0;
> >>> + setup.bvec_data = sg_map(sg, SG_KMAP_ATOMIC);
> >>> + if (IS_ERR(setup.bvec_data)) {
> >>> + /*
> >>> + * This should really never happen unless
> >>> + * the code is changed to use memory that is
> >>> + * not mappable in the sg. Seeing there is a
> >>> + * questionable error path out of here,
> >>> + * we WARN.
> >>> + */
> >>> + WARN(1, "Non-mappable memory used in sg!");
> >>> + return 1;
> >>> + }
> >> ...
> >>
> >> Perhaps add a flag to mark failure as 'unexpected' and trace (and panic?)
> >> inside sg_map().
>
> Thanks, that's a good suggestion. I'll make the change for v2.
>
> Logan
^ permalink raw reply
* Re: [PATCH 05/22] drm/i915: Make use of the new sg_map helper function
From: Logan Gunthorpe @ 2017-04-18 15:44 UTC (permalink / raw)
To: Christoph Hellwig, Martin K. Petersen, Sagi Grimberg, Jens Axboe,
Tejun Heo, Greg Kroah-Hartman, Dan Williams, Ross Zwisler,
Matthew Wilcox, Sumit Semwal, Ming Lin, linux-kernel,
linux-crypto, linux-media, dri-devel, linaro-mm-sig, intel-gfx,
linux-raid, linux-mmc, linux-nvme, linux-nvdimm, linux-scsi,
fcoe-devel, open-iscsi, megaraidlinux.pdl, sparmaintainer, devel
In-Reply-To: <20170418064427.r5ewu3p66p2zwdru@phenom.ffwll.local>
On 18/04/17 12:44 AM, Daniel Vetter wrote:
> On Thu, Apr 13, 2017 at 04:05:18PM -0600, Logan Gunthorpe wrote:
>> This is a single straightforward conversion from kmap to sg_map.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Probably makes sense to merge through some other tree, but please be aware
> of the considerable churn rate in i915 (i.e. make sure your tree is in
> linux-next before you send a pull request for this). Plane B would be to
> get the prep patch in first and then merge the i915 conversion one kernel
> release later.
Yes, as per what I said in my cover letter, I was leaning towards a
"Plan B" style approach.
Logan
^ permalink raw reply
* Re: [PATCH 16/22] xen-blkfront: Make use of the new sg_map helper function
From: Logan Gunthorpe @ 2017-04-18 15:42 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, David Laight, xen-devel
Cc: linux-nvdimm@lists.01.org, Steve Wise,
linux-nvme@lists.infradead.org, target-devel@vger.kernel.org,
Sumit Semwal, devel@driverdev.osuosl.org,
rds-devel@oss.oracle.com, Sagi Grimberg,
linux-scsi@vger.kernel.org, Matthew Wilcox,
linux-rdma@vger.kernel.org, Christoph Hellwig,
fcoe-devel@open-fcoe.org, Ross Zwisler,
open-iscsi@googlegroups.com, linux-media@vger.kernel.org,
Ming Lin, intel-gfx
In-Reply-To: <20170418142723.GA27133@char.us.oracle.com>
On 18/04/17 08:27 AM, Konrad Rzeszutek Wilk wrote:
> Interesting that you didn't CC any of the maintainers. Could you
> do that in the future please?
Please read the cover letter. The distribution list for the patchset
would have been way too large to cc every maintainer (even as limited as
it was, I had mailing lists yelling at me). My plan was to get buy in
for the first patch, get it merged and resend the rest independently to
their respective maintainers. Of course, though, I'd be open to other
suggestions.
>>>
>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>>> ---
>>> drivers/block/xen-blkfront.c | 33 +++++++++++++++++++++++++++------
>>> 1 file changed, 27 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>> index 5067a0a..7dcf41d 100644
>>> --- a/drivers/block/xen-blkfront.c
>>> +++ b/drivers/block/xen-blkfront.c
>>> @@ -807,8 +807,19 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri
>>> BUG_ON(sg->offset + sg->length > PAGE_SIZE);
>>>
>>> if (setup.need_copy) {
>>> - setup.bvec_off = sg->offset;
>>> - setup.bvec_data = kmap_atomic(sg_page(sg));
>>> + setup.bvec_off = 0;
>>> + setup.bvec_data = sg_map(sg, SG_KMAP_ATOMIC);
>>> + if (IS_ERR(setup.bvec_data)) {
>>> + /*
>>> + * This should really never happen unless
>>> + * the code is changed to use memory that is
>>> + * not mappable in the sg. Seeing there is a
>>> + * questionable error path out of here,
>>> + * we WARN.
>>> + */
>>> + WARN(1, "Non-mappable memory used in sg!");
>>> + return 1;
>>> + }
>> ...
>>
>> Perhaps add a flag to mark failure as 'unexpected' and trace (and panic?)
>> inside sg_map().
Thanks, that's a good suggestion. I'll make the change for v2.
Logan
^ permalink raw reply
* Re: [PATCH 16/22] xen-blkfront: Make use of the new sg_map helper function
From: Konrad Rzeszutek Wilk @ 2017-04-18 14:27 UTC (permalink / raw)
To: David Laight, xen-devel
Cc: linux-nvdimm@lists.01.org, Steve Wise,
linux-nvme@lists.infradead.org, target-devel@vger.kernel.org,
Sumit Semwal, devel@driverdev.osuosl.org,
rds-devel@oss.oracle.com, Sagi Grimberg,
linux-scsi@vger.kernel.org, Matthew Wilcox,
linux-rdma@vger.kernel.org, Christoph Hellwig,
fcoe-devel@open-fcoe.org, Ross Zwisler,
open-iscsi@googlegroups.com, linux-media@vger.kernel.org,
Ming Lin, intel-gfx
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DCFFD3CD7@AcuExch.aculab.com>
On Tue, Apr 18, 2017 at 02:13:59PM +0000, David Laight wrote:
> From: Logan Gunthorpe
> > Sent: 13 April 2017 23:05
> > Straightforward conversion to the new helper, except due to
> > the lack of error path, we have to warn if unmapable memory
> > is ever present in the sgl.
Interesting that you didn't CC any of the maintainers. Could you
do that in the future please?
> >
> > Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> > ---
> > drivers/block/xen-blkfront.c | 33 +++++++++++++++++++++++++++------
> > 1 file changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 5067a0a..7dcf41d 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -807,8 +807,19 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri
> > BUG_ON(sg->offset + sg->length > PAGE_SIZE);
> >
> > if (setup.need_copy) {
> > - setup.bvec_off = sg->offset;
> > - setup.bvec_data = kmap_atomic(sg_page(sg));
> > + setup.bvec_off = 0;
> > + setup.bvec_data = sg_map(sg, SG_KMAP_ATOMIC);
> > + if (IS_ERR(setup.bvec_data)) {
> > + /*
> > + * This should really never happen unless
> > + * the code is changed to use memory that is
> > + * not mappable in the sg. Seeing there is a
> > + * questionable error path out of here,
> > + * we WARN.
> > + */
> > + WARN(1, "Non-mappable memory used in sg!");
> > + return 1;
> > + }
> ...
>
> Perhaps add a flag to mark failure as 'unexpected' and trace (and panic?)
> inside sg_map().
>
> David
>
>
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox