* Re: Optimal chunk size for RAID5?
From: NeilBrown @ 2015-02-22 21:53 UTC (permalink / raw)
To: Alireza Haghdoost; +Cc: Roman Mamedov, Christer Solskogen, Linux RAID
In-Reply-To: <CAB-428=7y-KSbCeXo6y6o5Jfzgf-5h6pfYxY+4Z74bqYp=oMUg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]
On Sun, 22 Feb 2015 08:33:02 -0600 Alireza Haghdoost <alireza@cs.umn.edu>
wrote:
> On Sun, Feb 22, 2015 at 6:30 AM, Roman Mamedov <rm@romanrm.net> wrote:
> > On Sun, 22 Feb 2015 12:31:23 +0100
> > Christer Solskogen <christer.solskogen@gmail.com> wrote:
> >
> >> There are so many different views on the internet
> >
> > ...and yet you're asking for some more? :)
> >
> >> Is there even such a thing as optimal chunk size?
> >
> > 64K should be fine:
> > http://louwrentius.com/linux-raid-level-and-chunk-size-the-benchmarks.html
> >
>
> I have seen that people report 64K chunk size results better
> performance. However, I was not able to find why mdadm maintainers
> decided to switch into 512K default chunk size a few years ago ? Was
> that decision related to the write-intent bitmap overhead ?
No, write-intent-bitmap sizing is completely independent from chunk sizes.
I don't remember the detail for the change, but some measurement must have
gone faster with larger chunk size.
single threaded loads tend to prefer large chunk sizes.
multi-threaded small-request random IO tends to prefer smaller chunk sizes.
There is no "Optimal" without reference to a particular work load. Or
particular hardware.
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: Metadata > 0.90 and auto-assemble
From: Joshua Kinard @ 2015-02-22 21:54 UTC (permalink / raw)
To: Chris Murphy; +Cc: linux-raid
In-Reply-To: <CAJCQCtSPPm3ipb49SCeNFvgQ21y4HseqovBPg28LKXzvP5PY8Q@mail.gmail.com>
On 02/22/2015 16:29, Chris Murphy wrote:
> On Sun, Feb 22, 2015 at 11:54 AM, Joshua Kinard <kumba@gentoo.org> wrote:
>> I'd like to
>> avoid this if possible, as I haven't had to use an initramfs for normal booting
>> in the past, as long as I stay on metadata 0.90. So I thought I'd ask what the
>> official stance is on this.
>
> https://raid.wiki.kernel.org/index.php/Autodetect
>
> Official stance is that it's deprecated, but people still use it.
Yeah, but it's a pretty useful feature. I can't see why autodetect for simple
setups (several disks or partitions and building a basic array out of them) is
maintained, while userspace autodetect is required for the more complex setups.
But I suppose this has been discussed before in detail, though I cannot find
said discussion. The RAID Boot page has this one example only:
"This approach can cause problems in several situations (imagine moving part of
an old array onto another machine before wiping and repurposing it: reboot and
watch in horror as the piece of dead array gets assembled as part of the
running RAID array, ruining it); kernel autodetect is correspondingly deprecated."
Which I find to be rather unconvincing. The cited example is a fault of the
user not torching the superblock before moving the disks or trying to use
them...and I've done this to myself on several occasions. mdadm --misc
--zero-superblock and 'dd' saved the day in less than ~30s.
Are there any other discussions that might be more convincing, or offer up
other points of view? Perhaps there's a point I've yet to consider that might
be enlightening.
Thanks!,
--
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
4096R/D25D95E3 2011-03-28
"The past tempts us, the present confuses us, the future frightens us. And our
lives slip away, moment by moment, lost in that vast, terrible in-between."
--Emperor Turhan, Centauri Republic
^ permalink raw reply
* Re: Metadata > 0.90 and auto-assemble
From: Mark Knecht @ 2015-02-22 22:19 UTC (permalink / raw)
To: Joshua Kinard; +Cc: Linux-RAID
In-Reply-To: <54EA25D3.8020300@gentoo.org>
On Sun, Feb 22, 2015 at 10:54 AM, Joshua Kinard <kumba@gentoo.org> wrote:
> Hi,
>
> I tried a while back to use the newer metadata formats on my mdadm RAID5 on a
> few machines, and discovered that the kernel auto-assembly will only work with
> v0.90 metadata, not 1.0 or greater. Is there a solid reason for this? Based
> one what I can find regarding the differences in the metadata formats, and
> looking at the existing md code, it seems this is largely just because no one
> has had the time or motivation to change the code to support auto-assembly on
> the newer metdata formats.
>
> I am told that the "correct" solution is to embed a small initramfs to bring
> the RAID arrays online instead, before the real rootfs is loaded. I'd like to
> avoid this if possible, as I haven't had to use an initramfs for normal booting
> in the past, as long as I stay on metadata 0.90. So I thought I'd ask what the
> official stance is on this.
>
> Thanks!,
>
> --
> Joshua Kinard
> Gentoo/MIPS
> kumba@gentoo.org
> 4096R/D25D95E3 2011-03-28
>
I cannot speak to any of the reasons to support it or not but I'm a Gentoo
guy since late 2002 who avoided the initramfs for the longest time. I
finally bit the bullet and learned how to build it into my kernels so there are
no extra files and except for a recent problem with Gentoo devs making
changes to busybox defaults it's worked very well. The nice thing about
building it into the kernel is that old kernels continue to work perfectly as
best I can tell.
Anyway, from my perspective it was worth learning. I'm just a user type,
not a dev of any type.
Cheers,
Mark
^ permalink raw reply
* Re: An old "write-mostly" read balance issue
From: NeilBrown @ 2015-02-23 0:03 UTC (permalink / raw)
To: Dark Penguin; +Cc: linux-raid, tomas.hodek
In-Reply-To: <54D78357.7020808@yandex.ru>
[-- Attachment #1: Type: text/plain, Size: 5403 bytes --]
On Sun, 08 Feb 2015 18:40:07 +0300 Dark Penguin <darkpenguin@yandex.ru> wrote:
> There is an old issue about RAID1 read-balancing when "write-mostly"
> disks are present.
>
> The problem is, according to the manual, "md driver will avoid reading
> from these devices if at all possible".
>
> One way to understand this statement is that these drives will never be
> read from, except when the main drive can not be read from. There are A
> LOT of situations when this is the expected and desired behaviour:
> - People mirroring an SSD with an HDD and suffering a performance loss;
> - People mirroring a fast HDD with a slow HDD for reliability, for
> example, mirroring a 300Gb WD Raptor to a 300Gb partition on a 3Tb 5900
> "green" drive for backup; since the larger drive may be used for
> something other than this RAID, many would prefer it to be spared the
> workload.
> - In my case, I have a home RAID1 storage, which is idle 95% of the
> time, and 95% of the remaining 5% I only read from it. So I want one of
> the drives to spin down and never turn on, in order to avoid wearing
> down the mechanics. They say, "The best way to keep a device from
> breaking is to turn it off and not use it". :) But even if I simply
> retrieve the contents of my volume, that request is apparently enough to
> load the first drive to 100% for a split second, which causes the second
> drive to spin up, which is extremely undesirable.
>
> I've spent a lot of time looking for the answer "why does it spin up",
> and "normal forum users" couldn not even help me, but then I found out
> that there is another way to read that statement: apparently, there are
> other people who would like to see whatever little benefit reading from
> the second drive could give them. I can not say which side is a
> majority, but I respect their wishes as well, and personally I'm fine
> with any default behaviour as long as I have what I need.
>
> I've found a patch for that:
> http://marc.info/?l=linux-raid&m=135982797322422
> Apparently, it can be used with any kernel, but I'm not good enough to
> make sure nothing's broken everytime I upgrade the kernel, and frankly,
> I think there are A LOT of people who wish to see the behaviour I would
> expect. So my plea is for the developers to accept this patch and make
> this behaviour optional, if not default. At least give us a compile
> option to build the kernel this way! There are people out there who use
> RAID1 at home and not in production, and therefore care less about
> performance than home storage idling, and who understand the words "if
> at all possible" in the more obvious way! I think that's the whole
> reason why the "write-mostly" option is there in the first place, but if
> there are people who don't agree with me - I'm not going to argue, they
> can have it their way, just give us the option to do what we want, too!
>
>
Hi,
thanks for reporting this. It is definitely a bug. It was introduced by
commit 9dedf60313fa4dddfd5b9b226a0ef12a512bf9dc
md/raid1: read balance chooses idlest disk for SSD
I don't recall seeing the patch from Tomas Hodek which you provided a link
for - sorry Tomas.
I prefer the second of the two patches. I will submit the following to Linus
some time this week.
Thanks for pursuing this Dark Penguin.
NeilBrown
From: Tomas Hodek <tomas.hodek@volny.cz>
Date: Mon, 23 Feb 2015 11:00:38 +1100
Subject: [PATCH] Subject: md/raid1: fix read balance when a drive is
write-mostly.
When a drive is marked write-mostly it should only be the
target of reads if there is no other option.
This behaviour was broken by
commit 9dedf60313fa4dddfd5b9b226a0ef12a512bf9dc
md/raid1: read balance chooses idlest disk for SSD
which causes a write-mostly device to be *preferred* is some cases.
Restore correct behaviour by checking and setting
best_dist_disk and best_pending_disk rather than best_disk.
We only need to test one of these as they are both changed
from -1 or >=0 at the same time.
As we leave min_pending and best_dist unchanged, any non-write-mostly
device will appear better than the write-mostly device.
Reported-by: tomas.hodek@volny.cz
Reported-by: Dark Penguin <darkpenguin@yandex.ru>
Signed-off-by: NeilBrown <neilb@suse.de>
Link: http://marc.info/?l=linux-raid&m=135982797322422
Fixes: 9dedf60313fa4dddfd5b9b226a0ef12a512bf9dc
Cc: stable@vger.kernel.org (3.6+)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 0b6349f9c5c5..7742e0999bf2 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -560,7 +560,7 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
if (test_bit(WriteMostly, &rdev->flags)) {
/* Don't balance among write-mostly, just
* use the first as a last resort */
- if (best_disk < 0) {
+ if (best_dist_disk < 0) {
if (is_badblock(rdev, this_sector, sectors,
&first_bad, &bad_sectors)) {
if (first_bad < this_sector)
@@ -569,7 +569,8 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
best_good_sectors = first_bad - this_sector;
} else
best_good_sectors = sectors;
- best_disk = disk;
+ best_dist_disk = disk;
+ best_pending_disk = disk;
}
continue;
}
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply related
* Re: Metadata > 0.90 and auto-assemble
From: NeilBrown @ 2015-02-23 0:17 UTC (permalink / raw)
To: Joshua Kinard; +Cc: Chris Murphy, linux-raid
In-Reply-To: <54EA5004.3090104@gentoo.org>
[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]
On Sun, 22 Feb 2015 16:54:12 -0500 Joshua Kinard <kumba@gentoo.org> wrote:
> On 02/22/2015 16:29, Chris Murphy wrote:
> > On Sun, Feb 22, 2015 at 11:54 AM, Joshua Kinard <kumba@gentoo.org> wrote:
> >> I'd like to
> >> avoid this if possible, as I haven't had to use an initramfs for normal booting
> >> in the past, as long as I stay on metadata 0.90. So I thought I'd ask what the
> >> official stance is on this.
> >
> > https://raid.wiki.kernel.org/index.php/Autodetect
> >
> > Official stance is that it's deprecated, but people still use it.
>
> Yeah, but it's a pretty useful feature. I can't see why autodetect for simple
> setups (several disks or partitions and building a basic array out of them) is
> maintained, while userspace autodetect is required for the more complex setups.
The in-kernel autodetect is only maintained because tearing it out and
throwing it away (my preferred option) would be a user-visible regression,
and those are not permitted.
The user-space version is more general and more flexible. If the
kernel-space version works for you, you can keep using it. But if you want
features added to it, you are out of luck.
As others have said, creating a simple initrd is really not that hard. Once
you spend the time to make it work, you will find that it "just works" and
wonder why you ever cared before.
There is even a README.initramfs in the mdadm source. It was written 10
years ago so I cannot promise it is 100% correct, but it is a reasonably good
and very simple starting point.
NeilBrown
>
> But I suppose this has been discussed before in detail, though I cannot find
> said discussion. The RAID Boot page has this one example only:
>
> "This approach can cause problems in several situations (imagine moving part of
> an old array onto another machine before wiping and repurposing it: reboot and
> watch in horror as the piece of dead array gets assembled as part of the
> running RAID array, ruining it); kernel autodetect is correspondingly deprecated."
>
> Which I find to be rather unconvincing. The cited example is a fault of the
> user not torching the superblock before moving the disks or trying to use
> them...and I've done this to myself on several occasions. mdadm --misc
> --zero-superblock and 'dd' saved the day in less than ~30s.
>
> Are there any other discussions that might be more convincing, or offer up
> other points of view? Perhaps there's a point I've yet to consider that might
> be enlightening.
>
> Thanks!,
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [dm-devel] [PATCH 0/3] md raid: enhancements to support the device mapper dm-raid target
From: NeilBrown @ 2015-02-23 1:07 UTC (permalink / raw)
To: Heinz Mauelshagen
Cc: device-mapper development, jbras >> Brassow Jonathan,
linux RAID
In-Reply-To: <54E47C88.1080203@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 6379 bytes --]
On Wed, 18 Feb 2015 12:50:32 +0100 Heinz Mauelshagen <heinzm@redhat.com>
wrote:
> On 02/18/2015 03:03 AM, NeilBrown wrote:
> > On Fri, 13 Feb 2015 19:47:59 +0100 heinzm@redhat.com wrote:
> >
> >> From: Heinz Mauelshagen <heinzm@redhat.com>
> >>
> >> I'm enhancing the device mapper raid target (dm-raid) to take
> >> advantage of so far unused md raid kernel funtionality:
> >> takeover, reshape, resize, addition and removal of devices to/from raid sets.
> >>
> >> This series of patches remove constraints doing so.
> >>
> >>
> >> Patch #1:
> >> add 2 API functions to allow dm-raid to access the raid takeover
> >> and resize functionality (namely md_takeover() and md_resize());
> >> reshape APIs are not needed in lieu of the existing personalilty ones
> >>
> >> Patch #2:
> >> because device mapper core manages a request queue per mapped device
> >> utilizing the md make_request API to pass on bios via the dm-raid target,
> >> no md instance underneath it needs to manage a request queue of its own.
> >> Thus dm-raid can't use the md raid0 personality as is, because the latter
> >> accesses the request queue unconditionally in 3 places via mddev->queue
> >> which this patch addresses.
> >>
> >> Patch #3:
> >> when dm-raid processes a down takeover to raid0, it needs to destroy
> >> any existing bitmap, because raid0 does not require one. The patch
> >> exports the bitmap_destroy() API to allow dm-raid to remove bitmaps.
> >>
> >>
> >> Heinz Mauelshagen (3):
> >> md core: add 2 API functions for takeover and resize to support dm-raid
> >> md raid0: access mddev->queue (request queue member) conditionally
> >> because it is not set when accessed from dm-raid
> >> md bitmap: export bitmap_destroy() to support dm-raid down takover to raid0
> >>
> >> drivers/md/bitmap.c | 1 +
> >> drivers/md/md.c | 39 ++++++++++++++++++++++++++++++---------
> >> drivers/md/md.h | 3 +++
> >> drivers/md/raid0.c | 48 +++++++++++++++++++++++++++---------------------
> >> 4 files changed, 61 insertions(+), 30 deletions(-)
> >>
> > Hi Heinz,
> > I don't object to these patches if you will find the exported functionality
> > useful, but I am a little surprised by them.
>
> Hi Neil,
>
> I find them useful to allow for atomic takeover using the already given
> md raid
> code rather than duplicating ACID takeover in dm-raid/lvm. If I'd not
> use md for this,
> I'd have to keep copies of the given md superblocks and restore them in case
> the assembly of the array failed and superblocks have been updated.
This argument doesn't make much sense to me.
There is no reason that the assembling the array in a new configuration would
fail, except possible malloc error or similar which would make putting it
back into the original configuration fail as well.
There is no need to synchronise updating the metadata with a take-over.
In every case, the "Before" and "After" configurations are functionally
identical.
A 2-drive RAID1 behaves identically to a 2-drive RAID5, for example.
So it doesn't really matter whether or not the metadata match how the kernel
is configured. Once you start a reshape (e.g. 2-drive RAID5 to 3-drive
RAID5) or add a spare, then you need the metadata to be correct, but that is
just a sequencing issue:
- start: metadata says "raid1".
- suspend array, reconfigure as RAID5 with 2 drives, resume.
- if everything went well, update metadata to "raid5".
- now update metadata to "0 block of progress into reshape from 2-drives to
3-drives".
- now start the reshape, which will further update the metadata as it
proceeds.
There really are no atomicity requirements, only sequencing.
>
> >
> > I would expect that dm-raid wouldn't ask md to 'takeover' from one level to
> > another, but instead would
> > - suspend the dm device
> > - dismantle the array using the old level
> > - assemble the array using the new level
> > - resume the dm device
>
> That scenario is on my TODO, because it is for instance paritcularly
> useful to
> convert a "striped" array (or a "raid0" array without metadata for that
> purpose)
> directly into a raid6_n_6 one (i.e. dedicated xor and syndrome devices)
> thus avoding any interim levels.
> In these cases, I'd only need to drop the metadata devs allocations if
> the array does not start up properly and restart the previous mapping.
>
Given that you plan to do this, I really think the dm and LVM code would be
simpler if all reconfigurations use this same approach.
>
> >
> > The reason md needs 'takeover' is because it doesn't have the same
> > device/target separation that dm does.
>
> Correct.
> Nonetheless, I found accessing md's takeover functionality still useful
> for the atomic updates to be simpler in dm/lvm.
>
> >
> > I was particularly surprised that you wanted to use md/raid0.c It is no
> > better than dm/dm-stripe.c and managing two different stripe engines under
> > LVM doesn't see like a good idea.
>
> I actually see differences in performance which I have not explained yet.
>
> In some cases, dm-stripe performs better, in others md raid0 does for
> the same mappings
> and load; exact same mappings are possible, because I've got patches to
> lvconvert back
> and forth between "striped" and "raid0", hence accesing exactly the same
> physical extents.
That is surprising. I would be great if we could characterise what sort of
workloads work better with one or the other...
>
> So supporting "raid0" in dm-raid is senseful for 3 reasons:
> - replace dm-stripe with md raid0
> - atomic md takeover from "raid0" -> "raid5"
> - potential performance implications
>
> >
> > Is there some reason that I have missed which makes it easier to use
> > 'takeover' rather than suspend/resume?
>
> Use md takover for atomic updates as mentioned above.
>
> You don't have issues with md_resize() which I use to shrink existing
> arrays?
>
I have exactly the same issue with md_resize() as with md_takeover(), and for
the same reasons.
How about we wait until you do implement the
suspend/dismantle/reassemble/resume
approach, and see if you still want md_resize/md_takeover after that?
Thanks,
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: Optimal chunk size for RAID5?
From: Christer Solskogen @ 2015-02-23 1:36 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <20150223085358.302830d1@notabene.brown>
On 22.02.2015 22:53, NeilBrown wrote:
> There is no "Optimal" without reference to a particular work load. Or
> particular hardware.
>
Do you know of such a reference? I mean, some stats that show type of
workload / chunk size. The only one I've found is the 5 year old
benchmark that was done (
http://louwrentius.com/linux-raid-level-and-chunk-size-the-benchmarks.html)
- which shows that under benchmarking with dd that 64 is preferred.
--
chs
^ permalink raw reply
* Re: [PATCH 02/24] Add number of nodes to bitmap structure for clustering
From: NeilBrown @ 2015-02-23 1:38 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: lzhong, linux-raid
In-Reply-To: <20141218161523.GA29576@shrek.lan>
[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]
On Thu, 18 Dec 2014 10:15:23 -0600 Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/bitmap.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/bitmap.h b/drivers/md/bitmap.h
> index 30210b9..6872945 100644
> --- a/drivers/md/bitmap.h
> +++ b/drivers/md/bitmap.h
> @@ -128,10 +128,11 @@ typedef struct bitmap_super_s {
> __le32 chunksize; /* 52 the bitmap chunk size in bytes */
> __le32 daemon_sleep; /* 56 seconds between disk flushes */
> __le32 write_behind; /* 60 number of outstanding write-behind writes */
> - __le32 sectors_reserved; /* 64 number of 512-byte sectors that are
> + __le32 nodes; /* 64 the maximum number of nodes in cluster. */
> + __le32 sectors_reserved; /* 68 number of 512-byte sectors that are
> * reserved for the bitmap. */
>
> - __u8 pad[256 - 68]; /* set to zero */
> + __u8 pad[256 - 72]; /* set to zero */
> } bitmap_super_t;
>
> /* notes:
Hi Goldwyn,
I was reviewing you latest series to make sure it wouldn't affect the
non-clustered use case at all, and I found this. I really should have
noticed it earlier....
You are changing the location of 'sectors_reserved' in the bitmap superblock.
That obviously cannot be allowed - new fields must always be added to the
end.
Can you update the series in git to fix that please? Then I will pull it in
for -next.
Thanks,
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: Optimal chunk size for RAID5?
From: NeilBrown @ 2015-02-23 3:28 UTC (permalink / raw)
To: Christer Solskogen; +Cc: linux-raid
In-Reply-To: <mce06j$i2h$1@ger.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]
On Mon, 23 Feb 2015 02:36:18 +0100 Christer Solskogen
<christer.solskogen@gmail.com> wrote:
> On 22.02.2015 22:53, NeilBrown wrote:
>
> > There is no "Optimal" without reference to a particular work load. Or
> > particular hardware.
> >
>
> Do you know of such a reference? I mean, some stats that show type of
> workload / chunk size. The only one I've found is the 5 year old
> benchmark that was done (
> http://louwrentius.com/linux-raid-level-and-chunk-size-the-benchmarks.html)
> - which shows that under benchmarking with dd that 64 is preferred.
>
Interesting graphs ... but when you see a big jump like they show between 64
and 128K chunk sizes for RAID5/6, that doesn't mean "64K is better" but
"something strange is happening here". My guess is that read-ahead is
working very well for some reason.
If your actually workload is writing 10GB files with 'dd', then the graphs
might be useful. For other workloads ... it's hard to tell.
Nothing beats performing your own tests on your own hardware with your own
choice of filesystem and getting your own results.
I did some tests myself recently (which I really want to automate and turn
into web pages etc ... one day).
For RAID5 on 4 drives I used chunk sizes of 4, 16, 64, 256, 1024 and applied
a variety of fio loads use XFS.
The only load that showed significant variation of chunk sizes was sequential
read which gets generally faster with larger chunk sizes, though for some
layouts (I tried la, ls, ra, rs) 1024k chunks were worse than 256k.
So any reference you find will probably lead you astray.
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [dm-devel] [PATCH 0/3] md raid: enhancements to support the device mapper dm-raid target
From: Heinz Mauelshagen @ 2015-02-23 11:49 UTC (permalink / raw)
To: NeilBrown
Cc: device-mapper development, jbras >> Brassow Jonathan,
linux RAID
In-Reply-To: <20150223120718.03806a87@notabene.brown>
On 02/23/2015 02:07 AM, NeilBrown wrote:
> On Wed, 18 Feb 2015 12:50:32 +0100 Heinz Mauelshagen <heinzm@redhat.com>
> wrote:
>
>> On 02/18/2015 03:03 AM, NeilBrown wrote:
>>> On Fri, 13 Feb 2015 19:47:59 +0100 heinzm@redhat.com wrote:
>>>
>>>> From: Heinz Mauelshagen <heinzm@redhat.com>
>>>>
>>>> I'm enhancing the device mapper raid target (dm-raid) to take
>>>> advantage of so far unused md raid kernel funtionality:
>>>> takeover, reshape, resize, addition and removal of devices to/from raid sets.
>>>>
>>>> This series of patches remove constraints doing so.
>>>>
>>>>
>>>> Patch #1:
>>>> add 2 API functions to allow dm-raid to access the raid takeover
>>>> and resize functionality (namely md_takeover() and md_resize());
>>>> reshape APIs are not needed in lieu of the existing personalilty ones
>>>>
>>>> Patch #2:
>>>> because device mapper core manages a request queue per mapped device
>>>> utilizing the md make_request API to pass on bios via the dm-raid target,
>>>> no md instance underneath it needs to manage a request queue of its own.
>>>> Thus dm-raid can't use the md raid0 personality as is, because the latter
>>>> accesses the request queue unconditionally in 3 places via mddev->queue
>>>> which this patch addresses.
>>>>
>>>> Patch #3:
>>>> when dm-raid processes a down takeover to raid0, it needs to destroy
>>>> any existing bitmap, because raid0 does not require one. The patch
>>>> exports the bitmap_destroy() API to allow dm-raid to remove bitmaps.
>>>>
>>>>
>>>> Heinz Mauelshagen (3):
>>>> md core: add 2 API functions for takeover and resize to support dm-raid
>>>> md raid0: access mddev->queue (request queue member) conditionally
>>>> because it is not set when accessed from dm-raid
>>>> md bitmap: export bitmap_destroy() to support dm-raid down takover to raid0
>>>>
>>>> drivers/md/bitmap.c | 1 +
>>>> drivers/md/md.c | 39 ++++++++++++++++++++++++++++++---------
>>>> drivers/md/md.h | 3 +++
>>>> drivers/md/raid0.c | 48 +++++++++++++++++++++++++++---------------------
>>>> 4 files changed, 61 insertions(+), 30 deletions(-)
>>>>
>>> Hi Heinz,
>>> I don't object to these patches if you will find the exported functionality
>>> useful, but I am a little surprised by them.
>> Hi Neil,
>>
>> I find them useful to allow for atomic takeover using the already given
>> md raid
>> code rather than duplicating ACID takeover in dm-raid/lvm. If I'd not
>> use md for this,
>> I'd have to keep copies of the given md superblocks and restore them in case
>> the assembly of the array failed and superblocks have been updated.
> This argument doesn't make much sense to me.
>
> There is no reason that the assembling the array in a new configuration would
> fail, except possible malloc error or similar which would make putting it
> back into the original configuration fail as well.
>
> There is no need to synchronise updating the metadata with a take-over.
> In every case, the "Before" and "After" configurations are functionally
> identical.
> A 2-drive RAID1 behaves identically to a 2-drive RAID5, for example.
> So it doesn't really matter whether or not the metadata match how the kernel
> is configured. Once you start a reshape (e.g. 2-drive RAID5 to 3-drive
> RAID5) or add a spare, then you need the metadata to be correct, but that is
> just a sequencing issue:
>
> - start: metadata says "raid1".
> - suspend array, reconfigure as RAID5 with 2 drives, resume.
> - if everything went well, update metadata to "raid5".
> - now update metadata to "0 block of progress into reshape from 2-drives to
> 3-drives".
> - now start the reshape, which will further update the metadata as it
> proceeds.
>
> There really are no atomicity requirements, only sequencing.
Thanks for clarifying these conversions, I was presuming there were
atomicity issues in the md kernel code to conform to.
Canges to run those sequences look straightforward in the dm-raid target.
I'll implement them and test.
>
>
>>> I would expect that dm-raid wouldn't ask md to 'takeover' from one level to
>>> another, but instead would
>>> - suspend the dm device
>>> - dismantle the array using the old level
>>> - assemble the array using the new level
>>> - resume the dm device
>> That scenario is on my TODO, because it is for instance paritcularly
>> useful to
>> convert a "striped" array (or a "raid0" array without metadata for that
>> purpose)
>> directly into a raid6_n_6 one (i.e. dedicated xor and syndrome devices)
>> thus avoding any interim levels.
>> In these cases, I'd only need to drop the metadata devs allocations if
>> the array does not start up properly and restart the previous mapping.
>>
> Given that you plan to do this, I really think the dm and LVM code would be
> simpler if all reconfigurations use this same approach.
You got a point with regards to the dm-raid target:
if an MD takeover API is actually superfluous in the end, the target
won't have 2 code paths for
a) going from a non-metadata config to a metadata one (e.g. striped ->
raid5)
and
b) a metadata -> metadata one (e.g. raid6 -> raid5)
In lvm2/dm userspace there will be no difference, because it has to
update the userspace metadata and the kernel metadata comiting it
in the proper sequence and does not call any takeover api in userspace
at all which could be avoided as in the kernel.
>
>>> The reason md needs 'takeover' is because it doesn't have the same
>>> device/target separation that dm does.
>> Correct.
>> Nonetheless, I found accessing md's takeover functionality still useful
>> for the atomic updates to be simpler in dm/lvm.
>>
>>> I was particularly surprised that you wanted to use md/raid0.c It is no
>>> better than dm/dm-stripe.c and managing two different stripe engines under
>>> LVM doesn't see like a good idea.
>> I actually see differences in performance which I have not explained yet.
>>
>> In some cases, dm-stripe performs better, in others md raid0 does for
>> the same mappings
>> and load; exact same mappings are possible, because I've got patches to
>> lvconvert back
>> and forth between "striped" and "raid0", hence accesing exactly the same
>> physical extents.
> That is surprising. I would be great if we could characterise what sort of
> workloads work better with one or the other...
Agreed, we need more facts.
I've seen indications from "dd oflag=direct iflag=fullblock bs=1G
count=1 if=/dev/zero of=$LV
converting back and forth to/from raid0/striped mappings on an otherwise
idle system.
>> So supporting "raid0" in dm-raid is senseful for 3 reasons:
>> - replace dm-stripe with md raid0
>> - atomic md takeover from "raid0" -> "raid5"
>> - potential performance implications
>>
>>> Is there some reason that I have missed which makes it easier to use
>>> 'takeover' rather than suspend/resume?
>> Use md takover for atomic updates as mentioned above.
>>
>> You don't have issues with md_resize() which I use to shrink existing
>> arrays?
>>
> I have exactly the same issue with md_resize() as with md_takeover(), and for
> the same reasons.
Ok, let me do avoiding patches based on your clarifications
which'll take till next week including testing.
> How about we wait until you do implement the
> suspend/dismantle/reassemble/resume
> approach, and see if you still want md_resize/md_takeover after that?
Sure.
I'd like to see the raid0 conditonal request queue patch though.
Thanks,
Heinz
>
> Thanks,
> NeilBrown
>
^ permalink raw reply
* Re: [PATCH 02/24] Add number of nodes to bitmap structure for clustering
From: Goldwyn Rodrigues @ 2015-02-23 18:13 UTC (permalink / raw)
To: NeilBrown; +Cc: lzhong, linux-raid
In-Reply-To: <20150223123848.01416b69@notabene.brown>
Hi Neil,
On 02/22/2015 07:38 PM, NeilBrown wrote:
> On Thu, 18 Dec 2014 10:15:23 -0600 Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>> drivers/md/bitmap.h | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/bitmap.h b/drivers/md/bitmap.h
>> index 30210b9..6872945 100644
>> --- a/drivers/md/bitmap.h
>> +++ b/drivers/md/bitmap.h
>> @@ -128,10 +128,11 @@ typedef struct bitmap_super_s {
>> __le32 chunksize; /* 52 the bitmap chunk size in bytes */
>> __le32 daemon_sleep; /* 56 seconds between disk flushes */
>> __le32 write_behind; /* 60 number of outstanding write-behind writes */
>> - __le32 sectors_reserved; /* 64 number of 512-byte sectors that are
>> + __le32 nodes; /* 64 the maximum number of nodes in cluster. */
>> + __le32 sectors_reserved; /* 68 number of 512-byte sectors that are
>> * reserved for the bitmap. */
>>
>> - __u8 pad[256 - 68]; /* set to zero */
>> + __u8 pad[256 - 72]; /* set to zero */
>> } bitmap_super_t;
>>
>> /* notes:
>
> Hi Goldwyn,
> I was reviewing you latest series to make sure it wouldn't affect the
> non-clustered use case at all, and I found this. I really should have
> noticed it earlier....
>
> You are changing the location of 'sectors_reserved' in the bitmap superblock.
> That obviously cannot be allowed - new fields must always be added to the
> end.
Oops. Sorry.
>
> Can you update the series in git to fix that please? Then I will pull it in
> for -next.
I have done this and re-tested. However, I also rebased against
upstream. There were some conflicts with respect to md_personality. I
have re-done those patches, but it would be worth a second look. The one
which may need special review attention is "bitmap_create returns bitmap
pointer"
Regards,
--
Goldwyn
^ permalink raw reply
* Inject I/O latency for RAID5/6 read and writes
From: Alireza Haghdoost @ 2015-02-23 18:32 UTC (permalink / raw)
To: Linux RAID; +Cc: Neil Brown
I needed to inject I/O completion latency in the RAID5/6 codes for
test purpose. I was wondering where would be the good place in
md/raid5.c code to add delay ?
So far I have tried adding mdelay/udelay in raid5_end_write_request()
and it seems it works to increase I/O completion of writes. However,
adding delay in raid5_end_read_request() does not really change the
read I/O latency. Any idea ?
--Alireza
^ permalink raw reply
* RAID6 write I/O amplification?
From: Roman Mamedov @ 2015-02-23 23:58 UTC (permalink / raw)
To: linux-raid
Hello,
Got a bit of a "how does it actually work" question...
Suppose I have an MD RAID6 of 8 drives, with 64KB chunk size.
I am rewriting a 4KB filesystem sector somewhere on that RAID (not crossing
the stripe boundary).
What's the amount of disk I/O in total this will result in?
I assume the RAID will need to read data from all drives, recompute parity,
then write to the data stripe where the updated piece happened to be, and also
write to two parity stripes.
Is this done at a stripe granularity, so 6x64KB reads, 3x64KB writes?
Or down to individual sectors (pages), i.e. 6x4KB reads, 3x4KB writes?
Or am I describing this algorithm correctly at all?
--
With respect,
Roman
^ permalink raw reply
* AW: RAID6 write I/O amplification?
From: Markus Stockhausen @ 2015-02-24 6:29 UTC (permalink / raw)
To: Roman Mamedov, linux-raid@vger.kernel.org
In-Reply-To: <20150224045835.14e40dcb@natsu>
[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]
> Von: linux-raid-owner@vger.kernel.org [linux-raid-owner@vger.kernel.org]" im Auftrag von "Roman Mamedov [rm@romanrm.net]
> Gesendet: Dienstag, 24. Februar 2015 00:58
> An: linux-raid@vger.kernel.org
> Betreff: RAID6 write I/O amplification?
>
> Hello,
>
> Got a bit of a "how does it actually work" question...
>
> Suppose I have an MD RAID6 of 8 drives, with 64KB chunk size.
>
> I am rewriting a 4KB filesystem sector somewhere on that RAID (not crossing
> the stripe boundary).
>
> What's the amount of disk I/O in total this will result in?
>
> I assume the RAID will need to read data from all drives, recompute parity,
> then write to the data stripe where the updated piece happened to be, and also
> write to two parity stripes.
>
> Is this done at a stripe granularity, so 6x64KB reads, 3x64KB writes?
> Or down to individual sectors (pages), i.e. 6x4KB reads, 3x4KB writes?
> Or am I describing this algorithm correctly at all?
Implementation will work on "internal" stripe granularity and that is 4K
So your case will be 6x4KB read + 3x4KB write. That said, you can only
reduce the I/O overhead by writing data that is larger than your configured
stripe size (e.g. 64K).
Looking at Neils development GIT you will find patches that allow
read-modify-write cycles for RAID6. So we only need the old block, the
old parities, recaluclate them and write the new block and the new parities.
In your case that would reduce the I/Os to 3x4KB read + 3x4KB write.
See http://git.neil.brown.name/?p=md.git;a=shortlog;h=refs/heads/devel
I posted them 6 months ago but they did not made their way into the
stable tree. Additionally it conatins patches to batch adjacent writes
to be processed in less & larger I/Os. Currently Linux Raid will break
each operation into 4K I/Os.
Markus
=
[-- Attachment #2: InterScan_Disclaimer.txt --]
[-- Type: text/plain, Size: 1650 bytes --]
****************************************************************************
Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte
Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail
irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und
vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte
Weitergabe dieser Mail ist nicht gestattet.
Über das Internet versandte E-Mails können unter fremden Namen erstellt oder
manipuliert werden. Deshalb ist diese als E-Mail verschickte Nachricht keine
rechtsverbindliche Willenserklärung.
Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln
Vorstand:
Kadir Akin
Dr. Michael Höhnerbach
Vorsitzender des Aufsichtsrates:
Hans Kristian Langva
Registergericht: Amtsgericht Köln
Registernummer: HRB 52 497
This e-mail may contain confidential and/or privileged information. If you
are not the intended recipient (or have received this e-mail in error)
please notify the sender immediately and destroy this e-mail. Any
unauthorized copying, disclosure or distribution of the material in this
e-mail is strictly forbidden.
e-mails sent over the internet may have been written under a wrong name or
been manipulated. That is why this message sent as an e-mail is not a
legally binding declaration of intention.
Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln
executive board:
Kadir Akin
Dr. Michael Höhnerbach
President of the supervisory board:
Hans Kristian Langva
Registry office: district court Cologne
Register number: HRB 52 497
****************************************************************************
^ permalink raw reply
* Re: An old "write-mostly" read balance issue
From: Tomáš Hodek @ 2015-02-24 8:20 UTC (permalink / raw)
To: NeilBrown, Dark Penguin; +Cc: linux-raid
In-Reply-To: <20150223110332.4c135de9@notabene.brown>
Dne 23.2.2015 v 01:03 NeilBrown napsal(a):
> Hi,
> thanks for reporting this. It is definitely a bug. It was introduced by
>
>
Hello,
Thank you that you have marked a current write-mostly behaviour as the bug.
If you think that I can help you, please tell me.
Best regards,
Tomas
^ permalink raw reply
* [PATCH 0/5] Fix issues reported by covscan and newer GCC
From: Jes.Sorensen @ 2015-02-24 21:00 UTC (permalink / raw)
To: neilb; +Cc: artur.paszkiewicz, linux-raid, Jes Sorensen
From: Jes Sorensen <Jes.Sorensen@redhat.com>
I had some errors thrown at me by covscan and GCC 4.9.2 which prompted
some furthe inspection. In particular patch 3 could result in a local
stack variable passed back to the calling function, and patch 5 with
code not being executed as expected.
Please have a look.
Cheers,
Jes
Jes Sorensen (5):
Grow.c: Fix classic readlink() buffer overflow
Check return of stat() to avoid covscan complaining
add_orom(): Compare content of struct imsm_orom rather than pointers
to it
IncrementalScan(): Make sure 'st' is valid before dereferencing it
write_super_imsm_spares(): C statements are terminated by ;
Assemble.c | 6 +++++-
Grow.c | 2 +-
Incremental.c | 2 +-
platform-intel.c | 4 ++--
super-intel.c | 12 ++++++------
5 files changed, 15 insertions(+), 11 deletions(-)
--
2.1.0
^ permalink raw reply
* [PATCH 1/5] Grow.c: Fix classic readlink() buffer overflow
From: Jes.Sorensen @ 2015-02-24 21:00 UTC (permalink / raw)
To: neilb; +Cc: artur.paszkiewicz, linux-raid, Jes Sorensen
In-Reply-To: <1424811640-26569-1-git-send-email-Jes.Sorensen@redhat.com>
From: Jes Sorensen <Jes.Sorensen@redhat.com>
The buffer passed on to readlink() needs to contain space for the
terminating \0. See 'man 3 readlink' for details.
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
Grow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Grow.c b/Grow.c
index b78d063..a8bbf2b 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3319,7 +3319,7 @@ started:
bul = make_backup(sra->sys_name);
if (bul) {
char buf[1024];
- int l = readlink(bul, buf, sizeof(buf));
+ int l = readlink(bul, buf, sizeof(buf) - 1);
if (l > 0) {
buf[l]=0;
unlink(buf);
--
2.1.0
^ permalink raw reply related
* [PATCH 2/5] Check return of stat() to avoid covscan complaining
From: Jes.Sorensen @ 2015-02-24 21:00 UTC (permalink / raw)
To: neilb; +Cc: artur.paszkiewicz, linux-raid, Jes Sorensen
In-Reply-To: <1424811640-26569-1-git-send-email-Jes.Sorensen@redhat.com>
From: Jes Sorensen <Jes.Sorensen@redhat.com>
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
Assemble.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Assemble.c b/Assemble.c
index 131f871..b392214 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -688,7 +688,11 @@ static int load_devices(struct devs *devices, char *devmap,
close(dfd);
}
- stat(devname, &stb);
+ if (stat(devname, &stb)) {
+ pr_err("Unsable to stat(%s) - skipping device.\n",
+ devname);
+ continue;
+ }
if (c->verbose > 0)
pr_err("%s is identified as a member of %s, slot %d%s.\n",
--
2.1.0
^ permalink raw reply related
* [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it
From: Jes.Sorensen @ 2015-02-24 21:00 UTC (permalink / raw)
To: neilb; +Cc: artur.paszkiewicz, linux-raid, Jes Sorensen
In-Reply-To: <1424811640-26569-1-git-send-email-Jes.Sorensen@redhat.com>
From: Jes Sorensen <Jes.Sorensen@redhat.com>
This avoids adding the same orom entry to the oroms list multiple
times, as the comparison of pointers is never going to succeed, in
particular when '*orom' points to a local stack variable in the
calling function.
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
platform-intel.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/platform-intel.c b/platform-intel.c
index 37274da..a4ffa9f 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
int i;
for (i = 0; i < SYS_DEV_MAX; i++) {
- if (&oroms[i].orom == orom)
- return orom;
+ if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
+ return &oroms[i].orom;
if (oroms[i].orom.signature[0] == 0) {
oroms[i].orom = *orom;
return &oroms[i].orom;
--
2.1.0
^ permalink raw reply related
* [PATCH 4/5] IncrementalScan(): Make sure 'st' is valid before dereferencing it
From: Jes.Sorensen @ 2015-02-24 21:00 UTC (permalink / raw)
To: neilb; +Cc: artur.paszkiewicz, linux-raid, Jes Sorensen
In-Reply-To: <1424811640-26569-1-git-send-email-Jes.Sorensen@redhat.com>
From: Jes Sorensen <Jes.Sorensen@redhat.com>
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
Incremental.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Incremental.c b/Incremental.c
index 87d9114..33c0d7f 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -1354,7 +1354,7 @@ restart:
if (st && st->ss->load_container)
ret = st->ss->load_container(st, mdfd, NULL);
close(mdfd);
- if (!ret && st->ss->container_content) {
+ if (!ret && st && st->ss->container_content) {
if (map_lock(&map))
pr_err("failed to get exclusive lock on mapfile\n");
ret = Incremental_container(st, me->path, c, only);
--
2.1.0
^ permalink raw reply related
* [PATCH 5/5] write_super_imsm_spares(): C statements are terminated by ;
From: Jes.Sorensen @ 2015-02-24 21:00 UTC (permalink / raw)
To: neilb; +Cc: artur.paszkiewicz, linux-raid, Jes Sorensen
In-Reply-To: <1424811640-26569-1-git-send-email-Jes.Sorensen@redhat.com>
From: Jes Sorensen <Jes.Sorensen@redhat.com>
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
super-intel.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/super-intel.c b/super-intel.c
index 819e0da..7f75b53 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5115,13 +5115,13 @@ static int write_super_imsm_spares(struct intel_super *super, int doclose)
__u32 sum;
struct dl *d;
- spare->mpb_size = __cpu_to_le32(sizeof(struct imsm_super)),
- spare->generation_num = __cpu_to_le32(1UL),
+ spare->mpb_size = __cpu_to_le32(sizeof(struct imsm_super));
+ spare->generation_num = __cpu_to_le32(1UL);
spare->attributes = MPB_ATTRIB_CHECKSUM_VERIFY;
- spare->num_disks = 1,
- spare->num_raid_devs = 0,
- spare->cache_size = mpb->cache_size,
- spare->pwr_cycle_count = __cpu_to_le32(1),
+ spare->num_disks = 1;
+ spare->num_raid_devs = 0;
+ spare->cache_size = mpb->cache_size;
+ spare->pwr_cycle_count = __cpu_to_le32(1);
snprintf((char *) spare->sig, MAX_SIGNATURE_LENGTH,
MPB_SIGNATURE MPB_VERSION_RAID0);
--
2.1.0
^ permalink raw reply related
* Re: [PATCH 2/5] Check return of stat() to avoid covscan complaining
From: NeilBrown @ 2015-02-24 21:12 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: artur.paszkiewicz, linux-raid
In-Reply-To: <1424811640-26569-3-git-send-email-Jes.Sorensen@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2608 bytes --]
On Tue, 24 Feb 2015 16:00:37 -0500 Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
> Assemble.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Assemble.c b/Assemble.c
> index 131f871..b392214 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -688,7 +688,11 @@ static int load_devices(struct devs *devices, char *devmap,
> close(dfd);
> }
>
> - stat(devname, &stb);
> + if (stat(devname, &stb)) {
> + pr_err("Unsable to stat(%s) - skipping device.\n",
> + devname);
> + continue;
> + }
>
> if (c->verbose > 0)
> pr_err("%s is identified as a member of %s, slot %d%s.\n",
I've applied the other 4. I think I'd rather this one was fixed by changing
stat(devname,
to
fstat(dfd,
and keep dfd open a bit longer.
Does this look OK to you?
Thanks,
NeilBrown
diff --git a/Assemble.c b/Assemble.c
index 131f871a6d1e..1e529c1b3126 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -576,13 +576,13 @@ static int load_devices(struct devs *devices, char *devmap,
struct stat stb;
struct supertype *tst;
int i;
+ int dfd;
if (tmpdev->used != 1)
continue;
/* looks like a good enough match to update the super block if needed */
#ifndef MDASSEMBLE
if (c->update) {
- int dfd;
/* prepare useful information in info structures */
struct stat stb2;
int err;
@@ -652,7 +652,6 @@ static int load_devices(struct devs *devices, char *devmap,
if (tst->ss->store_super(tst, dfd))
pr_err("Could not re-write superblock on %s.\n",
devname);
- close(dfd);
if (strcmp(c->update, "uuid")==0 &&
ident->bitmap_fd >= 0 && !bitmap_done) {
@@ -666,9 +665,9 @@ static int load_devices(struct devs *devices, char *devmap,
} else
#endif
{
- int dfd = dev_open(devname,
- tmpdev->disposition == 'I'
- ? O_RDWR : (O_RDWR|O_EXCL));
+ dfd = dev_open(devname,
+ tmpdev->disposition == 'I'
+ ? O_RDWR : (O_RDWR|O_EXCL));
tst = dup_super(st);
if (dfd < 0 || tst->ss->load_super(tst, dfd, NULL) != 0) {
@@ -685,10 +684,10 @@ static int load_devices(struct devs *devices, char *devmap,
return -1;
}
tst->ss->getinfo_super(tst, content, devmap + devcnt * content->array.raid_disks);
- close(dfd);
}
- stat(devname, &stb);
+ fstat(dfd, &stb);
+ close(dfd);
if (c->verbose > 0)
pr_err("%s is identified as a member of %s, slot %d%s.\n",
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply related
* Re: [PATCH 2/5] Check return of stat() to avoid covscan complaining
From: Jes Sorensen @ 2015-02-24 21:56 UTC (permalink / raw)
To: NeilBrown; +Cc: artur.paszkiewicz, linux-raid
In-Reply-To: <20150225081243.1fe91420@notabene.brown>
NeilBrown <neilb@suse.de> writes:
> On Tue, 24 Feb 2015 16:00:37 -0500 Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>> Assemble.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Assemble.c b/Assemble.c
>> index 131f871..b392214 100644
>> --- a/Assemble.c
>> +++ b/Assemble.c
>> @@ -688,7 +688,11 @@ static int load_devices(struct devs *devices, char *devmap,
>> close(dfd);
>> }
>>
>> - stat(devname, &stb);
>> + if (stat(devname, &stb)) {
>> + pr_err("Unsable to stat(%s) - skipping device.\n",
>> + devname);
>> + continue;
>> + }
>>
>> if (c->verbose > 0)
>> pr_err("%s is identified as a member of %s, slot %d%s.\n",
>
> I've applied the other 4. I think I'd rather this one was fixed by changing
> stat(devname,
> to
> fstat(dfd,
> and keep dfd open a bit longer.
>
> Does this look OK to you?
I got the warning from covscan because we ignored the return value from
stat, so I think you still need to check the return value from fstat()
as well.
Cheers,
Jes
>
> Thanks,
> NeilBrown
>
> diff --git a/Assemble.c b/Assemble.c
> index 131f871a6d1e..1e529c1b3126 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -576,13 +576,13 @@ static int load_devices(struct devs *devices, char *devmap,
> struct stat stb;
> struct supertype *tst;
> int i;
> + int dfd;
>
> if (tmpdev->used != 1)
> continue;
> /* looks like a good enough match to update the super block if needed */
> #ifndef MDASSEMBLE
> if (c->update) {
> - int dfd;
> /* prepare useful information in info structures */
> struct stat stb2;
> int err;
> @@ -652,7 +652,6 @@ static int load_devices(struct devs *devices, char *devmap,
> if (tst->ss->store_super(tst, dfd))
> pr_err("Could not re-write superblock on %s.\n",
> devname);
> - close(dfd);
>
> if (strcmp(c->update, "uuid")==0 &&
> ident->bitmap_fd >= 0 && !bitmap_done) {
> @@ -666,9 +665,9 @@ static int load_devices(struct devs *devices, char *devmap,
> } else
> #endif
> {
> - int dfd = dev_open(devname,
> - tmpdev->disposition == 'I'
> - ? O_RDWR : (O_RDWR|O_EXCL));
> + dfd = dev_open(devname,
> + tmpdev->disposition == 'I'
> + ? O_RDWR : (O_RDWR|O_EXCL));
> tst = dup_super(st);
>
> if (dfd < 0 || tst->ss->load_super(tst, dfd, NULL) != 0) {
> @@ -685,10 +684,10 @@ static int load_devices(struct devs *devices, char *devmap,
> return -1;
> }
> tst->ss->getinfo_super(tst, content, devmap + devcnt * content->array.raid_disks);
> - close(dfd);
> }
>
> - stat(devname, &stb);
> + fstat(dfd, &stb);
> + close(dfd);
>
> if (c->verbose > 0)
> pr_err("%s is identified as a member of %s, slot %d%s.\n",
^ permalink raw reply
* Re: [PATCH 2/5] Check return of stat() to avoid covscan complaining
From: NeilBrown @ 2015-02-24 22:03 UTC (permalink / raw)
To: Jes Sorensen; +Cc: artur.paszkiewicz, linux-raid
In-Reply-To: <wrfj61ar55yq.fsf@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1674 bytes --]
On Tue, 24 Feb 2015 16:56:29 -0500 Jes Sorensen <Jes.Sorensen@redhat.com>
wrote:
> NeilBrown <neilb@suse.de> writes:
> > On Tue, 24 Feb 2015 16:00:37 -0500 Jes.Sorensen@redhat.com wrote:
> >
> >> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >>
> >> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> >> ---
> >> Assemble.c | 6 +++++-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Assemble.c b/Assemble.c
> >> index 131f871..b392214 100644
> >> --- a/Assemble.c
> >> +++ b/Assemble.c
> >> @@ -688,7 +688,11 @@ static int load_devices(struct devs *devices, char *devmap,
> >> close(dfd);
> >> }
> >>
> >> - stat(devname, &stb);
> >> + if (stat(devname, &stb)) {
> >> + pr_err("Unsable to stat(%s) - skipping device.\n",
> >> + devname);
> >> + continue;
> >> + }
> >>
> >> if (c->verbose > 0)
> >> pr_err("%s is identified as a member of %s, slot %d%s.\n",
> >
> > I've applied the other 4. I think I'd rather this one was fixed by changing
> > stat(devname,
> > to
> > fstat(dfd,
> > and keep dfd open a bit longer.
> >
> > Does this look OK to you?
>
> I got the warning from covscan because we ignored the return value from
> stat, so I think you still need to check the return value from fstat()
> as well.
I hope not.
You can only get errors from fstat if you do something stupid like passing
NULL as the stat pointer, or passing a non-open file descriptior.
So if covscan complains, then covscan is broken.
In contrast, stat can certainly given an error, such a ENOENT, which cannot
possibly be avoided by not being stupid.
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [dm-devel] [PATCH 0/3] md raid: enhancements to support the device mapper dm-raid target
From: NeilBrown @ 2015-02-24 22:12 UTC (permalink / raw)
To: Heinz Mauelshagen
Cc: device-mapper development, jbras >> Brassow Jonathan,
linux RAID
In-Reply-To: <54EB13AC.4010006@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 321 bytes --]
On Mon, 23 Feb 2015 12:49:00 +0100 Heinz Mauelshagen <heinzm@redhat.com>
wrote:
> Sure.
> I'd like to see the raid0 conditonal request queue patch though.
>
ok, I've applied that one. It should turn up in my -next by the end of the
week.
I'll look forward to your other results.
Thanks,
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ 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