* Re: [dm-devel] [PATCH v6 0/3] dm: boot a mapped device without an initramfs
From: Alasdair G Kergon @ 2017-05-04 13:20 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Kees Cook, Will Drewry, Guenter Roeck, Mike Snitzer,
linux-doc@vger.kernel.org, David Zeuthen, LKML, linux-raid,
dm-devel, Enric Balletbo i Serra, Shaohua Li, Alasdair Kergon
In-Reply-To: <20170504121452.GF4943@agk-dp.fab.redhat.com>
Some more thoughts with your example, dmsetup might look like:
# dmsetup create --bootformat "lroot:uuid,rw,0 2097152 linear 8:2 0, \
2097152 2097152 linear 8:3 0, 4194304 2097152 linear 8:4 0"
- also supporting creating multiple devices if the semi-colon is used
- colon to separate name from uuid, like we already do major:minor
- colon to separate other flags from rw if we need them in future
- splitting first on a unescaped semi-colons, then on the first two
unescaped commas, and then on unescaped commas and then unescaped spaces
within the table
- backslash escapes the following character so it is never a treated
as a separator
- lroot\:uuid\;\\\ \"\, would be a device with no uuid and the name
lroot:uuid;\ ", (on a non-udev system without name mangling)
# dmsetup ls --bootformat lroot
dm="lroot:uuid,rw,0 2097152 linear 8:2 0, 2097152 2097152 \
linear 8:3 0, 4194304 2097152 linear 8:4 0"
# dmsetup ls --bootformat
(all devices on one output line)
While the code also supports devices in the /dev/sda2 format for
convenience, please use the preferred 8:2 format in any implementation
and documented examples (to avoid the unnecessary dependency on /dev and
its dependencies).
Or with some alternative name for the option
--boot[format|param]
--short[format]
--kernelparam
--condensed
other suggestions?
dmsetup create --condensed
dmsetup ls --condensed
Alasdair
^ permalink raw reply
* Re: mdadm/mdmon Name vs argv[0]
From: Pawel Baldysiak @ 2017-05-04 13:40 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <a61470a2-2c3b-3f4a-5712-dfb87f3acad8@gmail.com>
On 05/02/2017 04:19 PM, Jes Sorensen wrote:
> Hi Pawel,
>
> In the below commit you changed the handling of how mdadm/mdmon handles
> printing the name of the running process.
>
> However, if people for some reason decided to change the names of
> mdadm/mdmon this would no longer be correct.
>
> Do you remember if there is any reason why you didn't use argv[0] for this?
>
> commit d56dd607ba433d9334f0fb4114fe081742ae4361
> Author: Pawel Baldysiak <pawel.baldysiak@intel.com>
> Date: Wed Feb 11 22:25:03 2015 +0100
>
> Change way of printing name of a process
>
> Cheers,
> Jes
Hi Jes,
We did not consider this option once we were implementing the change.
Also, I was not considering that someone would like to change the
mdadm/mdmon names :)
I see one potential issue with usage of argv[0]:
If you look into mdmon's code (start of the main()),
you will see that first char is replaced with '@'.
Thanks
Pawel
^ permalink raw reply
* Re: Fault tolerance in RAID0 with badblocks
From: Wols Lists @ 2017-05-04 13:44 UTC (permalink / raw)
To: Ravi (Tom) Hale, linux-raid
In-Reply-To: <03294ec0-2df0-8c1c-dd98-2e9e5efb6f4f@hale.ee>
On 04/05/17 11:04, Ravi (Tom) Hale wrote:
> Since btrfs doesn't support badblocks, this btrfs mailing list post[1]
> suggested to use mdadm RAID0 3.1+.
Having read the email you linked to, I don't think mdadm will be any
help at all ...
>
> Is there a way of having blocks from a spare device automatically
> replacing bad blocks when they are next written to (like SMART does for
> HDDs)?
What quite do you mean?
>
> Or would mdadm be able to add a "badblocks layer" to btrfs in some other
> way?
No. With modern hard drives, no filesystem should pay any attention to
badblocks - it's all handled in the drive firmware. Badblocks is an
unfortunate legacy from the past when drives really were CHS, and the
layer above needed some way of knowing which blocks were bad and should
be avoided. mdadm has had a lot of grief with its handling of badblocks,
and getting drives confused, and it's all totally unnecessary anyway.
Let the drive worry about what blocks are bad. One major point behind
LBA is it hides the actual disk layout from the computer, and allows the
drive to relocate blocks that aren't working properly. Let it do its job.
If you want to use raid, don't bother with 0. Use mdadm and raid 5 or 6
to combine your drives, and create a btrfs filesystem on top. (Don't
bother with raid1 - that part of btrfs apparently works well, so use the
filesystem variant, not an external one.)
>
> My use case is mining storj - I don't mind some data loss.
Using a badblock list will have no impact on this whatsoever.
>
> [1] https://www.spinics.net/lists/linux-btrfs/msg40909.html
>
Cheers,
Wol
^ permalink raw reply
* Re: mdadm/mdmon Name vs argv[0]
From: Jes Sorensen @ 2017-05-04 14:45 UTC (permalink / raw)
To: Pawel Baldysiak; +Cc: linux-raid
In-Reply-To: <3f64ba3e-e931-6055-8621-a91c637e0174@intel.com>
On 05/04/2017 09:40 AM, Pawel Baldysiak wrote:
> On 05/02/2017 04:19 PM, Jes Sorensen wrote:
>> Hi Pawel,
>>
>> In the below commit you changed the handling of how mdadm/mdmon
>> handles printing the name of the running process.
>>
>> However, if people for some reason decided to change the names of
>> mdadm/mdmon this would no longer be correct.
>>
>> Do you remember if there is any reason why you didn't use argv[0] for
>> this?
>>
>> commit d56dd607ba433d9334f0fb4114fe081742ae4361
>> Author: Pawel Baldysiak <pawel.baldysiak@intel.com>
>> Date: Wed Feb 11 22:25:03 2015 +0100
>>
>> Change way of printing name of a process
>>
>> Cheers,
>> Jes
> Hi Jes,
>
> We did not consider this option once we were implementing the change.
> Also, I was not considering that someone would like to change the
> mdadm/mdmon names :)
>
> I see one potential issue with usage of argv[0]:
> If you look into mdmon's code (start of the main()),
> you will see that first char is replaced with '@'.
Good point!
I somehow thought we weren't doing the name mangling anymore.
However we could copy from argv[0] before mangling the name. That way we
would have generic code, rather than hard coded names.
Cheers,
Jes
^ permalink raw reply
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked
From: Jes Sorensen @ 2017-05-04 14:54 UTC (permalink / raw)
To: Zhilong Liu; +Cc: linux-raid
In-Reply-To: <430f083a-2501-cb74-4ac1-89d1c4620a03@suse.com>
On 05/04/2017 08:20 AM, Zhilong Liu wrote:
> Hi Jes,
>
> apply for review, this is a bug I ever encountered.
Zhilong,
Under what circumstances do you see this?
Thanks,
Jes
>
> On 04/26/2017 03:03 PM, Zhilong Liu wrote:
>> The sysfs entry and devnm would be created once create_mddev()
>> performed successfully, but the creating isn't completed here,
>> move STOP_ARRAY to abort_locked, the purpose is to cleanup the
>> partially created array.
>>
>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>> ---
>> Create.c | 11 ++++-------
>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/Create.c b/Create.c
>> index 6ca0924..fe0ab7e 100644
>> --- a/Create.c
>> +++ b/Create.c
>> @@ -904,10 +904,8 @@ int Create(struct supertype *st, char *mddev,
>> remove_partitions(fd);
>> if (st->ss->add_to_super(st, &inf->disk,
>> fd, dv->devname,
>> - dv->data_offset)) {
>> - ioctl(mdfd, STOP_ARRAY, NULL);
>> + dv->data_offset))
>> goto abort_locked;
>> - }
>> st->ss->getinfo_super(st, inf, NULL);
>> safe_mode_delay = inf->safe_mode_delay;
>> @@ -1008,7 +1006,6 @@ int Create(struct supertype *st, char *mddev,
>> sysfs_set_safemode(&info, safe_mode_delay);
>> if (err) {
>> pr_err("failed to activate array.\n");
>> - ioctl(mdfd, STOP_ARRAY, NULL);
>> goto abort;
>> }
>> } else if (c->readonly &&
>> @@ -1018,7 +1015,6 @@ int Create(struct supertype *st, char *mddev,
>> "array_state", "readonly") < 0) {
>> pr_err("Failed to start array: %s\n",
>> strerror(errno));
>> - ioctl(mdfd, STOP_ARRAY, NULL);
>> goto abort;
>> }
>> } else {
>> @@ -1030,7 +1026,6 @@ int Create(struct supertype *st, char *mddev,
>> if (info.array.chunk_size &
>> (info.array.chunk_size-1)) {
>> cont_err("Problem may be that chunk size is not
>> a power of 2\n");
>> }
>> - ioctl(mdfd, STOP_ARRAY, NULL);
>> goto abort;
>> }
>> /* if start_ro module parameter is set, array is
>> @@ -1061,7 +1056,9 @@ int Create(struct supertype *st, char *mddev,
>> map_remove(&map, fd2devnm(mdfd));
>> map_unlock(&map);
>> - if (mdfd >= 0)
>> + if (mdfd >= 0) {
>> + ioctl(mdfd, STOP_ARRAY, NULL);
>> close(mdfd);
>> + }
>> return 1;
>> }
>
^ permalink raw reply
* Re: RAID creation resync behaviors
From: Brad Campbell @ 2017-05-04 15:20 UTC (permalink / raw)
To: Andreas Klauer, Shaohua Li; +Cc: linux-raid, jes.sorensen, neilb
In-Reply-To: <20170504075551.GA3929@metamorpher.de>
On 04/05/17 15:55, Andreas Klauer wrote:
> On Wed, May 03, 2017 at 07:22:58PM -0700, Shaohua Li wrote:
>>
>> Unfortunately not all SSD return zero after trim.
>>
>
> For example? I was under the impression that pretty much all of them do.
Pretty much every Samsung except the 840 Pro it seems. Certainly my 830s
and 850s don't.
> Even the ones that don't advertize it returned zero after trim for me.
You got lucky.
^ permalink raw reply
* Re: [PATCH v1 2/2] mdadm/util: unify stat checking blkdev into function
From: Jes Sorensen @ 2017-05-04 15:33 UTC (permalink / raw)
To: Zhilong Liu; +Cc: linux-raid
In-Reply-To: <20170504121622.4014-3-zlliu@suse.com>
On 05/04/2017 08:16 AM, Zhilong Liu wrote:
> declare function stat_is_blkdev() to integrate repeated stat
> checking blkdev operations, it returns 'true/1' when it is a
> block device, and returns 'false/0' when it isn't.
> The devname is necessary parameter, *rdev is optional, parse
> the pointer of dev_t *rdev, if valid, assigned device number
> to dev_t *rdev, if NULL, ignores.
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
> Assemble.c | 7 ++-----
> Build.c | 25 ++++---------------------
> Incremental.c | 21 ++++-----------------
> Manage.c | 11 +----------
> Monitor.c | 16 ++++------------
> mdadm.h | 1 +
> super-ddf.c | 10 ++++------
> super-intel.c | 10 ++++------
> util.c | 17 +++++++++++++++++
> 9 files changed, 41 insertions(+), 77 deletions(-)
> diff --git a/Manage.c b/Manage.c
> index af55266..2c82326 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -1510,24 +1510,16 @@ int Manage_subdevs(char *devname, int fd,
> */
> rdev = makedev(mj, mn);
> } else {
> - struct stat stb;
> tfd = dev_open(dv->devname, O_RDONLY);
> if (tfd >= 0) {
> fstat_is_blkdev(tfd, dv->devname, &rdev);
> close(tfd);
> } else {
> int open_err = errno;
> - if (stat(dv->devname, &stb) != 0) {
> - pr_err("Cannot find %s: %s\n",
> - dv->devname, strerror(errno));
> - goto abort;
> - }
> - if ((stb.st_mode & S_IFMT) != S_IFBLK) {
> + if (!stat_is_blkdev(dv->devname, &rdev))
> if (dv->disposition == 'M')
> /* non-fatal. Also improbable */
> continue;
> - pr_err("%s is not a block device.\n",
> - dv->devname);
> goto abort;
> }
> if (dv->disposition == 'r')
cc -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter -O2
-DSendmail=\""/usr/sbin/sendmail -t"\" -DCONFFILE=\"/etc/mdadm.conf\"
-DCONFFILE2=\"/etc/mdadm/mdadm.conf\" -DMAP_DIR=\"/run/mdadm\"
-DMAP_FILE=\"map\" -DMDMON_DIR=\"/run/mdadm\"
-DFAILED_SLOTS_DIR=\"/run/mdadm/failed-slots\" -DNO_COROSYNC -DNO_DLM
-DVERSION=\"4.0-108-g688a44b\" -DVERS_DATE="\"2017-05-04\""
-DUSE_PTHREADS -DBINDIR=\"/sbin\" -c -o Manage.o Manage.c
Manage.c: In function ‘Manage_subdevs’:
Manage.c:1519:5: error: this ‘if’ clause does not guard...
[-Werror=misleading-indentation]
if (!stat_is_blkdev(dv->devname, &rdev))
^~
Please at least test build your patch before posting it.
Jes
^ permalink raw reply
* Re: RAID creation resync behaviors
From: Wols Lists @ 2017-05-04 15:50 UTC (permalink / raw)
To: Shaohua Li, David Brown; +Cc: linux-raid, jes.sorensen, neilb
In-Reply-To: <20170504015454.d4obiuume6e3yrdv@kernel.org>
On 04/05/17 02:54, Shaohua Li wrote:
> On Wed, May 03, 2017 at 11:06:01PM +0200, David Brown wrote:
>> On 03/05/17 22:27, Shaohua Li wrote:
>>> Hi,
>>>
>>> Currently we have different resync behaviors in array creation.
>>>
>>> - raid1: copy data from disk 0 to disk 1 (overwrite)
>>> - raid10: read both disks, compare and write if there is difference (compare-write)
>>> - raid4/5: read first n-1 disks, calculate parity and then write parity to the last disk (overwrite)
>>> - raid6: read all disks, calculate parity and compare, and write if there is difference (compare-write)
>>>
>>> Write whole disk is very unfriendly for SSD, because it reduces lifetime. And
>>> if user already does a trim before creation, the unncessary write could make
>>> SSD slower in the future. Could we prefer compare-write to overwrite if mdadm
>>> detects the disks are SSD? Surely sometimes compare-write is slower than
>>> overwrite, so maybe add new option in mdadm. An option to let mdadm trim SSD
>>> before creation sounds reasonable too.
>>>
>>
>> When doing the first sync, md tracks how far its sync has got, keeping a
>> record in the metadata in case it has to be restarted (such as due to a
>> reboot while syncing). Why not simply /not/ sync stripes until you first
>> write to them? It may be that a counter of synced stripes is not enough,
>> and you need a bitmap (like the write intent bitmap), but it would reduce
>> the creation sync time to 0 and avoid any writes at all.
>
> For raid 4/5/6, this means we always must do a full stripe write for any normal
> write if it hits a range not synced. This would harm the performance of the
> norma write. For raid1/10, this sounds more appealing. But since each bit in
> the bitmap will stand for a range. If only part of the range is written by
> normal IO, we have two choices. sync the range immediately and clear the bit,
> this sync will impact normal IO. Don't do the sync immediately, but since the
> bit is set (which means the range isn't synced), read IO can only access the
> first disk, which is harmful too.
>
We're creating the array, right? So the user is sitting in front of
mdadm looking at its output, right?
So we just print a message saying "the disks aren't sync'd. If you don't
want a performance hit in normal use, fire up a sync now and take the
hit up front".
The question isn't "how do we avoid a performance hit?", it's "we're
going to take a hit, do we take it up-front on creation or defer it
until we're using the array?".
Cheers,
Wol
^ permalink raw reply
* Re: RAID creation resync behaviors
From: Wols Lists @ 2017-05-04 16:02 UTC (permalink / raw)
To: David Brown, Shaohua Li; +Cc: linux-raid, jes.sorensen, neilb
In-Reply-To: <590ADA3F.8070909@hesbynett.no>
On 04/05/17 08:37, David Brown wrote:
> On 04/05/17 03:54, Shaohua Li wrote:
>> > On Wed, May 03, 2017 at 11:06:01PM +0200, David Brown wrote:
>>> >> On 03/05/17 22:27, Shaohua Li wrote:
>>>> >>> Hi,
>>>> >>>
>>>> >>> Currently we have different resync behaviors in array creation.
>>>> >>>
>>>> >>> - raid1: copy data from disk 0 to disk 1 (overwrite)
>>>> >>> - raid10: read both disks, compare and write if there is difference (compare-write)
>>>> >>> - raid4/5: read first n-1 disks, calculate parity and then write parity to the last disk (overwrite)
>>>> >>> - raid6: read all disks, calculate parity and compare, and write if there is difference (compare-write)
>>>> >>>
>>>> >>> Write whole disk is very unfriendly for SSD, because it reduces lifetime. And
>>>> >>> if user already does a trim before creation, the unncessary write could make
>>>> >>> SSD slower in the future. Could we prefer compare-write to overwrite if mdadm
>>>> >>> detects the disks are SSD? Surely sometimes compare-write is slower than
>>>> >>> overwrite, so maybe add new option in mdadm. An option to let mdadm trim SSD
>>>> >>> before creation sounds reasonable too.
>>>> >>>
>>> >>
>>> >> When doing the first sync, md tracks how far its sync has got, keeping a
>>> >> record in the metadata in case it has to be restarted (such as due to a
>>> >> reboot while syncing). Why not simply /not/ sync stripes until you first
>>> >> write to them? It may be that a counter of synced stripes is not enough,
>>> >> and you need a bitmap (like the write intent bitmap), but it would reduce
>>> >> the creation sync time to 0 and avoid any writes at all.
>> >
>> > For raid 4/5/6, this means we always must do a full stripe write for any normal
>> > write if it hits a range not synced. This would harm the performance of the
>> > norma write.
> Agreed. The unused sectors could be set to 0, rather than read from the
> disks - that would reduce the latency and be friendly to high-end SSDs
> with compression (zero blocks compress quite well!).
>
>> > For raid1/10, this sounds more appealing. But since each bit in
>> > the bitmap will stand for a range. If only part of the range is written by
>> > normal IO, we have two choices. sync the range immediately and clear the bit,
>> > this sync will impact normal IO. Don't do the sync immediately, but since the
>> > bit is set (which means the range isn't synced), read IO can only access the
>> > first disk, which is harmful too.
>> >
> This could be done in a more sophisticated manner. (Yes, I appreciate
> that "sophisticated" or "complex" are a serious disadvantage - I'm just
> throwing up ideas that could be considered.)
>
> Divide the array into "sync blocks", each covering a range of stripes,
> with a bitmap of three states - unused, partially synced, fully synced.
> All blocks start off unused. If a write is made to a previously unused
> block, that block becomes partially synced, and the write has to be done
> as a full stripe write. For a partially synced block, keep a list of
> ranges of synced stripes (a list will normally be smaller than a bitmap
> here). Whenever there are partially synced blocks in the array, have a
> low priority process (like the normal array creation sync process, or
> rebuild processes) sync the stripes until the block is finished as a
> fully synced block.
>
> That should let you delay the time-consuming and write intensive
> creation sync until you actually need to sync the blocks, without /too/
> much overhead in metadata or in delays when using the disk.
I was thinking along those lines. You mentioned earlier what I would
think of as a "high water mark" - or "how far have we used the array".
The only snag I can think of there is if you start writing in the middle
of the array so your idea of blocks sounds a lot better.
The other thing - this would probably be a synonym of "--assume-clean"
but create a flag "--new-array". This would have to be an opt-in - it
tells mdadm that whatever is on the disk is garbage, and when it does
sync it can safely just stream zeroes to the disk - no reads or parity
checks required ... :-) (This idea might need a few tweaks :-)
Cheers,
Wol
^ permalink raw reply
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked
From: Zhilong @ 2017-05-04 17:42 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <2c1c277c-2fd2-27a8-e6a5-23b1ced3ec90@gmail.com>
发自我的 iPhone
> 在 2017年5月4日，22:54，Jes Sorensen <jes.sorensen@gmail.com> 写道：
>
>> On 05/04/2017 08:20 AM, Zhilong Liu wrote:
>> Hi Jes,
>>
>> apply for review, this is a bug I ever encountered.
>
> Zhilong,
>
> Under what circumstances do you see this?
>
I have tested this path after invoking create_mddev, such as issue the command:
./mdadm -CR /dev/md0 -b internal -l1 -n2 /dev/loop[0-1] --size 63
It would fail and print unsupported chunk size. But the device /dev/md0 and sysfs didn't cleanup after abort creating.
For raid1, the chunk size is not meaningful, but mdadm doesn't allow component size is less than 64k when creating bitmap, thus I choose this command to test.
Thanks,
Zhilong
> Thanks,
> Jes
>
>>
>>> On 04/26/2017 03:03 PM, Zhilong Liu wrote:
>>> The sysfs entry and devnm would be created once create_mddev()
>>> performed successfully, but the creating isn't completed here,
>>> move STOP_ARRAY to abort_locked, the purpose is to cleanup the
>>> partially created array.
>>>
>>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>>> ---
>>> Create.c | 11 ++++-------
>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Create.c b/Create.c
>>> index 6ca0924..fe0ab7e 100644
>>> --- a/Create.c
>>> +++ b/Create.c
>>> @@ -904,10 +904,8 @@ int Create(struct supertype *st, char *mddev,
>>> remove_partitions(fd);
>>> if (st->ss->add_to_super(st, &inf->disk,
>>> fd, dv->devname,
>>> - dv->data_offset)) {
>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>> + dv->data_offset))
>>> goto abort_locked;
>>> - }
>>> st->ss->getinfo_super(st, inf, NULL);
>>> safe_mode_delay = inf->safe_mode_delay;
>>> @@ -1008,7 +1006,6 @@ int Create(struct supertype *st, char *mddev,
>>> sysfs_set_safemode(&info, safe_mode_delay);
>>> if (err) {
>>> pr_err("failed to activate array.\n");
>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>> goto abort;
>>> }
>>> } else if (c->readonly &&
>>> @@ -1018,7 +1015,6 @@ int Create(struct supertype *st, char *mddev,
>>> "array_state", "readonly") < 0) {
>>> pr_err("Failed to start array: %s\n",
>>> strerror(errno));
>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>> goto abort;
>>> }
>>> } else {
>>> @@ -1030,7 +1026,6 @@ int Create(struct supertype *st, char *mddev,
>>> if (info.array.chunk_size &
>>> (info.array.chunk_size-1)) {
>>> cont_err("Problem may be that chunk size is not
>>> a power of 2\n");
>>> }
>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>> goto abort;
>>> }
>>> /* if start_ro module parameter is set, array is
>>> @@ -1061,7 +1056,9 @@ int Create(struct supertype *st, char *mddev,
>>> map_remove(&map, fd2devnm(mdfd));
>>> map_unlock(&map);
>>> - if (mdfd >= 0)
>>> + if (mdfd >= 0) {
>>> + ioctl(mdfd, STOP_ARRAY, NULL);
>>> close(mdfd);
>>> + }
>>> return 1;
>>> }
>>
>
> --
> 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: RAID creation resync behaviors
From: NeilBrown @ 2017-05-04 21:57 UTC (permalink / raw)
To: David Brown, Shaohua Li; +Cc: linux-raid, jes.sorensen, neilb
In-Reply-To: <590ADA3F.8070909@hesbynett.no>
[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]
On Thu, May 04 2017, David Brown wrote:
>
> I have another couple of questions that might be relevant, but I am
> really not sure about the correct answers.
>
> First, if you have a stripe that you know is unused - it has not been
> written to since the array was created - could the raid layer safely
> return all zeros if an attempt was made to read the stripe?
"know is unused" and "it has not been written to since the array was
created" are not necessarily the same thing.
If I have some devices which used to have a RAID5 array but for which
the metadata got destroyed, I might carefully "create" a RAID5 over the
devices and then have access to my data. This has been done more than
once - it is not just theoretical.
But if you really "know" it is unused, then returning zeros should be fine.
>
> Second, when syncing an unused stripe (such as during creation), rather
> than reading the old data and copying it or generating parities, could
> we simply write all zeros to all the blocks in the stripes? For many
> SSDs, this is very efficient.
If you were happy to destroy whatever was there before (see above
recovery example for when you wouldn't), then it might be possible to
make this work.
You would need to be careful not to write zeros over a region that the
filesystem has already used.
That means you either disable all writes until the initialization
completes (waste of time), or you add complexity to track which strips
have been written and which haven't, and only initialise strips that have
not been written. This complexity would only be used once in the entire
life of the RAID. That might not be best use of resources.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: RAID creation resync behaviors
From: NeilBrown @ 2017-05-04 22:00 UTC (permalink / raw)
To: Wols Lists, Shaohua Li, David Brown; +Cc: linux-raid, jes.sorensen, neilb
In-Reply-To: <590B4DCE.3070801@youngman.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2853 bytes --]
On Thu, May 04 2017, Wols Lists wrote:
> On 04/05/17 02:54, Shaohua Li wrote:
>> On Wed, May 03, 2017 at 11:06:01PM +0200, David Brown wrote:
>>> On 03/05/17 22:27, Shaohua Li wrote:
>>>> Hi,
>>>>
>>>> Currently we have different resync behaviors in array creation.
>>>>
>>>> - raid1: copy data from disk 0 to disk 1 (overwrite)
>>>> - raid10: read both disks, compare and write if there is difference (compare-write)
>>>> - raid4/5: read first n-1 disks, calculate parity and then write parity to the last disk (overwrite)
>>>> - raid6: read all disks, calculate parity and compare, and write if there is difference (compare-write)
>>>>
>>>> Write whole disk is very unfriendly for SSD, because it reduces lifetime. And
>>>> if user already does a trim before creation, the unncessary write could make
>>>> SSD slower in the future. Could we prefer compare-write to overwrite if mdadm
>>>> detects the disks are SSD? Surely sometimes compare-write is slower than
>>>> overwrite, so maybe add new option in mdadm. An option to let mdadm trim SSD
>>>> before creation sounds reasonable too.
>>>>
>>>
>>> When doing the first sync, md tracks how far its sync has got, keeping a
>>> record in the metadata in case it has to be restarted (such as due to a
>>> reboot while syncing). Why not simply /not/ sync stripes until you first
>>> write to them? It may be that a counter of synced stripes is not enough,
>>> and you need a bitmap (like the write intent bitmap), but it would reduce
>>> the creation sync time to 0 and avoid any writes at all.
>>
>> For raid 4/5/6, this means we always must do a full stripe write for any normal
>> write if it hits a range not synced. This would harm the performance of the
>> norma write. For raid1/10, this sounds more appealing. But since each bit in
>> the bitmap will stand for a range. If only part of the range is written by
>> normal IO, we have two choices. sync the range immediately and clear the bit,
>> this sync will impact normal IO. Don't do the sync immediately, but since the
>> bit is set (which means the range isn't synced), read IO can only access the
>> first disk, which is harmful too.
>>
> We're creating the array, right? So the user is sitting in front of
> mdadm looking at its output, right?
No, it might be anaconda or yast or some other sysadmin tool that is
running mdadm under the hood.
Presumably those tools could ask the question themselves.
NeilBrown
>
> So we just print a message saying "the disks aren't sync'd. If you don't
> want a performance hit in normal use, fire up a sync now and take the
> hit up front".
>
> The question isn't "how do we avoid a performance hit?", it's "we're
> going to take a hit, do we take it up-front on creation or defer it
> until we're using the array?".
>
> Cheers,
> Wol
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* [PATCH v2 2/2] mdadm/util: unify stat checking blkdev into function
From: Zhilong Liu @ 2017-05-05 3:09 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: linux-raid, Zhilong Liu
In-Reply-To: <2234ff90-2ed8-217f-bdcd-ef08008f9d2c@gmail.com>
declare function stat_is_blkdev() to integrate repeated stat
checking blkdev operations, it returns 'true/1' when it is a
block device, and returns 'false/0' when it isn't.
The devname is necessary parameter, *rdev is optional, parse
the pointer of dev_t *rdev, if valid, assigned device number
to dev_t *rdev, if NULL, ignores.
Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
Assemble.c | 7 ++-----
Build.c | 25 ++++---------------------
Incremental.c | 21 ++++-----------------
Manage.c | 11 +----------
Monitor.c | 16 ++++------------
mdadm.h | 1 +
super-ddf.c | 10 ++++------
super-intel.c | 10 ++++------
util.c | 17 +++++++++++++++++
9 files changed, 41 insertions(+), 77 deletions(-)
diff --git a/Assemble.c b/Assemble.c
index 9d0a89f..30d5838 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -512,15 +512,12 @@ static int select_devices(struct mddev_dev *devlist,
/* Now reject spares that don't match domains of identified members */
for (tmpdev = devlist; tmpdev; tmpdev = tmpdev->next) {
- struct stat stb;
if (tmpdev->used != 3)
continue;
- if (stat(tmpdev->devname, &stb)< 0) {
- pr_err("fstat failed for %s: %s\n",
- tmpdev->devname, strerror(errno));
+ if (!stat_is_blkdev(tmpdev->devname, &rdev)) {
tmpdev->used = 2;
} else {
- struct dev_policy *pol = devid_policy(stb.st_rdev);
+ struct dev_policy *pol = devid_policy(rdev);
int dt = domain_test(domains, pol, NULL);
if (inargv && dt != 0)
/* take this spare as domains match
diff --git a/Build.c b/Build.c
index 2d84b96..ad59867 100644
--- a/Build.c
+++ b/Build.c
@@ -41,7 +41,6 @@ int Build(char *mddev, struct mddev_dev *devlist,
* cc = chunk size factor: 0==4k, 1==8k etc.
*/
int i;
- struct stat stb;
dev_t rdev;
int subdevs = 0, missing_disks = 0;
struct mddev_dev *dv;
@@ -65,16 +64,8 @@ int Build(char *mddev, struct mddev_dev *devlist,
missing_disks++;
continue;
}
- if (stat(dv->devname, &stb)) {
- pr_err("Cannot find %s: %s\n",
- dv->devname, strerror(errno));
- return 1;
- }
- if ((stb.st_mode & S_IFMT) != S_IFBLK) {
- pr_err("%s is not a block device.\n",
- dv->devname);
+ if (!stat_is_blkdev(dv->devname, NULL))
return 1;
- }
}
if (s->raiddisks != subdevs) {
@@ -162,16 +153,8 @@ int Build(char *mddev, struct mddev_dev *devlist,
if (strcmp("missing", dv->devname) == 0)
continue;
- if (stat(dv->devname, &stb)) {
- pr_err("Weird: %s has disappeared.\n",
- dv->devname);
+ if (!stat_is_blkdev(dv->devname, &rdev))
goto abort;
- }
- if ((stb.st_mode & S_IFMT)!= S_IFBLK) {
- pr_err("Weird: %s is no longer a block device.\n",
- dv->devname);
- goto abort;
- }
fd = open(dv->devname, O_RDONLY|O_EXCL);
if (fd < 0) {
pr_err("Cannot open %s: %s\n",
@@ -187,8 +170,8 @@ int Build(char *mddev, struct mddev_dev *devlist,
disk.state = (1<<MD_DISK_SYNC) | (1<<MD_DISK_ACTIVE);
if (dv->writemostly == FlagSet)
disk.state |= 1<<MD_DISK_WRITEMOSTLY;
- disk.major = major(stb.st_rdev);
- disk.minor = minor(stb.st_rdev);
+ disk.major = major(rdev);
+ disk.minor = minor(rdev);
if (ioctl(mdfd, ADD_NEW_DISK, &disk)) {
pr_err("ADD_NEW_DISK failed for %s: %s\n",
dv->devname, strerror(errno));
diff --git a/Incremental.c b/Incremental.c
index 11a34e7..97b2e99 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -86,8 +86,7 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
* - if number of OK devices match expected, or -R and there are enough,
* start the array (auto-readonly).
*/
- struct stat stb;
- dev_t rdev;
+ dev_t rdev, rdev2;
struct mdinfo info, dinfo;
struct mdinfo *sra = NULL, *d;
struct mddev_ident *match;
@@ -108,18 +107,8 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
struct createinfo *ci = conf_get_create_info();
- if (stat(devname, &stb) < 0) {
- if (c->verbose >= 0)
- pr_err("stat failed for %s: %s.\n",
- devname, strerror(errno));
- return rv;
- }
- if ((stb.st_mode & S_IFMT) != S_IFBLK) {
- if (c->verbose >= 0)
- pr_err("%s is not a block device.\n",
- devname);
+ if (!stat_is_blkdev(devname, &rdev))
return rv;
- }
dfd = dev_open(devname, O_RDONLY);
if (dfd < 0) {
if (c->verbose >= 0)
@@ -158,10 +147,8 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
if (!devlist) {
devlist = conf_get_devs();
for (;devlist; devlist = devlist->next) {
- struct stat st2;
- if (stat(devlist->devname, &st2) == 0 &&
- (st2.st_mode & S_IFMT) == S_IFBLK &&
- st2.st_rdev == stb.st_rdev)
+ if (stat_is_blkdev(devlist->devname, &rdev2) &&
+ rdev2 == rdev)
break;
}
}
diff --git a/Manage.c b/Manage.c
index af55266..14276b7 100644
--- a/Manage.c
+++ b/Manage.c
@@ -1510,24 +1510,16 @@ int Manage_subdevs(char *devname, int fd,
*/
rdev = makedev(mj, mn);
} else {
- struct stat stb;
tfd = dev_open(dv->devname, O_RDONLY);
if (tfd >= 0) {
fstat_is_blkdev(tfd, dv->devname, &rdev);
close(tfd);
} else {
int open_err = errno;
- if (stat(dv->devname, &stb) != 0) {
- pr_err("Cannot find %s: %s\n",
- dv->devname, strerror(errno));
- goto abort;
- }
- if ((stb.st_mode & S_IFMT) != S_IFBLK) {
+ if (!stat_is_blkdev(dv->devname, &rdev)) {
if (dv->disposition == 'M')
/* non-fatal. Also improbable */
continue;
- pr_err("%s is not a block device.\n",
- dv->devname);
goto abort;
}
if (dv->disposition == 'r')
@@ -1544,7 +1536,6 @@ int Manage_subdevs(char *devname, int fd,
goto abort;
}
}
- rdev = stb.st_rdev;
}
switch(dv->disposition){
default:
diff --git a/Monitor.c b/Monitor.c
index 1f15377..e2b36ff 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -993,23 +993,13 @@ static void link_containers_with_subarrays(struct state *list)
/* Not really Monitor but ... */
int Wait(char *dev)
{
- struct stat stb;
char devnm[32];
- char *tmp;
int rv = 1;
int frozen_remaining = 3;
- if (stat(dev, &stb) != 0) {
- pr_err("Cannot find %s: %s\n", dev,
- strerror(errno));
+ if (!stat_is_blkdev(dev, NULL))
return 2;
- }
- tmp = stat2devnm(&stb);
- if (!tmp) {
- pr_err("%s is not a block device.\n", dev);
- return 2;
- }
- strcpy(devnm, tmp);
+ strcpy(devnm, dev);
while(1) {
struct mdstat_ent *ms = mdstat_read(1, 0);
@@ -1068,6 +1058,8 @@ int WaitClean(char *dev, int sock, int verbose)
int rv = 1;
char devnm[32];
+ if (!stat_is_blkdev(dev, NULL))
+ return 2;
fd = open(dev, O_RDONLY);
if (fd < 0) {
if (verbose)
diff --git a/mdadm.h b/mdadm.h
index 4adb840..a92feb2 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1435,6 +1435,7 @@ extern int check_partitions(int fd, char *dname,
unsigned long long freesize,
unsigned long long size);
extern int fstat_is_blkdev(int fd, char *devname, dev_t *rdev);
+extern int stat_is_blkdev(char *devname, dev_t *rdev);
extern int get_mdp_major(void);
extern int get_maj_min(char *dev, int *major, int *minor);
diff --git a/super-ddf.c b/super-ddf.c
index 796eaa5..9c82f4f 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -3490,7 +3490,7 @@ static int validate_geometry_ddf_bvd(struct supertype *st,
char *dev, unsigned long long *freesize,
int verbose)
{
- struct stat stb;
+ dev_t rdev;
struct ddf_super *ddf = st->sb;
struct dl *dl;
unsigned long long maxsize;
@@ -3526,13 +3526,11 @@ static int validate_geometry_ddf_bvd(struct supertype *st,
return 1;
}
/* This device must be a member of the set */
- if (stat(dev, &stb) < 0)
- return 0;
- if ((S_IFMT & stb.st_mode) != S_IFBLK)
+ if (!stat_is_blkdev(dev, NULL))
return 0;
for (dl = ddf->dlist ; dl ; dl = dl->next) {
- if (dl->major == (int)major(stb.st_rdev) &&
- dl->minor == (int)minor(stb.st_rdev))
+ if (dl->major == (int)major(rdev) &&
+ dl->minor == (int)minor(rdev))
break;
}
if (!dl) {
diff --git a/super-intel.c b/super-intel.c
index c4196ea..e13c940 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6855,7 +6855,7 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
unsigned long long *freesize,
int verbose)
{
- struct stat stb;
+ dev_t rdev;
struct intel_super *super = st->sb;
struct imsm_super *mpb;
struct dl *dl;
@@ -6920,13 +6920,11 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
}
/* This device must be a member of the set */
- if (stat(dev, &stb) < 0)
- return 0;
- if ((S_IFMT & stb.st_mode) != S_IFBLK)
+ if (!stat_is_blkdev(dev, &rdev))
return 0;
for (dl = super->disks ; dl ; dl = dl->next) {
- if (dl->major == (int)major(stb.st_rdev) &&
- dl->minor == (int)minor(stb.st_rdev))
+ if (dl->major == (int)major(rdev) &&
+ dl->minor == (int)minor(rdev))
break;
}
if (!dl) {
diff --git a/util.c b/util.c
index a92faf8..11ff2cc 100644
--- a/util.c
+++ b/util.c
@@ -747,6 +747,23 @@ int fstat_is_blkdev(int fd, char *devname, dev_t *rdev)
return 1;
}
+int stat_is_blkdev(char *devname, dev_t *rdev)
+{
+ struct stat stb;
+
+ if (stat(devname, &stb) != 0) {
+ pr_err("stat failed for %s: %s\n", devname, strerror(errno));
+ return 0;
+ }
+ if ((S_IFMT & stb.st_mode) != S_IFBLK) {
+ pr_err("%s is not a block device.\n", devname);
+ return 0;
+ }
+ if (rdev)
+ *rdev = stb.st_rdev;
+ return 1;
+}
+
int ask(char *mesg)
{
char *add = "";
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 25/27] block: remove the discard_zeroes_data flag
From: Nicholas A. Bellinger @ 2017-05-05 3:10 UTC (permalink / raw)
To: Mike Snitzer
Cc: hch@lst.de, Bart Van Assche, agk@redhat.com,
lars.ellenberg@linbit.com, martin.petersen@oracle.com,
philipp.reisner@linbit.com, axboe@kernel.dk, shli@kernel.org,
linux-scsi@vger.kernel.org, dm-devel@redhat.com,
drbd-dev@lists.linbit.com, linux-block@vger.kernel.org,
target-devel@vger.kernel.org, linux-raid@vger.kernel.org
In-Reply-To: <20170503143338.GA24111@redhat.com>
On Wed, 2017-05-03 at 10:33 -0400, Mike Snitzer wrote:
> On Tue, May 02 2017 at 11:33pm -0400,
> Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
>
> > On Tue, 2017-05-02 at 09:23 +0200, hch@lst.de wrote:
> > > On Tue, May 02, 2017 at 12:16:13AM -0700, Nicholas A. Bellinger wrote:
> > > > Or, another options is use bdev_write_zeroes_sectors() to determine when
> > > > dev_attrib->unmap_zeroes_data should be set.
> > >
> > > Yes, that in combination with your patch to use bdev_write_zeroes_sectors
> > > for zeroing from write same seems like the right fix.
> >
> > The larger target/iblock conversion patch looks like post v4.12 material
> > at this point, so to avoid breakage wrt to existing LBPRZ behavior, I'll
> > plan to push the following patch post -rc1.
> >
> > diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> > index d2f089c..e7caf78 100644
> > --- a/drivers/target/target_core_device.c
> > +++ b/drivers/target/target_core_device.c
> > @@ -851,7 +851,7 @@ bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
> > attrib->unmap_granularity = q->limits.discard_granularity / block_size;
> > attrib->unmap_granularity_alignment = q->limits.discard_alignment /
> > block_size;
> > - attrib->unmap_zeroes_data = 0;
> > + attrib->unmap_zeroes_data = (q->limits.max_write_zeroes_sectors);
> > return true;
> > }
> > EXPORT_SYMBOL(target_configure_unmap_from_queue);
> >
>
> Completely a nit but: why the extra parenthesis?
dev_attrib->unmap_zeros_data is only compared as a bool.
^ permalink raw reply
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked
From: Liu Zhilong @ 2017-05-05 3:31 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <2c1c277c-2fd2-27a8-e6a5-23b1ced3ec90@gmail.com>
On 05/04/2017 10:54 PM, Jes Sorensen wrote:
> On 05/04/2017 08:20 AM, Zhilong Liu wrote:
>> Hi Jes,
>>
>> apply for review, this is a bug I ever encountered.
>
> Zhilong,
>
> Under what circumstances do you see this?
>
Issued the command:
linux-g0sr:/home/test # ./mdadm -CR /dev/md0 -l1 -n2 -b internal
/dev/loop[0-1] --size 63
... ... ...
mdadm: Given bitmap chunk size not supported.
linux-g0sr:/home/test # ls /dev/md0
/dev/md0
linux-g0sr:/home/test # ls /sys/block/md0/md/
array_size bitmap component_size level metadata_version
raid_disks reshape_position safe_mode_delay
array_state chunk_size layout max_read_errors
new_dev reshape_direction resync_start
create_mddev() writes the devnm to /sys/module/md_mod/parameter/new_array,
then in md.c, module_param_call() called the 'set' function of
add_named_array(),
md_alloc() init_and_add the kobject for devm, finally the devnm device
has created
and sysfs has registered after create_mddev executed successfully. Thus
it's better
to STOP_ARRAY in any case after create_mddev() invoked.
Thanks,
-Zhilong
> Thanks,
> Jes
>
>>
>> On 04/26/2017 03:03 PM, Zhilong Liu wrote:
>>> The sysfs entry and devnm would be created once create_mddev()
>>> performed successfully, but the creating isn't completed here,
>>> move STOP_ARRAY to abort_locked, the purpose is to cleanup the
>>> partially created array.
>>>
>>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>>> ---
>>> Create.c | 11 ++++-------
>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Create.c b/Create.c
>>> index 6ca0924..fe0ab7e 100644
>>> --- a/Create.c
>>> +++ b/Create.c
>>> @@ -904,10 +904,8 @@ int Create(struct supertype *st, char *mddev,
>>> remove_partitions(fd);
>>> if (st->ss->add_to_super(st, &inf->disk,
>>> fd, dv->devname,
>>> - dv->data_offset)) {
>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>> + dv->data_offset))
>>> goto abort_locked;
>>> - }
>>> st->ss->getinfo_super(st, inf, NULL);
>>> safe_mode_delay = inf->safe_mode_delay;
>>> @@ -1008,7 +1006,6 @@ int Create(struct supertype *st, char *mddev,
>>> sysfs_set_safemode(&info, safe_mode_delay);
>>> if (err) {
>>> pr_err("failed to activate array.\n");
>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>> goto abort;
>>> }
>>> } else if (c->readonly &&
>>> @@ -1018,7 +1015,6 @@ int Create(struct supertype *st, char *mddev,
>>> "array_state", "readonly") < 0) {
>>> pr_err("Failed to start array: %s\n",
>>> strerror(errno));
>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>> goto abort;
>>> }
>>> } else {
>>> @@ -1030,7 +1026,6 @@ int Create(struct supertype *st, char *mddev,
>>> if (info.array.chunk_size &
>>> (info.array.chunk_size-1)) {
>>> cont_err("Problem may be that chunk size is not
>>> a power of 2\n");
>>> }
>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>> goto abort;
>>> }
>>> /* if start_ro module parameter is set, array is
>>> @@ -1061,7 +1056,9 @@ int Create(struct supertype *st, char *mddev,
>>> map_remove(&map, fd2devnm(mdfd));
>>> map_unlock(&map);
>>> - if (mdfd >= 0)
>>> + if (mdfd >= 0) {
>>> + ioctl(mdfd, STOP_ARRAY, NULL);
>>> close(mdfd);
>>> + }
>>> return 1;
>>> }
>>
>
> --
> 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: Fault tolerance with badblocks
From: Ravi (Tom) Hale @ 2017-05-05 4:03 UTC (permalink / raw)
To: Wols Lists, linux-raid
In-Reply-To: <590B3039.3060000@youngman.org.uk>
On 04/05/17 20:44, Wols Lists wrote:
> On 04/05/17 11:04, Ravi (Tom) Hale wrote:
>> Is there a way of having blocks from a spare device automatically
>> replacing bad blocks when they are next written to (like SMART does for
>> HDDs)?
>
> What quite do you mean?
I mean: should a bad block be identified, any writes to that virtual
block are redirected to another good LBA block held in a spare pool
which would need to be inaccessible for other purposes (so that they are
indeed spare).
>> Or would mdadm be able to add a "badblocks layer" to btrfs in some other
>> way?
>
> No. With modern hard drives, no filesystem should pay any attention to
> badblocks - it's all handled in the drive firmware.
ext4 supports this, and is a relatively modern filesystem released in
December 2008. While it could be argued that this is for legacy support,
This feature still adds value (see below).
> mdadm has had a lot of grief with its handling of badblocks,
> and getting drives confused, and it's all totally unnecessary anyway.
The use case is simple: What if I want to have more goodblocks to
correct for badblocks than Seagate thinks I should have?
Eg, a charity or poor student wanting to get the most out of their old
hardware.
In my case, I don't care about actual data loss (RAID0).
However, in the usual case, running RAID 1, 5 or 6 with a pool of spare
goodblocks would allow extending the life of hardware considerably while
still providing a poor-man's margin of redundancy.
> Let the drive worry about what blocks are bad. One major point behind
> LBA is it hides the actual disk layout from the computer, and allows the
> drive to relocate blocks that aren't working properly. Let it do its job.
Until it can't do its job any more because it runs out of its
manufacturer determined fixed-size spare pool. Yes there are things to
consider for performance like having the physical good sector being
close to the physical bad sector, so a spare data area could be
allocated every N usable data areas.
And perhaps I could write that one day. :)
>> My use case is mining storj - I don't mind some data loss.
>
> Using a badblock list will have no impact on this whatsoever.
A corrupted file is a corrupted file, and can be deleted at minimal
loss. I just don't want the next file being corrupted by the same badblock.
--
Tom
^ permalink raw reply
* [mdadm PATCH] Fix typo in new udev rule.
From: NeilBrown @ 2017-05-05 5:16 UTC (permalink / raw)
To: Peter Rajnoha, Jes Sorensen; +Cc: linux-raid, dm-devel
In-Reply-To: <85c9e9e1-3d2c-9f88-e923-235fb69a4188@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]
As pointed out by Peter Rajnoha, the correct usage in udev is
TEST=="file", not TEST="file".
Also improve a related comment which was a bit informal.
Reported-by: Peter Rajnoha <prajnoha@redhat.com>
Fixes: cd6cbb08c458 ("Create: tell udev md device is not ready when first created.")
Signed-off-by: NeilBrown <neilb@suse.com>
---
Thanks Peter! Properly tested as well :-)
NeilBrown
lib.c | 2 +-
udev-md-raid-creating.rules | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib.c b/lib.c
index 7e44b1f27fcc..be093e8c30cb 100644
--- a/lib.c
+++ b/lib.c
@@ -165,7 +165,7 @@ char *fd2devnm(int fd)
/* When we create a new array, we don't want the content to
* be immediately examined by udev - it is probably meaningless.
- * So create /run/mdadm/creating-FOO and expect that a udev
+ * So create /run/mdadm/creating-mdXXX and expect that a udev
* rule will noticed this and act accordingly.
*/
static char block_path[] = "/run/mdadm/creating-%s";
diff --git a/udev-md-raid-creating.rules b/udev-md-raid-creating.rules
index 2be466bcefd2..9bef8d1103f5 100644
--- a/udev-md-raid-creating.rules
+++ b/udev-md-raid-creating.rules
@@ -4,4 +4,4 @@
# the array is not "ready" and we should make sure the
# content is ignored.
-KERNEL=="md*", TEST="/run/mdadm/creating-$kernel", ENV{SYSTEMD_READY}="0"
+KERNEL=="md*", TEST=="/run/mdadm/creating-$kernel", ENV{SYSTEMD_READY}="0"
--
2.12.2
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply related
* Re: RAID creation resync behaviors
From: David Brown @ 2017-05-05 6:46 UTC (permalink / raw)
To: NeilBrown, Shaohua Li; +Cc: linux-raid, jes.sorensen, neilb
In-Reply-To: <877f1wp9o3.fsf@notabene.neil.brown.name>
On 04/05/17 23:57, NeilBrown wrote:
> On Thu, May 04 2017, David Brown wrote:
>
>>
>> I have another couple of questions that might be relevant, but I am
>> really not sure about the correct answers.
>>
>> First, if you have a stripe that you know is unused - it has not been
>> written to since the array was created - could the raid layer safely
>> return all zeros if an attempt was made to read the stripe?
>
> "know is unused" and "it has not been written to since the array was
> created" are not necessarily the same thing.
>
> If I have some devices which used to have a RAID5 array but for which
> the metadata got destroyed, I might carefully "create" a RAID5 over the
> devices and then have access to my data. This has been done more than
> once - it is not just theoretical.
That is true, of course - anything like this would have to be optional
(command line switches in mdadm, for example).
There is also the opposite situation - when you /have/ had something
written to the array, but now you know it is unused (due to a trim).
Knowing the stripe is unused might make a later partial write a little
faster, and it would certainly speed up a scrub or other consistency
check since unused stripes can be skipped.
>
> But if you really "know" it is unused, then returning zeros should be fine.
>
>>
>> Second, when syncing an unused stripe (such as during creation), rather
>> than reading the old data and copying it or generating parities, could
>> we simply write all zeros to all the blocks in the stripes? For many
>> SSDs, this is very efficient.
>
> If you were happy to destroy whatever was there before (see above
> recovery example for when you wouldn't), then it might be possible to
> make this work.
As above, this would have to be option-controlled. (I have had occasion
to pull disks from one dead server to recover them on another machine -
it's nerve-racking enough at the best of times, without fearing that you
will zero out your remaining good disks!)
> You would need to be careful not to write zeros over a region that the
> filesystem has already used.
Yes, but that should not be a difficult problem - the array is created
before the filesystem.
> That means you either disable all writes until the initialization
> completes (waste of time), or you add complexity to track which strips
> have been written and which haven't, and only initialise strips that have
> not been written. This complexity would only be used once in the entire
> life of the RAID. That might not be best use of resources.
>
I am not sure I see how this would be a problem. But it is something
that would need to be considered carefully when looking at details of
implementing these ideas (if anyone thinks they would be worth
implementing).
mvh.,
David
^ permalink raw reply
* Re: [PATCH v2 2/2] mdadm/util: unify stat checking blkdev into function
From: Jes Sorensen @ 2017-05-05 15:06 UTC (permalink / raw)
To: Zhilong Liu; +Cc: linux-raid
In-Reply-To: <20170505030941.14911-1-zlliu@suse.com>
On 05/04/2017 11:09 PM, Zhilong Liu wrote:
> declare function stat_is_blkdev() to integrate repeated stat
> checking blkdev operations, it returns 'true/1' when it is a
> block device, and returns 'false/0' when it isn't.
> The devname is necessary parameter, *rdev is optional, parse
> the pointer of dev_t *rdev, if valid, assigned device number
> to dev_t *rdev, if NULL, ignores.
Both applied!
Jes
^ permalink raw reply
* Re: [mdadm PATCH] Fix typo in new udev rule.
From: Jes Sorensen @ 2017-05-05 15:07 UTC (permalink / raw)
To: NeilBrown, Peter Rajnoha; +Cc: linux-raid, dm-devel
In-Reply-To: <87r303opcw.fsf@notabene.neil.brown.name>
On 05/05/2017 01:16 AM, NeilBrown wrote:
>
> As pointed out by Peter Rajnoha, the correct usage in udev is
> TEST=="file", not TEST="file".
>
> Also improve a related comment which was a bit informal.
>
> Reported-by: Peter Rajnoha <prajnoha@redhat.com>
> Fixes: cd6cbb08c458 ("Create: tell udev md device is not ready when first created.")
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>
> Thanks Peter! Properly tested as well :-)
>
> NeilBrown
Applied!
Thanks both of you!
Jes
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Anthony Youngman @ 2017-05-05 19:20 UTC (permalink / raw)
To: Ravi (Tom) Hale, linux-raid
In-Reply-To: <84184eb3-52c4-e7ad-cd5b-5021b5cf47ee@hale.ee>
On 05/05/17 05:03, Ravi (Tom) Hale wrote:
> On 04/05/17 20:44, Wols Lists wrote:
>> On 04/05/17 11:04, Ravi (Tom) Hale wrote:
>>> Is there a way of having blocks from a spare device automatically
>>> replacing bad blocks when they are next written to (like SMART does for
>>> HDDs)?
>>
>> What quite do you mean?
>
> I mean: should a bad block be identified, any writes to that virtual
> block are redirected to another good LBA block held in a spare pool
> which would need to be inaccessible for other purposes (so that they are
> indeed spare).
>
>>> Or would mdadm be able to add a "badblocks layer" to btrfs in some other
>>> way?
>>
>> No. With modern hard drives, no filesystem should pay any attention to
>> badblocks - it's all handled in the drive firmware.
>
> ext4 supports this, and is a relatively modern filesystem released in
> December 2008. While it could be argued that this is for legacy support,
> This feature still adds value (see below).
>
>> mdadm has had a lot of grief with its handling of badblocks,
>> and getting drives confused, and it's all totally unnecessary anyway.
>
> The use case is simple: What if I want to have more goodblocks to
> correct for badblocks than Seagate thinks I should have?
Understood. Except that when you get to that state, your drive is
probably dying anyway. Or tiny by modern standards.
>
> Eg, a charity or poor student wanting to get the most out of their old
> hardware.
>
> In my case, I don't care about actual data loss (RAID0).
>
> However, in the usual case, running RAID 1, 5 or 6 with a pool of spare
> goodblocks would allow extending the life of hardware considerably while
> still providing a poor-man's margin of redundancy.
>
>> Let the drive worry about what blocks are bad. One major point behind
>> LBA is it hides the actual disk layout from the computer, and allows the
>> drive to relocate blocks that aren't working properly. Let it do its job.
>
> Until it can't do its job any more because it runs out of its
> manufacturer determined fixed-size spare pool.
Bear in mind I'm speculating slightly here ... but how are you going to
know when the drive has run out of its spare-pool? Bear in mind that
most SSDs, it seems, will commit suicide at this point ...
Bear in mind also, that any *within* *spec* drive can have an "accident"
every 10TB and still be considered perfectly okay. Which means that if
you do what you are supposed to do (rewrite the block) you're risking
the drive remapping the block - and getting closer to the drive bricking
itself. But if you trap the error yourself and add it to the badblocks
list, you are risking throwing away perfectly decent blocks that just
hiccuped.
Bear in mind also, that with raid we recommend "scrubbing". That's
basically reading the entire disk looking for errors, because data does
fade. So if you "look after" a 3TB drive, you could be losing a block a
month to your badblock list. Not good.
Yes there are things to
> consider for performance like having the physical good sector being
> close to the physical bad sector, so a spare data area could be
> allocated every N usable data areas.
>
> And perhaps I could write that one day. :)
>
>>> My use case is mining storj - I don't mind some data loss.
>>
>> Using a badblock list will have no impact on this whatsoever.
>
> A corrupted file is a corrupted file, and can be deleted at minimal
> loss. I just don't want the next file being corrupted by the same badblock.
>
As we say, YMMV. If that's what you want to do, fine. Which is going to
happen first - the drive bricks itself because it runs out of
manufacturer-supplied spare blocks, or you bin the drive because your
bad-blocks-list has got too big to handle? I suspect your bad block list
will fill up long before the drive runs out of manufacturer-supplied blocks.
Cheers,
Wol
^ permalink raw reply
* Re: [RndTbl] Big-endian RAID5 recovery problem
From: Adam Thompson @ 2017-05-05 19:22 UTC (permalink / raw)
To: Continuation of Round Table discussion; +Cc: linux-raid
In-Reply-To: <20170502135934.59548e17@natsu>
On 2017-05-02 03:59, Roman Mamedov wrote:
> On Mon, 01 May 2017 16:39:07 -0500
> Adam Thompson <athompso@athompso.net> wrote:
>
>> I can't find any modern big-endian Linux systems... looks like all the
>> ARM distros run in little-endian mode.
>
> Here are QEMU images for debian-mips (should be big-endian, as opposed
> to
> debian-mipsel): https://people.debian.org/~aurel32/qemu/mips/
>
> Of course it will run purely in software, but most likely more than
> fast enough
> to copy away the data.
>
> Not entirely sure that particular emulated MIPS system support more
> than 4
> drives), but it appears that a starting point could be (man
> qemu-system):
>
> Instead of -hda, -hdb, -hdc, -hdd, you can use:
>
> qemu-system-i386 -drive file=file,index=0,media=disk
> qemu-system-i386 -drive file=file,index=1,media=disk
> qemu-system-i386 -drive file=file,index=2,media=disk
> qemu-system-i386 -drive file=file,index=3,media=disk
>
> with indexes 0..5, as you need the boot disk, all 4 drives, and one
> more as
> the backup destination.
>
> May or may not be the best way, but IMO beats trying to hex-edit the
> superblock right away.
So I now have:
4 x old IDE hard drives,
plugged into 4 x USB3-to-IDE adapters,
plugged into a 4x USB3 PCIe 1x adapter card,
plugged into an Ubuntu desktop i5 system @ 3.3GHz,
passed through into QEMU as virtual SCSI devices,
connected to a virtual AMD am53c974 SCSI adapter,
connected to a virtual Malta-series MIPS64 series,
emulated by QEMU.
Yikes!
The good news is that as soon as the Debian kernel booted, it
auto-detected the RAID arrays and started re-silvering them.
The bad news is: 1) that I thought I had attached the devices in
read-only mode (oops); and 2) it's re-syncing the MD array at
~5000Kb/sec.
I'll leave it to sync over the weekend (praying for no power outages),
but I sure hope I can upgrade the metadata block instead of doing this
all through a QEMU (non-accelerated) VM!
Thanks for the suggestions so far,
-Adam
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Peter Grandi @ 2017-05-05 20:23 UTC (permalink / raw)
To: Linux RAID
In-Reply-To: <84184eb3-52c4-e7ad-cd5b-5021b5cf47ee@hale.ee>
>> No. With modern hard drives, no filesystem should pay any
>> attention to badblocks - it's all handled in the drive firmware.
> ext4 supports this,
Also JFS also supports bad-block avoidance, but only at 'mkfs' time
and JFS does this for legacy reason: Linux JFS supports this because
it is a port of JFS/2 from OS/2 which was a port of JFS version 1
from AIX in 1990.
> and is a relatively modern filesystem released in December
> 2008.
It is just a retread of 'ext3' which itself was a recycling of
'ext2' which was in turn a clone of the 4BSD FFS, and we are talking
of design decisions taken in 1982-3, not 2008.
> While it could be argued that this is for legacy support,
It is for legacy support. Once upon a time a drive's controller was
the main CPU itself, and the kernel had to manage bad block sparing
(as well as rotational layout and track buffering). That was up to
around 20-30 years ago :-).
> This feature still adds value (see below).
It adds value if one underestimates typical disk drive failure
modes. It is quite irritating even for me that a drive with way
less than 1% bad blocks becomes effectively unusable, but long
experience tells me that once a drive starts to grow defects to the
point that manufacturer spare sectors run out there is usually a
reason for it and sooner than later it will be almost completely
unusable.
[ ... ]
> The use case is simple: What if I want to have more goodblocks to
> correct for badblocks than Seagate thinks I should have?
The answer is also simple: if you think you know better than
Seagate, or if you think that Seagate deliberately allocates too few
spare sector, you ask Seagate for custom firmware that allocates
more of a disk capacity for spares. I suspect that with an order of
at least 100,000 drives they will be happy to help. :-)
> Eg, a charity or poor student wanting to get the most out of their
> old hardware.
If it is your itch, and you think you know better than the rest
of the industry, scratch your itch: send patches :-).
Other people know that usually keeping decaying drives in use is
fairly pointless. Legend is that USSR computer engineers perfected
that art though, but they worked in special circumstances. For a
similar example look at the BadRAM and similar modules:
https://help.ubuntu.com/community/BadRAM
http://rick.vanrein.org/linux/badram/
They haven't become that popular... :-)
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Nix @ 2017-05-05 22:14 UTC (permalink / raw)
To: Peter Grandi; +Cc: Linux RAID
In-Reply-To: <22796.57128.627832.878830@tree.ty.sabi.co.uk>
On 5 May 2017, Peter Grandi stated:
>> This feature still adds value (see below).
>
> It adds value if one underestimates typical disk drive failure
> modes. It is quite irritating even for me that a drive with way
> less than 1% bad blocks becomes effectively unusable, but long
> experience tells me that once a drive starts to grow defects to the
> point that manufacturer spare sectors run out there is usually a
> reason for it and sooner than later it will be almost completely
> unusable.
Quite. In my experience, if there are that many bad blocks on rotational
storage, it generally means either that a head has died or that the disk
surface is damaged. If the disk surface is damaged to that degree, there
will be crap flying around inside the drive at very high speed, abrading
the drive surface further with every passing minute. Such a drive is
walking dead. Get any surviving data off now and throw it away with
extreme prejudice, possibly pulling it apart first to gawp at the
horribleness that is all that remains of your disk surfaces. As for the
dead-head case, the question is whether whatever killed the head
produced debris. If it did, you're back at the previous problem, and if
it's electronic failure, frankly the whole drive is untrustworthy IMHO.
(There *are* other possibilities: catastrophically buggy drive firmware,
for instance -- but in such cases the drive is *also* walking dead.)
--
NULL && (void)
^ permalink raw reply
* Re: [bug report] md-cluster: Fix adding of new disk with new reload code
From: Goldwyn Rodrigues @ 2017-05-05 22:30 UTC (permalink / raw)
To: Guoqing Jiang, Dan Carpenter; +Cc: linux-raid
In-Reply-To: <59015CD0.3080505@suse.com>
On 04/26/2017 09:52 PM, Guoqing Jiang wrote:
> HI,
>
> Thanks for check.
>
> On 04/27/2017 03:59 AM, Dan Carpenter wrote:
>> Hello Goldwyn Rodrigues,
>>
>> The patch dbb64f8635f5: "md-cluster: Fix adding of new disk with new
>> reload code" from Oct 1, 2015, leads to the following static checker
>> warning:
>>
>> drivers/md/md-cluster.c:1341 add_new_disk()
>> warn: inconsistent returns 'cinfo->recv_mutex'.
>> Locked on : 1315,1341
>> Unlocked on: 1341
>>
>> drivers/md/md-cluster.c
>> 1300 static int add_new_disk(struct mddev *mddev, struct md_rdev
>> *rdev)
>> 1301 {
>> 1302 struct md_cluster_info *cinfo = mddev->cluster_info;
>> 1303 struct cluster_msg cmsg;
>> 1304 int ret = 0;
>> 1305 struct mdp_superblock_1 *sb =
>> page_address(rdev->sb_page);
>> 1306 char *uuid = sb->device_uuid;
>> 1307
>> 1308 memset(&cmsg, 0, sizeof(cmsg));
>> 1309 cmsg.type = cpu_to_le32(NEWDISK);
>> 1310 memcpy(cmsg.uuid, uuid, 16);
>> 1311 cmsg.raid_slot = cpu_to_le32(rdev->desc_nr);
>> 1312 lock_comm(cinfo, 1);
>> ^^^^^^^^^^^^^^^^^^^
>> We take the lock here.
>>
>> 1313 ret = __sendmsg(cinfo, &cmsg);
>> 1314 if (ret)
>> 1315 return ret;
>>
>> Should we unlock on failure here?
>
> I agree we may need the unlock here, since both add_new_disk_cancel and
> unlock_comm are not called for the failure case, it is not right.
>
> But I think Goldwyn knows better, let's wait for his comment.
Yes, you are right. We should unlock here. Thanks.
Could you please send a patch?
>
>
>> 1316 cinfo->no_new_dev_lockres->flags |= DLM_LKF_NOQUEUE;
>> 1317 ret = dlm_lock_sync(cinfo->no_new_dev_lockres,
>> DLM_LOCK_EX);
>> 1318 cinfo->no_new_dev_lockres->flags &= ~DLM_LKF_NOQUEUE;
>> 1319 /* Some node does not "see" the device */
>> 1320 if (ret == -EAGAIN)
>> 1321 ret = -ENOENT;
>> 1322 if (ret)
>> 1323 unlock_comm(cinfo);
>>
>> Because we do here. I think we're only supposed to hold the lock on
>> success but how this all works with cancel etc looked slightly
>> complicated so I decided to ask instead of sending a patch.
>
> Please take a look with comments from L1326 to L1337, unlock will be
> called eventually by
> metadata_update_finish (for successful case) or
> metadata_update_cancel/add_new_disk_cancel
> (for failure case).
>
>>
>> 1324 else {
>> 1325 dlm_lock_sync(cinfo->no_new_dev_lockres,
>> DLM_LOCK_CR);
>> 1326 /* Since MD_CHANGE_DEVS will be set in
>> add_bound_rdev which
>> 1327 * will run soon after add_new_disk, the
>> below path will be
>> 1328 * invoked:
>> 1329 * md_wakeup_thread(mddev->thread)
>> 1330 * -> conf->thread (raid1d)
>> 1331 * -> md_check_recovery -> md_update_sb
>> 1332 * -> metadata_update_start/finish
>> 1333 * MD_CLUSTER_SEND_LOCKED_ALREADY will be
>> cleared eventually.
>> 1334 *
>> 1335 * For other failure cases,
>> metadata_update_cancel and
>> 1336 * add_new_disk_cancel also clear below bit
>> as well.
>> 1337 * */
>> 1338 set_bit(MD_CLUSTER_SEND_LOCKED_ALREADY,
>> &cinfo->state);
>> 1339 wake_up(&cinfo->wait);
>> 1340 }
>> 1341 return ret;
>> 1342 }
>>
>
> Regards,
> Guoqing
--
Goldwyn
^ 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