[PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[shli]

From: Shaohua Li <shli@fb.com>

.quiesce is called with mddev lock hold at most places. There are few
exceptions. Calling .quesce without the lock hold could create races. For
example, the .quesce of raid1 can't be recursively. The purpose of the patches
is to fix a race in raid5-cache. The raid5-cache .quesce will write md
superblock and should be called with mddev lock hold.

Cc: NeilBrown <neilb@suse.com>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 drivers/md/md.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 2c3ab6f..0550445 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7945,8 +7945,10 @@ void md_do_sync(struct md_thread *thread)
 		 * region.
 		 */
 		if (mddev->bitmap) {
+			mddev_lock_nointr(mddev);
 			mddev->pers->quiesce(mddev, 1);
 			mddev->pers->quiesce(mddev, 0);
+			mddev_unlock(mddev);
 		}
 	}
 
-- 
2.7.4

[PATCH 2/3] MD: hold mddev lock to change bitmap location

[shli]

From: Shaohua Li <shli@fb.com>

Changing the location changes a lot of things. Holding the lock to avoid race.
This makes the .quiesce called with mddev lock hold too.

Cc: NeilBrown <neilb@suse.com>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 drivers/md/bitmap.c | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index 6fff794..13041ee 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -2183,19 +2183,29 @@ location_show(struct mddev *mddev, char *page)
 static ssize_t
 location_store(struct mddev *mddev, const char *buf, size_t len)
 {
+	int rv;
 
+	rv = mddev_lock(mddev);
+	if (rv)
+		return rv;
 	if (mddev->pers) {
-		if (!mddev->pers->quiesce)
-			return -EBUSY;
-		if (mddev->recovery || mddev->sync_thread)
-			return -EBUSY;
+		if (!mddev->pers->quiesce) {
+			rv = -EBUSY;
+			goto out;
+		}
+		if (mddev->recovery || mddev->sync_thread) {
+			rv = -EBUSY;
+			goto out;
+		}
 	}
 
 	if (mddev->bitmap || mddev->bitmap_info.file ||
 	    mddev->bitmap_info.offset) {
 		/* bitmap already configured.  Only option is to clear it */
-		if (strncmp(buf, "none", 4) != 0)
-			return -EBUSY;
+		if (strncmp(buf, "none", 4) != 0) {
+			rv = -EBUSY;
+			goto out;
+		}
 		if (mddev->pers) {
 			mddev->pers->quiesce(mddev, 1);
 			bitmap_destroy(mddev);
@@ -2214,21 +2224,25 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
 			/* nothing to be done */;
 		else if (strncmp(buf, "file:", 5) == 0) {
 			/* Not supported yet */
-			return -EINVAL;
+			rv = -EINVAL;
+			goto out;
 		} else {
-			int rv;
 			if (buf[0] == '+')
 				rv = kstrtoll(buf+1, 10, &offset);
 			else
 				rv = kstrtoll(buf, 10, &offset);
 			if (rv)
-				return rv;
-			if (offset == 0)
-				return -EINVAL;
+				goto out;
+			if (offset == 0) {
+				rv = -EINVAL;
+				goto out;
+			}
 			if (mddev->bitmap_info.external == 0 &&
 			    mddev->major_version == 0 &&
-			    offset != mddev->bitmap_info.default_offset)
-				return -EINVAL;
+			    offset != mddev->bitmap_info.default_offset) {
+				rv = -EINVAL;
+				goto out;
+			}
 			mddev->bitmap_info.offset = offset;
 			if (mddev->pers) {
 				struct bitmap *bitmap;
@@ -2245,7 +2259,7 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
 				mddev->pers->quiesce(mddev, 0);
 				if (rv) {
 					bitmap_destroy(mddev);
-					return rv;
+					goto out;
 				}
 			}
 		}
@@ -2257,6 +2271,11 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
 		set_bit(MD_CHANGE_DEVS, &mddev->flags);
 		md_wakeup_thread(mddev->thread);
 	}
+	rv = 0;
+out:
+	mddev_unlock(mddev);
+	if (rv)
+		return rv;
 	return len;
 }
 
-- 
2.7.4

Re: [PATCH 2/3] MD: hold mddev lock to change bitmap location

[NeilBrown]

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

On Sun, Jul 31 2016, shli@kernel.org wrote:

> From: Shaohua Li <shli@fb.com>
>
> Changing the location changes a lot of things. Holding the lock to avoid race.
> This makes the .quiesce called with mddev lock hold too.
>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Shaohua Li <shli@fb.com>

Acked-by: NeilBrown <neilb@suse.com>

Yes, I think this is justified.  As you say, location_store is a fairly
significant change.

Thanks,
NeilBrown

> ---
>  drivers/md/bitmap.c | 47 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index 6fff794..13041ee 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -2183,19 +2183,29 @@ location_show(struct mddev *mddev, char *page)
>  static ssize_t
>  location_store(struct mddev *mddev, const char *buf, size_t len)
>  {
> +	int rv;
>  
> +	rv = mddev_lock(mddev);
> +	if (rv)
> +		return rv;
>  	if (mddev->pers) {
> -		if (!mddev->pers->quiesce)
> -			return -EBUSY;
> -		if (mddev->recovery || mddev->sync_thread)
> -			return -EBUSY;
> +		if (!mddev->pers->quiesce) {
> +			rv = -EBUSY;
> +			goto out;
> +		}
> +		if (mddev->recovery || mddev->sync_thread) {
> +			rv = -EBUSY;
> +			goto out;
> +		}
>  	}
>  
>  	if (mddev->bitmap || mddev->bitmap_info.file ||
>  	    mddev->bitmap_info.offset) {
>  		/* bitmap already configured.  Only option is to clear it */
> -		if (strncmp(buf, "none", 4) != 0)
> -			return -EBUSY;
> +		if (strncmp(buf, "none", 4) != 0) {
> +			rv = -EBUSY;
> +			goto out;
> +		}
>  		if (mddev->pers) {
>  			mddev->pers->quiesce(mddev, 1);
>  			bitmap_destroy(mddev);
> @@ -2214,21 +2224,25 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
>  			/* nothing to be done */;
>  		else if (strncmp(buf, "file:", 5) == 0) {
>  			/* Not supported yet */
> -			return -EINVAL;
> +			rv = -EINVAL;
> +			goto out;
>  		} else {
> -			int rv;
>  			if (buf[0] == '+')
>  				rv = kstrtoll(buf+1, 10, &offset);
>  			else
>  				rv = kstrtoll(buf, 10, &offset);
>  			if (rv)
> -				return rv;
> -			if (offset == 0)
> -				return -EINVAL;
> +				goto out;
> +			if (offset == 0) {
> +				rv = -EINVAL;
> +				goto out;
> +			}
>  			if (mddev->bitmap_info.external == 0 &&
>  			    mddev->major_version == 0 &&
> -			    offset != mddev->bitmap_info.default_offset)
> -				return -EINVAL;
> +			    offset != mddev->bitmap_info.default_offset) {
> +				rv = -EINVAL;
> +				goto out;
> +			}
>  			mddev->bitmap_info.offset = offset;
>  			if (mddev->pers) {
>  				struct bitmap *bitmap;
> @@ -2245,7 +2259,7 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
>  				mddev->pers->quiesce(mddev, 0);
>  				if (rv) {
>  					bitmap_destroy(mddev);
> -					return rv;
> +					goto out;
>  				}
>  			}
>  		}
> @@ -2257,6 +2271,11 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
>  		set_bit(MD_CHANGE_DEVS, &mddev->flags);
>  		md_wakeup_thread(mddev->thread);
>  	}
> +	rv = 0;
> +out:
> +	mddev_unlock(mddev);
> +	if (rv)
> +		return rv;
>  	return len;
>  }
>  
> -- 
> 2.7.4

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[PATCH 3/3] MD: hold mddev lock for md-cluster receive thread

[shli]

From: Shaohua Li <shli@fb.com>

md-cluster receive thread calls .quiesce too, let it hold mddev lock.

Cc: NeilBrown <neilb@suse.com>
Cc: Guoqing Jiang <gqjiang@suse.com>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 drivers/md/md-cluster.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index 41573f1..f420060 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -567,11 +567,13 @@ static void recv_daemon(struct md_thread *thread)
 	struct cluster_msg msg;
 	int ret;
 
+	mddev_lock_nointr(thread->mddev);
 	mutex_lock(&cinfo->recv_mutex);
 	/*get CR on Message*/
 	if (dlm_lock_sync(message_lockres, DLM_LOCK_CR)) {
 		pr_err("md/raid1:failed to get CR on MESSAGE\n");
 		mutex_unlock(&cinfo->recv_mutex);
+		mddev_unlock(thread->mddev);
 		return;
 	}
 
@@ -599,6 +601,7 @@ static void recv_daemon(struct md_thread *thread)
 	if (unlikely(ret != 0))
 		pr_info("unlock msg failed return %d\n", ret);
 	mutex_unlock(&cinfo->recv_mutex);
+	mddev_unlock(thread->mddev);
 }
 
 /* lock_token()
-- 
2.7.4

Re: [PATCH 3/3] MD: hold mddev lock for md-cluster receive thread

[Guoqing Jiang]

Hi,

On 07/31/2016 07:54 AM, shli@kernel.org wrote:
> From: Shaohua Li <shli@fb.com>
>
> md-cluster receive thread calls .quiesce too, let it hold mddev lock.

I'd suggest hold on for the patchset, I can find lock problem easily with
the patchset applied. Take a resyncing  clusteed raid1 as example.

md127_raid1 thread held reconfig_mutex then update sb, so it needs dlm
token lock. Meanwhile md127_resync thread got token lock and wants
EX on ack lock but recv_daemon can't release ack lock since recv_daemon
doesn't get reconfig_mutex.

etalinux135:~ # ps aux|grep md|grep D
root      2028  0.0  0.0      0     0 ?        D    16:24   0:00 
[md127_raid1]
root      2041  0.0  0.0      0     0 ?        D    16:24   0:00 
[md127_resync]
betalinux135:~ # cat /proc/2028/stack
[<ffffffffa05f2660>] metadata_update_start+0xa0/0xb0 [md_cluster]
[<ffffffffa06cb1ce>] md_update_sb.part.50+0x8e/0x810 [md_mod]
[<ffffffffa06ccb8c>] md_check_recovery+0x23c/0x4f0 [md_mod]
[<ffffffffa06f6312>] raid1d+0x42/0x7d0 [raid1]
[<ffffffffa06c6d10>] md_thread+0x130/0x150 [md_mod]
[<ffffffff810995ed>] kthread+0xbd/0xe0
[<ffffffff815e96bf>] ret_from_fork+0x3f/0x70
[<ffffffff81099530>] kthread+0x0/0xe0
[<ffffffffffffffff>] 0xffffffffffffffff
betalinux135:~ # cat /proc/2041/stack
[<ffffffffa05f24cb>] dlm_lock_sync+0x6b/0x80 [md_cluster]
[<ffffffffa05f2708>] __sendmsg+0x98/0x130 [md_cluster]
[<ffffffffa05f282d>] sendmsg+0x1d/0x30 [md_cluster]
[<ffffffffa05f2b31>] resync_info_update+0x81/0xb0 [md_cluster]
[<ffffffffa06f3ad7>] sync_request+0xa57/0xaf0 [raid1]
[<ffffffffa06ca2dd>] md_do_sync+0x90d/0xe80 [md_mod]
[<ffffffffa06c6d10>] md_thread+0x130/0x150 [md_mod]
[<ffffffff810995ed>] kthread+0xbd/0xe0
[<ffffffff815e96bf>] ret_from_fork+0x3f/0x70
[<ffffffff81099530>] kthread+0x0/0xe0
[<ffffffffffffffff>] 0xffffffffffffffff


Thanks,
Guoqing

Re: [PATCH 3/3] MD: hold mddev lock for md-cluster receive thread

[Shaohua Li]

On Mon, Aug 01, 2016 at 04:38:59PM +0800, Guoqing Jiang wrote:
> Hi,
> 
> On 07/31/2016 07:54 AM, shli@kernel.org wrote:
> >From: Shaohua Li <shli@fb.com>
> >
> >md-cluster receive thread calls .quiesce too, let it hold mddev lock.
> 
> I'd suggest hold on for the patchset, I can find lock problem easily with
> the patchset applied. Take a resyncing  clusteed raid1 as example.
> 
> md127_raid1 thread held reconfig_mutex then update sb, so it needs dlm
> token lock. Meanwhile md127_resync thread got token lock and wants
> EX on ack lock but recv_daemon can't release ack lock since recv_daemon
> doesn't get reconfig_mutex.

Thansk, I'll drop this one. Other two patches are still safe for md-cluster,
right? I really hope to have consistent locking for .quiesce. For the
process_recvd_msg, I'm wondering what's protecting the datas? for example,
md-cluster uses md_find_rdev_nr_rcu, which access the disks list without
locking. Is there a race? Does it work if we move the mddev lock to
process_recvd_msg?

Thanks,
Shaohua

Re: [PATCH 3/3] MD: hold mddev lock for md-cluster receive thread

[Guoqing Jiang]

On 08/02/2016 05:45 AM, Shaohua Li wrote:
> On Mon, Aug 01, 2016 at 04:38:59PM +0800, Guoqing Jiang wrote:
>> Hi,
>>
>> On 07/31/2016 07:54 AM, shli@kernel.org wrote:
>>> From: Shaohua Li <shli@fb.com>
>>>
>>> md-cluster receive thread calls .quiesce too, let it hold mddev lock.
>> I'd suggest hold on for the patchset, I can find lock problem easily with
>> the patchset applied. Take a resyncing  clusteed raid1 as example.
>>
>> md127_raid1 thread held reconfig_mutex then update sb, so it needs dlm
>> token lock. Meanwhile md127_resync thread got token lock and wants
>> EX on ack lock but recv_daemon can't release ack lock since recv_daemon
>> doesn't get reconfig_mutex.
> Thansk, I'll drop this one. Other two patches are still safe for md-cluster,
> right?

 From the latest test, I can't find  lock issues with the first two patches,
but I doubt it would have side effect for the performance of resync.

> I really hope to have consistent locking for .quiesce. For the
> process_recvd_msg, I'm wondering what's protecting the datas? for example,
> md-cluster uses md_find_rdev_nr_rcu, which access the disks list without
> locking. Is there a race?

Yes, it should be protected by rcu lock, I will post a patch for it, thanks
for reminder.

> Does it work if we move the mddev lock to
> process_recvd_msg?

I tried that, but It still have lock issue, eg, when node B and C have 
status
as "resync=PENDING", then try to stop the resyncing array in node A.


Thanks,
Guoqing

Re: [PATCH 3/3] MD: hold mddev lock for md-cluster receive thread

[Shaohua Li]

On Tue, Aug 02, 2016 at 05:52:41PM +0800, Guoqing Jiang wrote:
> 
> 
> On 08/02/2016 05:45 AM, Shaohua Li wrote:
> >On Mon, Aug 01, 2016 at 04:38:59PM +0800, Guoqing Jiang wrote:
> >>Hi,
> >>
> >>On 07/31/2016 07:54 AM, shli@kernel.org wrote:
> >>>From: Shaohua Li <shli@fb.com>
> >>>
> >>>md-cluster receive thread calls .quiesce too, let it hold mddev lock.
> >>I'd suggest hold on for the patchset, I can find lock problem easily with
> >>the patchset applied. Take a resyncing  clusteed raid1 as example.
> >>
> >>md127_raid1 thread held reconfig_mutex then update sb, so it needs dlm
> >>token lock. Meanwhile md127_resync thread got token lock and wants
> >>EX on ack lock but recv_daemon can't release ack lock since recv_daemon
> >>doesn't get reconfig_mutex.
> >Thansk, I'll drop this one. Other two patches are still safe for md-cluster,
> >right?
> 
> From the latest test, I can't find  lock issues with the first two patches,
> but I doubt it would have side effect for the performance of resync.

That's not need to be worried. The .quiesce() call is way heavier than
hold/release the mutex.

> >I really hope to have consistent locking for .quiesce. For the
> >process_recvd_msg, I'm wondering what's protecting the datas? for example,
> >md-cluster uses md_find_rdev_nr_rcu, which access the disks list without
> >locking. Is there a race?
> 
> Yes, it should be protected by rcu lock, I will post a patch for it, thanks
> for reminder.
> 
> >Does it work if we move the mddev lock to
> >process_recvd_msg?
> 
> I tried that, but It still have lock issue, eg, when node B and C have
> status
> as "resync=PENDING", then try to stop the resyncing array in node A.

can you elaborate it?

For the raid5-cache issue, ignoring the md-cluster .quiesce() call is fine
currently as we don't support raid5 cluster. We probably should add another
parameter for .quiesce to indicate if the mddev lock is hold in the future.

Thanks,
Shaohua

Re: [PATCH 3/3] MD: hold mddev lock for md-cluster receive thread

[Guoqing Jiang]

On 08/03/2016 06:44 AM, Shaohua Li wrote:
> On Tue, Aug 02, 2016 at 05:52:41PM +0800, Guoqing Jiang wrote:
>>
>> On 08/02/2016 05:45 AM, Shaohua Li wrote:
>>> On Mon, Aug 01, 2016 at 04:38:59PM +0800, Guoqing Jiang wrote:
>>>> Hi,
>>>>
>>>> On 07/31/2016 07:54 AM, shli@kernel.org wrote:
>>>>> From: Shaohua Li <shli@fb.com>
>>>>>
>>>>> md-cluster receive thread calls .quiesce too, let it hold mddev lock.
>>>> I'd suggest hold on for the patchset, I can find lock problem easily with
>>>> the patchset applied. Take a resyncing  clusteed raid1 as example.
>>>>
>>>> md127_raid1 thread held reconfig_mutex then update sb, so it needs dlm
>>>> token lock. Meanwhile md127_resync thread got token lock and wants
>>>> EX on ack lock but recv_daemon can't release ack lock since recv_daemon
>>>> doesn't get reconfig_mutex.
>>> Thansk, I'll drop this one. Other two patches are still safe for md-cluster,
>>> right?
>>  From the latest test, I can't find  lock issues with the first two patches,
>> but I doubt it would have side effect for the performance of resync.
> That's not need to be worried. The .quiesce() call is way heavier than
> hold/release the mutex.
>
>>> I really hope to have consistent locking for .quiesce. For the
>>> process_recvd_msg, I'm wondering what's protecting the datas? for example,
>>> md-cluster uses md_find_rdev_nr_rcu, which access the disks list without
>>> locking. Is there a race?
>> Yes, it should be protected by rcu lock, I will post a patch for it, thanks
>> for reminder.
>>
>>> Does it work if we move the mddev lock to
>>> process_recvd_msg?
>> I tried that, but It still have lock issue, eg, when node B and C have
>> status
>> as "resync=PENDING", then try to stop the resyncing array in node A.
> can you elaborate it?

I am not lucky enough to do the same test as yesterday, but I even
can't assemble clustered raid1 in other nodes.

1. node135: mdadm --create md0 --bitmap=clustered --raid-devices=2 
--level=mirror /dev/vdb /dev/vdc
2. Then node240 and node244 try to assemble it, but both of them would hang.

betalinux135:~ # cat /proc/mdstat
Personalities : [raid1]
md127 : active raid1 vdc[1] vdb[0]
       2095104 blocks super 1.2 [2/2] [UU]
       [=>...................]  resync =  6.2% (130816/2095104) 
finish=1.5min speed=21802K/sec
       bitmap: 1/1 pages [4KB], 65536KB chunk

unused devices: <none>
betalinux135:~ # ssh betalinux240
Last login: Wed Aug  3 11:11:47 2016 from 192.168.100.1
betalinux240:~ # ps aux|grep md|grep D
root      1901  0.0  0.2  20896  2592 pts/0    D+   11:12   0:00 mdadm 
--assemble md0 /dev/vdb /dev/vdc
root      1914  0.0  0.2  19852  2032 ?        S    11:12   0:00 
/sbin/mdadm --incremental --export /dev/vdb --offroot ${DEVLINKS}
root      1915  0.0  0.1  19852  1940 ?        S    11:12   0:00 
/sbin/mdadm --incremental --export /dev/vdc --offroot ${DEVLINKS}
betalinux240:~ # cat /proc/1901/stack
[<ffffffffa069d4cb>] dlm_lock_sync+0x6b/0x80 [md_cluster]
[<ffffffffa069e7e6>] join+0x286/0x430 [md_cluster]
[<ffffffffa068a9f4>] bitmap_create+0x5f4/0x980 [md_mod]
[<ffffffffa0682f35>] md_run+0x595/0xa60 [md_mod]
[<ffffffffa06834cf>] do_md_run+0xf/0xb0 [md_mod]
[<ffffffffa0686781>] md_ioctl+0x11b1/0x1680 [md_mod]
[<ffffffff812ed158>] blkdev_ioctl+0x258/0x920
[<ffffffff8122f81d>] block_ioctl+0x3d/0x40
[<ffffffff8120ac0d>] do_vfs_ioctl+0x2cd/0x4a0
[<ffffffff8120ae54>] SyS_ioctl+0x74/0x80
[<ffffffff815e936e>] entry_SYSCALL_64_fastpath+0x12/0x6d
[<ffffffffffffffff>] 0xffffffffffffffff
betalinux240:~ # exit
logout
Connection to betalinux240 closed.
betalinux135:~ # ssh betalinux244
Last login: Wed Aug  3 11:11:49 2016 from 192.168.100.1
betalinux244:~ # ps aux|grep md|grep D
root      1903  0.0  0.2  20896  2660 pts/0    D+   11:12   0:00 mdadm 
--assemble md0 /dev/vdb /dev/vdc
root      1923  0.0  0.2  19852  2112 ?        S    11:12   0:00 
/sbin/mdadm --incremental --export /dev/vdc --offroot ${DEVLINKS}
root      1928  0.0  0.2  19852  2092 ?        S    11:12   0:00 
/sbin/mdadm --incremental --export /dev/vdb --offroot ${DEVLINKS}
root      1938  0.0  0.0      0     0 ?        D    11:12   0:00 
[md0_cluster_rec]
betalinux244:~ # cat /proc/1903/stack
[<ffffffffa06904cb>] dlm_lock_sync+0x6b/0x80 [md_cluster]
[<ffffffffa06904fb>] lock_token+0x1b/0x50 [md_cluster]
[<ffffffffa06905fd>] metadata_update_start+0x3d/0xb0 [md_cluster]
[<ffffffffa06751ee>] md_update_sb.part.50+0x8e/0x810 [md_mod]
[<ffffffffa067646e>] md_allow_write+0x6e/0xc0 [md_mod]
[<ffffffffa0676505>] do_md_run+0x45/0xb0 [md_mod]
[<ffffffffa0679781>] md_ioctl+0x11b1/0x1680 [md_mod]
[<ffffffff812ed158>] blkdev_ioctl+0x258/0x920
[<ffffffff8122f81d>] block_ioctl+0x3d/0x40
[<ffffffff8120ac0d>] do_vfs_ioctl+0x2cd/0x4a0
[<ffffffff8120ae54>] SyS_ioctl+0x74/0x80
[<ffffffff815e936e>] entry_SYSCALL_64_fastpath+0x12/0x6d
[<ffffffffffffffff>] 0xffffffffffffffff
betalinux244:~ # cat /proc/1938/stack
[<ffffffffa0691d90>] recv_daemon+0xc0/0x4a0 [md_cluster]
[<ffffffffa0670d30>] md_thread+0x130/0x150 [md_mod]
[<ffffffff810995ed>] kthread+0xbd/0xe0
[<ffffffff815e96bf>] ret_from_fork+0x3f/0x70
[<ffffffff81099530>] kthread+0x0/0xe0
[<ffffffffffffffff>] 0xffffffffffffffff

> For the raid5-cache issue, ignoring the md-cluster .quiesce() call is fine
> currently as we don't support raid5 cluster. We probably should add another
> parameter for .quiesce to indicate if the mddev lock is hold in the future.

Pls update me with the change in future, since it may have huge influence
for md-cluster.

Thanks,
GUoqing

Re: [PATCH 3/3] MD: hold mddev lock for md-cluster receive thread

[NeilBrown]

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

On Tue, Aug 02 2016, Guoqing Jiang wrote:
>
>> Does it work if we move the mddev lock to
>> process_recvd_msg?
>
> I tried that, but It still have lock issue, eg, when node B and C have 
> status
> as "resync=PENDING", then try to stop the resyncing array in node A.
>

Maybe the reconfig_mutex locking needs to go down in
process_suspend_info() ?? That is the only part that calls quiesce.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 3/3] MD: hold mddev lock for md-cluster receive thread

[Guoqing Jiang]

On 08/03/2016 08:09 AM, NeilBrown wrote:
> On Tue, Aug 02 2016, Guoqing Jiang wrote:
>>> Does it work if we move the mddev lock to
>>> process_recvd_msg?
>> I tried that, but It still have lock issue, eg, when node B and C have
>> status
>> as "resync=PENDING", then try to stop the resyncing array in node A.
>>
> Maybe the reconfig_mutex locking needs to go down in
> process_suspend_info() ?? That is the only part that calls quiesce.

Yes, but with this change, I still have lock issue for below steps:
1. create a resyning array in nodeA
2. Assemble the array in node B and nodeC

I can see A can't continue perform resync while C can't assemble
array successfully.

Thanks,
Guoqing

Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[yizhan]

I tested these patch and fixed the bug I reported before[1].

[1] WARNING: CPU: 4 PID: 10512 at drivers/md/raid5-cache.c:728 
r5l_do_reclaim+0x415/0x430 [raid456]

Thanks

Yi Zhang


On 07/31/2016 07:54 AM, shli@kernel.org wrote:
> From: Shaohua Li <shli@fb.com>
>
> .quiesce is called with mddev lock hold at most places. There are few
> exceptions. Calling .quesce without the lock hold could create races. For
> example, the .quesce of raid1 can't be recursively. The purpose of the patches
> is to fix a race in raid5-cache. The raid5-cache .quesce will write md
> superblock and should be called with mddev lock hold.
>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>   drivers/md/md.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 2c3ab6f..0550445 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7945,8 +7945,10 @@ void md_do_sync(struct md_thread *thread)
>   		 * region.
>   		 */
>   		if (mddev->bitmap) {
> +			mddev_lock_nointr(mddev);
>   			mddev->pers->quiesce(mddev, 1);
>   			mddev->pers->quiesce(mddev, 0);
> +			mddev_unlock(mddev);
>   		}
>   	}
>   

Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[NeilBrown]

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

On Sun, Jul 31 2016, shli@kernel.org wrote:

> From: Shaohua Li <shli@fb.com>
>
> .quiesce is called with mddev lock hold at most places. There are few
> exceptions. Calling .quesce without the lock hold could create races. For
> example, the .quesce of raid1 can't be recursively. The purpose of the patches
> is to fix a race in raid5-cache. The raid5-cache .quesce will write md
> superblock and should be called with mddev lock hold.
>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Shaohua Li <shli@fb.com>

Acked-by: NeilBrown <neilb@suse.com>

This should be safe but I'm not sure I really like it.
The raid1 quiesce could be changed so that it can be called recursively.
The raid5-cache situation would be harder to get right and maybe this is
the best solution... It's just that 'quiesce' should be a fairly
light-weight operation, just waiting for pending requests to flush.  It
shouldn't really *need* a lock.

NeilBrown


> ---
>  drivers/md/md.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 2c3ab6f..0550445 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7945,8 +7945,10 @@ void md_do_sync(struct md_thread *thread)
>  		 * region.
>  		 */
>  		if (mddev->bitmap) {
> +			mddev_lock_nointr(mddev);
>  			mddev->pers->quiesce(mddev, 1);
>  			mddev->pers->quiesce(mddev, 0);
> +			mddev_unlock(mddev);
>  		}
>  	}
>  
> -- 
> 2.7.4

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[NeilBrown]

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

On Wed, Aug 03 2016, NeilBrown wrote:

> [ Unknown signature status ]
> On Sun, Jul 31 2016, shli@kernel.org wrote:
>
>> From: Shaohua Li <shli@fb.com>
>>
>> .quiesce is called with mddev lock hold at most places. There are few
>> exceptions. Calling .quesce without the lock hold could create races. For
>> example, the .quesce of raid1 can't be recursively. The purpose of the patches
>> is to fix a race in raid5-cache. The raid5-cache .quesce will write md
>> superblock and should be called with mddev lock hold.
>>
>> Cc: NeilBrown <neilb@suse.com>
>> Signed-off-by: Shaohua Li <shli@fb.com>
>
> Acked-by: NeilBrown <neilb@suse.com>
>
> This should be safe but I'm not sure I really like it.
> The raid1 quiesce could be changed so that it can be called recursively.
> The raid5-cache situation would be harder to get right and maybe this is
> the best solution... It's just that 'quiesce' should be a fairly
> light-weight operation, just waiting for pending requests to flush.  It
> shouldn't really *need* a lock.

Actually, the more I think about this, the less I like it.

I would much rather make .quiesce lighter weight so that no locking was
needed.

For r5l_quiesce, that probable means removed the "r5l_do_reclaim()".
Stopping and restarting the reclaim thread seems reasonable, but calling
r5l_do_reclaim() should not be needed.  It should be done periodically
by the thread, and at 'stop' time, but otherwise isn't needed.
You would need to hold some mutex while calling md_register_thread, but
that could be probably be log->io_mutex, or maybe even some other new
mutex

Could you explore following that path instead?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[Shaohua Li]

On Thu, Aug 04, 2016 at 01:16:49PM +1000, Neil Brown wrote:
> On Wed, Aug 03 2016, NeilBrown wrote:
> 
> > [ Unknown signature status ]
> > On Sun, Jul 31 2016, shli@kernel.org wrote:
> >
> >> From: Shaohua Li <shli@fb.com>
> >>
> >> .quiesce is called with mddev lock hold at most places. There are few
> >> exceptions. Calling .quesce without the lock hold could create races. For
> >> example, the .quesce of raid1 can't be recursively. The purpose of the patches
> >> is to fix a race in raid5-cache. The raid5-cache .quesce will write md
> >> superblock and should be called with mddev lock hold.
> >>
> >> Cc: NeilBrown <neilb@suse.com>
> >> Signed-off-by: Shaohua Li <shli@fb.com>
> >
> > Acked-by: NeilBrown <neilb@suse.com>
> >
> > This should be safe but I'm not sure I really like it.
> > The raid1 quiesce could be changed so that it can be called recursively.
> > The raid5-cache situation would be harder to get right and maybe this is
> > the best solution... It's just that 'quiesce' should be a fairly
> > light-weight operation, just waiting for pending requests to flush.  It
> > shouldn't really *need* a lock.
> 
> Actually, the more I think about this, the less I like it.
> 
> I would much rather make .quiesce lighter weight so that no locking was
> needed.
> 
> For r5l_quiesce, that probable means removed the "r5l_do_reclaim()".
> Stopping and restarting the reclaim thread seems reasonable, but calling
> r5l_do_reclaim() should not be needed.  It should be done periodically
> by the thread, and at 'stop' time, but otherwise isn't needed.
> You would need to hold some mutex while calling md_register_thread, but
> that could be probably be log->io_mutex, or maybe even some other new
> mutex

We will have the same deadlock issue with just stopping/restarting the reclaim
thread. As stopping the thread will wait for the thread, which probably is
doing r5l_do_reclaim and writting the superblock. Since we are writting the
superblock, we must hold the reconfig_mutex.

Letting raid5_quiesce call r5l_do_reclaim gives us a clean log. Just
stop/restart the reclaim thread can't guarantee this, as it's possible some
space aren't reclaimed yet. A clean log will simplify a lot of things, for
example we change the layout of the array. The log doesn't need to remember
which part is for the old layout and which part is the new layout.

I think we can add a new parameter for .quiesce to indicate if reconfig_mutex
is hold. raid5_quiesce can check the parameter and hold reconfig_mutex if
necessary.

Thanks,
Shaohua

Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[NeilBrown]

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

On Sat, Aug 06 2016, Shaohua Li wrote:

> On Thu, Aug 04, 2016 at 01:16:49PM +1000, Neil Brown wrote:
>> On Wed, Aug 03 2016, NeilBrown wrote:
>> 
>> > [ Unknown signature status ]
>> > On Sun, Jul 31 2016, shli@kernel.org wrote:
>> >
>> >> From: Shaohua Li <shli@fb.com>
>> >>
>> >> .quiesce is called with mddev lock hold at most places. There are few
>> >> exceptions. Calling .quesce without the lock hold could create races. For
>> >> example, the .quesce of raid1 can't be recursively. The purpose of the patches
>> >> is to fix a race in raid5-cache. The raid5-cache .quesce will write md
>> >> superblock and should be called with mddev lock hold.
>> >>
>> >> Cc: NeilBrown <neilb@suse.com>
>> >> Signed-off-by: Shaohua Li <shli@fb.com>
>> >
>> > Acked-by: NeilBrown <neilb@suse.com>
>> >
>> > This should be safe but I'm not sure I really like it.
>> > The raid1 quiesce could be changed so that it can be called recursively.
>> > The raid5-cache situation would be harder to get right and maybe this is
>> > the best solution... It's just that 'quiesce' should be a fairly
>> > light-weight operation, just waiting for pending requests to flush.  It
>> > shouldn't really *need* a lock.
>> 
>> Actually, the more I think about this, the less I like it.
>> 
>> I would much rather make .quiesce lighter weight so that no locking was
>> needed.
>> 
>> For r5l_quiesce, that probable means removed the "r5l_do_reclaim()".
>> Stopping and restarting the reclaim thread seems reasonable, but calling
>> r5l_do_reclaim() should not be needed.  It should be done periodically
>> by the thread, and at 'stop' time, but otherwise isn't needed.
>> You would need to hold some mutex while calling md_register_thread, but
>> that could be probably be log->io_mutex, or maybe even some other new
>> mutex
>
> We will have the same deadlock issue with just stopping/restarting the reclaim
> thread. As stopping the thread will wait for the thread, which probably is
> doing r5l_do_reclaim and writting the superblock. Since we are writting the
> superblock, we must hold the reconfig_mutex.

When you say "writing the superblock" you presumably mean "blocked in
r5l_write_super_and_discard_space(), waiting for  MD_CHANGE_PENDING to
be cleared" ??
With a bit of care you could wait for MD_CHANGE_PENDING to clear, or for
->quiesce to be set, and then exit gracefully.

>
> Letting raid5_quiesce call r5l_do_reclaim gives us a clean log. Just
> stop/restart the reclaim thread can't guarantee this, as it's possible some
> space aren't reclaimed yet. A clean log will simplify a lot of things, for
> example we change the layout of the array. The log doesn't need to remember
> which part is for the old layout and which part is the new layout.

I really think you are putting too much functionality into quiesce.
When we change the shape of the array, we do much more than just
quiesce it.  We also call check_reshape and start_reshape etc.
They are called with reconfig_mutex held and it would be perfectly
appropriate to finish of the r5l_do_reclaim() work in there.

>
> I think we can add a new parameter for .quiesce to indicate if reconfig_mutex
> is hold. raid5_quiesce can check the parameter and hold reconfig_mutex if
> necessary.

Adding a new parameter because it happens to be convenient in one case
is not necessarily a good idea.  It is often a sign that the interface
isn't well designed, or isn't well understood, or is being used poorly.

I really really don't think ->quiesce() should care about whether
reconfig_mutex is held.  All it should do is drain all IO and stop new
IO so that other threads can do unusually things in race-free ways.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[Shaohua Li]

On Fri, Aug 12, 2016 at 10:04:05AM +1000, Neil Brown wrote:
> On Sat, Aug 06 2016, Shaohua Li wrote:
> 
> > On Thu, Aug 04, 2016 at 01:16:49PM +1000, Neil Brown wrote:
> >> On Wed, Aug 03 2016, NeilBrown wrote:
> >> 
> >> > [ Unknown signature status ]
> >> > On Sun, Jul 31 2016, shli@kernel.org wrote:
> >> >
> >> >> From: Shaohua Li <shli@fb.com>
> >> >>
> >> >> .quiesce is called with mddev lock hold at most places. There are few
> >> >> exceptions. Calling .quesce without the lock hold could create races. For
> >> >> example, the .quesce of raid1 can't be recursively. The purpose of the patches
> >> >> is to fix a race in raid5-cache. The raid5-cache .quesce will write md
> >> >> superblock and should be called with mddev lock hold.
> >> >>
> >> >> Cc: NeilBrown <neilb@suse.com>
> >> >> Signed-off-by: Shaohua Li <shli@fb.com>
> >> >
> >> > Acked-by: NeilBrown <neilb@suse.com>
> >> >
> >> > This should be safe but I'm not sure I really like it.
> >> > The raid1 quiesce could be changed so that it can be called recursively.
> >> > The raid5-cache situation would be harder to get right and maybe this is
> >> > the best solution... It's just that 'quiesce' should be a fairly
> >> > light-weight operation, just waiting for pending requests to flush.  It
> >> > shouldn't really *need* a lock.
> >> 
> >> Actually, the more I think about this, the less I like it.
> >> 
> >> I would much rather make .quiesce lighter weight so that no locking was
> >> needed.
> >> 
> >> For r5l_quiesce, that probable means removed the "r5l_do_reclaim()".
> >> Stopping and restarting the reclaim thread seems reasonable, but calling
> >> r5l_do_reclaim() should not be needed.  It should be done periodically
> >> by the thread, and at 'stop' time, but otherwise isn't needed.
> >> You would need to hold some mutex while calling md_register_thread, but
> >> that could be probably be log->io_mutex, or maybe even some other new
> >> mutex
> >
> > We will have the same deadlock issue with just stopping/restarting the reclaim
> > thread. As stopping the thread will wait for the thread, which probably is
> > doing r5l_do_reclaim and writting the superblock. Since we are writting the
> > superblock, we must hold the reconfig_mutex.
> 
> When you say "writing the superblock" you presumably mean "blocked in
> r5l_write_super_and_discard_space(), waiting for  MD_CHANGE_PENDING to
> be cleared" ??
right
> With a bit of care you could wait for MD_CHANGE_PENDING to clear, or for
> ->quiesce to be set, and then exit gracefully.

Can you give details about this please? .quiesce is called with reconfig_mutex
hold, so the MD_CHANGE_PENDING will never get cleared.

> >
> > Letting raid5_quiesce call r5l_do_reclaim gives us a clean log. Just
> > stop/restart the reclaim thread can't guarantee this, as it's possible some
> > space aren't reclaimed yet. A clean log will simplify a lot of things, for
> > example we change the layout of the array. The log doesn't need to remember
> > which part is for the old layout and which part is the new layout.
> 
> I really think you are putting too much functionality into quiesce.
> When we change the shape of the array, we do much more than just
> quiesce it.  We also call check_reshape and start_reshape etc.
> They are called with reconfig_mutex held and it would be perfectly
> appropriate to finish of the r5l_do_reclaim() work in there.

This makes sense. But I think we don't need worry 'finish of the
r5l_do_reclaim()' does too much things. In most cases, stopping the reclaim
thread will already finish all reclaim.

> >
> > I think we can add a new parameter for .quiesce to indicate if reconfig_mutex
> > is hold. raid5_quiesce can check the parameter and hold reconfig_mutex if
> > necessary.
> 
> Adding a new parameter because it happens to be convenient in one case
> is not necessarily a good idea.  It is often a sign that the interface
> isn't well designed, or isn't well understood, or is being used poorly.
> 
> I really really don't think ->quiesce() should care about whether
> reconfig_mutex is held.  All it should do is drain all IO and stop new
> IO so that other threads can do unusually things in race-free ways.

I agree this isn't a good interface, but I don't have a better solution for
this issue. Ingore reshape now. It's possible .quiesce and reclaim thread could
deadlock. One thread hold reconfig mutex and call raid5_quiesce(), which will
wait for IO finish. reclaim thread write super (wait for reconfig mutex), free
log space and then IO write can finish. So the first thread hold reconfig mutex
and wait reclaim thread to finish IO, while reclaim thread waits for reconfig
mutex.

Thanks,
Shaohua

Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[NeilBrown]

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

On Wed, Aug 17 2016, Shaohua Li wrote:
>> >
>> > We will have the same deadlock issue with just stopping/restarting the reclaim
>> > thread. As stopping the thread will wait for the thread, which probably is
>> > doing r5l_do_reclaim and writting the superblock. Since we are writting the
>> > superblock, we must hold the reconfig_mutex.
>> 
>> When you say "writing the superblock" you presumably mean "blocked in
>> r5l_write_super_and_discard_space(), waiting for  MD_CHANGE_PENDING to
>> be cleared" ??
> right
>> With a bit of care you could wait for MD_CHANGE_PENDING to clear, or for
>> ->quiesce to be set, and then exit gracefully.
>
> Can you give details about this please? .quiesce is called with reconfig_mutex
> hold, so the MD_CHANGE_PENDING will never get cleared.

raid5_quiesce(mddev, 1) sets conf->quiesce and then calls r5l_quiesce().

r5l_quiesce() tells the reclaim_thread to exit and waits for it to do so.

But the reclaim thread might be in
   r5l_do_reclaim() -> r5l_write_super_and_discard_space()
waiting for MD_CHANGE_PENDING to clear.  That will only get cleared when
the main thread can get the reconfig_mutex, which the thread calling
raid5_quiesce() might hold.  So we get a deadlock.

My suggestion is to change r5l_write_super_and_discard_space() so that
it waits for *either* MD_CHANGE_PENDING to be clear, or conf->quiesce
to be set.  That will avoid the deadlock.

Whatever thread called raid5_quiesce() will now be in control of the
array without any async IO going on.  If it needs the metadata to be
sync, it can do that itself.  If not, then it doesn't really matter that
r5l_write_super_and_discard_space() didn't wait.

r5l_write_super_and_discard_space() shouldn't call discard if the
superblock write didn't complete, and probably r5l_do_reclaim()
shouldn't update last_checkpoint and last_cp_seq in that case.
This is what I mean by "with a bit of care" and "exit gracefully".
Maybe I should have said "abort cleanly".  The goal is to get the thread
to exit.  It doesn't need to complete what it was doing, it just needs
to make sure that it leaves things in a tidy state so that when it
starts up again, it can pick up where it left off.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[Shaohua Li]

On Wed, Aug 24, 2016 at 02:49:57PM +1000, Neil Brown wrote:
> On Wed, Aug 17 2016, Shaohua Li wrote:
> >> >
> >> > We will have the same deadlock issue with just stopping/restarting the reclaim
> >> > thread. As stopping the thread will wait for the thread, which probably is
> >> > doing r5l_do_reclaim and writting the superblock. Since we are writting the
> >> > superblock, we must hold the reconfig_mutex.
> >> 
> >> When you say "writing the superblock" you presumably mean "blocked in
> >> r5l_write_super_and_discard_space(), waiting for  MD_CHANGE_PENDING to
> >> be cleared" ??
> > right
> >> With a bit of care you could wait for MD_CHANGE_PENDING to clear, or for
> >> ->quiesce to be set, and then exit gracefully.
> >
> > Can you give details about this please? .quiesce is called with reconfig_mutex
> > hold, so the MD_CHANGE_PENDING will never get cleared.
> 
> raid5_quiesce(mddev, 1) sets conf->quiesce and then calls r5l_quiesce().
> 
> r5l_quiesce() tells the reclaim_thread to exit and waits for it to do so.
> 
> But the reclaim thread might be in
>    r5l_do_reclaim() -> r5l_write_super_and_discard_space()
> waiting for MD_CHANGE_PENDING to clear.  That will only get cleared when
> the main thread can get the reconfig_mutex, which the thread calling
> raid5_quiesce() might hold.  So we get a deadlock.
> 
> My suggestion is to change r5l_write_super_and_discard_space() so that
> it waits for *either* MD_CHANGE_PENDING to be clear, or conf->quiesce
> to be set.  That will avoid the deadlock.
> 
> Whatever thread called raid5_quiesce() will now be in control of the
> array without any async IO going on.  If it needs the metadata to be
> sync, it can do that itself.  If not, then it doesn't really matter that
> r5l_write_super_and_discard_space() didn't wait.

I'm afraid waiting conf->quiesce set isn't safe. The reason to wait for
superblock write isn't because of async IO. discard could zero data, so before
we do discard, we must make sure superblock points to correct log tail,
otherwise recovery will not work. This is the reason we wait for superblock
write.

> r5l_write_super_and_discard_space() shouldn't call discard if the
> superblock write didn't complete, and probably r5l_do_reclaim()
> shouldn't update last_checkpoint and last_cp_seq in that case.
> This is what I mean by "with a bit of care" and "exit gracefully".
> Maybe I should have said "abort cleanly".  The goal is to get the thread
> to exit.  It doesn't need to complete what it was doing, it just needs
> to make sure that it leaves things in a tidy state so that when it
> starts up again, it can pick up where it left off.

Agree, we could ignore discard sometime, which happens occasionally, so impact
is little. I tested something like below recently. Assume this is the solution
we agree on?


diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 5504ce2..cd34e66 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -96,7 +96,6 @@ struct r5l_log {
 	spinlock_t no_space_stripes_lock;
 
 	bool need_cache_flush;
-	bool in_teardown;
 };
 
 /*
@@ -703,32 +702,22 @@ static void r5l_write_super_and_discard_space(struct r5l_log *log,
 		return;
 
 	mddev = log->rdev->mddev;
+
 	/*
-	 * This is to avoid a deadlock. r5l_quiesce holds reconfig_mutex and
-	 * wait for this thread to finish. This thread waits for
-	 * MD_CHANGE_PENDING clear, which is supposed to be done in
-	 * md_check_recovery(). md_check_recovery() tries to get
-	 * reconfig_mutex. Since r5l_quiesce already holds the mutex,
-	 * md_check_recovery() fails, so the PENDING never get cleared. The
-	 * in_teardown check workaround this issue.
+	 * Discard could zero data, so before discard we must make sure
+	 * superblock is updated to new log tail. Updating superblock (either
+	 * directly call md_update_sb() or depend on md thread) must hold
+	 * reconfig mutex. On the other hand, raid5_quiesce is called with
+	 * reconfig_mutex hold. The first step of raid5_quiesce() is waitting
+	 * for all IO finish, hence waitting for reclaim thread, while reclaim
+	 * thread is calling this function and waitting for reconfig mutex. So
+	 * there is a deadlock. We workaround this issue with a trylock.
+	 * FIXME: we could miss discard if we can't take reconfig mutex
 	 */
-	if (!log->in_teardown) {
-		set_mask_bits(&mddev->flags, 0,
-			      BIT(MD_CHANGE_DEVS) | BIT(MD_CHANGE_PENDING));
-		md_wakeup_thread(mddev->thread);
-		wait_event(mddev->sb_wait,
-			!test_bit(MD_CHANGE_PENDING, &mddev->flags) ||
-			log->in_teardown);
-		/*
-		 * r5l_quiesce could run after in_teardown check and hold
-		 * mutex first. Superblock might get updated twice.
-		 */
-		if (log->in_teardown)
-			md_update_sb(mddev, 1);
-	} else {
-		WARN_ON(!mddev_is_locked(mddev));
-		md_update_sb(mddev, 1);
-	}
+	if (!mddev_trylock(mddev))
+		return;
+	md_update_sb(mddev, 1);
+	mddev_unlock(mddev);
 
 	/* discard IO error really doesn't matter, ignore it */
 	if (log->last_checkpoint < end) {
@@ -827,7 +816,6 @@ void r5l_quiesce(struct r5l_log *log, int state)
 	if (!log || state == 2)
 		return;
 	if (state == 0) {
-		log->in_teardown = 0;
 		/*
 		 * This is a special case for hotadd. In suspend, the array has
 		 * no journal. In resume, journal is initialized as well as the
@@ -838,11 +826,6 @@ void r5l_quiesce(struct r5l_log *log, int state)
 		log->reclaim_thread = md_register_thread(r5l_reclaim_thread,
 					log->rdev->mddev, "reclaim");
 	} else if (state == 1) {
-		/*
-		 * at this point all stripes are finished, so io_unit is at
-		 * least in STRIPE_END state
-		 */
-		log->in_teardown = 1;
 		/* make sure r5l_write_super_and_discard_space exits */
 		mddev = log->rdev->mddev;
 		wake_up(&mddev->sb_wait);

Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[NeilBrown]

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

On Wed, Aug 24 2016, Shaohua Li wrote:

> On Wed, Aug 24, 2016 at 02:49:57PM +1000, Neil Brown wrote:
>> On Wed, Aug 17 2016, Shaohua Li wrote:
>> >> >
>> >> > We will have the same deadlock issue with just stopping/restarting the reclaim
>> >> > thread. As stopping the thread will wait for the thread, which probably is
>> >> > doing r5l_do_reclaim and writting the superblock. Since we are writting the
>> >> > superblock, we must hold the reconfig_mutex.
>> >> 
>> >> When you say "writing the superblock" you presumably mean "blocked in
>> >> r5l_write_super_and_discard_space(), waiting for  MD_CHANGE_PENDING to
>> >> be cleared" ??
>> > right
>> >> With a bit of care you could wait for MD_CHANGE_PENDING to clear, or for
>> >> ->quiesce to be set, and then exit gracefully.
>> >
>> > Can you give details about this please? .quiesce is called with reconfig_mutex
>> > hold, so the MD_CHANGE_PENDING will never get cleared.
>> 
>> raid5_quiesce(mddev, 1) sets conf->quiesce and then calls r5l_quiesce().
>> 
>> r5l_quiesce() tells the reclaim_thread to exit and waits for it to do so.
>> 
>> But the reclaim thread might be in
>>    r5l_do_reclaim() -> r5l_write_super_and_discard_space()
>> waiting for MD_CHANGE_PENDING to clear.  That will only get cleared when
>> the main thread can get the reconfig_mutex, which the thread calling
>> raid5_quiesce() might hold.  So we get a deadlock.
>> 
>> My suggestion is to change r5l_write_super_and_discard_space() so that
>> it waits for *either* MD_CHANGE_PENDING to be clear, or conf->quiesce
>> to be set.  That will avoid the deadlock.
>> 
>> Whatever thread called raid5_quiesce() will now be in control of the
>> array without any async IO going on.  If it needs the metadata to be
>> sync, it can do that itself.  If not, then it doesn't really matter that
>> r5l_write_super_and_discard_space() didn't wait.
>
> I'm afraid waiting conf->quiesce set isn't safe. The reason to wait for
> superblock write isn't because of async IO. discard could zero data, so before
> we do discard, we must make sure superblock points to correct log tail,
> otherwise recovery will not work. This is the reason we wait for superblock
> write.
>
>> r5l_write_super_and_discard_space() shouldn't call discard if the
>> superblock write didn't complete, and probably r5l_do_reclaim()
>> shouldn't update last_checkpoint and last_cp_seq in that case.
>> This is what I mean by "with a bit of care" and "exit gracefully".
>> Maybe I should have said "abort cleanly".  The goal is to get the thread
>> to exit.  It doesn't need to complete what it was doing, it just needs
>> to make sure that it leaves things in a tidy state so that when it
>> starts up again, it can pick up where it left off.
>
> Agree, we could ignore discard sometime, which happens occasionally, so impact
> is little. I tested something like below recently. Assume this is the solution
> we agree on?

Yes, this definitely looks like it is heading in the right direction.

I thought that

> -		set_mask_bits(&mddev->flags, 0,
> -			      BIT(MD_CHANGE_DEVS) | BIT(MD_CHANGE_PENDING));
> -		md_wakeup_thread(mddev->thread);

would still be there in the case that the lock cannot be claimed.

You could even record the ->events value before setting the flags,
and record the range that needs to be discarded.  Next time
r5l_do_reclaim is entered, if ->events has moved on, then it should be
safe to discard the recorded range.  Maybe.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[Shaohua Li]

On Thu, Aug 25, 2016 at 02:59:13PM +1000, Neil Brown wrote:
> On Wed, Aug 24 2016, Shaohua Li wrote:
> 
> > On Wed, Aug 24, 2016 at 02:49:57PM +1000, Neil Brown wrote:
> >> On Wed, Aug 17 2016, Shaohua Li wrote:
> >> >> >
> >> >> > We will have the same deadlock issue with just stopping/restarting the reclaim
> >> >> > thread. As stopping the thread will wait for the thread, which probably is
> >> >> > doing r5l_do_reclaim and writting the superblock. Since we are writting the
> >> >> > superblock, we must hold the reconfig_mutex.
> >> >> 
> >> >> When you say "writing the superblock" you presumably mean "blocked in
> >> >> r5l_write_super_and_discard_space(), waiting for  MD_CHANGE_PENDING to
> >> >> be cleared" ??
> >> > right
> >> >> With a bit of care you could wait for MD_CHANGE_PENDING to clear, or for
> >> >> ->quiesce to be set, and then exit gracefully.
> >> >
> >> > Can you give details about this please? .quiesce is called with reconfig_mutex
> >> > hold, so the MD_CHANGE_PENDING will never get cleared.
> >> 
> >> raid5_quiesce(mddev, 1) sets conf->quiesce and then calls r5l_quiesce().
> >> 
> >> r5l_quiesce() tells the reclaim_thread to exit and waits for it to do so.
> >> 
> >> But the reclaim thread might be in
> >>    r5l_do_reclaim() -> r5l_write_super_and_discard_space()
> >> waiting for MD_CHANGE_PENDING to clear.  That will only get cleared when
> >> the main thread can get the reconfig_mutex, which the thread calling
> >> raid5_quiesce() might hold.  So we get a deadlock.
> >> 
> >> My suggestion is to change r5l_write_super_and_discard_space() so that
> >> it waits for *either* MD_CHANGE_PENDING to be clear, or conf->quiesce
> >> to be set.  That will avoid the deadlock.
> >> 
> >> Whatever thread called raid5_quiesce() will now be in control of the
> >> array without any async IO going on.  If it needs the metadata to be
> >> sync, it can do that itself.  If not, then it doesn't really matter that
> >> r5l_write_super_and_discard_space() didn't wait.
> >
> > I'm afraid waiting conf->quiesce set isn't safe. The reason to wait for
> > superblock write isn't because of async IO. discard could zero data, so before
> > we do discard, we must make sure superblock points to correct log tail,
> > otherwise recovery will not work. This is the reason we wait for superblock
> > write.
> >
> >> r5l_write_super_and_discard_space() shouldn't call discard if the
> >> superblock write didn't complete, and probably r5l_do_reclaim()
> >> shouldn't update last_checkpoint and last_cp_seq in that case.
> >> This is what I mean by "with a bit of care" and "exit gracefully".
> >> Maybe I should have said "abort cleanly".  The goal is to get the thread
> >> to exit.  It doesn't need to complete what it was doing, it just needs
> >> to make sure that it leaves things in a tidy state so that when it
> >> starts up again, it can pick up where it left off.
> >
> > Agree, we could ignore discard sometime, which happens occasionally, so impact
> > is little. I tested something like below recently. Assume this is the solution
> > we agree on?
> 
> Yes, this definitely looks like it is heading in the right direction.
> 
> I thought that
> 
> > -		set_mask_bits(&mddev->flags, 0,
> > -			      BIT(MD_CHANGE_DEVS) | BIT(MD_CHANGE_PENDING));
> > -		md_wakeup_thread(mddev->thread);
> 
> would still be there in the case that the lock cannot be claimed.

yep, this makes sense.
> You could even record the ->events value before setting the flags,
> and record the range that needs to be discarded.  Next time
> r5l_do_reclaim is entered, if ->events has moved on, then it should be
> safe to discard the recorded range.  Maybe.

I thought something like this too, but looks there are more works to do to make
this happen. We updated the log, so the range could be reused soon. And if it's
a raid array stop, we don't have the chance to reenter reclaim, which I believe
it's the most common case the lock can't be hold. And missing discard isn't a
big issue especially since the miss happens rarely. I'm going to commit below
if no objection.

Thanks,
Shaohua


commit 93e297c0b152667cc4a17db6fe7360dab7e3e9d5
Author: Shaohua Li <shli@fb.com>
Date:   Thu Aug 25 10:09:39 2016 -0700

    raid5-cache: fix a deadlock in superblock write
    
    There is a potential deadlock in superblock write. Discard could zero data, so
    before discard we must make sure superblock is updated to new log tail.
    Updating superblock (either directly call md_update_sb() or depend on md
    thread) must hold reconfig mutex. On the other hand, raid5_quiesce is called
    with reconfig_mutex hold. The first step of raid5_quiesce() is waitting for all
    IO finish, hence waitting for reclaim thread, while reclaim thread is calling
    this function and waitting for reconfig mutex. So there is a deadlock. We
    workaround this issue with a trylock. The downside of the solution is we could
    miss discard if we can't take reconfig mutex. But this should happen rarely
    (mainly in raid array stop), so miss discard shouldn't be a big problem.
    
    Cc: NeilBrown <neilb@suse.com>
    Signed-off-by: Shaohua Li <shli@fb.com>

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 5504ce2..2b0589f 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -96,7 +96,6 @@ struct r5l_log {
 	spinlock_t no_space_stripes_lock;
 
 	bool need_cache_flush;
-	bool in_teardown;
 };
 
 /*
@@ -704,31 +703,22 @@ static void r5l_write_super_and_discard_space(struct r5l_log *log,
 
 	mddev = log->rdev->mddev;
 	/*
-	 * This is to avoid a deadlock. r5l_quiesce holds reconfig_mutex and
-	 * wait for this thread to finish. This thread waits for
-	 * MD_CHANGE_PENDING clear, which is supposed to be done in
-	 * md_check_recovery(). md_check_recovery() tries to get
-	 * reconfig_mutex. Since r5l_quiesce already holds the mutex,
-	 * md_check_recovery() fails, so the PENDING never get cleared. The
-	 * in_teardown check workaround this issue.
+	 * Discard could zero data, so before discard we must make sure
+	 * superblock is updated to new log tail. Updating superblock (either
+	 * directly call md_update_sb() or depend on md thread) must hold
+	 * reconfig mutex. On the other hand, raid5_quiesce is called with
+	 * reconfig_mutex hold. The first step of raid5_quiesce() is waitting
+	 * for all IO finish, hence waitting for reclaim thread, while reclaim
+	 * thread is calling this function and waitting for reconfig mutex. So
+	 * there is a deadlock. We workaround this issue with a trylock.
+	 * FIXME: we could miss discard if we can't take reconfig mutex
 	 */
-	if (!log->in_teardown) {
-		set_mask_bits(&mddev->flags, 0,
-			      BIT(MD_CHANGE_DEVS) | BIT(MD_CHANGE_PENDING));
-		md_wakeup_thread(mddev->thread);
-		wait_event(mddev->sb_wait,
-			!test_bit(MD_CHANGE_PENDING, &mddev->flags) ||
-			log->in_teardown);
-		/*
-		 * r5l_quiesce could run after in_teardown check and hold
-		 * mutex first. Superblock might get updated twice.
-		 */
-		if (log->in_teardown)
-			md_update_sb(mddev, 1);
-	} else {
-		WARN_ON(!mddev_is_locked(mddev));
-		md_update_sb(mddev, 1);
-	}
+	set_mask_bits(&mddev->flags, 0,
+		BIT(MD_CHANGE_DEVS) | BIT(MD_CHANGE_PENDING));
+	if (!mddev_trylock(mddev))
+		return;
+	md_update_sb(mddev, 1);
+	mddev_unlock(mddev);
 
 	/* discard IO error really doesn't matter, ignore it */
 	if (log->last_checkpoint < end) {
@@ -827,7 +817,6 @@ void r5l_quiesce(struct r5l_log *log, int state)
 	if (!log || state == 2)
 		return;
 	if (state == 0) {
-		log->in_teardown = 0;
 		/*
 		 * This is a special case for hotadd. In suspend, the array has
 		 * no journal. In resume, journal is initialized as well as the
@@ -838,11 +827,6 @@ void r5l_quiesce(struct r5l_log *log, int state)
 		log->reclaim_thread = md_register_thread(r5l_reclaim_thread,
 					log->rdev->mddev, "reclaim");
 	} else if (state == 1) {
-		/*
-		 * at this point all stripes are finished, so io_unit is at
-		 * least in STRIPE_END state
-		 */
-		log->in_teardown = 1;
 		/* make sure r5l_write_super_and_discard_space exits */
 		mddev = log->rdev->mddev;
 		wake_up(&mddev->sb_wait);