* [PATCH mdadm] ddf: use 64bit 'size', not 32bit 'info->size' for create.
From: NeilBrown @ 2016-03-10 7:06 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: Dan Russell, linux-raid
[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]
The 'size' field of mdu_disk_info_t is 32bit and should not be used
except for legacy ioctls. super-ddf got this wrong :-(
This change makes it possible to create ddf arrays which used more than
2TB of each device.
Reported-by: Dan Russell <dpr@aol.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
super-ddf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/super-ddf.c b/super-ddf.c
index faaf0a7ca9e0..0e00d17dd169 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -2688,10 +2688,10 @@ static int init_super_ddf_bvd(struct supertype *st,
free(vcl);
return 0;
}
- vc->blocks = cpu_to_be64(info->size * 2);
+ vc->blocks = cpu_to_be64(size * 2);
vc->array_blocks = cpu_to_be64(
calc_array_size(info->level, info->raid_disks, info->layout,
- info->chunk_size, info->size*2));
+ info->chunk_size, size*2));
memset(vc->pad1, 0xff, 8);
vc->spare_refs[0] = cpu_to_be32(0xffffffff);
vc->spare_refs[1] = cpu_to_be32(0xffffffff);
--
2.7.2
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply related
* Re: [PATCH 4/8] Grow: Grow_addbitmap(): Add check to quiet down static code checkers
From: NeilBrown @ 2016-03-10 7:21 UTC (permalink / raw)
To: Jes Sorensen, Guoqing Jiang; +Cc: linux-raid, pawel.baldysiak
In-Reply-To: <wrfjr3fj92ej.fsf@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]
On Thu, Mar 10 2016, Jes Sorensen wrote:
> Guoqing Jiang <gqjiang@suse.com> writes:
>> On 03/09/2016 01:30 AM, Jes.Sorensen@redhat.com wrote:
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> Grow_addbitmap() is only ever called with s->bitmap_file != NULL, but
>>> not all static code checkers catch this. This adds a check to quiet
>>> down the false positive warnings.
>>>
>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>> ---
>>> Grow.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Grow.c b/Grow.c
>>> index 0fa776d..c453eb6 100755
>>> --- a/Grow.c
>>> +++ b/Grow.c
>>> @@ -297,7 +297,14 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
>>> " between different architectures. Consider upgrading the Linux kernel.\n");
>>> }
>>> - if (s->bitmap_file && strcmp(s->bitmap_file, "clustered") ==
>>> 0)
>>> + /*
>>> + * We only ever get called if s->bitmap_file is != NULL, so this check
>>> + * is just here to quiet down static code checkers.
>>> + */
>>> + if (!s->bitmap_file)
>>> + return 1;
>>
>> Is it really need to make all static code checkers happy? ;-)
>> Otherwise, I would prefer remove above check.
>>
>> Anyway, I am fine with the changes.
>
> We had a check in one place, but not in the remaining places. I just
> made it more consistent.
I wonder if maybe the checker was only complaining because the code was
inconsistent.
i.e. if we just got rid of the existing test on s->bitmap_file, maybe
that would make the checker happy.
It would be interesting to experiment even if you ultimately decide to
leave the new test there.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] scatterlist: Introduce some helper functions
From: Robert Jarzmik @ 2016-03-10 9:42 UTC (permalink / raw)
To: Baolin Wang
Cc: Herbert Xu, David Miller, Alasdair G Kergon, Mike Snitzer, axboe,
dm-devel, akpm, david.s.gordon, Tom Lendacky, yamada.masahiro,
smueller, tadeusz.struk, standby24x7, shli, Mark Brown,
Linus Walleij, Arnd Bergmann, LKML, linux-crypto, linux-raid
In-Reply-To: <CAMz4kuKtwCkgG-3GeCtfnAX5zN8MkwyVr2XVYzaV3Z6SFFcZww@mail.gmail.com>
Baolin Wang <baolin.wang@linaro.org> writes:
> Hi Robert,
>
> On 4 March 2016 at 03:15, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> +static inline bool sg_is_contiguous(struct scatterlist *sga,
>>> + struct scatterlist *sgb)
>>> +{
>>> + return ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
>>> + (sgb->page_link & ~0x3UL));
>>> +}
>> I don't understand that one.
>> sga->page_link is a pointer to a "struct page *". How can it be added to an
>> offset within a page ???
>
>
> Ah, sorry that's a mistake. It should check as below:
> static inline bool sg_is_contiguous(struct scatterlist *sga, struct
> scatterlist *sgb)
> {
> return (unsigned int)sg_virt(sga) + sga->length == (unsigned
> int)sg_virt(sgb);
> }
NAK.
First, I don't like the cast.
Second ask yourself: what if you compile on a 64 bit architecture ?
Third, I don't like void* arithmetics, some compilers might refuse it.
And as a last comment, is it "virtual" contiguity you're looking after, physical
contiguity, or dma_addr_t contiguity ?
>>> @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int
>>> nents, gfp_t gfp_mask)
>> ...
>>> /**
>>> + * sg_add_sg_to_table - Add one scatterlist into sg table
>>> + * @sgt: The sg table header to use
>>> + * @src: The sg need to be added into sg table
>>> + *
>>> + * Description:
>>> + * The 'nents' member indicates how many scatterlists added in the sg table.
>>> + * Copy the @src@ scatterlist into sg table and increase 'nents' member.
>>> + *
>>> + **/
>>> +int sg_add_sg_to_table(struct sg_table *sgt, struct scatterlist *src)
>>> +{
>>> + unsigned int i = 0, orig_nents = sgt->orig_nents;
>>> + struct scatterlist *sgl = sgt->sgl;
>>> + struct scatterlist *sg;
>>> +
>>> + /* Check if there are enough space for the new sg to be added */
>>> + if (sgt->nents >= sgt->orig_nents)
>>> + return -EINVAL;
>> I must admit I don't understand that one either : how do comparing the number of
>> "mapped" entries against the number of "allocated" entries determines if there
>> is enough room ?
>
> That's for a dynamic sg table. If there is one sg table allocated
> 'orig_nents' scatterlists, and we need copy another mapped scatterlist
> into the sg table if there are some requirements. So we use 'nents' to
> record how many scatterlists have been copied into the sg table.
As I'm still having difficulities to understand, please explain to me :
- what is a dynamic sg_table
- how it works
- if it's "struct sg_table", how the field of this structure are different when
it's a dynamic sg_table from a "static sg_table" ?
>>> +/**
>>> + * sg_alloc_empty_table - Allocate one empty sg table
>>> + * @sgt: The sg table header to use
>>> + * @nents: Number of entries in sg list
>>> + * @gfp_mask: GFP allocation mask
>>> + *
>>> + * Description:
>>> + * Allocate and initialize an sg table. The 'nents' member of sg_table
>>> + * indicates how many scatterlists added in the sg table. It should set
>>> + * 0 which means there are no scatterlists added in this sg table now.
>>> + *
>>> + **/
>>> +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
>>> + gfp_t gfp_mask)
>> As for this one, there has to be a purpose for it I fail to see. From far away
>> it looks exactly like sg_alloc_table(), excepting it "works around" the nents >
>> 0 protection of __sg_alloc_table().
>> What is exactly the need for this one, and if it's usefull why not simply
>> changing the __sg_alloc_table() "nents > 0" test and see what the outcome of the
>> review will be ?
>
> Like I said above. If we want to copy some mapped scatterlists into
> one sg table, we should set the 'nents' to 0 to indicates how many
> scatterlists coppied in the sg table.
So how do you unmap this scatterlist once you've lost the number of mapped
entries ?
I think we'll come back to this once I understand how a "dynamic" sg_table is
different from a "static" one.
Cheers.
--
Robert
^ permalink raw reply
* Re: [PATCH 5/8] {platform,super}-intel: Fix two resource leaks
From: Baldysiak, Pawel @ 2016-03-10 11:14 UTC (permalink / raw)
To: neilb@suse.de, Jes.Sorensen@redhat.com
Cc: linux-raid@vger.kernel.org, gqjiang@suse.com
In-Reply-To: <wrfjegbjvcvd.fsf@redhat.com>
On Wed, 2016-03-09 at 11:23 -0500, Jes Sorensen wrote:
> NeilBrown <neilb@suse.de> writes:
> >
> > On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
> >
> > >
> > > From: Jes Sorensen <Jes.Sorensen@redhat.com>
> > >
> > > The code did not free 'dir' allocated by opendir(). An additional
> > > benefit is that this simplifies the for() loops.
> > >
> > > Fixes: 60f0f54d ("IMSM: Add support for VMD")
> > > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> > > ---
> > > platform-intel.c | 7 ++++++-
> > > super-intel.c | 6 +++++-
> > > 2 files changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/platform-intel.c b/platform-intel.c
> > > index 88818f3..c60fd9e 100644
> > > --- a/platform-intel.c
> > > +++ b/platform-intel.c
> > > @@ -724,8 +724,10 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
> > > return NULL;
> > >
> > > dir = opendir("/sys/bus/pci/drivers/vmd");
> > > + if (!dir)
> > > + return NULL;
> > >
> > > - for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
> > > + for (ent = readdir(dir); ent; ent = readdir(dir)) {
> > > sprintf(path, "/sys/bus/pci/drivers/vmd/%s/domain/device",
> > > ent->d_name);
> > >
> > > @@ -734,8 +736,11 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
> > >
> > > if (strncmp(buf, hba->path, strlen(buf)) == 0) {
> > > sprintf(path, "/sys/bus/pci/drivers/vmd/%s", ent->d_name);
> > > + closedir(dir);
> > > return realpath(path, buf);
> > > }
> > > }
> > > +
> > > + closedir(dir);
> > > return NULL;
> > > }
> > > diff --git a/super-intel.c b/super-intel.c
> > > index 158f4e8..e1bee75 100644
> > > --- a/super-intel.c
> > > +++ b/super-intel.c
> > > @@ -1781,7 +1781,10 @@ static int print_vmd_attached_devs(struct sys_dev *hba)
> > > * this hba
> > > */
> > > dir = opendir("/sys/bus/pci/drivers/nvme");
> > > - for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
> > > + if (!dir)
> > > + return 1;
> > > +
> > Returning '1' looks really weird here. I can see it is consistent with
> > if (hba->type != SYS_DEV_VMD)
> > return 1;
> >
> > above, but still....
> > As the return value is never used, should we just make it 'void' ??
> Seems reasonable - I'll put that in a separate patch.
>
> Cheers,
> Jes
Hello,
Thanks for the fix Jes.
I thinks that instead of making it "void" we can actually check the return value and print a proper message if something goes wrong.
Also "simplified for() loop" can be applied to ahci_enumerate_ports() as well.
@@ -1624,7 +1624,10 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
* this hba
*/
dir = opendir("/sys/dev/block");
- for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
+ if (!dir)
+ return 1;
+
+ for (ent = readdir(dir); ent; ent = readdir(dir)) {
int fd;
char model[64];
char vendor[64];
@@ -2021,7 +2024,11 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
print_imsm_capability(&entry->orom);
printf(" I/O Controller : %s (%s)\n",
vmd_domain_to_controller(hba, buf), get_sys_dev_type(hba->type));
- print_vmd_attached_devs(hba);
+ if (print_vmd_attached_devs(hba)) {
+ if (verbose > 0)
+ pr_err("failed to get devices attached to VMD domain.\n");
+ result |= 2;
+ }
printf("\n");
}
}
Is that ok with you? Should I prepare a proper patch with this?
Thanks,
Pawel Baldysiak
^ permalink raw reply
* Re: [PATCH 1/4] scatterlist: Introduce some helper functions
From: Baolin Wang @ 2016-03-10 12:58 UTC (permalink / raw)
To: Robert Jarzmik
Cc: Herbert Xu, David Miller, Alasdair G Kergon, Mike Snitzer, axboe,
dm-devel, akpm, david.s.gordon, Tom Lendacky, Masahiro Yamada,
smueller, tadeusz.struk, Masanari Iida, shli, Mark Brown,
Linus Walleij, Arnd Bergmann, LKML, linux-crypto, linux-raid
In-Reply-To: <87vb4ufz4b.fsf@belgarion.home>
On 10 March 2016 at 17:42, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>>
>>
>> Ah, sorry that's a mistake. It should check as below:
>> static inline bool sg_is_contiguous(struct scatterlist *sga, struct
>> scatterlist *sgb)
>> {
>> return (unsigned int)sg_virt(sga) + sga->length == (unsigned
>> int)sg_virt(sgb);
>> }
> NAK.
> First, I don't like the cast.
> Second ask yourself: what if you compile on a 64 bit architecture ?
Sorry, it's a mistake. I've resend one email to correct that with
'unsigned long'.
> Third, I don't like void* arithmetics, some compilers might refuse it.
OK. I will modify that.
>
> And as a last comment, is it "virtual" contiguity you're looking after, physical
> contiguity, or dma_addr_t contiguity ?
Yes, I need check the virtual contiguity.
>
>>>> @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int
>>>> nents, gfp_t gfp_mask)
>>> ...
>>>> /**
>>>> + * sg_add_sg_to_table - Add one scatterlist into sg table
>>>> + * @sgt: The sg table header to use
>>>> + * @src: The sg need to be added into sg table
>>>> + *
>>>> + * Description:
>>>> + * The 'nents' member indicates how many scatterlists added in the sg table.
>>>> + * Copy the @src@ scatterlist into sg table and increase 'nents' member.
>>>> + *
>>>> + **/
>>>> +int sg_add_sg_to_table(struct sg_table *sgt, struct scatterlist *src)
>>>> +{
>>>> + unsigned int i = 0, orig_nents = sgt->orig_nents;
>>>> + struct scatterlist *sgl = sgt->sgl;
>>>> + struct scatterlist *sg;
>>>> +
>>>> + /* Check if there are enough space for the new sg to be added */
>>>> + if (sgt->nents >= sgt->orig_nents)
>>>> + return -EINVAL;
>>> I must admit I don't understand that one either : how do comparing the number of
>>> "mapped" entries against the number of "allocated" entries determines if there
>>> is enough room ?
>>
>> That's for a dynamic sg table. If there is one sg table allocated
>> 'orig_nents' scatterlists, and we need copy another mapped scatterlist
>> into the sg table if there are some requirements. So we use 'nents' to
>> record how many scatterlists have been copied into the sg table.
> As I'm still having difficulities to understand, please explain to me :
> - what is a dynamic sg_table
OK. That's for some special usage. For example, the dm-crypt will send
one request mapped one scatterlist to engine level to handle the
request at one time. But we want to collect some requests together to
handle together at one time to improve engine efficiency. So in crypto
engine layer we need to copy the mapped scatterlists into one sg
table, which has been allocated some sg memory.
> - how it works
The 'orig_nents' means how many scatterlists the sg table can hold.
The 'nents' means how many scatterlists have been copied into sg
table. So the 'nents' will be not equal as 'orig_nents' unless the sg
table is full.
> - if it's "struct sg_table", how the field of this structure are different when
> it's a dynamic sg_table from a "static sg_table" ?
>
>>>> +/**
>>>> + * sg_alloc_empty_table - Allocate one empty sg table
>>>> + * @sgt: The sg table header to use
>>>> + * @nents: Number of entries in sg list
>>>> + * @gfp_mask: GFP allocation mask
>>>> + *
>>>> + * Description:
>>>> + * Allocate and initialize an sg table. The 'nents' member of sg_table
>>>> + * indicates how many scatterlists added in the sg table. It should set
>>>> + * 0 which means there are no scatterlists added in this sg table now.
>>>> + *
>>>> + **/
>>>> +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
>>>> + gfp_t gfp_mask)
>>> As for this one, there has to be a purpose for it I fail to see. From far away
>>> it looks exactly like sg_alloc_table(), excepting it "works around" the nents >
>>> 0 protection of __sg_alloc_table().
>>> What is exactly the need for this one, and if it's usefull why not simply
>>> changing the __sg_alloc_table() "nents > 0" test and see what the outcome of the
>>> review will be ?
>>
>> Like I said above. If we want to copy some mapped scatterlists into
>> one sg table, we should set the 'nents' to 0 to indicates how many
>> scatterlists coppied in the sg table.
> So how do you unmap this scatterlist once you've lost the number of mapped
> entries ?
I will not lost the number of mapped in sg table, 'nents' member
means how many mapped scatterlists in the dynamic table. Every time
one mapped sg copied into the dynamic table, the 'nents' will increase
1.
>
> I think we'll come back to this once I understand how a "dynamic" sg_table is
> different from a "static" one.
>
> Cheers.
>
> --
> Robert
--
Baolin.wang
Best Regards
^ permalink raw reply
* Re: [PATCH 5/8] {platform,super}-intel: Fix two resource leaks
From: Jes Sorensen @ 2016-03-10 16:37 UTC (permalink / raw)
To: Baldysiak, Pawel
Cc: neilb@suse.de, linux-raid@vger.kernel.org, gqjiang@suse.com
In-Reply-To: <1457608450.28311.10.camel@intel.com>
"Baldysiak, Pawel" <pawel.baldysiak@intel.com> writes:
> On Wed, 2016-03-09 at 11:23 -0500, Jes Sorensen wrote:
>> NeilBrown <neilb@suse.de> writes:
>> >
>> > On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>> > Returning '1' looks really weird here. I can see it is consistent with
>> > if (hba->type != SYS_DEV_VMD)
>> > return 1;
>> >
>> > above, but still....
>> > As the return value is never used, should we just make it 'void' ??
>> Seems reasonable - I'll put that in a separate patch.
>>
>> Cheers,
>> Jes
> Hello,
> Thanks for the fix Jes.
>
> I thinks that instead of making it "void" we can actually check the
> return value and print a proper message if something goes wrong.
> Also "simplified for() loop" can be applied to ahci_enumerate_ports() as well.
>
> @@ -1624,7 +1624,10 @@ static int ahci_enumerate_ports(const char
> *hba_path, int port_count, int host_b
> * this hba
> */
> dir = opendir("/sys/dev/block");
> - for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
> + if (!dir)
> + return 1;
> +
> + for (ent = readdir(dir); ent; ent = readdir(dir)) {
> int fd;
> char model[64];
> char vendor[64];
> @@ -2021,7 +2024,11 @@ static int detail_platform_imsm(int verbose,
> int enumerate_only, char *controlle
> print_imsm_capability(&entry->orom);
> printf(" I/O Controller : %s (%s)\n",
> vmd_domain_to_controller(hba, buf), get_sys_dev_type(hba->type));
> - print_vmd_attached_devs(hba);
> + if (print_vmd_attached_devs(hba)) {
> + if (verbose > 0)
> + pr_err("failed to get devices attached to VMD domain.\n");
> + result |= 2;
> + }
> printf("\n");
> }
> }
>
> Is that ok with you? Should I prepare a proper patch with this?
Hi Pawel,
I already pushed this into git, but if you want to send me a patch (or
two) for this, I am happy to apply them. Lets make the
ahci_enumerate_ports() change in a separate patch.
Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/8] Grow: Grow_addbitmap(): Add check to quiet down static code checkers
From: Jes Sorensen @ 2016-03-10 16:40 UTC (permalink / raw)
To: NeilBrown; +Cc: Guoqing Jiang, linux-raid, pawel.baldysiak
In-Reply-To: <877fhakdcw.fsf@notabene.neil.brown.name>
NeilBrown <neilb@suse.de> writes:
> On Thu, Mar 10 2016, Jes Sorensen wrote:
>
>> Guoqing Jiang <gqjiang@suse.com> writes:
>>> On 03/09/2016 01:30 AM, Jes.Sorensen@redhat.com wrote:
>>>> @@ -297,7 +297,14 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
>>>> " between different architectures. Consider upgrading the Linux kernel.\n");
>>>> }
>>>> - if (s->bitmap_file && strcmp(s->bitmap_file, "clustered") ==
>>>> 0)
>>>> + /*
>>>> + * We only ever get called if s->bitmap_file is != NULL, so this check
>>>> + * is just here to quiet down static code checkers.
>>>> + */
>>>> + if (!s->bitmap_file)
>>>> + return 1;
>>>
>>> Is it really need to make all static code checkers happy? ;-)
>>> Otherwise, I would prefer remove above check.
>>>
>>> Anyway, I am fine with the changes.
>>
>> We had a check in one place, but not in the remaining places. I just
>> made it more consistent.
>
> I wonder if maybe the checker was only complaining because the code
> was inconsistent. i.e. if we just got rid of the existing test on
> s->bitmap_file, maybe that would make the checker happy. It would be
> interesting to experiment even if you ultimately decide to leave the
> new test there.
I went back and checked the scan logs and it basically complained about
all the cases that didn't check the validity of s->bitmap_file. I think
it's safer to just have the one check at the top, even if we expect it
never to be called with an invalid pointer.
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH mdadm] ddf: use 64bit 'size', not 32bit 'info->size' for create.
From: Jes Sorensen @ 2016-03-10 16:43 UTC (permalink / raw)
To: NeilBrown; +Cc: Dan Russell, linux-raid
In-Reply-To: <87a8m6ke0c.fsf@notabene.neil.brown.name>
NeilBrown <neilb@suse.com> writes:
> The 'size' field of mdu_disk_info_t is 32bit and should not be used
> except for legacy ioctls. super-ddf got this wrong :-(
>
> This change makes it possible to create ddf arrays which used more than
> 2TB of each device.
>
> Reported-by: Dan Russell <dpr@aol.com>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> super-ddf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/super-ddf.c b/super-ddf.c
> index faaf0a7ca9e0..0e00d17dd169 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -2688,10 +2688,10 @@ static int init_super_ddf_bvd(struct supertype *st,
> free(vcl);
> return 0;
> }
> - vc->blocks = cpu_to_be64(info->size * 2);
> + vc->blocks = cpu_to_be64(size * 2);
> vc->array_blocks = cpu_to_be64(
> calc_array_size(info->level, info->raid_disks, info->layout,
> - info->chunk_size, info->size*2));
> + info->chunk_size, size*2));
> memset(vc->pad1, 0xff, 8);
> vc->spare_refs[0] = cpu_to_be32(0xffffffff);
> vc->spare_refs[1] = cpu_to_be32(0xffffffff);
Thanks for the fix! I applied it with a small mod, adding the missing
spaces in the last line:
> + info->chunk_size, size * 2));
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH] Fix regression during add devices
From: Jes Sorensen @ 2016-03-10 16:44 UTC (permalink / raw)
To: Coly Li; +Cc: linux-raid, Hannes Reinecke, Neil Brown
In-Reply-To: <56E0FF30.1060807@suse.de>
Coly Li <colyli@suse.de> writes:
> 在 16/3/10 上午2:30, Jes Sorensen 写道:
>> Coly Li <colyli@suse.de> writes:
>>> From: Hannes Reinecke <hare@suse.de>
>>>
>>> Commit d180d2aa2a17 ("Manage: fix test for 'is array failed'.")
>>> introduced a regression which would not allow to re-add new
>>> drivers to a failed array.
>>>
>>> The patch is written by Hannes Reinecke, Neil Brown points out
>>> the buggy commit ID is d180d2aa2a17. Coly helps to submit the
>>> patch to mdadm upstream.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> Cc: Coly Li <colyli@suse.de>
>>> Cc: Neil Brown <neilb@suse.com>
>>> ---
>>> Manage.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Coly,
>>
>> Thanks for the fix - please use "Fixes: " to indicate which patch
>> introduced the bug, and the story of pushing the patch really belongs in
>> a comment below the --- line, so it doesn't go into git.
>>
>> I modified it to look like this - please let me know if you are OK with
>> this.
>>
>> Thanks,
>> Jes
>>
>
> Hi Jes,
>
> It is OK to me, so I don't resend another fixed version.
> Thanks for the fix up.
Great, yes you're all set!
Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] Grow: analyse_change add notification about only 2-device can be convert from RAID1 to RAID5
From: Yi Zhang @ 2016-03-11 9:26 UTC (permalink / raw)
To: linux-raid; +Cc: Jes.Sorensen, neilb, shli, xni, Yi Zhang
Notify "Can only convert a 2-device array to RAID5" instead of
"Impossibly level change request for RAID1" when convert from
RAID1 to RAID5 if the disk num is not equal two like RAID4/5->RAID1
did.
Signed-off-by: Yi Zhang <yizhan@redhat.com>
---
Grow.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Grow.c b/Grow.c
index cf2750a..4e2fc08 100755
--- a/Grow.c
+++ b/Grow.c
@@ -1077,6 +1077,9 @@ char *analyse_change(char *devname, struct mdinfo *info, struct reshape *re)
re->level = 1;
return NULL;
}
+ if (info->array.raid_disks != 2 &&
+ info->new_level == 5)
+ return "Can only convert a 2-device array to RAID5";
if (info->array.raid_disks == 2 &&
info->new_level == 5) {
--
2.5.0
^ permalink raw reply related
* [PATCH 1/1] Grow: close fd earlier to avoid "cannot get excl access" when stopping
From: Pawel Baldysiak @ 2016-03-11 12:49 UTC (permalink / raw)
To: linux-raid; +Cc: jes.sorensen, artur.paszkiewicz, Pawel Baldysiak
If this file descriptor is not closed here, it remains open during
reshape process and stopping process will end up with
"cannot get exclusive access to container".
Once this file descriptor is no longer needed - it can be closed.
Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
---
Grow.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Grow.c b/Grow.c
index cf2750a..d9db00b 100755
--- a/Grow.c
+++ b/Grow.c
@@ -2070,6 +2070,10 @@ size_change_error:
* number of devices (On-Line Capacity Expansion) must be
* performed at the level of the container
*/
+ if (fd > 0) {
+ close(fd);
+ fd = -1;
+ }
rv = reshape_container(container, devname, -1, st, &info,
c->force, c->backup_file, c->verbose, 0, 0, 0);
frozen = 0;
--
2.5.0
^ permalink raw reply related
* [PATCH 1/2] super-intel: Make print_vmd_attached_devs() return int again
From: Pawel Baldysiak @ 2016-03-11 15:47 UTC (permalink / raw)
To: linux-raid; +Cc: jes.sorensen, artur.paszkiewicz, Pawel Baldysiak
This patch reverts a0abe1e
(super-intel: Make print_found_intel_controllers() return void)
and make this function "return int" again.
Also, interpreting the return value is added.
Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
---
super-intel.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/super-intel.c b/super-intel.c
index 3fe304a..8a80c5b 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1766,7 +1766,7 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
return err;
}
-static void print_vmd_attached_devs(struct sys_dev *hba)
+static int print_vmd_attached_devs(struct sys_dev *hba)
{
struct dirent *ent;
DIR *dir;
@@ -1775,14 +1775,14 @@ static void print_vmd_attached_devs(struct sys_dev *hba)
char *c, *rp;
if (hba->type != SYS_DEV_VMD)
- return;
+ return 1;
/* scroll through /sys/dev/block looking for devices attached to
* this hba
*/
dir = opendir("/sys/bus/pci/drivers/nvme");
if (!dir)
- return;
+ return 1;
for (ent = readdir(dir); ent; ent = readdir(dir)) {
int n;
@@ -1818,6 +1818,7 @@ static void print_vmd_attached_devs(struct sys_dev *hba)
}
closedir(dir);
+ return 0;
}
static void print_found_intel_controllers(struct sys_dev *elem)
@@ -2024,7 +2025,11 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
print_imsm_capability(&entry->orom);
printf(" I/O Controller : %s (%s)\n",
vmd_domain_to_controller(hba, buf), get_sys_dev_type(hba->type));
- print_vmd_attached_devs(hba);
+ if (print_vmd_attached_devs(hba)) {
+ if (verbose > 0)
+ pr_err("failed to get devices attached to VMD domain.\n");
+ result |= 2;
+ }
printf("\n");
}
}
--
2.5.0
^ permalink raw reply related
* [PATCH 2/2] super-intel: Simplify for() loop in ahci_enumerate_ports
From: Pawel Baldysiak @ 2016-03-11 15:47 UTC (permalink / raw)
To: linux-raid; +Cc: jes.sorensen, artur.paszkiewicz, Pawel Baldysiak
In-Reply-To: <1457711236-16990-1-git-send-email-pawel.baldysiak@intel.com>
This patch simplifies for() loop used in
ahci_enumerate_ports(). It makes it more readable.
Similar thing was done in b913501
({platform,super}-intel: Fix two resource leaks).
Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
---
super-intel.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/super-intel.c b/super-intel.c
index 8a80c5b..1bc3688 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1624,7 +1624,10 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
* this hba
*/
dir = opendir("/sys/dev/block");
- for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
+ if (!dir)
+ return 1;
+
+ for (ent = readdir(dir); ent; ent = readdir(dir)) {
int fd;
char model[64];
char vendor[64];
--
2.5.0
^ permalink raw reply related
* Re: [PATCH 1/1] Grow: close fd earlier to avoid "cannot get excl access" when stopping
From: Jes Sorensen @ 2016-03-11 17:31 UTC (permalink / raw)
To: Pawel Baldysiak; +Cc: linux-raid, artur.paszkiewicz
In-Reply-To: <1457700547-13253-1-git-send-email-pawel.baldysiak@intel.com>
Pawel Baldysiak <pawel.baldysiak@intel.com> writes:
> If this file descriptor is not closed here, it remains open during
> reshape process and stopping process will end up with
> "cannot get exclusive access to container".
> Once this file descriptor is no longer needed - it can be closed.
>
> Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> ---
> Grow.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Grow.c b/Grow.c
> index cf2750a..d9db00b 100755
> --- a/Grow.c
> +++ b/Grow.c
> @@ -2070,6 +2070,10 @@ size_change_error:
> * number of devices (On-Line Capacity Expansion) must be
> * performed at the level of the container
> */
> + if (fd > 0) {
> + close(fd);
> + fd = -1;
> + }
> rv = reshape_container(container, devname, -1, st, &info,
> c->force, c->backup_file, c->verbose, 0, 0, 0);
> frozen = 0;
I went in and looked at this one to see how we deal with the open file
descriptior. I was worried something else could try and access or close
it later, but it looks like we just let it dangle and rely on exit()
cleaning up. So I'll apply this one as is and hopefully someone will
feel excited enough about it to clean it up down the line ;)
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH 1/2] super-intel: Make print_vmd_attached_devs() return int again
From: Jes Sorensen @ 2016-03-11 17:36 UTC (permalink / raw)
To: Pawel Baldysiak; +Cc: linux-raid, artur.paszkiewicz
In-Reply-To: <1457711236-16990-1-git-send-email-pawel.baldysiak@intel.com>
Pawel Baldysiak <pawel.baldysiak@intel.com> writes:
> This patch reverts a0abe1e
> (super-intel: Make print_found_intel_controllers() return void)
> and make this function "return int" again.
> Also, interpreting the return value is added.
>
> Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> ---
> super-intel.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
Applied!
Thanks,
Jes
^ permalink raw reply
* Re: [PATCH 2/2] super-intel: Simplify for() loop in ahci_enumerate_ports
From: Jes Sorensen @ 2016-03-11 17:36 UTC (permalink / raw)
To: Pawel Baldysiak; +Cc: linux-raid, artur.paszkiewicz
In-Reply-To: <1457711236-16990-2-git-send-email-pawel.baldysiak@intel.com>
Pawel Baldysiak <pawel.baldysiak@intel.com> writes:
> This patch simplifies for() loop used in
> ahci_enumerate_ports(). It makes it more readable.
> Similar thing was done in b913501
> ({platform,super}-intel: Fix two resource leaks).
>
> Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> ---
> super-intel.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
Applied!
Thanks,
Jes
^ permalink raw reply
* Re: [PATCH] Grow: analyse_change add notification about only 2-device can be convert from RAID1 to RAID5
From: Jes Sorensen @ 2016-03-11 17:41 UTC (permalink / raw)
To: Yi Zhang; +Cc: linux-raid, neilb, shli, xni
In-Reply-To: <1457688400-12666-1-git-send-email-yizhan@redhat.com>
Yi Zhang <yizhan@redhat.com> writes:
> Notify "Can only convert a 2-device array to RAID5" instead of
> "Impossibly level change request for RAID1" when convert from
> RAID1 to RAID5 if the disk num is not equal two like RAID4/5->RAID1
> did.
>
> Signed-off-by: Yi Zhang <yizhan@redhat.com>
> ---
> Grow.c | 3 +++
> 1 file changed, 3 insertions(+)
Applied!
Thanks,
Jes
>
> diff --git a/Grow.c b/Grow.c
> index cf2750a..4e2fc08 100755
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1077,6 +1077,9 @@ char *analyse_change(char *devname, struct mdinfo *info, struct reshape *re)
> re->level = 1;
> return NULL;
> }
> + if (info->array.raid_disks != 2 &&
> + info->new_level == 5)
> + return "Can only convert a 2-device array to RAID5";
> if (info->array.raid_disks == 2 &&
> info->new_level == 5) {
^ permalink raw reply
* Re: multipath: I/O hanging forever
From: Andrea Righi @ 2016-03-11 22:24 UTC (permalink / raw)
To: Kent Overstreet; +Cc: Shaohua Li, linux-raid, linux-kernel
In-Reply-To: <20160306053103.GA31060@kmo-pixel>
On Sat, Mar 05, 2016 at 08:31:03PM -0900, Kent Overstreet wrote:
> On Fri, Mar 04, 2016 at 10:30:44AM -0700, Andrea Righi wrote:
> > On Sun, Feb 28, 2016 at 08:46:16PM -0700, Andrea Righi wrote:
> > > On Sun, Feb 28, 2016 at 06:53:33PM -0700, Andrea Righi wrote:
> > > ...
> > > > I'm using 4.5.0-rc5+, from Linus' git. I'll try to do a git bisect
> > > > later, I'm pretty sure this problem has been introduced recently (i.e.,
> > > > I've never seen this issue with 4.1.x).
> > >
> > > I confirm, just tested kernel 4.1 and this problem doesn't happen.
> >
> > Alright, I had some spare time to bisect this problem and I found that
> > the commit that introduced this issue is c66a14d.
> >
> > So, I tried to revert the commit (with some changes to fix conflicts and
> > ABI changes) and now multipath seems to work fine for me (no hung task).
>
> Is it hanging on first IO, first large IO, or just randomly?
It's always the very first O_DIRECT I/O, in general the task gets stuck
in do_blockdev_direct_IO().
Thanks,
-Andrea
^ permalink raw reply
* Re: multipath: I/O hanging forever
From: Ming Lei @ 2016-03-12 1:47 UTC (permalink / raw)
To: Andrea Righi
Cc: Kent Overstreet, Shaohua Li, linux-raid, linux-kernel,
tom.leiming
In-Reply-To: <20160311222433.GA2617@Dell>
On Fri, 11 Mar 2016 15:24:33 -0700
Andrea Righi <righi.andrea@gmail.com> wrote:
> On Sat, Mar 05, 2016 at 08:31:03PM -0900, Kent Overstreet wrote:
> > On Fri, Mar 04, 2016 at 10:30:44AM -0700, Andrea Righi wrote:
> > > On Sun, Feb 28, 2016 at 08:46:16PM -0700, Andrea Righi wrote:
> > > > On Sun, Feb 28, 2016 at 06:53:33PM -0700, Andrea Righi wrote:
> > > > ...
> > > > > I'm using 4.5.0-rc5+, from Linus' git. I'll try to do a git bisect
> > > > > later, I'm pretty sure this problem has been introduced recently (i.e.,
> > > > > I've never seen this issue with 4.1.x).
> > > >
> > > > I confirm, just tested kernel 4.1 and this problem doesn't happen.
> > >
> > > Alright, I had some spare time to bisect this problem and I found that
> > > the commit that introduced this issue is c66a14d.
> > >
> > > So, I tried to revert the commit (with some changes to fix conflicts and
> > > ABI changes) and now multipath seems to work fine for me (no hung task).
> >
> > Is it hanging on first IO, first large IO, or just randomly?
>
> It's always the very first O_DIRECT I/O, in general the task gets stuck
> in do_blockdev_direct_IO().
I can reproduce the issue too, and looks it is a MD issue instead of block.
Andrea, could you try the following patch to see if it can fix your issue?
---
From 43fc9c221e53c64f2df7c100c77cc25c4a98c607 Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@canonical.com>
Date: Sat, 12 Mar 2016 09:29:40 +0800
Subject: [PATCH] md: multipath: don't hardcopy bio in .make_request path
Inside multipath_make_request(), multipath maps the incoming
bio into low level device's bio, but it is totally wrong to
copy the bio into mapped bio via '*mapped_bio = *bio'. For
example, .__bi_remaining is kept in the copy, especially if
the incoming bio is chained to via bio splitting, so .bi_end_io
can't be called for the mapped bio at all in the completing path
in this kind of situation.
This patch fixes the issue by using clone style.
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
drivers/md/multipath.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
index 0a72ab6..dd483bb 100644
--- a/drivers/md/multipath.c
+++ b/drivers/md/multipath.c
@@ -129,7 +129,9 @@ static void multipath_make_request(struct mddev *mddev, struct bio * bio)
}
multipath = conf->multipaths + mp_bh->path;
- mp_bh->bio = *bio;
+ bio_init(&mp_bh->bio);
+ __bio_clone_fast(&mp_bh->bio, bio);
+
mp_bh->bio.bi_iter.bi_sector += multipath->rdev->data_offset;
mp_bh->bio.bi_bdev = multipath->rdev->bdev;
mp_bh->bio.bi_rw |= REQ_FAILFAST_TRANSPORT;
--
1.9.1
Thanks,
^ permalink raw reply related
* Re: RAID6 Array crash during reshape.....now will not re-assemble.
From: Another Sillyname @ 2016-03-12 11:38 UTC (permalink / raw)
To: NeilBrown; +Cc: Linux-RAID
In-Reply-To: <874mcglcsd.fsf@notabene.neil.brown.name>
Neil
Thanks for the insight, much appreciated.
I've tried what you suggested and still get stuck.
>:losetup /dev/loop0 /tmp/foo/sdb1
>:losetup /dev/loop1 /tmp/foo/sdc1
>:losetup /dev/loop2 /tmp/foo/sdd1
>:losetup /dev/loop3 /tmp/foo/sde1
>:losetup /dev/loop4 /tmp/foo/sdf1
>:losetup /dev/loop5 /tmp/foo/sdg1
>:losetup /dev/loop6 /tmp/foo/sdh1
>:mdadm --assemble --force --update=revert-reshape --invalid-backup /dev/md127 /dev/loop0 /dev/loop1 /dev/loop2 /dev/loop3 /dev/loop4 /dev/loop5 /dev/loop6
mdadm: /dev/md127: Need a backup file to complete reshape of this array.
mdadm: Please provided one with "--backup-file=..."
mdadm: (Don't specify --update=revert-reshape again, that part succeeded.)
As you can see it 'seems' to have accepted the revert command, but
even though I've told it the backup is invalid it's still insisting on
the backup being made available.
Any further thoughts or insights would be gratefully received.
On 9 March 2016 at 00:23, NeilBrown <nfbrown@novell.com> wrote:
> On Wed, Mar 02 2016, Another Sillyname wrote:
>
>> I have a 30TB RAID6 array using 7 x 6TB drives that I wanted to
>> migrate to RAID5 to take one of the drives offline and use in a new
>> array for a migration.
>>
>> sudo mdadm --grow /dev/md127 --level=raid5 --raid-device=6
>> --backup-file=mdadm_backupfile
>
> First observation: Don't use --backup-file unless mdadm tell you that
> you have to. New mdadm on new kernel with newly create arrays don't
> need a backup file at all. Your array is sufficiently newly created and
> I think your mdadm/kernel are new enough too. Note in the --examine output:
>
>> Unused Space : before=262056 sectors, after=143 sectors
>
> This means there is (nearly) 128M of free space in the start of each
> device. md can perform the reshape by copying a few chunks down into
> this space, then the next few chunks into the space just freed, then the
> next few chunks ... and so on. No backup file needed. That is
> providing the chunk size is quite a bit smaller than the space, and your
> 512K chunk size certainly is.
>
> A reshape which increases the size of the array needs 'before' space, a
> reshape which decreases the size of the array needs 'after' space. A
> reshape which doesn't change the size of the array (like yours) can use
> either.
>
>>
>> I watched this using cat /proc/mdstat and even after an hour the
>> percentage of the reshape was still 0.0%.
>
> A more useful number to watch is the (xxx/yyy) after the percentage.
> The first number should change at least every few seconds.
>
>>
>> Reboot.....
>>
>> Array will not come back online at all.
>>
>> Bring the server up without the array trying to automount.
>>
>> cat /proc/mdstat shows the array offline.
>>
>> Personalities :
>> md127 : inactive sdf1[2](S) sde1[3](S) sdg1[0](S) sdb1[8](S)
>> sdh1[7](S) sdc1[1](S) sdd1[6](S)
>> 41022733300 blocks super 1.2
>>
>> unused devices: <none>
>>
>> Try to reassemble the array.
>>
>>>sudo mdadm --assemble /dev/md127 /dev/sdb1 /dev/sdc1 /dev/sdd1 /dev/sde1 /dev/sdf1 /dev/sdg1 /dev/sdh1
>> mdadm: /dev/sdg1 is busy - skipping
>> mdadm: /dev/sdh1 is busy - skipping
>> mdadm: Merging with already-assembled /dev/md/server187.internallan.com:1
>
> It looks like you are getting races with udev. mdadm is detecting the
> race and says that it is "Merging" rather than creating a separate array
> but still the result isn't very useful...
>
>
> When you run "mdadm --assemble /dev/md127 ...." mdadm notices that /dev/md127
> already exists but isn't active, so it stops it properly so that all the
> devices become available to be assembled.
> As the devices become available they tell udev "Hey, I've changed
> status" and udev says "Hey, you look like part of an md array, let's put
> you back together".... or something like that. I might have the details
> a little wrong - it is a while since I looked at this.
> Anyway it seems that udev called "mdadm -I" to put some of the devices
> together so they were busy when your "mdadm --assemble" looked at them.
>
>
>> mdadm: Failed to restore critical section for reshape, sorry.
>> Possibly you needed to specify the --backup-file
>>
>>
>> Have no idea where the server187 stuff has come from.
>
> That is in the 'Name' field in the metadata, which must have been put
> there when the array was created
>> Name : server187.internallan.com:1
>> Creation Time : Sun May 10 14:47:51 2015
>
> It is possible to change it after-the-fact, but unlikely unless someone
> explicitly tried.
> I doesn't really matter how it got there as all the devices are the
> same.
> When "mdadm -I /dev/sdb1" etc is run by udev, mdadm needs to deduce a
> name for the array. It looks in the Name filed and creates
>
> /dev/md/server187.internallan.com:1
>
>>
>> stop the array.
>>
>>>sudo mdadm --stop /dev/md127
>>
>> try to re-assemble
>>
>>>sudo mdadm --assemble /dev/md127 /dev/sdb1 /dev/sdc1 /dev/sdd1 /dev/sde1 /dev/sdf1 /dev/sdg1 /dev/sdh1
>>
>> mdadm: Failed to restore critical section for reshape, sorry.
>> Possibly you needed to specify the --backup-file
>>
>>
>> try to re-assemble using the backup file
>>
>>>sudo mdadm --assemble /dev/md127 /dev/sdb1 /dev/sdc1 /dev/sdd1 /dev/sde1 /dev/sdf1 /dev/sdg1 /dev/sdh1 --backup-file=mdadm_backupfile
>>
>> mdadm: Failed to restore critical section for reshape, sorry.
>
> As you have noted else where, the backup file contains nothing useful.
> That is causing the problem.
>
> When an in-place reshape like yours (not changing the size of the array,
> just changing the configuration) starts the sequence is something like:
>
> - make sure reshape doesn't progress at all (set md/sync_max to zero)
> - tell the kernel about the new shape of the array
> - start the reshape (this won't make any progress, but will update the
> metadata)
> Start:
> - suspend user-space writes to the next few stripes
> - read the next few stripes and write to the backup file
> - tell the kernel that it is allowed to progress to the end of those
> 'few stripes'
> - wait for the kernel to do that
> - invalidate the backup
> - resume user-space writes to those next few stripes
> - goto Start
>
> (the process is actually 'double-buffered' so it is more complex, but
> this gives the idea close enough)
>
> If the system crashes or is shut down, on restart the kernel cannot know
> if the "next few stripes" started reshaping or not, so it depends on
> mdadm to load the backup file, check if there is valid data, and write
> it out.
>
> I suspect that part of the problem is that mdadm --grow doesn't initialize the
> backup file in quite the right way, so when mdadm --assemble looks at it
> it doesn't see "Nothing has been written yet" but instead sees
> "confusion" and gives up.
>
> If you --stop and then run the same --assemble command, including the
> --backup, but this time add --invalid-backup (a bit like Wol
> suggested) it should assemble and restart the reshape. --invalid-backup
> tells mdadm "I know the backup file is invalid, I know that means there
> could be inconsistent data which won't be restored, but I know what is
> going on and I'm willing to take that risk. Just don't restore anything,
> it'll be find. Really".
>
> I don't actually recommend doing that though.
>
> It would be better to revert the current reshape and start again with no
> --backup file. This will use the new mechanism of changing the "Data
> Offset" which is easier to work with and should be faster.
>
> If you have the very latest mdadm (3.4) you can add
> --update=revert-reshape together with --invalid-backup and in your case
> this will cancel the reshape and let you start again.
>
> You can test this out fairly safely if you want to.
>
> mkdir /tmp/foo
> mdadm --dump /tmp/foo /dev/.... list of all devices in the array
>
> This will create sparse files in /tmp/foo containing just the md
> metadata from those devices. Use "losetup /dev/loop0 /tmp/foo/sdb1" etc
> to create loop-back device for all those files (there are multiple hard
> links to each file - just choose 1 each).
> Then you can experiment with mdadm on those /dev/loopXX files to see
> what happens.
>
> Once you have the array reverted, you can start a new --grow, but don't
> specify a --backup file. That should DoTheRightThing.
>
> This still leaves the question of why it didn't start a reshape in the
> first place. If someone would like to experiment (probably with
> loop-back files) and produce a test case that reliably (or even just
> occasionally) hangs, then I'm happy to have a look at it.
>
> It also doesn't answer the question of why mdadm doesn't create the
> backup file in a format that it knows is safe to ignore. Maybe someone
> could look into that.
>
>
> Good luck :-)
>
> NeilBrown
^ permalink raw reply
* Re: multipath: I/O hanging forever
From: Andrea Righi @ 2016-03-13 15:29 UTC (permalink / raw)
To: Ming Lei; +Cc: Kent Overstreet, Shaohua Li, linux-raid, linux-kernel
In-Reply-To: <20160312094723.7c6a4ff4@tom-T450>
On Sat, Mar 12, 2016 at 09:47:23AM +0800, Ming Lei wrote:
> On Fri, 11 Mar 2016 15:24:33 -0700
> Andrea Righi <righi.andrea@gmail.com> wrote:
>
> > On Sat, Mar 05, 2016 at 08:31:03PM -0900, Kent Overstreet wrote:
> > > On Fri, Mar 04, 2016 at 10:30:44AM -0700, Andrea Righi wrote:
> > > > On Sun, Feb 28, 2016 at 08:46:16PM -0700, Andrea Righi wrote:
> > > > > On Sun, Feb 28, 2016 at 06:53:33PM -0700, Andrea Righi wrote:
> > > > > ...
> > > > > > I'm using 4.5.0-rc5+, from Linus' git. I'll try to do a git bisect
> > > > > > later, I'm pretty sure this problem has been introduced recently (i.e.,
> > > > > > I've never seen this issue with 4.1.x).
> > > > >
> > > > > I confirm, just tested kernel 4.1 and this problem doesn't happen.
> > > >
> > > > Alright, I had some spare time to bisect this problem and I found that
> > > > the commit that introduced this issue is c66a14d.
> > > >
> > > > So, I tried to revert the commit (with some changes to fix conflicts and
> > > > ABI changes) and now multipath seems to work fine for me (no hung task).
> > >
> > > Is it hanging on first IO, first large IO, or just randomly?
> >
> > It's always the very first O_DIRECT I/O, in general the task gets stuck
> > in do_blockdev_direct_IO().
>
> I can reproduce the issue too, and looks it is a MD issue instead of block.
> Andrea, could you try the following patch to see if it can fix your issue?
It works perfectly to me.
Thanks!
-Andrea
Tested-by: Andrea Righi <righi.andrea@gmail.com>
>
> ---
> From 43fc9c221e53c64f2df7c100c77cc25c4a98c607 Mon Sep 17 00:00:00 2001
> From: Ming Lei <ming.lei@canonical.com>
> Date: Sat, 12 Mar 2016 09:29:40 +0800
> Subject: [PATCH] md: multipath: don't hardcopy bio in .make_request path
>
> Inside multipath_make_request(), multipath maps the incoming
> bio into low level device's bio, but it is totally wrong to
> copy the bio into mapped bio via '*mapped_bio = *bio'. For
> example, .__bi_remaining is kept in the copy, especially if
> the incoming bio is chained to via bio splitting, so .bi_end_io
> can't be called for the mapped bio at all in the completing path
> in this kind of situation.
>
> This patch fixes the issue by using clone style.
>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> drivers/md/multipath.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
> index 0a72ab6..dd483bb 100644
> --- a/drivers/md/multipath.c
> +++ b/drivers/md/multipath.c
> @@ -129,7 +129,9 @@ static void multipath_make_request(struct mddev *mddev, struct bio * bio)
> }
> multipath = conf->multipaths + mp_bh->path;
>
> - mp_bh->bio = *bio;
> + bio_init(&mp_bh->bio);
> + __bio_clone_fast(&mp_bh->bio, bio);
> +
> mp_bh->bio.bi_iter.bi_sector += multipath->rdev->data_offset;
> mp_bh->bio.bi_bdev = multipath->rdev->bdev;
> mp_bh->bio.bi_rw |= REQ_FAILFAST_TRANSPORT;
> --
> 1.9.1
>
>
> Thanks,
^ permalink raw reply
* Re: RAID6 Array crash during reshape.....now will not re-assemble.
From: NeilBrown @ 2016-03-14 1:08 UTC (permalink / raw)
To: Another Sillyname; +Cc: Linux-RAID
In-Reply-To: <CAOS+5GEu0xku0bjf=SAbx8=bkjn33wLg-1c1YK4DK5fOs_QhTQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]
On Sat, Mar 12 2016, Another Sillyname wrote:
> Neil
>
> Thanks for the insight, much appreciated.
>
> I've tried what you suggested and still get stuck.
>
>>:losetup /dev/loop0 /tmp/foo/sdb1
>>:losetup /dev/loop1 /tmp/foo/sdc1
>>:losetup /dev/loop2 /tmp/foo/sdd1
>>:losetup /dev/loop3 /tmp/foo/sde1
>>:losetup /dev/loop4 /tmp/foo/sdf1
>>:losetup /dev/loop5 /tmp/foo/sdg1
>>:losetup /dev/loop6 /tmp/foo/sdh1
>
>
>>:mdadm --assemble --force --update=revert-reshape --invalid-backup /dev/md127 /dev/loop0 /dev/loop1 /dev/loop2 /dev/loop3 /dev/loop4 /dev/loop5 /dev/loop6
> mdadm: /dev/md127: Need a backup file to complete reshape of this array.
> mdadm: Please provided one with "--backup-file=..."
> mdadm: (Don't specify --update=revert-reshape again, that part succeeded.)
>
> As you can see it 'seems' to have accepted the revert command, but
> even though I've told it the backup is invalid it's still insisting on
> the backup being made available.
>
> Any further thoughts or insights would be gratefully received.
Try giving it a backup file too.
It doesn't matter what the contents of the file are because you have
told it the file is invalid. But I guess it thinks it might still need
a file to write new backups to temporarily.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: multipath: I/O hanging forever
From: Christoph Hellwig @ 2016-03-14 7:41 UTC (permalink / raw)
To: Andrea Righi; +Cc: Shaohua Li, Kent Overstreet, linux-raid, linux-kernel
In-Reply-To: <20160304173044.GA2636@Dell>
Just curious: why are you using the old MD multipath driver that's
been depcrecated in favor of DM multipath a long time ago?
^ permalink raw reply
* [PATCH 0/3] md: trivial changes
From: Guoqing Jiang @ 2016-03-14 9:01 UTC (permalink / raw)
To: shli; +Cc: linux-raid, Guoqing Jiang
Hi,
I found some trivial issues when look into md code, and
all the changes are based on for-next branch of new md
tree.
Thanks,
Guoqing
Guoqing Jiang (3):
md/raid1: remove unnecessary BUG_ON
md/bitmap: remove redundant return in bitmap_checkpage
md: fix typos for stipe
drivers/md/bitmap.c | 1 -
drivers/md/bitmap.h | 4 ++--
drivers/md/raid1.c | 1 -
3 files changed, 2 insertions(+), 4 deletions(-)
--
2.6.2
^ permalink raw reply
* [PATCH 1/3] md/raid1: remove unnecessary BUG_ON
From: Guoqing Jiang @ 2016-03-14 9:01 UTC (permalink / raw)
To: shli; +Cc: linux-raid, Guoqing Jiang
In-Reply-To: <1457946099-1878-1-git-send-email-gqjiang@suse.com>
Since bitmap_start_sync will not return until
sync_blocks is not less than PAGE_SIZE>>9, so
the BUG_ON is not needed anymore.
Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
---
drivers/md/raid1.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 4e3843f..d1d5363 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2695,7 +2695,6 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
!conf->fullsync &&
!test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery))
break;
- BUG_ON(sync_blocks < (PAGE_SIZE>>9));
if ((len >> 9) > sync_blocks)
len = sync_blocks<<9;
}
--
2.6.2
^ permalink raw reply related
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