On Fri, 13 Jun 2014 09:48:16 +0000 "Baldysiak, Pawel"
<pawel.baldysiak@xxxxxxxxx> wrote:
> >On Thursday, June 05, 2014 8:28 AM NeilBrown [neilb@xxxxxxx] wrote:
> > To: Baldysiak, Pawel
> > Cc: linux-raid@xxxxxxxxxxxxxxx; Paszkiewicz, Artur
> > Subject: Re: [PATCH] Forbid merging with partially assembled IMSM
> >array
> >
> > On Wed, 4 Jun 2014 09:25:47 +0000 "Baldysiak, Pawel"
> > <pawel.baldysiak@xxxxxxxxx> wrote:
> >
> >
> > > Hi Neil
> > >
> > > There are several cases where not forbidding this operation can
> > > cause
> > problems.
> > > For example, if user will create IMSM array at one platform, then
> > > stop it, detach devices, and hotplug them to another platform (mixed
> > > between two different controllers) - everything will be looking good
> > > (array
> > will be incrementally assembled without any errors) until reboot.
> > > After that, user will be confused why array is degraded/failed and
> > > will be at
> > risk of losing data.
> > > Also, creation of IMSM array under different controllers is
> > > forbidden, so
> > there is no point in allowing that it in assemble process.
> > >
> > > If you really think this is no justification - I will prepare a
> > > patch that will print warning, but in my opinion this operation
> > > should be allowed
> > only with "--force"...
> > >
> >
> > I came across a situation once where someone was trying to crash-dump
> > onto an IMSM array. 'crashdump' does some sort of kexec thing and
> > runs a very minimal kernel/installation which just does crash dumps.
> >
> > It didn't work because mdadm couldn't find the OpROM in that context
> > and so refused the assemble the array.
> > I really don't like that sort of thing happening.
> > If the OpROM is going to destroy your data (or whatever it does) when
> > you plug different devices into different controllers, then that is
> > the OpROM's problem, not mine. I just want to make sure people can
> > get at their data in as many situations as possible.
> >
> >
> > A warning is probably OK, though I wonder if anyone would ever see it.
> > However the code would need improvement.
> > Doing a strcmp for "imsm" is not acceptable.
> >
> > I'm not even sure I understand what they rest of the code is trying to do.
> > It doesn't seem to make reference to separate OpROMs....
> >
> > NeilBrown
> >
>
> Hi Neil,
>
> Here is the patch that adds warning message.
>
> Thanks
> Pawel Baldysiak
>
> >From a0270724eb42e24a74508f5b0ec35afc64f17cf7 Mon Sep 17 00:00:00 2001
> From: Pawel Baldysiak <pawel.baldysiak@xxxxxxxxx>
> Date: Fri, 13 Jun 2014 10:55:19 +0200
> Subject: [PATCH 1/1] IMSM: Add warning message when assemble spanned container
>
> Due to several changes in code assemble with disks
> spanned between different controllers can be obtained
> in some cases. After IMSM container will be assembled, check HBA of
> disks, and print proper warning if mismatch is detected.
>
> Signed-off-by: Pawel Baldysiak <pawel.baldysiak@xxxxxxxxx>
> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@xxxxxxxxx>
> ---
> Assemble.c | 28 ++++++++++++++++++++++++++++
> platform-intel.h | 1 +
> 2 files changed, 29 insertions(+)
>
> diff --git a/Assemble.c b/Assemble.c
> index 63e09ac..3542b16 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -23,6 +23,7 @@
> */
>
> #include "mdadm.h"
> +#include "platform-intel.h"
> #include <ctype.h>
>
> static int name_matches(char *found, char *required, char *homehost)
> @@ -1001,6 +1002,33 @@ static int start_array(int mdfd,
> content->array.raid_disks);
> fprintf(stderr, "\n");
> }
> + if (st->ss == &super_imsm && !check_env("IMSM_NO_PLATFORM")) {
> + struct sys_dev *idev;
> + char *hba_path = NULL;
> + char *dev_path = devt_to_devpath(makedev(devices->i.disk.major,
> + devices->i.disk.minor));
> +
> + for (idev = find_intel_devices(); idev; idev = idev->next) {
> + if (strstr(dev_path, idev->path)) {
> + hba_path = idev->path;
> + break;
> + }
> + }
> + free(dev_path);
> +
> + if (hba_path) {
> + for (i = 1; (unsigned)i < okcnt+sparecnt; i++) {
> + if (!devt_attached_to_hba(makedev(devices[i].i.disk.major,
> + devices[i].i.disk.minor), hba_path)) {
> + pr_err("WARNING - IMSM container assembled with disks "
> + "under different HBAs!\n"
> + " This operation is not supported "
> + "and can lead to data loss.\n");
> + break;
> + }
> + }
> + }
> + }
> st->ss->free_super(st);
> sysfs_uevent(content, "change");
> if (err_ok && okcnt < (unsigned)content->array.raid_disks)
> diff --git a/platform-intel.h b/platform-intel.h
> index bcd84b7..999a601 100644
> --- a/platform-intel.h
> +++ b/platform-intel.h
> @@ -203,6 +203,7 @@ struct sys_dev *find_driver_devices(const char *bus, const char *driver);
> struct sys_dev *find_intel_devices(void);
> const struct imsm_orom *find_imsm_capability(enum sys_dev_type hba_id);
> const struct imsm_orom *find_imsm_orom(void);
> +int devt_attached_to_hba(dev_t dev, const char *hba_path);
> int disk_attached_to_hba(int fd, const char *hba_path);
> char *devt_to_devpath(dev_t dev);
> int path_attached_to_hba(const char *disk_path, const char *hba_path);
Could you please:
1/ move the code to super-intel.c and add a new superswitch method called
"validate_array" or something like that.
You can't pass the 'devices' array in as only Assemble.c knows about that,
so filling the ->links in the mdinfo structure and pass the devices in as a
list.
2/ Don't use a signed variable when you really want an unsigned variable!
('i' in the above code)
3/ Don't break strings to fit 80 columns. I know we have done that in the
past, but I think it is more trouble than it is worth. Do break the
string after "\n", but not elsewhere.
Then I'll apply it.
Thanks,
NeilBrown
Attachment:
signature.asc
Description: PGP signature
