[PATCH 0/3] md-cluster: A better way for METADATA_UPDATED processing

[PATCH 0/3] md-cluster: A better way for METADATA_UPDATED processing

[Goldwyn Rodrigues]

The processing of METADATA_UPDATED message is too simple and prone to
errors. Besides, it would not update the internal data structures as
required.

This set of patches reads the superblock from one of the device of the MD
and checks for changes in the in-memory data structures. If there is a change,
it performs the necessary actions to keep the internal data structures
as it would be in the primary node.

An example is if a devices turns faulty. The algorithm is:

1. The primary node marks the device as faulty and updates the superblock
2. The primary node sends METADATA_UPDATED with an advisory  device number to the rest of the nodes.
3. The secondary node on receiving the METADATA_UPDATED message
	3.1 Reads the superblock
	3.2 Detects a device has failed by comparing with memory structure
	3.3 Calls the necessary functions to record the failure and get the device out of the active array.
	3.4 Acknowledges the message.

[PATCH 1/3] remove_and_add_spares() to activate specific rdev

[Goldwyn Rodrigues]

remove_and_add_spares() checks for all devices to activate spare.
Change it to activate a specific device passed as spare.

So, remove_and_add_spares() can be used to activate spares in
slot_store() as well.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 drivers/md/md.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 9798a99..883f675 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2691,15 +2691,9 @@ slot_store(struct md_rdev *rdev, const char *buf, size_t len)
 			rdev->saved_raid_disk = -1;
 		clear_bit(In_sync, &rdev->flags);
 		clear_bit(Bitmap_sync, &rdev->flags);
-		err = rdev->mddev->pers->
-			hot_add_disk(rdev->mddev, rdev);
-		if (err) {
-			rdev->raid_disk = -1;
-			return err;
-		} else
-			sysfs_notify_dirent_safe(rdev->sysfs_state);
-		if (sysfs_link_rdev(rdev->mddev, rdev))
-			/* failure here is OK */;
+		remove_and_add_spares(rdev->mddev, rdev);
+		if (rdev->raid_disk == -1)
+			return -EBUSY;
 		/* don't wakeup anyone, leave that to userspace. */
 	} else {
 		if (slot >= rdev->mddev->raid_disks &&
@@ -8018,10 +8012,12 @@ static int remove_and_add_spares(struct mddev *mddev,
 	if (removed && mddev->kobj.sd)
 		sysfs_notify(&mddev->kobj, NULL, "degraded");
 
-	if (this)
+	if (this && removed)
 		goto no_add;
 
 	rdev_for_each(rdev, mddev) {
+		if (this && this != rdev)
+			continue;
 		if (rdev->raid_disk >= 0 &&
 		    !test_bit(In_sync, &rdev->flags) &&
 		    !test_bit(Faulty, &rdev->flags))
-- 
1.8.5.6

[PATCH 2/3] md-cluster: Improve md_reload_sb to be less error prone

[Goldwyn Rodrigues]

md_reload_sb is too simplistic and it explicitly needs to determine
the changes made by the writing node. However, there are multiple areas
where a simple reload could fail.

Instead, read the superblock of one of the "good" rdevs and update
the necessary information:

- read the superblock into a newly allocated page, by temporarily
  swapping out rdev->sb_page and calling ->load_super.
- if that fails return
- if it succeeds, call check_sb_changes
1. iterates over list of active devices and checks the matching
   dev_roles[] value.
   	If that is 'faulty', the device must be  marked as faulty
	 - call md_error to mark the device as faulty. Make sure
	   not to set CHANGE_DEVS and wakeup mddev->thread or else
	   it would initiate a resync process, which is the responsibility
	   of the "primary" node.
	 - clear the Blocked bit
	 - Call remove_and_add_spares() to hot remove the device.
	If the device is 'spare':
	 - call remove_and_add_spares() to get the number of spares
	   added in this operation.
	 - Reduce mddev->degraded to mark the array as not degraded.
2. reset recovery_cp

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 drivers/md/md-cluster.c |  27 ++++++-----
 drivers/md/md.c         | 119 ++++++++++++++++++++++++++++++++++++++++--------
 drivers/md/md.h         |   2 +-
 3 files changed, 117 insertions(+), 31 deletions(-)

diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index b94a2e6..4645a93 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -424,8 +424,7 @@ static void process_add_new_disk(struct mddev *mddev, struct cluster_msg *cmsg)
 static void process_metadata_update(struct mddev *mddev, struct cluster_msg *msg)
 {
 	struct md_cluster_info *cinfo = mddev->cluster_info;
-
-	md_reload_sb(mddev);
+	md_reload_sb(mddev, le32_to_cpu(msg->raid_slot));
 	dlm_lock_sync(cinfo->no_new_dev_lockres, DLM_LOCK_CR);
 }
 
@@ -811,11 +810,23 @@ static int metadata_update_finish(struct mddev *mddev)
 {
 	struct md_cluster_info *cinfo = mddev->cluster_info;
 	struct cluster_msg cmsg;
-	int ret;
+	struct md_rdev *rdev;
+	int ret = 0;
 
 	memset(&cmsg, 0, sizeof(cmsg));
 	cmsg.type = cpu_to_le32(METADATA_UPDATED);
-	ret = __sendmsg(cinfo, &cmsg);
+	cmsg.raid_slot = -1;
+	/* Pick up a good active device number to send.
+	 */
+	rdev_for_each(rdev, mddev)
+		if (rdev->raid_disk > -1 && !test_bit(Faulty, &rdev->flags)) {
+			cmsg.raid_slot = cpu_to_le32(rdev->desc_nr);
+			break;
+		}
+	if (cmsg.raid_slot >= 0)
+		ret = __sendmsg(cinfo, &cmsg);
+	else
+		pr_warn("md-cluster: No good device id found to send\n");
 	unlock_comm(cinfo);
 	return ret;
 }
@@ -899,15 +910,9 @@ static int add_new_disk_start(struct mddev *mddev, struct md_rdev *rdev)
 
 static int add_new_disk_finish(struct mddev *mddev)
 {
-	struct cluster_msg cmsg;
-	struct md_cluster_info *cinfo = mddev->cluster_info;
-	int ret;
 	/* Write sb and inform others */
 	md_update_sb(mddev, 1);
-	cmsg.type = METADATA_UPDATED;
-	ret = __sendmsg(cinfo, &cmsg);
-	unlock_comm(cinfo);
-	return ret;
+	return metadata_update_finish(mddev);
 }
 
 static int new_disk_ack(struct mddev *mddev, bool ack)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 883f675..fe3ce06 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7035,7 +7035,7 @@ void md_unregister_thread(struct md_thread **threadp)
 }
 EXPORT_SYMBOL(md_unregister_thread);
 
-void md_error(struct mddev *mddev, struct md_rdev *rdev)
+void __md_error(struct mddev *mddev, struct md_rdev *rdev, bool writeout)
 {
 	if (!rdev || test_bit(Faulty, &rdev->flags))
 		return;
@@ -7046,15 +7046,24 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev)
 	if (mddev->degraded)
 		set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
 	sysfs_notify_dirent_safe(rdev->sysfs_state);
-	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
-	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
-	md_wakeup_thread(mddev->thread);
+	if (writeout) {
+		set_bit(MD_RECOVERY_INTR, &mddev->recovery);
+		set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
+		md_wakeup_thread(mddev->thread);
+	}
 	if (mddev->event_work.func)
 		queue_work(md_misc_wq, &mddev->event_work);
 	md_new_event_inintr(mddev);
 }
 EXPORT_SYMBOL(md_error);
 
+void md_error(struct mddev *mddev, struct md_rdev *rdev)
+{
+	__md_error(mddev, rdev, true);
+
+}
+
+
 /* seq_file implementation /proc/mdstat */
 
 static void status_unused(struct seq_file *seq)
@@ -7989,7 +7998,7 @@ void md_do_sync(struct md_thread *thread)
 EXPORT_SYMBOL_GPL(md_do_sync);
 
 static int remove_and_add_spares(struct mddev *mddev,
-				 struct md_rdev *this)
+				struct md_rdev *this)
 {
 	struct md_rdev *rdev;
 	int spares = 0;
@@ -8919,25 +8928,97 @@ err_wq:
 	return ret;
 }
 
-void md_reload_sb(struct mddev *mddev)
+static void check_sb_changes(struct mddev *mddev, struct md_rdev *rdev)
 {
-	struct md_rdev *rdev, *tmp;
+	struct mdp_superblock_1 *sb = page_address(rdev->sb_page);
+	struct md_rdev *rdev2;
+	int role, ret;
+	char b[BDEVNAME_SIZE];
 
-	rdev_for_each_safe(rdev, tmp, mddev) {
-		rdev->sb_loaded = 0;
-		ClearPageUptodate(rdev->sb_page);
+	/* Check for change of roles in the active devices */
+	rdev_for_each(rdev2, mddev) {
+		if (test_bit(Faulty, &rdev2->flags))
+			continue;
+
+		/* Check if the roles changed */
+		role = le16_to_cpu(sb->dev_roles[rdev2->desc_nr]);
+		if (role != rdev2->raid_disk) {
+			/* got activated */
+			if (rdev2->raid_disk == -1 && role != 0xffff) {
+				ret = remove_and_add_spares(mddev, rdev2);
+				/* This should ideally be done at the end of
+				 * a resync operation.
+				 */
+				if (ret) {
+					set_bit(In_sync, &rdev2->flags);
+					mddev->degraded -= ret;
+				}
+				pr_info("Activated spare: %s\n",
+						bdevname(rdev2->bdev,b));
+				continue;
+			}
+			/* device faulty
+			 * We just want to do the minimum to mark the disk
+			 * as faulty. The recovery is performed by the
+			 * one who initiated the error.
+			 */
+			if ((role == 0xfffe) || (role == 0xfffd)) {
+				__md_error(mddev, rdev2, false);
+				clear_bit(Blocked, &rdev2->flags);
+				remove_and_add_spares(mddev, rdev2);
+			}
+		}
 	}
-	mddev->raid_disks = 0;
-	analyze_sbs(mddev);
-	rdev_for_each_safe(rdev, tmp, mddev) {
-		struct mdp_superblock_1 *sb = page_address(rdev->sb_page);
-		/* since we don't write to faulty devices, we figure out if the
-		 *  disk is faulty by comparing events
-		 */
-		if (mddev->events > sb->events)
-			set_bit(Faulty, &rdev->flags);
+
+	/* recovery_cp changed */
+	if (le64_to_cpu(sb->resync_offset) != mddev->recovery_cp) {
+		pr_info("%s:%d recovery_cp changed from %lu to %lu\n", __func__,
+				__LINE__, mddev->recovery_cp,
+				(unsigned long) le64_to_cpu(sb->resync_offset));
+		mddev->recovery_cp = le64_to_cpu(sb->resync_offset);
+	}
+
+	/* Finally set the event to be up to date */
+	mddev->events = le64_to_cpu(sb->events);
+}
+
+void md_reload_sb(struct mddev *mddev, int nr)
+{
+	struct md_rdev *rdev;
+	struct page *swapout;
+	int err;
+
+	/* Find the rdev */
+	rdev_for_each_rcu(rdev, mddev) {
+		if (rdev->desc_nr == nr)
+			break;
+	}
+
+	if (!rdev || rdev->desc_nr != nr) {
+		pr_warn("%s: %d Could not find rdev with nr %d\n", __func__, __LINE__, nr);
+		return;
+	}
+	/* Store the sb page of the rdev in the swapout temporary
+	 * variable in case we err in the future
+	 */
+	swapout = rdev->sb_page;
+	rdev->sb_page = NULL;
+	alloc_disk_sb(rdev);
+	ClearPageUptodate(rdev->sb_page);
+	rdev->sb_loaded = 0;
+	err = super_types[mddev->major_version].load_super(rdev, NULL, mddev->minor_version);
+
+	if (err < 0) {
+		pr_warn("%s: %d Could not reload rdev(%d) err: %d. Restoring old values\n",
+				__func__, __LINE__, nr, err);
+		put_page(rdev->sb_page);
+		rdev->sb_page = swapout;
+		rdev->sb_loaded = 1;
+		return;
 	}
 
+	check_sb_changes(mddev, rdev);
+	put_page(swapout);
 }
 EXPORT_SYMBOL(md_reload_sb);
 
diff --git a/drivers/md/md.h b/drivers/md/md.h
index ab33957..2ea0035 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -658,7 +658,7 @@ extern struct bio *bio_alloc_mddev(gfp_t gfp_mask, int nr_iovecs,
 				   struct mddev *mddev);
 
 extern void md_unplug(struct blk_plug_cb *cb, bool from_schedule);
-extern void md_reload_sb(struct mddev *mddev);
+extern void md_reload_sb(struct mddev *mddev, int raid_disk);
 extern void md_update_sb(struct mddev *mddev, int force);
 extern void md_kick_rdev_from_array(struct md_rdev * rdev);
 struct md_rdev *md_find_rdev_nr_rcu(struct mddev *mddev, int nr);
-- 
1.8.5.6

Re: [PATCH 2/3] md-cluster: Improve md_reload_sb to be less error prone

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]

Hi Goldwyn,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: parisc-c3000_defconfig (attached as .config)
reproduce:
  wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
  chmod +x ~/bin/make.cross
  git checkout 825961765139461e82fb00395588061f26b21693
  # save the attached .config to linux build tree
  make.cross ARCH=parisc 

All warnings (new ones prefixed by >>):

   drivers/md/md.c: In function 'check_sb_changes':
>> drivers/md/md.c:8975:3: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'sector_t' [-Wformat=]
      pr_info("%s:%d recovery_cp changed from %lu to %lu\n", __func__,
      ^

vim +8975 drivers/md/md.c

  8959				}
  8960				/* device faulty
  8961				 * We just want to do the minimum to mark the disk
  8962				 * as faulty. The recovery is performed by the
  8963				 * one who initiated the error.
  8964				 */
  8965				if ((role == 0xfffe) || (role == 0xfffd)) {
  8966					__md_error(mddev, rdev2, false);
  8967					clear_bit(Blocked, &rdev2->flags);
  8968					remove_and_add_spares(mddev, rdev2);
  8969				}
  8970			}
  8971		}
  8972	
  8973		/* recovery_cp changed */
  8974		if (le64_to_cpu(sb->resync_offset) != mddev->recovery_cp) {
> 8975			pr_info("%s:%d recovery_cp changed from %lu to %lu\n", __func__,
  8976					__LINE__, mddev->recovery_cp,
  8977					(unsigned long) le64_to_cpu(sb->resync_offset));
  8978			mddev->recovery_cp = le64_to_cpu(sb->resync_offset);
  8979		}
  8980	
  8981		/* Finally set the event to be up to date */
  8982		mddev->events = le64_to_cpu(sb->events);
  8983	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 13572 bytes --]

Re: [PATCH 2/3] md-cluster: Improve md_reload_sb to be less error prone

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2435 bytes --]

Hi Goldwyn,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: i386-defconfig (attached as .config)
reproduce:
  git checkout 825961765139461e82fb00395588061f26b21693
  # save the attached .config to linux build tree
  make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:6:0,
                    from include/linux/kernel.h:13,
                    from include/linux/sched.h:17,
                    from include/linux/kthread.h:5,
                    from drivers/md/md.c:35:
   drivers/md/md.c: In function 'check_sb_changes':
>> include/linux/kern_levels.h:4:18: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'sector_t {aka long long unsigned int}' [-Wformat=]
    #define KERN_SOH "\001"  /* ASCII Start Of Header */
                     ^
   include/linux/kern_levels.h:13:19: note: in expansion of macro 'KERN_SOH'
    #define KERN_INFO KERN_SOH "6" /* informational */
                      ^
   include/linux/printk.h:259:9: note: in expansion of macro 'KERN_INFO'
     printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
            ^
>> drivers/md/md.c:8975:3: note: in expansion of macro 'pr_info'
      pr_info("%s:%d recovery_cp changed from %lu to %lu\n", __func__,
      ^

vim +/pr_info +8975 drivers/md/md.c

  8959				}
  8960				/* device faulty
  8961				 * We just want to do the minimum to mark the disk
  8962				 * as faulty. The recovery is performed by the
  8963				 * one who initiated the error.
  8964				 */
  8965				if ((role == 0xfffe) || (role == 0xfffd)) {
  8966					__md_error(mddev, rdev2, false);
  8967					clear_bit(Blocked, &rdev2->flags);
  8968					remove_and_add_spares(mddev, rdev2);
  8969				}
  8970			}
  8971		}
  8972	
  8973		/* recovery_cp changed */
  8974		if (le64_to_cpu(sb->resync_offset) != mddev->recovery_cp) {
> 8975			pr_info("%s:%d recovery_cp changed from %lu to %lu\n", __func__,
  8976					__LINE__, mddev->recovery_cp,
  8977					(unsigned long) le64_to_cpu(sb->resync_offset));
  8978			mddev->recovery_cp = le64_to_cpu(sb->resync_offset);
  8979		}
  8980	
  8981		/* Finally set the event to be up to date */
  8982		mddev->events = le64_to_cpu(sb->events);
  8983	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23761 bytes --]

Re: [PATCH 2/3] md-cluster: Improve md_reload_sb to be less error prone

[kbuild test robot]

Hi Goldwyn,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

reproduce:
  # apt-get install sparse
  make ARCH=x86_64 allmodconfig
  make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   drivers/md/md-cluster.c:213:15: sparse: restricted __le64 degrades to integer
>> drivers/md/md-cluster.c:424:29: sparse: cast to restricted __le32
   drivers/md/md-cluster.c:564:20: sparse: incorrect type in assignment (different base types)
   drivers/md/md-cluster.c:564:20:    expected int [signed] slot
   drivers/md/md-cluster.c:564:20:    got restricted __le32 [usertype] <noident>
   drivers/md/md-cluster.c:823:19: sparse: incorrect type in assignment (different base types)
   drivers/md/md-cluster.c:823:19:    expected int [signed] [addressable] type
   drivers/md/md-cluster.c:823:19:    got restricted __le32 [usertype] <noident>
>> drivers/md/md-cluster.c:829:40: sparse: incorrect type in assignment (different base types)
   drivers/md/md-cluster.c:829:40:    expected int [signed] [addressable] [assigned] raid_slot
   drivers/md/md-cluster.c:829:40:    got restricted __le32 [usertype] <noident>
   drivers/md/md-cluster.c:858:19: sparse: incorrect type in assignment (different base types)
   drivers/md/md-cluster.c:858:19:    expected int [signed] type
   drivers/md/md-cluster.c:858:19:    got restricted __le32 [usertype] <noident>
   drivers/md/md-cluster.c:859:19: sparse: incorrect type in assignment (different base types)
   drivers/md/md-cluster.c:859:19:    expected int [signed] slot
   drivers/md/md-cluster.c:859:19:    got restricted __le32 [usertype] <noident>
   drivers/md/md-cluster.c:860:18: sparse: incorrect type in assignment (different base types)
   drivers/md/md-cluster.c:860:18:    expected unsigned long [unsigned] [usertype] low
   drivers/md/md-cluster.c:860:18:    got restricted __le64 [usertype] <noident>
   drivers/md/md-cluster.c:861:19: sparse: incorrect type in assignment (different base types)
   drivers/md/md-cluster.c:861:19:    expected unsigned long [unsigned] [usertype] high
   drivers/md/md-cluster.c:861:19:    got restricted __le64 [usertype] <noident>
   drivers/md/md-cluster.c:880:27: sparse: incorrect type in assignment (different base types)
   drivers/md/md-cluster.c:880:27:    expected int [signed] type
   drivers/md/md-cluster.c:880:27:    got restricted __le32 [usertype] <noident>
   drivers/md/md-cluster.c:881:27: sparse: incorrect type in assignment (different base types)
   drivers/md/md-cluster.c:881:27:    expected int [signed] slot
   drivers/md/md-cluster.c:881:27:    got restricted __le32 [usertype] <noident>
   drivers/md/md-cluster.c:919:19: sparse: incorrect type in assignment (different base types)
   drivers/md/md-cluster.c:919:19:    expected int [signed] [addressable] type
   drivers/md/md-cluster.c:919:19:    got restricted __le32 [usertype] <noident>
--
   drivers/md/md.c:1480:34: sparse: cast to restricted __le64
   drivers/md/md.c:1760:40: sparse: incorrect type in assignment (different base types)
   drivers/md/md.c:1760:40:    expected unsigned long long [unsigned] [long] [long long] [usertype] <noident>
   drivers/md/md.c:1760:40:    got restricted __le64 [usertype] <noident>
   drivers/md/md.c:1835:26: sparse: incorrect type in assignment (different base types)
   drivers/md/md.c:1835:26:    expected restricted __le64 [usertype] super_offset
   drivers/md/md.c:1835:26:    got unsigned long [unsigned] [usertype] sb_start
>> drivers/md/md.c:7031:6: sparse: symbol '__md_error' was not declared. Should it be static?

Please review and possibly fold the followup patch.

vim +424 drivers/md/md-cluster.c

   408		len = snprintf(disk_uuid, 64, "DEVICE_UUID=");
   409		sprintf(disk_uuid + len, "%pU", cmsg->uuid);
   410		snprintf(raid_slot, 16, "RAID_DISK=%d", cmsg->raid_slot);
   411		pr_info("%s:%d Sending kobject change with %s and %s\n", __func__, __LINE__, disk_uuid, raid_slot);
   412		init_completion(&cinfo->newdisk_completion);
   413		set_bit(MD_CLUSTER_WAITING_FOR_NEWDISK, &cinfo->state);
   414		kobject_uevent_env(&disk_to_dev(mddev->gendisk)->kobj, KOBJ_CHANGE, envp);
   415		wait_for_completion_timeout(&cinfo->newdisk_completion,
   416				NEW_DEV_TIMEOUT);
   417		clear_bit(MD_CLUSTER_WAITING_FOR_NEWDISK, &cinfo->state);
   418	}
   419	
   420	
   421	static void process_metadata_update(struct mddev *mddev, struct cluster_msg *msg)
   422	{
   423		struct md_cluster_info *cinfo = mddev->cluster_info;
 > 424		md_reload_sb(mddev, le32_to_cpu(msg->raid_slot));
   425		dlm_lock_sync(cinfo->no_new_dev_lockres, DLM_LOCK_CR);
   426	}
   427	
   428	static void process_remove_disk(struct mddev *mddev, struct cluster_msg *msg)
   429	{
   430		struct md_rdev *rdev = md_find_rdev_nr_rcu(mddev, msg->raid_slot);
   431	
   432		if (rdev)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] md-cluster: __md_error() can be static

[kbuild test robot]

Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 md.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 2db697b..294f9e4 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7028,7 +7028,7 @@ void md_unregister_thread(struct md_thread **threadp)
 }
 EXPORT_SYMBOL(md_unregister_thread);
 
-void __md_error(struct mddev *mddev, struct md_rdev *rdev, bool writeout)
+static void __md_error(struct mddev *mddev, struct md_rdev *rdev, bool writeout)
 {
 	if (!rdev || test_bit(Faulty, &rdev->flags))
 		return;

Re: [PATCH 2/3] md-cluster: Improve md_reload_sb to be less error prone

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

Hi Goldwyn,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: openrisc-allmodconfig (attached as .config)
reproduce:
  wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
  chmod +x ~/bin/make.cross
  git checkout 825961765139461e82fb00395588061f26b21693
  # save the attached .config to linux build tree
  make.cross ARCH=openrisc 

All warnings (new ones prefixed by >>):

   drivers/md/md.c: In function 'check_sb_changes':
>> drivers/md/md.c:8975:3: warning: format '%lu' expects type 'long unsigned int', but argument 4 has type 'sector_t'

vim +8975 drivers/md/md.c

  8959				}
  8960				/* device faulty
  8961				 * We just want to do the minimum to mark the disk
  8962				 * as faulty. The recovery is performed by the
  8963				 * one who initiated the error.
  8964				 */
  8965				if ((role == 0xfffe) || (role == 0xfffd)) {
  8966					__md_error(mddev, rdev2, false);
  8967					clear_bit(Blocked, &rdev2->flags);
  8968					remove_and_add_spares(mddev, rdev2);
  8969				}
  8970			}
  8971		}
  8972	
  8973		/* recovery_cp changed */
  8974		if (le64_to_cpu(sb->resync_offset) != mddev->recovery_cp) {
> 8975			pr_info("%s:%d recovery_cp changed from %lu to %lu\n", __func__,
  8976					__LINE__, mddev->recovery_cp,
  8977					(unsigned long) le64_to_cpu(sb->resync_offset));
  8978			mddev->recovery_cp = le64_to_cpu(sb->resync_offset);
  8979		}
  8980	
  8981		/* Finally set the event to be up to date */
  8982		mddev->events = le64_to_cpu(sb->events);
  8983	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 35004 bytes --]

[PATCH 3/3] md-cluster: Perform a lazy update

[Goldwyn Rodrigues]

In a clustered environment, a change such as marking a device faulty,
can be recorded by any of the nodes. This is communicated to all the
nodes and re-recording such a change is unnecessary, and quite often
pretty disruptive.

With this patch, just before the update, we detect for the changes
and if the changes are already in superblock, we abort the update
after clearing all the flags

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 drivers/md/md.c | 115 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 66 insertions(+), 49 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index fe3ce06..e732d73 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2199,6 +2199,46 @@ static void sync_sbs(struct mddev *mddev, int nospares)
 	}
 }
 
+bool does_sb_need_changing(struct mddev *mddev)
+{
+	struct md_rdev *rdev;
+	struct mdp_superblock_1 *sb;
+	int role;
+
+	/* Find a good rdev */
+	rdev_for_each_rcu(rdev, mddev)
+		if ((rdev->raid_disk >= 0) && !test_bit(Faulty, &rdev->flags))
+			break;
+
+	/* No good device found. */
+	if (!rdev)
+		return false;
+
+	sb = page_address(rdev->sb_page);
+	/* Check if a device has become faulty or a spare become active */
+	rdev_for_each(rdev, mddev) {
+		role = le16_to_cpu(sb->dev_roles[rdev->desc_nr]);
+		/* Device activated? */
+		if (role == 0xffff && rdev->raid_disk >=0 && !test_bit(Faulty, &rdev->flags))
+			return true;
+		/* Device turned faulty? */
+		if (test_bit(Faulty, &rdev->flags) && (role < 0xfffd))
+			return true;
+	}
+
+	/* Check if any mddev parameters have changed */
+	if ((mddev->dev_sectors != le64_to_cpu(sb->size)) ||
+			(mddev->reshape_position != le64_to_cpu(sb->reshape_position)) ||
+			(mddev->recovery_cp != le64_to_cpu(sb->resync_offset)) ||
+			(mddev->layout != le64_to_cpu(sb->layout)) ||
+			(mddev->raid_disks != le32_to_cpu(sb->raid_disks)) ||
+			(mddev->chunk_sectors != le32_to_cpu(sb->chunksize))
+			 )
+		return true;
+
+	return false;
+}
+
 void md_update_sb(struct mddev *mddev, int force_change)
 {
 	struct md_rdev *rdev;
@@ -2211,6 +2251,27 @@ void md_update_sb(struct mddev *mddev, int force_change)
 			set_bit(MD_CHANGE_DEVS, &mddev->flags);
 		return;
 	}
+
+	if (mddev_is_clustered(mddev)) {
+		int change_devs;
+recheck:
+		change_devs = test_and_clear_bit(MD_CHANGE_DEVS, &mddev->flags);
+		md_cluster_ops->metadata_update_start(mddev);
+		/* Has someone else has updated the sb */
+		if (!does_sb_need_changing(mddev)) {
+			md_cluster_ops->metadata_update_cancel(mddev);
+			/* Check if someone set the bits while we
+			 *  were waiting on lock
+			 */
+			if (test_bit(MD_CHANGE_DEVS, &mddev->flags))
+				goto recheck;
+			clear_bit(MD_CHANGE_PENDING, &mddev->flags);
+			return;
+		}
+
+		if (change_devs)
+			set_bit(MD_CHANGE_DEVS, &mddev->flags);
+	}
 repeat:
 	/* First make sure individual recovery_offsets are correct */
 	rdev_for_each(rdev, mddev) {
@@ -2359,6 +2420,9 @@ repeat:
 		clear_bit(BlockedBadBlocks, &rdev->flags);
 		wake_up(&rdev->blocked_wait);
 	}
+
+	if (mddev_is_clustered(mddev))
+		md_cluster_ops->metadata_update_finish(mddev);
 }
 EXPORT_SYMBOL(md_update_sb);
 
@@ -2496,13 +2560,9 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
 			if (mddev_is_clustered(mddev))
 				md_cluster_ops->remove_disk(mddev, rdev);
 			md_kick_rdev_from_array(rdev);
-			if (mddev_is_clustered(mddev))
-				md_cluster_ops->metadata_update_start(mddev);
 			if (mddev->pers)
 				md_update_sb(mddev, 1);
 			md_new_event(mddev);
-			if (mddev_is_clustered(mddev))
-				md_cluster_ops->metadata_update_finish(mddev);
 			err = 0;
 		}
 	} else if (cmd_match(buf, "writemostly")) {
@@ -4063,12 +4123,8 @@ size_store(struct mddev *mddev, const char *buf, size_t len)
 	if (err)
 		return err;
 	if (mddev->pers) {
-		if (mddev_is_clustered(mddev))
-			md_cluster_ops->metadata_update_start(mddev);
 		err = update_size(mddev, sectors);
 		md_update_sb(mddev, 1);
-		if (mddev_is_clustered(mddev))
-			md_cluster_ops->metadata_update_finish(mddev);
 	} else {
 		if (mddev->dev_sectors == 0 ||
 		    mddev->dev_sectors > sectors)
@@ -5306,8 +5362,6 @@ static void md_clean(struct mddev *mddev)
 
 static void __md_stop_writes(struct mddev *mddev)
 {
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_start(mddev);
 	set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
 	flush_workqueue(md_misc_wq);
 	if (mddev->sync_thread) {
@@ -5326,8 +5380,6 @@ static void __md_stop_writes(struct mddev *mddev)
 		mddev->in_sync = 1;
 		md_update_sb(mddev, 1);
 	}
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_finish(mddev);
 }
 
 void md_stop_writes(struct mddev *mddev)
@@ -5995,9 +6047,6 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)
 	if (!rdev)
 		return -ENXIO;
 
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_start(mddev);
-
 	clear_bit(Blocked, &rdev->flags);
 	remove_and_add_spares(mddev, rdev);
 
@@ -6011,13 +6060,8 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)
 	md_update_sb(mddev, 1);
 	md_new_event(mddev);
 
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_finish(mddev);
-
 	return 0;
 busy:
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_cancel(mddev);
 	printk(KERN_WARNING "md: cannot remove active disk %s from %s ...\n",
 		bdevname(rdev->bdev,b), mdname(mddev));
 	return -EBUSY;
@@ -6068,14 +6112,12 @@ static int hot_add_disk(struct mddev *mddev, dev_t dev)
 		goto abort_export;
 	}
 
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_start(mddev);
 	clear_bit(In_sync, &rdev->flags);
 	rdev->desc_nr = -1;
 	rdev->saved_raid_disk = -1;
 	err = bind_rdev_to_array(rdev, mddev);
 	if (err)
-		goto abort_clustered;
+		goto abort_export;
 
 	/*
 	 * The rest should better be atomic, we can have disk failures
@@ -6085,9 +6127,6 @@ static int hot_add_disk(struct mddev *mddev, dev_t dev)
 	rdev->raid_disk = -1;
 
 	md_update_sb(mddev, 1);
-
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_finish(mddev);
 	/*
 	 * Kick recovery, maybe this spare has to be added to the
 	 * array immediately.
@@ -6097,9 +6136,6 @@ static int hot_add_disk(struct mddev *mddev, dev_t dev)
 	md_new_event(mddev);
 	return 0;
 
-abort_clustered:
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_cancel(mddev);
 abort_export:
 	export_rdev(rdev);
 	return err;
@@ -6417,8 +6453,6 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
 			return rv;
 		}
 	}
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_start(mddev);
 	if (info->size >= 0 && mddev->dev_sectors / 2 != info->size)
 		rv = update_size(mddev, (sector_t)info->size * 2);
 
@@ -6476,12 +6510,8 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
 		}
 	}
 	md_update_sb(mddev, 1);
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_finish(mddev);
 	return rv;
 err:
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_cancel(mddev);
 	return rv;
 }
 
@@ -7603,11 +7633,7 @@ int md_allow_write(struct mddev *mddev)
 		    mddev->safemode == 0)
 			mddev->safemode = 1;
 		spin_unlock(&mddev->lock);
-		if (mddev_is_clustered(mddev))
-			md_cluster_ops->metadata_update_start(mddev);
 		md_update_sb(mddev, 0);
-		if (mddev_is_clustered(mddev))
-			md_cluster_ops->metadata_update_finish(mddev);
 		sysfs_notify_dirent_safe(mddev->sysfs_state);
 	} else
 		spin_unlock(&mddev->lock);
@@ -8186,13 +8212,8 @@ void md_check_recovery(struct mddev *mddev)
 				sysfs_notify_dirent_safe(mddev->sysfs_state);
 		}
 
-		if (mddev->flags & MD_UPDATE_SB_FLAGS) {
-			if (mddev_is_clustered(mddev))
-				md_cluster_ops->metadata_update_start(mddev);
+		if (mddev->flags & MD_UPDATE_SB_FLAGS)
 			md_update_sb(mddev, 0);
-			if (mddev_is_clustered(mddev))
-				md_cluster_ops->metadata_update_finish(mddev);
-		}
 
 		if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) &&
 		    !test_bit(MD_RECOVERY_DONE, &mddev->recovery)) {
@@ -8290,8 +8311,6 @@ void md_reap_sync_thread(struct mddev *mddev)
 			set_bit(MD_CHANGE_DEVS, &mddev->flags);
 		}
 	}
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_start(mddev);
 	if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
 	    mddev->pers->finish_reshape)
 		mddev->pers->finish_reshape(mddev);
@@ -8304,8 +8323,6 @@ void md_reap_sync_thread(struct mddev *mddev)
 			rdev->saved_raid_disk = -1;
 
 	md_update_sb(mddev, 1);
-	if (mddev_is_clustered(mddev))
-		md_cluster_ops->metadata_update_finish(mddev);
 	clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
 	clear_bit(MD_RECOVERY_DONE, &mddev->recovery);
 	clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
-- 
1.8.5.6

Re: [PATCH 3/3] md-cluster: Perform a lazy update

[kbuild test robot]

Hi Goldwyn,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

reproduce:
  # apt-get install sparse
  make ARCH=x86_64 allmodconfig
  make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   drivers/md/md.c:1480:34: sparse: cast to restricted __le64
   drivers/md/md.c:1760:40: sparse: incorrect type in assignment (different base types)
   drivers/md/md.c:1760:40:    expected unsigned long long [unsigned] [long] [long long] [usertype] <noident>
   drivers/md/md.c:1760:40:    got restricted __le64 [usertype] <noident>
   drivers/md/md.c:1835:26: sparse: incorrect type in assignment (different base types)
   drivers/md/md.c:1835:26:    expected restricted __le64 [usertype] super_offset
   drivers/md/md.c:1835:26:    got unsigned long [unsigned] [usertype] sb_start
   drivers/md/md.c:2230:43: sparse: cast to restricted __le64
>> drivers/md/md.c:2230:43: sparse: cast from restricted __le32
>> drivers/md/md.c:2199:6: sparse: symbol 'does_sb_need_changing' was not declared. Should it be static?
   drivers/md/md.c:7061:6: sparse: symbol '__md_error' was not declared. Should it be static?

Please review and possibly fold the followup patch.

vim +2230 drivers/md/md.c

  2193				sync_super(mddev, rdev);
  2194				rdev->sb_loaded = 1;
  2195			}
  2196		}
  2197	}
  2198	
> 2199	bool does_sb_need_changing(struct mddev *mddev)
  2200	{
  2201		struct md_rdev *rdev;
  2202		struct mdp_superblock_1 *sb;
  2203		int role;
  2204	
  2205		/* Find a good rdev */
  2206		rdev_for_each_rcu(rdev, mddev)
  2207			if ((rdev->raid_disk >= 0) && !test_bit(Faulty, &rdev->flags))
  2208				break;
  2209	
  2210		/* No good device found. */
  2211		if (!rdev)
  2212			return false;
  2213	
  2214		sb = page_address(rdev->sb_page);
  2215		/* Check if a device has become faulty or a spare become active */
  2216		rdev_for_each(rdev, mddev) {
  2217			role = le16_to_cpu(sb->dev_roles[rdev->desc_nr]);
  2218			/* Device activated? */
  2219			if (role == 0xffff && rdev->raid_disk >=0 && !test_bit(Faulty, &rdev->flags))
  2220				return true;
  2221			/* Device turned faulty? */
  2222			if (test_bit(Faulty, &rdev->flags) && (role < 0xfffd))
  2223				return true;
  2224		}
  2225	
  2226		/* Check if any mddev parameters have changed */
  2227		if ((mddev->dev_sectors != le64_to_cpu(sb->size)) ||
  2228				(mddev->reshape_position != le64_to_cpu(sb->reshape_position)) ||
  2229				(mddev->recovery_cp != le64_to_cpu(sb->resync_offset)) ||
> 2230				(mddev->layout != le64_to_cpu(sb->layout)) ||
  2231				(mddev->raid_disks != le32_to_cpu(sb->raid_disks)) ||
  2232				(mddev->chunk_sectors != le32_to_cpu(sb->chunksize))
  2233				 )

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] md-cluster: does_sb_need_changing() can be static

[kbuild test robot]

Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 md.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 09d1384..ae90e3c 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2196,7 +2196,7 @@ static void sync_sbs(struct mddev *mddev, int nospares)
 	}
 }
 
-bool does_sb_need_changing(struct mddev *mddev)
+static bool does_sb_need_changing(struct mddev *mddev)
 {
 	struct md_rdev *rdev;
 	struct mdp_superblock_1 *sb;