* Re: [PATCH] md: make suspend range wait timed out
From: NeilBrown @ 2017-06-18 21:30 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Shaohua Li, Mikulas Patocka
In-Reply-To: <20170616155204.myffyxp5tuoctcoo@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 2939 bytes --]
On Fri, Jun 16 2017, Shaohua Li wrote:
> On Fri, Jun 16, 2017 at 01:26:00PM +1000, Neil Brown wrote:
>> On Fri, Jun 09 2017, Shaohua Li wrote:
>>
>> > From: Shaohua Li <shli@fb.com>
>> >
>> > suspend range is controlled by userspace. If userspace doesn't clear suspend
>> > range, it's possible a thread will wait for the range forever, we can't even
>> > kill it. This is bad behavior. Add a timeout in the wait. If timeout happens,
>> > we return IO error. The app controlling suspend range looks like part of disk
>> > firmware, if disk isn't responded for a long time, timed out IO error is
>> > returned.
>> >
>> > A simple search in SCSI code shows maximum IO timeout is 120s, so I use this
>> > value here too.
>>
>> I really don't like this. It is an API change with no really
>> justification. Has the current behavior caused a problem?
>
> This centainly causes problem. Set the suspend range will make application
> stall for ever, don't you think this is a problem?
I agree that it could cause a problem. I'm asking it is actually, in
practice, causes a problem. Do you have reports from people saying "the
IO to my RAID array is hanging, what is wrong?" and you look into it and
find out that suspend_hi is larger than suspend_lo?
And if that does happen, is this really the best way to fix it?
>
>> Both md and dm export APIs which allow IO to be suspended while
>> changes are made. Changing that to a timed-out period needs, I think,
>> to be clearly justified.
>>
>> If it is changed to a timed-out period, then that should be explicit,
>> rather than having each request independently time out.
>> i.e. when the suspend is initiated, the end-time should be computed, and
>> any IO would block until that time, not block for some number of
>> seconds.
>
> Ok, this makes sense. We can add a timeout. If it's expired, we clear suspend
> range. Userspace should know what they are doing.
>
>>
>> If an md device is left suspended, then the current code will block IO
>> indefinitely. This patch will at a 20minute times to every single
>> request, which will mean IO proceeds, but extremely slowly. I don't see
>> that as a useful improvement.
>
> It returns error, so application will not dispatch more IO. But I agree a
> timeout to clear the suspend looks a better policy.
Write errors only get back to the application if it calls fsync(), and
many don't do that. Write errors can easily cause a filesystem to go
read-only, and require an fsck. I think we should be very cautious
about triggering write errors.
NFS will hang indefinitely rather then return an error if the server is
not available. That can certainly be annoying, but the alternative has
been tried, and it leads to random data corruption.
The two cases are only comparable at a very high level, but I think
this result should encourage substantial caution.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: mismatch_cnt > 0 during initial sync?
From: NeilBrown @ 2017-06-18 21:34 UTC (permalink / raw)
To: Stephane Thiell, linux-raid@vger.kernel.org
In-Reply-To: <D974501A-3A8C-438B-A043-8E44DE6D18C7@stanford.edu>
[-- Attachment #1: Type: text/plain, Size: 3201 bytes --]
On Tue, Apr 25 2017, Stephane Thiell wrote:
> I just did some tests and this seems normal if one of the raid disk previously has some non-zero data. Perhaps there should be a note about that in md(4) or elsewhere? (sorry if I missed it)
From the perspective of md, the initial sync is no different from any
other sync. It will count the number of mismatches that it finds and
fixes.
Feel free to post a patch for md.4 or elsewhere to clarify this.
Thanks,
NeilBrown
>
> Thanks,
>
> Stephan
>
>> On Apr 24, 2017, at 3:01 PM, Stephane Thiell <sthiell@stanford.edu> wrote:
>>
>>
>> Hi all,
>>
>> Is it normal to get any mismatch_cnt > 0 during the initial sync? I just created a few new new raid6 volumes, and one of them is showing an increasing number of mismatch_cnt during its initial resync. Nothing is visible in the console nor after a smart short test (I also started a long test just in case).
>>
>>
>> $ mdadm --detail /dev/md7
>> /dev/md7:
>> Version : 1.2
>> Creation Time : Mon Apr 24 10:07:18 2017
>> Raid Level : raid6
>> Array Size : 62511163904 (59615.29 GiB 64011.43 GB)
>> Used Dev Size : 7813895488 (7451.91 GiB 8001.43 GB)
>> Raid Devices : 10
>> Total Devices : 10
>> Persistence : Superblock is persistent
>>
>> Intent Bitmap : Internal
>>
>> Update Time : Mon Apr 24 14:52:12 2017
>> State : clean, resyncing
>> Active Devices : 10
>> Working Devices : 10
>> Failed Devices : 0
>> Spare Devices : 0
>>
>> Layout : left-symmetric
>> Chunk Size : 64K
>>
>> Resync Status : 24% complete
>>
>> Name : oak-io1-s2:7 (local to host oak-io1-s2)
>> UUID : 6ece9d71:a97f9497:4612ed73:44dfdc0a
>> Events : 3308
>>
>> Number Major Minor RaidDevice State
>> 0 253 62 0 active sync /dev/dm-62
>> 1 253 63 1 active sync /dev/dm-63
>> 2 253 72 2 active sync /dev/dm-72
>> 3 253 75 3 active sync /dev/dm-75
>> 4 253 89 4 active sync /dev/dm-89
>> 5 253 87 5 active sync /dev/dm-87
>> 6 253 119 6 active sync /dev/dm-119
>> 7 253 97 7 active sync /dev/dm-97
>> 8 253 103 8 active sync /dev/dm-103
>> 9 253 107 9 active sync /dev/dm-107
>>
>> $ cat /sys/block/md7/md/sync_completed
>> 3795284824 / 15627790976
>>
>> $ cat /sys/block/md7/md/mismatch_cnt
>> 3424
>>
>> versions:
>> mdadm-3.4-14.el7_3.1.x86_64
>> CentOS7 (3.10.0-514.10.2.el7_lustre.x86_64)
>>
>> Thanks!
>>
>> Stephan
>> --
>> 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
>
>
> --
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH] md/bitmap: don't read page from device with Bitmap_sync
From: NeilBrown @ 2017-06-18 23:11 UTC (permalink / raw)
To: Shaohua Li, Guoqing Jiang; +Cc: linux-raid
In-Reply-To: <20170616173613.c2cp6qt2hmoebgct@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 3056 bytes --]
On Fri, Jun 16 2017, Shaohua Li wrote:
> On Fri, Jun 16, 2017 at 05:19:27PM +0800, Guoqing Jiang wrote:
>> The device owns Bitmap_sync flag needs recovery
>> to become in sync, and read page from this type
>> device could get stale status.
>>
>> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
>> ---
>> When develop for clustered raid10 feature, if add a
>> disk under grow mode in master node, I could get
>> the "bitmap superblock UUID mismatch" warning due
>> to the page is read from Bitmap_sync device.
>>
>> drivers/md/bitmap.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
>> index bf7419a..bf34cd8 100644
>> --- a/drivers/md/bitmap.c
>> +++ b/drivers/md/bitmap.c
>> @@ -156,7 +156,8 @@ static int read_sb_page(struct mddev *mddev, loff_t offset,
>>
>> rdev_for_each(rdev, mddev) {
>> if (! test_bit(In_sync, &rdev->flags)
>
> Why In_sync is set for the rdev? I think it shoudn't.
There are several states a device can be in.
If ->raid_disk is < 0, then the device is a spare, and doesn't contain
and valid data.
Otherwise ->raid_disk >=0 and:
In_sync is clear, which means that blocks before ->recovery_offset
contain valid data, and blocks after there don't
In_sync is set, which means ->recovery_offset is irrelevant and
should be treated as MaxSector
Bitmap_sync is set, which could apply in either of the above cases,
and means blocks corresponding to bits that are set in the bitmap
may not be up to date.
Bitmap_sync is only relevant before a device has been handed to the
personality. After it has been added, ->recovery_cp ensures that the
blocks that might be wrong are not read until until the bitmap-based
recovery has fixed them up.
Bitmap_sync is only use to stop the device from being given to the
personality if a recovery won't be started because the array is
read-only.
So it is perfectly valid for both In_sync and Bitmap_sync to be set.
If they are, it makes sense to avoid reading bitmap information from the
Bitmap_sync device, as that will be out-of-date.
I'm not quite sure why Guoqing is getting a UUID mismatch ... it
suggests that the new device wasn't initialized properly. So there
might be another bug. But I think this is definitely a bug.
>
>> - || test_bit(Faulty, &rdev->flags))
>> + || test_bit(Faulty, &rdev->flags)
>> + || test_bit(Bitmap_sync, &rdev->flags))
>
> I didn't see code clears the Bitmap_sync bit after disk is synced, so likely
> there is bug in this side.
There is no need to clear Bitmap_sync. It stays set until it becomes
irrelevant.
Thanks,
NeilBrown
>
>> continue;
>>
>> target = offset + index * (PAGE_SIZE/512);
>> --
>> 2.10.0
>>
> --
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH] Monitor: don't assume mdadm parameter is a block device
From: Tomasz Majchrzak @ 2017-06-19 9:16 UTC (permalink / raw)
To: Zhilong Liu; +Cc: linux-raid, jes.sorensen
In-Reply-To: <585FFF8D-0238-4AB3-AA77-4C3C00C3C55D@suse.com>
On Sun, Jun 18, 2017 at 10:10:02PM +0800, Zhilong Liu wrote:
>
>
> > 在 2017年6月16日，22:02，Tomasz Majchrzak <tomasz.majchrzak@intel.com> 写道：
> >
> > If symlink (e.g. /dev/md/raid) is passed as a parameter to mdadm --wait,
> > it fails as it's not able to find a corresponding entry in /proc/mdstat
> > output. Get parameter file major:minor and look for block device name in
> > sysfs. This commit is partial revert of commit 9e04ac1c43e6
> > ("mdadm/util: unify fstat checking blkdev into function").
> >
>
> Thanks very much for catching this. Just remind a typo in (commit title), correct fstat as stat.
Will do.
>
> > Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> > ---
> > Monitor.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > v2:
> > next Zhilong Liu commit id
> >
> > diff --git a/Monitor.c b/Monitor.c
> > index bef2f1b..c9f24bd 100644
> > --- a/Monitor.c
> > +++ b/Monitor.c
> > @@ -982,12 +982,21 @@ static void link_containers_with_subarrays(struct state *list)
> > int Wait(char *dev)
> > {
> > char devnm[32];
> > + dev_t rdev;
> > + char *tmp;
> > int rv = 1;
> > int frozen_remaining = 3;
> >
> > - if (!stat_is_blkdev(dev, NULL))
> > + if (!stat_is_blkdev(dev, &rdev))
> > + return 2;
> > +
> > + tmp = devid2devnm(rdev);
> > + if (!tmp) {
> > + pr_err("Cannot get md device name.\n");
> > return 2;
> > - strcpy(devnm, dev);
> > + }
> > +
> > + strcpy(devnm, tmp);
> >
>
> Quite agreed with the code here, just a small question, I may use
> > strcpy(devnm, devid2devnm(rdev));
> directly since stat_is_blkdev() has returned the major/minor devid. I don't know "get md device name " would be failed in which scenario.
> I'm asking this question since I haven't been familiar with all raids situations. :-)
>
> Thanks again,
> Zhilong
Well, there could be a race and RAID array might get stopped between two
calls (stat_is_blkdev, devid2devnm) so corresponding sysfs entry would not
exist. Also I don't think we should assume devid2devnm implementation won't
change one day. I just did it safe way, otherwise static code analyzers
would complain.
Tomek
^ permalink raw reply
* [PATCH v2] Monitor: don't assume mdadm parameter is a block device
From: Tomasz Majchrzak @ 2017-06-19 9:19 UTC (permalink / raw)
To: linux-raid; +Cc: jes.sorensen, zlliu, Tomasz Majchrzak
In-Reply-To: <20170619091658.GA9305@proton.igk.intel.com>
If symlink (e.g. /dev/md/raid) is passed as a parameter to mdadm --wait,
it fails as it's not able to find a corresponding entry in /proc/mdstat
output. Get parameter file major:minor and look for block device name in
sysfs. This commit is partial revert of commit 9e04ac1c43e6
("mdadm/util: unify stat checking blkdev into function").
Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
Monitor.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
v2:
corrected referenced commit title in commit message
diff --git a/Monitor.c b/Monitor.c
index bef2f1b..c9f24bd 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -982,12 +982,21 @@ static void link_containers_with_subarrays(struct state *list)
int Wait(char *dev)
{
char devnm[32];
+ dev_t rdev;
+ char *tmp;
int rv = 1;
int frozen_remaining = 3;
- if (!stat_is_blkdev(dev, NULL))
+ if (!stat_is_blkdev(dev, &rdev))
+ return 2;
+
+ tmp = devid2devnm(rdev);
+ if (!tmp) {
+ pr_err("Cannot get md device name.\n");
return 2;
- strcpy(devnm, dev);
+ }
+
+ strcpy(devnm, tmp);
while(1) {
struct mdstat_ent *ms = mdstat_read(1, 0);
--
1.8.3.1
^ permalink raw reply related
* [PATCH] fs: buffer: Modify alloc_page_buffers.
From: Sean Fu @ 2017-06-19 13:01 UTC (permalink / raw)
To: viro
Cc: shli, anton, jack, axboe, ebiggers, rpeterso, bmarzins,
linux-raid, linux-kernel, linux-fsdevel, linux-ntfs-dev, Sean Fu
Make alloc_page_buffers support circular buffer list and initialise
b_state field.
Optimize the performance by removing the buffer list traversal to create
circular buffer list.
Signed-off-by: Sean Fu <fxinrong@gmail.com>
---
drivers/md/bitmap.c | 2 +-
fs/buffer.c | 37 +++++++++++++++----------------------
fs/ntfs/aops.c | 2 +-
fs/ntfs/mft.c | 2 +-
include/linux/buffer_head.h | 2 +-
5 files changed, 19 insertions(+), 26 deletions(-)
diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index f4eace5..615a46e 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -367,7 +367,7 @@ static int read_page(struct file *file, unsigned long index,
pr_debug("read bitmap file (%dB @ %llu)\n", (int)PAGE_SIZE,
(unsigned long long)index << PAGE_SHIFT);
- bh = alloc_page_buffers(page, 1<<inode->i_blkbits, 0);
+ bh = alloc_page_buffers(page, 1<<inode->i_blkbits, 0, 0, 0);
if (!bh) {
ret = -ENOMEM;
goto out;
diff --git a/fs/buffer.c b/fs/buffer.c
index 161be58..8111eca 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -864,9 +864,9 @@ int remove_inode_buffers(struct inode *inode)
* which may not fail from ordinary buffer allocations.
*/
struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
- int retry)
+ int retry, int circular, unsigned long b_state)
{
- struct buffer_head *bh, *head;
+ struct buffer_head *bh, *head, *tail;
long offset;
try_again:
@@ -879,13 +879,21 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
bh->b_this_page = head;
bh->b_blocknr = -1;
- head = bh;
+ if (head == NULL)
+ tail = bh;
+
+ head = bh;
+ bh->b_state = b_state;
bh->b_size = size;
/* Link the buffer to its page */
set_bh_page(bh, page, offset);
}
+
+ if (circular)
+ tail->b_this_page = head;
+
return head;
/*
* In case anything failed, we just free everything we got.
@@ -922,14 +930,6 @@ EXPORT_SYMBOL_GPL(alloc_page_buffers);
static inline void
link_dev_buffers(struct page *page, struct buffer_head *head)
{
- struct buffer_head *bh, *tail;
-
- bh = head;
- do {
- tail = bh;
- bh = bh->b_this_page;
- } while (bh);
- tail->b_this_page = head;
attach_page_buffers(page, head);
}
@@ -1024,7 +1024,7 @@ grow_dev_page(struct block_device *bdev, sector_t block,
/*
* Allocate some buffers for this page
*/
- bh = alloc_page_buffers(page, size, 0);
+ bh = alloc_page_buffers(page, size, 0, 1, 0);
if (!bh)
goto failed;
@@ -1578,16 +1578,9 @@ EXPORT_SYMBOL(block_invalidatepage);
void create_empty_buffers(struct page *page,
unsigned long blocksize, unsigned long b_state)
{
- struct buffer_head *bh, *head, *tail;
+ struct buffer_head *bh, *head;
- head = alloc_page_buffers(page, blocksize, 1);
- bh = head;
- do {
- bh->b_state |= b_state;
- tail = bh;
- bh = bh->b_this_page;
- } while (bh);
- tail->b_this_page = head;
+ head = alloc_page_buffers(page, blocksize, 1, 1, b_state);
spin_lock(&page->mapping->private_lock);
if (PageUptodate(page) || PageDirty(page)) {
@@ -2642,7 +2635,7 @@ int nobh_write_begin(struct address_space *mapping,
* Be careful: the buffer linked list is a NULL terminated one, rather
* than the circular one we're used to.
*/
- head = alloc_page_buffers(page, blocksize, 0);
+ head = alloc_page_buffers(page, blocksize, 0, 0, 0);
if (!head) {
ret = -ENOMEM;
goto out_release;
diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
index cc91856..e692142 100644
--- a/fs/ntfs/aops.c
+++ b/fs/ntfs/aops.c
@@ -1739,7 +1739,7 @@ void mark_ntfs_record_dirty(struct page *page, const unsigned int ofs) {
spin_lock(&mapping->private_lock);
if (unlikely(!page_has_buffers(page))) {
spin_unlock(&mapping->private_lock);
- bh = head = alloc_page_buffers(page, bh_size, 1);
+ bh = head = alloc_page_buffers(page, bh_size, 1, 0, 0);
spin_lock(&mapping->private_lock);
if (likely(!page_has_buffers(page))) {
struct buffer_head *tail;
diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
index b6f4021..175a02b 100644
--- a/fs/ntfs/mft.c
+++ b/fs/ntfs/mft.c
@@ -507,7 +507,7 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const unsigned long mft_no,
if (unlikely(!page_has_buffers(page))) {
struct buffer_head *tail;
- bh = head = alloc_page_buffers(page, blocksize, 1);
+ bh = head = alloc_page_buffers(page, blocksize, 1, 0, 0);
do {
set_buffer_uptodate(bh);
tail = bh;
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index bd029e52..9a29826 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -155,7 +155,7 @@ void set_bh_page(struct buffer_head *bh,
struct page *page, unsigned long offset);
int try_to_free_buffers(struct page *);
struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
- int retry);
+ int retry, int circular, unsigned long b_state);
void create_empty_buffers(struct page *, unsigned long,
unsigned long b_state);
void end_buffer_read_sync(struct buffer_head *bh, int uptodate);
--
2.6.2
^ permalink raw reply related
* Re: [PATCH] fs: buffer: Modify alloc_page_buffers.
From: Jan Kara @ 2017-06-19 13:21 UTC (permalink / raw)
To: Sean Fu
Cc: viro, shli, anton, jack, axboe, ebiggers, rpeterso, bmarzins,
linux-raid, linux-kernel, linux-fsdevel, linux-ntfs-dev
In-Reply-To: <1497877296-22441-1-git-send-email-fxinrong@gmail.com>
On Mon 19-06-17 21:01:36, Sean Fu wrote:
> Make alloc_page_buffers support circular buffer list and initialise
> b_state field.
> Optimize the performance by removing the buffer list traversal to create
> circular buffer list.
>
> Signed-off-by: Sean Fu <fxinrong@gmail.com>
IMHO this has unmeasurable performance gain and complicates the code. So I
don't think this is really worth it...
Honza
> ---
> drivers/md/bitmap.c | 2 +-
> fs/buffer.c | 37 +++++++++++++++----------------------
> fs/ntfs/aops.c | 2 +-
> fs/ntfs/mft.c | 2 +-
> include/linux/buffer_head.h | 2 +-
> 5 files changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index f4eace5..615a46e 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -367,7 +367,7 @@ static int read_page(struct file *file, unsigned long index,
> pr_debug("read bitmap file (%dB @ %llu)\n", (int)PAGE_SIZE,
> (unsigned long long)index << PAGE_SHIFT);
>
> - bh = alloc_page_buffers(page, 1<<inode->i_blkbits, 0);
> + bh = alloc_page_buffers(page, 1<<inode->i_blkbits, 0, 0, 0);
> if (!bh) {
> ret = -ENOMEM;
> goto out;
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 161be58..8111eca 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -864,9 +864,9 @@ int remove_inode_buffers(struct inode *inode)
> * which may not fail from ordinary buffer allocations.
> */
> struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> - int retry)
> + int retry, int circular, unsigned long b_state)
> {
> - struct buffer_head *bh, *head;
> + struct buffer_head *bh, *head, *tail;
> long offset;
>
> try_again:
> @@ -879,13 +879,21 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>
> bh->b_this_page = head;
> bh->b_blocknr = -1;
> - head = bh;
>
> + if (head == NULL)
> + tail = bh;
> +
> + head = bh;
> + bh->b_state = b_state;
> bh->b_size = size;
>
> /* Link the buffer to its page */
> set_bh_page(bh, page, offset);
> }
> +
> + if (circular)
> + tail->b_this_page = head;
> +
> return head;
> /*
> * In case anything failed, we just free everything we got.
> @@ -922,14 +930,6 @@ EXPORT_SYMBOL_GPL(alloc_page_buffers);
> static inline void
> link_dev_buffers(struct page *page, struct buffer_head *head)
> {
> - struct buffer_head *bh, *tail;
> -
> - bh = head;
> - do {
> - tail = bh;
> - bh = bh->b_this_page;
> - } while (bh);
> - tail->b_this_page = head;
> attach_page_buffers(page, head);
> }
>
> @@ -1024,7 +1024,7 @@ grow_dev_page(struct block_device *bdev, sector_t block,
> /*
> * Allocate some buffers for this page
> */
> - bh = alloc_page_buffers(page, size, 0);
> + bh = alloc_page_buffers(page, size, 0, 1, 0);
> if (!bh)
> goto failed;
>
> @@ -1578,16 +1578,9 @@ EXPORT_SYMBOL(block_invalidatepage);
> void create_empty_buffers(struct page *page,
> unsigned long blocksize, unsigned long b_state)
> {
> - struct buffer_head *bh, *head, *tail;
> + struct buffer_head *bh, *head;
>
> - head = alloc_page_buffers(page, blocksize, 1);
> - bh = head;
> - do {
> - bh->b_state |= b_state;
> - tail = bh;
> - bh = bh->b_this_page;
> - } while (bh);
> - tail->b_this_page = head;
> + head = alloc_page_buffers(page, blocksize, 1, 1, b_state);
>
> spin_lock(&page->mapping->private_lock);
> if (PageUptodate(page) || PageDirty(page)) {
> @@ -2642,7 +2635,7 @@ int nobh_write_begin(struct address_space *mapping,
> * Be careful: the buffer linked list is a NULL terminated one, rather
> * than the circular one we're used to.
> */
> - head = alloc_page_buffers(page, blocksize, 0);
> + head = alloc_page_buffers(page, blocksize, 0, 0, 0);
> if (!head) {
> ret = -ENOMEM;
> goto out_release;
> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
> index cc91856..e692142 100644
> --- a/fs/ntfs/aops.c
> +++ b/fs/ntfs/aops.c
> @@ -1739,7 +1739,7 @@ void mark_ntfs_record_dirty(struct page *page, const unsigned int ofs) {
> spin_lock(&mapping->private_lock);
> if (unlikely(!page_has_buffers(page))) {
> spin_unlock(&mapping->private_lock);
> - bh = head = alloc_page_buffers(page, bh_size, 1);
> + bh = head = alloc_page_buffers(page, bh_size, 1, 0, 0);
> spin_lock(&mapping->private_lock);
> if (likely(!page_has_buffers(page))) {
> struct buffer_head *tail;
> diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
> index b6f4021..175a02b 100644
> --- a/fs/ntfs/mft.c
> +++ b/fs/ntfs/mft.c
> @@ -507,7 +507,7 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const unsigned long mft_no,
> if (unlikely(!page_has_buffers(page))) {
> struct buffer_head *tail;
>
> - bh = head = alloc_page_buffers(page, blocksize, 1);
> + bh = head = alloc_page_buffers(page, blocksize, 1, 0, 0);
> do {
> set_buffer_uptodate(bh);
> tail = bh;
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index bd029e52..9a29826 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -155,7 +155,7 @@ void set_bh_page(struct buffer_head *bh,
> struct page *page, unsigned long offset);
> int try_to_free_buffers(struct page *);
> struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> - int retry);
> + int retry, int circular, unsigned long b_state);
> void create_empty_buffers(struct page *, unsigned long,
> unsigned long b_state);
> void end_buffer_read_sync(struct buffer_head *bh, int uptodate);
> --
> 2.6.2
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH] fs: buffer: Modify alloc_page_buffers.
From: Al Viro @ 2017-06-19 16:03 UTC (permalink / raw)
To: Sean Fu
Cc: shli, anton, jack, axboe, ebiggers, rpeterso, bmarzins,
linux-raid, linux-kernel, linux-fsdevel, linux-ntfs-dev
In-Reply-To: <1497877296-22441-1-git-send-email-fxinrong@gmail.com>
On Mon, Jun 19, 2017 at 09:01:36PM +0800, Sean Fu wrote:
> Make alloc_page_buffers support circular buffer list and initialise
> b_state field.
> Optimize the performance by removing the buffer list traversal to create
> circular buffer list.
> - bh = head = alloc_page_buffers(page, blocksize, 1);
> + bh = head = alloc_page_buffers(page, blocksize, 1, 0, 0);
Frankly, I don't like that change of calling conventions; it's very easy to
mess the order of arguments when using interfaces like that and it's hell
to find when trying to debug the resulting mess.
Do you really get an observable change in performance? What loads are
triggering it?
^ permalink raw reply
* Re: [PATCH] fs: buffer: Modify alloc_page_buffers.
From: kbuild test robot @ 2017-06-19 18:20 UTC (permalink / raw)
Cc: kbuild-all, viro, shli, anton, jack, axboe, ebiggers, rpeterso,
bmarzins, linux-raid, linux-kernel, linux-fsdevel, linux-ntfs-dev,
Sean Fu
In-Reply-To: <1497877296-22441-1-git-send-email-fxinrong@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]
Hi Sean,
[auto build test WARNING on linus/master]
[also build test WARNING on v4.12-rc6 next-20170619]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sean-Fu/fs-buffer-Modify-alloc_page_buffers/20170620-012328
config: x86_64-randconfig-x015-201725 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
fs/buffer.c: In function 'alloc_page_buffers':
>> fs/buffer.c:895:21: warning: 'tail' may be used uninitialized in this function [-Wmaybe-uninitialized]
tail->b_this_page = head;
~~~~~~~~~~~~~~~~~~^~~~~~
vim +/tail +895 fs/buffer.c
879
880 bh->b_this_page = head;
881 bh->b_blocknr = -1;
882
883 if (head == NULL)
884 tail = bh;
885
886 head = bh;
887 bh->b_state = b_state;
888 bh->b_size = size;
889
890 /* Link the buffer to its page */
891 set_bh_page(bh, page, offset);
892 }
893
894 if (circular)
> 895 tail->b_this_page = head;
896
897 return head;
898 /*
899 * In case anything failed, we just free everything we got.
900 */
901 no_grow:
902 if (head) {
903 do {
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28003 bytes --]
^ permalink raw reply
* Re: Reply Urgent
From: INFO @ 2017-06-19 19:39 UTC (permalink / raw)
Hello,
How are you doing? I have been sent to inform you that, We have an
inheritance of a deceased client with your surname. Contact Mr Andrew
Bailey Reply Email To: myinfand@gmail.com with your "Full Names" for
more info. Thanks for your understanding.
Reply ASAP thank you.
Melissa.
----------------------------------
Correo Corporativo Hospital Universitario del Valle E.S.E
*******************************************************************************
"Estamos re-dimensionandonos para crecer!"
******************************************************************************
^ permalink raw reply
* RE: mismatch_cnt > 0 during initial sync?
From: Peter Sangas @ 2017-06-19 21:54 UTC (permalink / raw)
To: 'NeilBrown', 'Stephane Thiell', linux-raid
In-Reply-To: <87shix3rpi.fsf@notabene.neil.brown.name>
> From: NeilBrown [mailto:neilb@suse.com]
> Sent: Sunday, June 18, 2017 2:35 PM
> Subject: Re: mismatch_cnt > 0 during initial sync?
>
>
> From the perspective of md, the initial sync is no different from any
other sync. It
> will count the number of mismatches that it finds and fixes.
>
Should a sync always fix a mismatch it encounters? I have a RAID1 with 3
disks. Sometimes I need to replace one disk and after adding a replacement
disk syslog indicates "RebuildFinished event detected on md device
/dev/md/2, component device mismatches found: 256 (on raid level 1)" but
says nothing about fixing it.
cat /sys/block/md2/md/last_sync_action
recovery
mdadm -V
mdadm - v3.3 - 3rd September 2013
Thank you,
Pete
^ permalink raw reply
* Re: [PATCH] Monitor: don't assume mdadm parameter is a block device
From: Zhilong Liu @ 2017-06-20 0:30 UTC (permalink / raw)
To: Tomasz Majchrzak; +Cc: linux-raid, jes.sorensen
In-Reply-To: <20170619091658.GA9305@proton.igk.intel.com>
> 在 2017年6月19日，17:16，Tomasz Majchrzak <tomasz.majchrzak@intel.com> 写道：
>
>> On Sun, Jun 18, 2017 at 10:10:02PM +0800, Zhilong Liu wrote:
>>
>>
>>> 在 2017年6月16日，22:02，Tomasz Majchrzak <tomasz.majchrzak@intel.com> 写道：
>>>
>>> If symlink (e.g. /dev/md/raid) is passed as a parameter to mdadm --wait,
>>> it fails as it's not able to find a corresponding entry in /proc/mdstat
>>> output. Get parameter file major:minor and look for block device name in
>>> sysfs. This commit is partial revert of commit 9e04ac1c43e6
>>> ("mdadm/util: unify fstat checking blkdev into function").
>>>
>>
>> Thanks very much for catching this. Just remind a typo in (commit title), correct fstat as stat.
>
> Will do.
>
>>
>>> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
>>> ---
>>> Monitor.c | 13 +++++++++++--
>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> v2:
>>> next Zhilong Liu commit id
>>>
>>> diff --git a/Monitor.c b/Monitor.c
>>> index bef2f1b..c9f24bd 100644
>>> --- a/Monitor.c
>>> +++ b/Monitor.c
>>> @@ -982,12 +982,21 @@ static void link_containers_with_subarrays(struct state *list)
>>> int Wait(char *dev)
>>> {
>>> char devnm[32];
>>> + dev_t rdev;
>>> + char *tmp;
>>> int rv = 1;
>>> int frozen_remaining = 3;
>>>
>>> - if (!stat_is_blkdev(dev, NULL))
>>> + if (!stat_is_blkdev(dev, &rdev))
>>> + return 2;
>>> +
>>> + tmp = devid2devnm(rdev);
>>> + if (!tmp) {
>>> + pr_err("Cannot get md device name.\n");
>>> return 2;
>>> - strcpy(devnm, dev);
>>> + }
>>> +
>>> + strcpy(devnm, tmp);
>>>
>>
>> Quite agreed with the code here, just a small question, I may use
>>> strcpy(devnm, devid2devnm(rdev));
>> directly since stat_is_blkdev() has returned the major/minor devid. I don't know "get md device name " would be failed in which scenario.
>> I'm asking this question since I haven't been familiar with all raids situations. :-)
>>
>> Thanks again,
>> Zhilong
>
> Well, there could be a race and RAID array might get stopped between two
> calls (stat_is_blkdev, devid2devnm) so corresponding sysfs entry would not
> exist. Also I don't think we should assume devid2devnm implementation won't
> change one day. I just did it safe way, otherwise static code analyzers
> would complain.
>
Thanks very much for the correction. :-)
Zhilong
> Tomek
>
^ permalink raw reply
* Re: [PATCH] md/bitmap: don't read page from device with Bitmap_sync
From: Shaohua Li @ 2017-06-20 0:41 UTC (permalink / raw)
To: NeilBrown; +Cc: Guoqing Jiang, linux-raid
In-Reply-To: <87h8zc51t0.fsf@notabene.neil.brown.name>
On Mon, Jun 19, 2017 at 09:11:23AM +1000, Neil Brown wrote:
> On Fri, Jun 16 2017, Shaohua Li wrote:
>
> > On Fri, Jun 16, 2017 at 05:19:27PM +0800, Guoqing Jiang wrote:
> >> The device owns Bitmap_sync flag needs recovery
> >> to become in sync, and read page from this type
> >> device could get stale status.
> >>
> >> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> >> ---
> >> When develop for clustered raid10 feature, if add a
> >> disk under grow mode in master node, I could get
> >> the "bitmap superblock UUID mismatch" warning due
> >> to the page is read from Bitmap_sync device.
> >>
> >> drivers/md/bitmap.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> >> index bf7419a..bf34cd8 100644
> >> --- a/drivers/md/bitmap.c
> >> +++ b/drivers/md/bitmap.c
> >> @@ -156,7 +156,8 @@ static int read_sb_page(struct mddev *mddev, loff_t offset,
> >>
> >> rdev_for_each(rdev, mddev) {
> >> if (! test_bit(In_sync, &rdev->flags)
> >
> > Why In_sync is set for the rdev? I think it shoudn't.
>
> There are several states a device can be in.
> If ->raid_disk is < 0, then the device is a spare, and doesn't contain
> and valid data.
> Otherwise ->raid_disk >=0 and:
> In_sync is clear, which means that blocks before ->recovery_offset
> contain valid data, and blocks after there don't
> In_sync is set, which means ->recovery_offset is irrelevant and
> should be treated as MaxSector
> Bitmap_sync is set, which could apply in either of the above cases,
> and means blocks corresponding to bits that are set in the bitmap
> may not be up to date.
>
> Bitmap_sync is only relevant before a device has been handed to the
> personality. After it has been added, ->recovery_cp ensures that the
> blocks that might be wrong are not read until until the bitmap-based
> recovery has fixed them up.
>
> Bitmap_sync is only use to stop the device from being given to the
> personality if a recovery won't be started because the array is
> read-only.
>
> So it is perfectly valid for both In_sync and Bitmap_sync to be set.
> If they are, it makes sense to avoid reading bitmap information from the
> Bitmap_sync device, as that will be out-of-date.
>
> I'm not quite sure why Guoqing is getting a UUID mismatch ... it
> suggests that the new device wasn't initialized properly. So there
> might be another bug. But I think this is definitely a bug.
Can you describe a scenario a disk has both bits set? I had hard time to
imagine it.
md_update_sb will update super and bitmap super at almost the same time. It's
possible we update super but not bitmap super and there is power loss. Is this
the case we have the both bits set? There seems no mechanism to prevent the
first disk has older bitmap super than the second disk in this scenario.
> >
> >> - || test_bit(Faulty, &rdev->flags))
> >> + || test_bit(Faulty, &rdev->flags)
> >> + || test_bit(Bitmap_sync, &rdev->flags))
> >
> > I didn't see code clears the Bitmap_sync bit after disk is synced, so likely
> > there is bug in this side.
>
> There is no need to clear Bitmap_sync. It stays set until it becomes
> irrelevant.
Hmm, if we use in this way, we should add comments there to declare this bit
can only be used very early.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH] md: make suspend range wait timed out
From: Shaohua Li @ 2017-06-20 0:54 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, Shaohua Li, Mikulas Patocka
In-Reply-To: <87vant3rw5.fsf@notabene.neil.brown.name>
On Mon, Jun 19, 2017 at 07:30:50AM +1000, Neil Brown wrote:
> On Fri, Jun 16 2017, Shaohua Li wrote:
>
> > On Fri, Jun 16, 2017 at 01:26:00PM +1000, Neil Brown wrote:
> >> On Fri, Jun 09 2017, Shaohua Li wrote:
> >>
> >> > From: Shaohua Li <shli@fb.com>
> >> >
> >> > suspend range is controlled by userspace. If userspace doesn't clear suspend
> >> > range, it's possible a thread will wait for the range forever, we can't even
> >> > kill it. This is bad behavior. Add a timeout in the wait. If timeout happens,
> >> > we return IO error. The app controlling suspend range looks like part of disk
> >> > firmware, if disk isn't responded for a long time, timed out IO error is
> >> > returned.
> >> >
> >> > A simple search in SCSI code shows maximum IO timeout is 120s, so I use this
> >> > value here too.
> >>
> >> I really don't like this. It is an API change with no really
> >> justification. Has the current behavior caused a problem?
> >
> > This centainly causes problem. Set the suspend range will make application
> > stall for ever, don't you think this is a problem?
>
> I agree that it could cause a problem. I'm asking it is actually, in
> practice, causes a problem. Do you have reports from people saying "the
> IO to my RAID array is hanging, what is wrong?" and you look into it and
> find out that suspend_hi is larger than suspend_lo?
>
> And if that does happen, is this really the best way to fix it?
I don't have reports about this issue. Just think the behavior is bad.
> >
> >> Both md and dm export APIs which allow IO to be suspended while
> >> changes are made. Changing that to a timed-out period needs, I think,
> >> to be clearly justified.
> >>
> >> If it is changed to a timed-out period, then that should be explicit,
> >> rather than having each request independently time out.
> >> i.e. when the suspend is initiated, the end-time should be computed, and
> >> any IO would block until that time, not block for some number of
> >> seconds.
> >
> > Ok, this makes sense. We can add a timeout. If it's expired, we clear suspend
> > range. Userspace should know what they are doing.
> >
> >>
> >> If an md device is left suspended, then the current code will block IO
> >> indefinitely. This patch will at a 20minute times to every single
> >> request, which will mean IO proceeds, but extremely slowly. I don't see
> >> that as a useful improvement.
> >
> > It returns error, so application will not dispatch more IO. But I agree a
> > timeout to clear the suspend looks a better policy.
>
> Write errors only get back to the application if it calls fsync(), and
> many don't do that. Write errors can easily cause a filesystem to go
> read-only, and require an fsck. I think we should be very cautious
> about triggering write errors.
>
> NFS will hang indefinitely rather then return an error if the server is
> not available. That can certainly be annoying, but the alternative has
> been tried, and it leads to random data corruption.
> The two cases are only comparable at a very high level, but I think
> this result should encourage substantial caution.
It's hard to say if an IO error or an infinite wait is better, but since there
is better option in this case, I don't want to argue. I'll repost a patch to
reset suspend range after a timeout, assume this is your suggestion.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH] md/bitmap: don't read page from device with Bitmap_sync
From: NeilBrown @ 2017-06-20 3:59 UTC (permalink / raw)
To: Shaohua Li; +Cc: Guoqing Jiang, linux-raid
In-Reply-To: <20170620004158.lrry5ig5epvmcqfc@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 4794 bytes --]
On Mon, Jun 19 2017, Shaohua Li wrote:
> On Mon, Jun 19, 2017 at 09:11:23AM +1000, Neil Brown wrote:
>> On Fri, Jun 16 2017, Shaohua Li wrote:
>>
>> > On Fri, Jun 16, 2017 at 05:19:27PM +0800, Guoqing Jiang wrote:
>> >> The device owns Bitmap_sync flag needs recovery
>> >> to become in sync, and read page from this type
>> >> device could get stale status.
>> >>
>> >> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
>> >> ---
>> >> When develop for clustered raid10 feature, if add a
>> >> disk under grow mode in master node, I could get
>> >> the "bitmap superblock UUID mismatch" warning due
>> >> to the page is read from Bitmap_sync device.
>> >>
>> >> drivers/md/bitmap.c | 3 ++-
>> >> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
>> >> index bf7419a..bf34cd8 100644
>> >> --- a/drivers/md/bitmap.c
>> >> +++ b/drivers/md/bitmap.c
>> >> @@ -156,7 +156,8 @@ static int read_sb_page(struct mddev *mddev, loff_t offset,
>> >>
>> >> rdev_for_each(rdev, mddev) {
>> >> if (! test_bit(In_sync, &rdev->flags)
>> >
>> > Why In_sync is set for the rdev? I think it shoudn't.
>>
>> There are several states a device can be in.
>> If ->raid_disk is < 0, then the device is a spare, and doesn't contain
>> and valid data.
>> Otherwise ->raid_disk >=0 and:
>> In_sync is clear, which means that blocks before ->recovery_offset
>> contain valid data, and blocks after there don't
>> In_sync is set, which means ->recovery_offset is irrelevant and
>> should be treated as MaxSector
>> Bitmap_sync is set, which could apply in either of the above cases,
>> and means blocks corresponding to bits that are set in the bitmap
>> may not be up to date.
>>
>> Bitmap_sync is only relevant before a device has been handed to the
>> personality. After it has been added, ->recovery_cp ensures that the
>> blocks that might be wrong are not read until until the bitmap-based
>> recovery has fixed them up.
>>
>> Bitmap_sync is only use to stop the device from being given to the
>> personality if a recovery won't be started because the array is
>> read-only.
>>
>> So it is perfectly valid for both In_sync and Bitmap_sync to be set.
>> If they are, it makes sense to avoid reading bitmap information from the
>> Bitmap_sync device, as that will be out-of-date.
>>
>> I'm not quite sure why Guoqing is getting a UUID mismatch ... it
>> suggests that the new device wasn't initialized properly. So there
>> might be another bug. But I think this is definitely a bug.
>
> Can you describe a scenario a disk has both bits set? I had hard time to
> imagine it.
An array with a bitmap has a device fail (cable break?).
So the bitmap stops clearing bits and ->events_cleared stays
where it was, with the same value as events in the superblock
of the failed device.
The device then starts working (cable is replaced) and
it device is added back to the array.
super_1_validate notices that the array has a bitmap and that the event
count in the new device is >= mddev->bitmap->events_cleared, but <
mddev->events.
i.e. it is in the range for which a bitmap-based recovery is both
possible and needed.
So it sets Bitmap_sync.
Then it notices that the 'role' in a valid role for the array,
so that role is stored in rdev->saved_raid_disk and, as there is no
MD_FEATURE_RECOVERY_OFFSET, In_sync is set.
A similar sequence can happen in super_90_validate.
>
> md_update_sb will update super and bitmap super at almost the same time. It's
> possible we update super but not bitmap super and there is power loss. Is this
> the case we have the both bits set? There seems no mechanism to prevent the
> first disk has older bitmap super than the second disk in this scenario.
This isn't really about the superblock and bitmap on the one device
being in sync or not.
It is the superblock on one (failed) device having a different event
count than the bitmap (on the other devices) that is actively being used
by the array.
>
>> >
>> >> - || test_bit(Faulty, &rdev->flags))
>> >> + || test_bit(Faulty, &rdev->flags)
>> >> + || test_bit(Bitmap_sync, &rdev->flags))
>> >
>> > I didn't see code clears the Bitmap_sync bit after disk is synced, so likely
>> > there is bug in this side.
>>
>> There is no need to clear Bitmap_sync. It stays set until it becomes
>> irrelevant.
>
> Hmm, if we use in this way, we should add comments there to declare this bit
> can only be used very early.
Certainly would be appropriate to document that it is only meaningful
before the device has been passed to ->hot_add_disk().
Thanks,
NeilBrown
>
> Thanks,
> Shaohua
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* RE: mismatch_cnt > 0 during initial sync?
From: NeilBrown @ 2017-06-20 4:14 UTC (permalink / raw)
To: Peter Sangas, 'Stephane Thiell', linux-raid
In-Reply-To: <009901d2e946$a3eff400$ebcfdc00$@wnsdev.com>
[-- Attachment #1: Type: text/plain, Size: 2556 bytes --]
On Mon, Jun 19 2017, Peter Sangas wrote:
>> From: NeilBrown [mailto:neilb@suse.com]
>> Sent: Sunday, June 18, 2017 2:35 PM
>> Subject: Re: mismatch_cnt > 0 during initial sync?
>>
>>
>> From the perspective of md, the initial sync is no different from any
> other sync. It
>> will count the number of mismatches that it finds and fixes.
>>
>
> Should a sync always fix a mismatch it encounters? I have a RAID1 with 3
> disks. Sometimes I need to replace one disk and after adding a replacement
> disk syslog indicates "RebuildFinished event detected on md device
> /dev/md/2, component device mismatches found: 256 (on raid level 1)" but
> says nothing about fixing it.
No, it wouldn't say anything about fixing things. That is assumed.
>
> cat /sys/block/md2/md/last_sync_action
> recovery
This is a recovery, not a resync. They are different.
Recovery is when you add a device to an array, and the data that should
be there is recovered from elsewhere.
Resync is when the redundancy in the array might be compromised, so it
is repaired, possibly by read-check-maybe_write. Possibly by
read-write.
For raid1, recovery and resync and require similar. For raid5 they are
very different.
The mismatch count is only reset when a resync starts, not when a
recovery (or reshape) starts. So mdadm shouldn't really report the
mismatches when the recovery finishes. The number is left over from the
most recent resync.
raid1 only counts when a resync is requested, either by writing "check"
or "repair" to the sync_action file in sysfs. An automatic resync after
and unclean shutdown (or when array is started) just copies blocks
without checking, so it has nothing to count.
"repair" repairs any inconsistencies found, "check" doesn't. Both count
inconsistencies.
raid5/raid6 counts for resync, check, and repair (but not for recover or
reshape).
By default, when the array is created, raid5 performs a recovery,
nominating one of the devices to be the "spare" with the others assumed
to have "correct" data. This is faster than assuming they are all
"correct", and performing a resync.
RAID6 (the array used in the original question) does resync, rather than
recovery, on initial creation - because 2-drive recovery is/was thought
to have performance issues.
So: mdadm should be modified to not report "mismatches found" is
"last_sync_action" is recovery or reshape.
Thanks,
NeilBrown
>
> mdadm -V
> mdadm - v3.3 - 3rd September 2013
>
>
> Thank you,
> Pete
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* (unknown),
From: xa0ajutor @ 2017-06-20 6:29 UTC (permalink / raw)
To: linux-raid
[-- Attachment #1: 3786494.zip --]
[-- Type: application/zip, Size: 5133 bytes --]
^ permalink raw reply
* caculate chunk size for the future
From: d tbsky @ 2017-06-20 16:43 UTC (permalink / raw)
To: linux-raid
hi:
I have a 4 disk software raid6. I found chunk size 512k give me
best performance for my workload.
if I grow up the raid to 32 disks, may I assume chunk size 64K
will give me similar performance? or there will other impact when I
grow the disks in the future?
I saw discussion about internal bitmap hit the performance for 20
disks raid10. is raid5/6 ok under the same situation?
thanks a lot for help!
Regards,
tbskyd
^ permalink raw reply
* Re: 9 second recovery when re-adding a drive that got kicked out?
From: Marc MERLIN @ 2017-06-20 18:27 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid@vger.kernel.org
In-Reply-To: <87r2yxdazc.fsf@notabene.neil.brown.name>
On Tue, Jun 06, 2017 at 01:57:27PM +1000, NeilBrown wrote:
> Had you run "mdadm --examine-bitmap /dev/sdk1" before the re-add, it
> would have told you how many bits were set at that time.
>
> That "x/y pages" information never should have appeared in /proc/mdstat
> - it is really just of interest to developers. But it is there now, so
> removing it is awkward.
So, I got the problem again, re-added a drive that had just been missing
for maybe 2mn, and this time I'm getting a very long (but not full)
recovery:
gargamel:~# mdadm --examine-bitmap /dev/sdh1
Filename : /dev/sdh1
Magic : 6d746962
Version : 4
UUID : 589f1176:8ee48905:d102340b:23f98ca1
Events : 11588
Events Cleared : 10625
State : OK
Chunksize : 64 MB
Daemon : 5s flush period
Write Mode : Normal
Sync Size : 1953380928 (1862.89 GiB 2000.26 GB)
Bitmap : 29807 bits (chunks), 267 dirty (0.9%)
0.9% dirty still gives me a 5H recovery?
md8 : active raid5 sdi1[5] sdd1[0] sdh1[3] sdg1[2] sdf1[1]
7813523712 blocks super 1.2 level 5, 64k chunk, algorithm 2 [5/4] [UUUU_]
[==>..................] recovery = 12.4% (243429660/1953380928) finish=297.6min speed=95741K/sec
bitmap: 5/15 pages [20KB], 65536KB chunk
gargamel:~# mdadm --query --detail /dev/md8
/dev/md8:
Version : 1.2
Creation Time : Sun May 14 08:59:13 2017
Raid Level : raid5
Array Size : 7813523712 (7451.56 GiB 8001.05 GB)
Used Dev Size : 1953380928 (1862.89 GiB 2000.26 GB)
Raid Devices : 5
Total Devices : 4
Persistence : Superblock is persistent
Intent Bitmap : Internal
Update Time : Tue Jun 20 11:01:02 2017
State : clean, degraded
Active Devices : 4
Working Devices : 4
Failed Devices : 0
Spare Devices : 0
Layout : left-symmetric
Chunk Size : 64K
Name : gargamel.svh.merlins.org:8 (local to host gargamel.svh.merlins.org)
UUID : 589f1176:8ee48905:d102340b:23f98ca1
Events : 11588
Number Major Minor RaidDevice State
0 8 49 0 active sync /dev/sdd1
1 8 81 1 active sync /dev/sdf1
2 8 97 2 active sync /dev/sdg1
3 8 113 3 active sync /dev/sdh1
- 0 0 4 removed
This was the drive that got kicked out (shown before it was re-added):
gargamel:~# mdadm --examine-bitmap /dev/sdi1
Filename : /dev/sdi1
Magic : 6d746962
Version : 4
UUID : 589f1176:8ee48905:d102340b:23f98ca1
Events : 10630
Events Cleared : 10625
State : OK
Chunksize : 64 MB
Daemon : 5s flush period
Write Mode : Normal
Sync Size : 1953380928 (1862.89 GiB 2000.26 GB)
Bitmap : 29807 bits (chunks), 0 dirty (0.0%)
Marc
--
"A mouse is a device used to point at the xterm you want to type in" - A.S.R.
Microsoft is to operating systems ....
.... what McDonalds is to gourmet cooking
Home page: http://marc.merlins.org/ | PGP 1024R/763BE901
^ permalink raw reply
* Re: 9 second recovery when re-adding a drive that got kicked out?
From: Marc MERLIN @ 2017-06-20 18:31 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid@vger.kernel.org
In-Reply-To: <20170620182745.rbufznqe4tuxqttq@merlins.org>
On Tue, Jun 20, 2017 at 11:27:45AM -0700, Marc MERLIN wrote:
> On Tue, Jun 06, 2017 at 01:57:27PM +1000, NeilBrown wrote:
> > Had you run "mdadm --examine-bitmap /dev/sdk1" before the re-add, it
> > would have told you how many bits were set at that time.
> >
> > That "x/y pages" information never should have appeared in /proc/mdstat
> > - it is really just of interest to developers. But it is there now, so
> > removing it is awkward.
>
> So, I got the problem again, re-added a drive that had just been missing
> for maybe 2mn, and this time I'm getting a very long (but not full)
> recovery:
Mmmh, this is puzzling.
The progress meter was wrong, it recovered in 3mn:
Jun 20 11:19:28 gargamel kernel: [ 916.007017] md: recovery of RAID array md8
Jun 20 11:22:41 gargamel kernel: [ 1108.395580] md: md8: recovery done.
Marc
--
"A mouse is a device used to point at the xterm you want to type in" - A.S.R.
Microsoft is to operating systems ....
.... what McDonalds is to gourmet cooking
Home page: http://marc.merlins.org/
^ permalink raw reply
* Re: 9 second recovery when re-adding a drive that got kicked out?
From: Roman Mamedov @ 2017-06-20 18:40 UTC (permalink / raw)
To: Marc MERLIN; +Cc: linux-raid@vger.kernel.org
In-Reply-To: <20170620183154.GA5303@merlins.org>
On Tue, 20 Jun 2017 11:31:54 -0700
Marc MERLIN <marc@merlins.org> wrote:
> On Tue, Jun 20, 2017 at 11:27:45AM -0700, Marc MERLIN wrote:
> > On Tue, Jun 06, 2017 at 01:57:27PM +1000, NeilBrown wrote:
> > > Had you run "mdadm --examine-bitmap /dev/sdk1" before the re-add, it
> > > would have told you how many bits were set at that time.
> > >
> > > That "x/y pages" information never should have appeared in /proc/mdstat
> > > - it is really just of interest to developers. But it is there now, so
> > > removing it is awkward.
> >
> > So, I got the problem again, re-added a drive that had just been missing
> > for maybe 2mn, and this time I'm getting a very long (but not full)
> > recovery:
>
> Mmmh, this is puzzling.
>
> The progress meter was wrong, it recovered in 3mn:
> Jun 20 11:19:28 gargamel kernel: [ 916.007017] md: recovery of RAID array md8
> Jun 20 11:22:41 gargamel kernel: [ 1108.395580] md: md8: recovery done.
Yes the progress bar in effect just visualizes the entire drive area, and
thanks to the bitmap md will do syncing only where it needs to, skipping over
the parts which are in sync. So the progress will instantly jump dozens of
percents (skipping hundreds of GBs), stopping only here and there for a while.
--
With respect,
Roman
^ permalink raw reply
* RE: mismatch_cnt > 0 during initial sync?
From: Peter Sangas @ 2017-06-20 20:23 UTC (permalink / raw)
To: 'NeilBrown', 'Stephane Thiell', linux-raid
In-Reply-To: <87lgon2t30.fsf@notabene.neil.brown.name>
> From: NeilBrown [mailto:neilb@suse.com]
> Sent: Monday, June 19, 2017 9:15 PM
> Subject: RE: mismatch_cnt > 0 during initial sync?
>
> This is a recovery, not a resync. They are different.
Thank you for pointing this out and the nice explanation.
From reading this list and other sources issuing a repair command as
follows:
"echo repair > /sys/block/md2/md/sync_action"
might be ill-advised since it's luck of the draw whether or not the
operation gets the right data instead of the bad data.
Is this correct?
For reference I have a RAID1 with 3 disks. mdadm - v3.3 - 3rd September
2013
Thank you,
Pete
^ permalink raw reply
* Re: 9 second recovery when re-adding a drive that got kicked out?
From: NeilBrown @ 2017-06-20 21:02 UTC (permalink / raw)
To: Marc MERLIN; +Cc: linux-raid@vger.kernel.org
In-Reply-To: <20170620183154.GA5303@merlins.org>
[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]
On Tue, Jun 20 2017, Marc MERLIN wrote:
> On Tue, Jun 20, 2017 at 11:27:45AM -0700, Marc MERLIN wrote:
>> On Tue, Jun 06, 2017 at 01:57:27PM +1000, NeilBrown wrote:
>> > Had you run "mdadm --examine-bitmap /dev/sdk1" before the re-add, it
>> > would have told you how many bits were set at that time.
>> >
>> > That "x/y pages" information never should have appeared in /proc/mdstat
>> > - it is really just of interest to developers. But it is there now, so
>> > removing it is awkward.
>>
>> So, I got the problem again, re-added a drive that had just been missing
>> for maybe 2mn, and this time I'm getting a very long (but not full)
>> recovery:
>
> Mmmh, this is puzzling.
>
> The progress meter was wrong, it recovered in 3mn:
> Jun 20 11:19:28 gargamel kernel: [ 916.007017] md: recovery of RAID array md8
> Jun 20 11:22:41 gargamel kernel: [ 1108.395580] md: md8: recovery done.
It is a progress bar - haven't you learned by now that they are *always*
wrong :-)
recovery always reports progress in sectors completed, and estimates
time based on how many sectors were processes in the last 30 seconds,
and how many are left.
With a bitmap based recovery, most sectors are handled very quickly
(instantly?), while some take milliseconds. That makes the estimate
imprecise.
NeilBrown
>
> Marc
> --
> "A mouse is a device used to point at the xterm you want to type in" - A.S.R.
> Microsoft is to operating systems ....
> .... what McDonalds is to gourmet cooking
> Home page: http://marc.merlins.org/
> --
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* RE: mismatch_cnt > 0 during initial sync?
From: NeilBrown @ 2017-06-20 21:26 UTC (permalink / raw)
To: Peter Sangas, 'Stephane Thiell', linux-raid
In-Reply-To: <002e01d2ea03$232cb660$69862320$@wnsdev.com>
[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]
On Tue, Jun 20 2017, Peter Sangas wrote:
>> From: NeilBrown [mailto:neilb@suse.com]
>> Sent: Monday, June 19, 2017 9:15 PM
>> Subject: RE: mismatch_cnt > 0 during initial sync?
>>
>> This is a recovery, not a resync. They are different.
>
> Thank you for pointing this out and the nice explanation.
>
> From reading this list and other sources issuing a repair command as
> follows:
>
> "echo repair > /sys/block/md2/md/sync_action"
>
> might be ill-advised since it's luck of the draw whether or not the
> operation gets the right data instead of the bad data.
>
> Is this correct?
How do you define "right data" and "bad data"?
What is your threat-model which explains how the two blocks are
different?
In most likely cases, neither version if the data is more correct than
the other. In some others, your hardware is broken and needs to be
replaced.
If you have no understanding of why there might be a difference, then
issuing a repair is not likely to be worse than not issuing a repair.
If you do have that understanding, then use that to make your decision.
NeilBrown
>
> For reference I have a RAID1 with 3 disks. mdadm - v3.3 - 3rd September
> 2013
>
> Thank you,
> Pete
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: 9 second recovery when re-adding a drive that got kicked out?
From: Marc MERLIN @ 2017-06-20 21:32 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid@vger.kernel.org
In-Reply-To: <87efue2x06.fsf@notabene.neil.brown.name>
[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]
On Wed, Jun 21, 2017 at 07:02:33AM +1000, NeilBrown wrote:
> > The progress meter was wrong, it recovered in 3mn:
> > Jun 20 11:19:28 gargamel kernel: [ 916.007017] md: recovery of RAID array md8
> > Jun 20 11:22:41 gargamel kernel: [ 1108.395580] md: md8: recovery done.
>
> It is a progress bar - haven't you learned by now that they are *always*
> wrong :-)
>
> recovery always reports progress in sectors completed, and estimates
> time based on how many sectors were processes in the last 30 seconds,
> and how many are left.
>
> With a bitmap based recovery, most sectors are handled very quickly
> (instantly?), while some take milliseconds. That makes the estimate
> imprecise.
Indeed.
Sorry that I jumped the gun, I think it looked like it was going to take
hours, and then it completed a few seconds after I had sent the Email.
Sorry for the noise, and obviously it's awesome that it worked so quickly
again
(been dealing with some unstable underlying SAS card that tends to kill
drives if the cable isn't plugged in perfectly after a good amount of
compressed air sent to the connector and the cable's plug)
Marc
--
"A mouse is a device used to point at the xterm you want to type in" - A.S.R.
Microsoft is to operating systems ....
.... what McDonalds is to gourmet cooking
Home page: http://marc.merlins.org/
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 291 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