raid1: only write mismatch sectors in sync

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

...

Quoted 7 lines

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

For clarity: the risk you are talking about is that the sync will result in more reads, as well as more CPU cycles spent comparing data. Is that right? I.e. there is no risk whatsoever to data integrity?

Can you comment on the magnitude of this risk? For example, if this functionality is inadvertently applied to hard disks, and assuming that the components reside on separate spindles (which is a safe bet), wouldn't those reads happen in parallel thereby not significantly contributing to the slowdown? In other words: the principal component of the slowdown is the in-memory data comparison?

But isn't there then an upside resulting from the avoidance of some writes where data is found to already match?

2012/7/28 Jan Ceuleers <[email protected]>:

Quoted 10 lines · Shaohua Li

...

Quoted 7 lines

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Quoted 5 lines

For clarity: the risk you are talking about is that the sync will result in more reads, as well as more CPU cycles spent comparing data. Is that right? I.e. there is no risk whatsoever to data integrity?

Can you comment on the magnitude of this risk? For example, if this functionality is inadvertently applied to hard disks, and assuming that the components reside on separate spindles (which is a safe bet), wouldn't those reads happen in parallel thereby not significantly contributing to the slowdown? In other words: the principal component of the slowdown is the in-memory data comparison?

But isn't there then an upside resulting from the avoidance of some writes where data is found to already match?

data integrity should be guaranteed. To do sync, without the patch, we read disk1, write disk 2. With it,
we read disk1/disk2, memory compare, if match, skip write, otherwise, write disk2. So if data match,
the tradeoff is a memory compare (and write is replaced with read). If not match, the tradeoff is
memory compare and extra read.

Thanks,
Shaohua
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

2012/7/31 NeilBrown <[email protected]>:

Quoted 49 lines · On Thu, 26 Jul 2012 16:01:50 +0800 Shaohua Li <[email protected]> wrote:

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

Quoted 6 lines

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

It's true, the behavior likes MD_RECOVERY_REQUESTED, ie, read all disks, only do write if two disk data not match. But I hope it works as soon as the array is assembled. Surely we can set in the metadata, but I didn't want to enable it by default, since even with SSD, the read-compare-write isn't optimal some times, because it involves extra IO read and memory compare.

It appears MD_RECOVERY_REQUESTED assumes disks are insync. It doesn't read disk !insync, so it doesn't work for assemble.

In my mind, user frozen the sync first (with sync_action setting), and then enable read-compare-write, and finally continue the sync. I can't stop a recovery and set MD_RECOVERY_REQUESTED, so I added a flag. Anyway, please suggest what the preferred way for this.

The second patch is for a special case. two disks data don't match, but disk1 data are all 0. I'd like to do trim for disk2, instead of write 0 to disk2,
this can be helpful for SSD.

Good to know raid10 already does read-compare-write. From the comments, looks it only supports resync not recovery.

Thanks,
Shaohua

Quoted 66 lines · On Tue, 31 Jul 2012 16:12:04 +0800 Shaohua Li <[email protected]> wrote:

2012/7/31 NeilBrown <[email protected]>:

Quoted 49 lines · On Thu, 26 Jul 2012 16:01:50 +0800 Shaohua Li <[email protected]> wrote:

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

Quoted 6 lines

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

It's true, the behavior likes MD_RECOVERY_REQUESTED, ie, read all disks, only do write if two disk data not match. But I hope it works as soon as the array is assembled. Surely we can set in the metadata, but I didn't want to enable it by default, since even with SSD, the read-compare-write isn't optimal some times, because it involves extra IO read and memory compare.

It appears MD_RECOVERY_REQUESTED assumes disks are insync. It doesn't read disk !insync, so it doesn't work for assemble.

Quoted 4 lines

In my mind, user frozen the sync first (with sync_action setting), and then enable read-compare-write, and finally continue the sync. I can't stop a recovery and set MD_RECOVERY_REQUESTED, so I added a flag. Anyway, please suggest what the preferred way for this.

I guess I just don't find this functionality at all convincing. It isn't clear when anyone would use it, or how they would use it. It seems best not to provide it.

Quoted 68 lines · NeilBrown

Quoted 66 lines · On Tue, 31 Jul 2012 16:12:04 +0800 Shaohua Li <[email protected]> wrote:

2012/7/31 NeilBrown <[email protected]>:

Quoted 49 lines · On Thu, 26 Jul 2012 16:01:50 +0800 Shaohua Li <[email protected]> wrote:

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

Quoted 6 lines

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

It's true, the behavior likes MD_RECOVERY_REQUESTED, ie, read all disks, only do write if two disk data not match. But I hope it works as soon as the array is assembled. Surely we can set in the metadata, but I didn't want to enable it by default, since even with SSD, the read-compare-write isn't optimal some times, because it involves extra IO read and memory compare.

It appears MD_RECOVERY_REQUESTED assumes disks are insync. It doesn't read disk !insync, so it doesn't work for assemble.

Quoted 8 lines

Quoted 4 lines

In my mind, user frozen the sync first (with sync_action setting), and then enable read-compare-write, and finally continue the sync. I can't stop a recovery and set MD_RECOVERY_REQUESTED, so I added a flag. Anyway, please suggest what the preferred way for this.

I guess I just don't find this functionality at all convincing. It isn't clear when anyone would use it, or how they would use it. It seems best not to provide it.

The background is: For SSD, writting a hard formatted disk and fully filled disk, the first case could be 300% faster than the second case depending on SSD firmware and how fragmented the filled disk is. So this isn't a trival performance issue.

The usage model is something like this: one disk is borken, add a new disk to the raid. Currently we copy the whole disk of the first disk to the second. For SSD, if the first disk and second disk have most data identical or most content of the first is 0, the copy is very bad. In this scenario, we can avoid some copy, which will make latter write to the disk faster. Thinking about 300% faster :).

I don't think we can do this in underlayer. We don't want to do it always. Detecting 0 is very expensive.

And this isn't intrusive to me. We only do the 'copy avoidness' in sync, and sync is rare event.

I hope to convince you this is a useful functionality, then we can discuss the implementation.

Quoted 90 lines · On Wed, 12 Sep 2012 13:29:41 +0800 Shaohua Li <[email protected]> wrote:

Quoted 68 lines · NeilBrown

Quoted 66 lines · On Tue, 31 Jul 2012 16:12:04 +0800 Shaohua Li <[email protected]> wrote:

2012/7/31 NeilBrown <[email protected]>:

Quoted 49 lines · On Thu, 26 Jul 2012 16:01:50 +0800 Shaohua Li <[email protected]> wrote:

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

Quoted 6 lines

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

It's true, the behavior likes MD_RECOVERY_REQUESTED, ie, read all disks, only do write if two disk data not match. But I hope it works as soon as the array is assembled. Surely we can set in the metadata, but I didn't want to enable it by default, since even with SSD, the read-compare-write isn't optimal some times, because it involves extra IO read and memory compare.

It appears MD_RECOVERY_REQUESTED assumes disks are insync. It doesn't read disk !insync, so it doesn't work for assemble.

Quoted 8 lines

Quoted 4 lines

In my mind, user frozen the sync first (with sync_action setting), and then enable read-compare-write, and finally continue the sync. I can't stop a recovery and set MD_RECOVERY_REQUESTED, so I added a flag. Anyway, please suggest what the preferred way for this.

I guess I just don't find this functionality at all convincing. It isn't clear when anyone would use it, or how they would use it. It seems best not to provide it.

The background is: For SSD, writting a hard formatted disk and fully filled disk, the first case could be 300% faster than the second case depending on SSD firmware and how fragmented the filled disk is. So this isn't a trival performance issue.

The usage model is something like this: one disk is borken, add a new disk to the raid. Currently we copy the whole disk of the first disk to the second. For SSD, if the first disk and second disk have most data identical or most content of the first is 0, the copy is very bad. In this scenario, we can avoid some copy, which will make latter write to the disk faster. Thinking about 300% faster :).

Quoted 9 lines

I don't think we can do this in underlayer. We don't want to do it always. Detecting 0 is very expensive.

And this isn't intrusive to me. We only do the 'copy avoidness' in sync, and sync is rare event.

I hope to convince you this is a useful functionality, then we can discuss the implementation.

Let's start with "when would someone use it".

You said that you don't want to make it the default. If something isn't a default it will not be used very often, and will only be used at all if it is reasonably easy to use.
So how would someone use this? What would make them choose to use it, and what action would that take to make it happen?

Quoted 102 lines · NeilBrown

Quoted 90 lines · On Wed, 12 Sep 2012 13:29:41 +0800 Shaohua Li <[email protected]> wrote:

Quoted 68 lines · NeilBrown

Quoted 66 lines · On Tue, 31 Jul 2012 16:12:04 +0800 Shaohua Li <[email protected]> wrote:

2012/7/31 NeilBrown <[email protected]>:

Quoted 49 lines · On Thu, 26 Jul 2012 16:01:50 +0800 Shaohua Li <[email protected]> wrote:

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

Quoted 6 lines

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

It's true, the behavior likes MD_RECOVERY_REQUESTED, ie, read all disks, only do write if two disk data not match. But I hope it works as soon as the array is assembled. Surely we can set in the metadata, but I didn't want to enable it by default, since even with SSD, the read-compare-write isn't optimal some times, because it involves extra IO read and memory compare.

It appears MD_RECOVERY_REQUESTED assumes disks are insync. It doesn't read disk !insync, so it doesn't work for assemble.

Quoted 8 lines

Quoted 4 lines

In my mind, user frozen the sync first (with sync_action setting), and then enable read-compare-write, and finally continue the sync. I can't stop a recovery and set MD_RECOVERY_REQUESTED, so I added a flag. Anyway, please suggest what the preferred way for this.

I guess I just don't find this functionality at all convincing. It isn't clear when anyone would use it, or how they would use it. It seems best not to provide it.

The background is: For SSD, writting a hard formatted disk and fully filled disk, the first case could be 300% faster than the second case depending on SSD firmware and how fragmented the filled disk is. So this isn't a trival performance issue.

The usage model is something like this: one disk is borken, add a new disk to the raid. Currently we copy the whole disk of the first disk to the second. For SSD, if the first disk and second disk have most data identical or most content of the first is 0, the copy is very bad. In this scenario, we can avoid some copy, which will make latter write to the disk faster. Thinking about 300% faster :).

Quoted 9 lines

I don't think we can do this in underlayer. We don't want to do it always. Detecting 0 is very expensive.

And this isn't intrusive to me. We only do the 'copy avoidness' in sync, and sync is rare event.

I hope to convince you this is a useful functionality, then we can discuss the implementation.

Quoted 7 lines

Let's start with "when would someone use it".

You said that you don't want to make it the default. If something isn't a default it will not be used very often, and will only be used at all if it is reasonably easy to use.
So how would someone use this? What would make them choose to use it, and what action would that take to make it happen?

Ok, let me explain the usage model.

For 'compare and avoid write if equal' case:
1. update SSD firmware. This doesn't change the data, but we need take one disk off from the raid one time.
2. One disk has errors, but these errors don't ruin most of the data (for example, a pcie error)
3. driver/os crash.
In all these cases, two raid disks must be resync, and they have almost identical data. write avoidness will be very helpful for these.

For 'compare and trim if source disk data is 0' case:
If filesystem support trim, this can be enabled always (or at least we can enable it if used capacity of the disk is less than 80% for example).

User enables these features and then add disk to downgrade raid. raid resync then avoids write or does trim.

Does this make sense?

Thanks,
Shaohua

Quoted 113 lines · On Wed, 19 Sep 2012 13:51:06 +0800 Shaohua Li <[email protected]> wrote:

Quoted 102 lines · NeilBrown

Quoted 90 lines · On Wed, 12 Sep 2012 13:29:41 +0800 Shaohua Li <[email protected]> wrote:

Quoted 68 lines · NeilBrown

Quoted 66 lines · On Tue, 31 Jul 2012 16:12:04 +0800 Shaohua Li <[email protected]> wrote:

2012/7/31 NeilBrown <[email protected]>:

Quoted 49 lines · On Thu, 26 Jul 2012 16:01:50 +0800 Shaohua Li <[email protected]> wrote:

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

Quoted 6 lines

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

It's true, the behavior likes MD_RECOVERY_REQUESTED, ie, read all disks, only do write if two disk data not match. But I hope it works as soon as the array is assembled. Surely we can set in the metadata, but I didn't want to enable it by default, since even with SSD, the read-compare-write isn't optimal some times, because it involves extra IO read and memory compare.

It appears MD_RECOVERY_REQUESTED assumes disks are insync. It doesn't read disk !insync, so it doesn't work for assemble.

Quoted 8 lines

Quoted 4 lines

In my mind, user frozen the sync first (with sync_action setting), and then enable read-compare-write, and finally continue the sync. I can't stop a recovery and set MD_RECOVERY_REQUESTED, so I added a flag. Anyway, please suggest what the preferred way for this.

I guess I just don't find this functionality at all convincing. It isn't clear when anyone would use it, or how they would use it. It seems best not to provide it.

The background is: For SSD, writting a hard formatted disk and fully filled disk, the first case could be 300% faster than the second case depending on SSD firmware and how fragmented the filled disk is. So this isn't a trival performance issue.

The usage model is something like this: one disk is borken, add a new disk to the raid. Currently we copy the whole disk of the first disk to the second. For SSD, if the first disk and second disk have most data identical or most content of the first is 0, the copy is very bad. In this scenario, we can avoid some copy, which will make latter write to the disk faster. Thinking about 300% faster :).

Quoted 9 lines

I don't think we can do this in underlayer. We don't want to do it always. Detecting 0 is very expensive.

And this isn't intrusive to me. We only do the 'copy avoidness' in sync, and sync is rare event.

I hope to convince you this is a useful functionality, then we can discuss the implementation.

Quoted 7 lines

Let's start with "when would someone use it".

You said that you don't want to make it the default. If something isn't a default it will not be used very often, and will only be used at all if it is reasonably easy to use.
So how would someone use this? What would make them choose to use it, and what action would that take to make it happen?

Ok, let me explain the usage model.

Quoted 8 lines

For 'compare and avoid write if equal' case:
1. update SSD firmware. This doesn't change the data, but we need take one disk off from the raid one time.
2. One disk has errors, but these errors don't ruin most of the data (for example, a pcie error)
3. driver/os crash.
In all these cases, two raid disks must be resync, and they have almost identical data. write avoidness will be very helpful for these.

This is all valid, but doesn't explain how it actually happens.

Maybe the rsync should initially do a comparison and write-if-different. If at any time after the first megabyte the fraction of blocks that requires a write exceeds (say) 10%, we switch to "read one, write the others". ??

Quoted 3 lines

For 'compare and trim if source disk data is 0' case:
If filesystem support trim, this can be enabled always (or at least we can enable it if used capacity of the disk is less than 80% for example).

One of my concerns about this is that I would expect smart flash drives to actually do this zero-detection internally. Maybe they don't now, but surely they will soon. By the time we get this feature into production there seems a reasonably chance that it won't be needed - at least on new hardware.

And on old hardware I believe that 'discard' isn't particularly fast (as it cannot be queued ... or something). So I would only want to send a discard request if it was very large, though I'm not sure how large is large enough.

Quoted 2 lines

User enables these features and then add disk to downgrade raid. raid resync then avoids write or does trim.

This is the bit that concerns me the most - expecting the user to enable this sort of feature. In many cases they won't making the code pointless. If an optimisation is needed, we should use it automatically. Maybe we can for the first first case. Not sure yet about the second one.

Quoted 124 lines · NeilBrown

Quoted 113 lines · On Wed, 19 Sep 2012 13:51:06 +0800 Shaohua Li <[email protected]> wrote:

Quoted 102 lines · NeilBrown

Quoted 90 lines · On Wed, 12 Sep 2012 13:29:41 +0800 Shaohua Li <[email protected]> wrote:

Quoted 68 lines · NeilBrown

Quoted 66 lines · On Tue, 31 Jul 2012 16:12:04 +0800 Shaohua Li <[email protected]> wrote:

2012/7/31 NeilBrown <[email protected]>:

Quoted 49 lines · On Thu, 26 Jul 2012 16:01:50 +0800 Shaohua Li <[email protected]> wrote:

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

Quoted 6 lines

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

It's true, the behavior likes MD_RECOVERY_REQUESTED, ie, read all disks, only do write if two disk data not match. But I hope it works as soon as the array is assembled. Surely we can set in the metadata, but I didn't want to enable it by default, since even with SSD, the read-compare-write isn't optimal some times, because it involves extra IO read and memory compare.

It appears MD_RECOVERY_REQUESTED assumes disks are insync. It doesn't read disk !insync, so it doesn't work for assemble.

Quoted 8 lines

Quoted 4 lines

In my mind, user frozen the sync first (with sync_action setting), and then enable read-compare-write, and finally continue the sync. I can't stop a recovery and set MD_RECOVERY_REQUESTED, so I added a flag. Anyway, please suggest what the preferred way for this.

I guess I just don't find this functionality at all convincing. It isn't clear when anyone would use it, or how they would use it. It seems best not to provide it.

The background is: For SSD, writting a hard formatted disk and fully filled disk, the first case could be 300% faster than the second case depending on SSD firmware and how fragmented the filled disk is. So this isn't a trival performance issue.

The usage model is something like this: one disk is borken, add a new disk to the raid. Currently we copy the whole disk of the first disk to the second. For SSD, if the first disk and second disk have most data identical or most content of the first is 0, the copy is very bad. In this scenario, we can avoid some copy, which will make latter write to the disk faster. Thinking about 300% faster :).

Quoted 9 lines

I don't think we can do this in underlayer. We don't want to do it always. Detecting 0 is very expensive.

And this isn't intrusive to me. We only do the 'copy avoidness' in sync, and sync is rare event.

I hope to convince you this is a useful functionality, then we can discuss the implementation.

Quoted 7 lines

Let's start with "when would someone use it".

You said that you don't want to make it the default. If something isn't a default it will not be used very often, and will only be used at all if it is reasonably easy to use.
So how would someone use this? What would make them choose to use it, and what action would that take to make it happen?

Ok, let me explain the usage model.

Quoted 8 lines

For 'compare and avoid write if equal' case:
1. update SSD firmware. This doesn't change the data, but we need take one disk off from the raid one time.
2. One disk has errors, but these errors don't ruin most of the data (for example, a pcie error)
3. driver/os crash.
In all these cases, two raid disks must be resync, and they have almost identical data. write avoidness will be very helpful for these.

Quoted 1 line

This is all valid, but doesn't explain how it actually happens.

Those are real use cases actually.

Quoted 4 lines

Maybe the rsync should initially do a comparison and write-if-different. If at any time after the first megabyte the fraction of blocks that requires a write exceeds (say) 10%, we switch to "read one, write the others". ??

Not sure, but this will limit the usage. For example, in my case 1, the first disk can be written slightly when the second disk is doing firmware upgrade. The write can be in the first 1M for sure.

Quoted 8 lines

Quoted 3 lines

For 'compare and trim if source disk data is 0' case:
If filesystem support trim, this can be enabled always (or at least we can enable it if used capacity of the disk is less than 80% for example).

One of my concerns about this is that I would expect smart flash drives to actually do this zero-detection internally. Maybe they don't now, but surely they will soon. By the time we get this feature into production there seems a reasonably chance that it won't be needed - at least on new hardware.

Doesn't make sense to me. zero-detection is very expensive. blindly doing it for every request is insane. While my proposal is just doing it for sync, which is a rare scenario.

Quoted 3 lines

And on old hardware I believe that 'discard' isn't particularly fast (as it cannot be queued ... or something). So I would only want to send a discard request if it was very large, though I'm not sure how large is large enough.

Yep, discard speed varies between SSDs. Some are very slow, much slower than write, some SATA SSD discard speed is even slower than harddisk write. While our card is very fast, faster than write.

Quoted 3 lines

Quoted 2 lines

User enables these features and then add disk to downgrade raid. raid resync then avoids write or does trim.

Quoted 4 lines

This is the bit that concerns me the most - expecting the user to enable this sort of feature. In many cases they won't making the code pointless. If an optimisation is needed, we should use it automatically. Maybe we can for the first first case. Not sure yet about the second one.

I understood your concerns. Giving this feature can harm/benefit, depending on different SSDs and workload/usage model, an interface to enable is preferred.

Quoted 167 lines · Shaohua Li

Quoted 124 lines · NeilBrown

Quoted 113 lines · On Wed, 19 Sep 2012 13:51:06 +0800 Shaohua Li <[email protected]> wrote:

Quoted 102 lines · NeilBrown

Quoted 90 lines · On Wed, 12 Sep 2012 13:29:41 +0800 Shaohua Li <[email protected]> wrote:

Quoted 68 lines · NeilBrown

Quoted 66 lines · On Tue, 31 Jul 2012 16:12:04 +0800 Shaohua Li <[email protected]> wrote:

2012/7/31 NeilBrown <[email protected]>:

Quoted 49 lines · On Thu, 26 Jul 2012 16:01:50 +0800 Shaohua Li <[email protected]> wrote:

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

Quoted 6 lines

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

It's true, the behavior likes MD_RECOVERY_REQUESTED, ie, read all disks, only do write if two disk data not match. But I hope it works as soon as the array is assembled. Surely we can set in the metadata, but I didn't want to enable it by default, since even with SSD, the read-compare-write isn't optimal some times, because it involves extra IO read and memory compare.

It appears MD_RECOVERY_REQUESTED assumes disks are insync. It doesn't read disk !insync, so it doesn't work for assemble.

Quoted 8 lines

Quoted 4 lines

In my mind, user frozen the sync first (with sync_action setting), and then enable read-compare-write, and finally continue the sync. I can't stop a recovery and set MD_RECOVERY_REQUESTED, so I added a flag. Anyway, please suggest what the preferred way for this.

I guess I just don't find this functionality at all convincing. It isn't clear when anyone would use it, or how they would use it. It seems best not to provide it.

The background is: For SSD, writting a hard formatted disk and fully filled disk, the first case could be 300% faster than the second case depending on SSD firmware and how fragmented the filled disk is. So this isn't a trival performance issue.

The usage model is something like this: one disk is borken, add a new disk to the raid. Currently we copy the whole disk of the first disk to the second. For SSD, if the first disk and second disk have most data identical or most content of the first is 0, the copy is very bad. In this scenario, we can avoid some copy, which will make latter write to the disk faster. Thinking about 300% faster :).

Quoted 9 lines

I don't think we can do this in underlayer. We don't want to do it always. Detecting 0 is very expensive.

And this isn't intrusive to me. We only do the 'copy avoidness' in sync, and sync is rare event.

I hope to convince you this is a useful functionality, then we can discuss the implementation.

Quoted 7 lines

Let's start with "when would someone use it".

You said that you don't want to make it the default. If something isn't a default it will not be used very often, and will only be used at all if it is reasonably easy to use.
So how would someone use this? What would make them choose to use it, and what action would that take to make it happen?

Ok, let me explain the usage model.

Quoted 8 lines

For 'compare and avoid write if equal' case:
1. update SSD firmware. This doesn't change the data, but we need take one disk off from the raid one time.
2. One disk has errors, but these errors don't ruin most of the data (for example, a pcie error)
3. driver/os crash.
In all these cases, two raid disks must be resync, and they have almost identical data. write avoidness will be very helpful for these.

Quoted 1 line

This is all valid, but doesn't explain how it actually happens.

Those are real use cases actually.

Quoted 4 lines

Maybe the rsync should initially do a comparison and write-if-different. If at any time after the first megabyte the fraction of blocks that requires a write exceeds (say) 10%, we switch to "read one, write the others". ??

Not sure, but this will limit the usage. For example, in my case 1, the first disk can be written slightly when the second disk is doing firmware upgrade. The write can be in the first 1M for sure.

Quoted 8 lines

Quoted 3 lines

For 'compare and trim if source disk data is 0' case:
If filesystem support trim, this can be enabled always (or at least we can enable it if used capacity of the disk is less than 80% for example).

One of my concerns about this is that I would expect smart flash drives to actually do this zero-detection internally. Maybe they don't now, but surely they will soon. By the time we get this feature into production there seems a reasonably chance that it won't be needed - at least on new hardware.

Doesn't make sense to me. zero-detection is very expensive. blindly doing it for every request is insane. While my proposal is just doing it for sync, which is a rare scenario.

Quoted 3 lines

And on old hardware I believe that 'discard' isn't particularly fast (as it cannot be queued ... or something). So I would only want to send a discard request if it was very large, though I'm not sure how large is large enough.

Yep, discard speed varies between SSDs. Some are very slow, much slower than write, some SATA SSD discard speed is even slower than harddisk write. While our card is very fast, faster than write.

Quoted 7 lines

Quoted 2 lines

User enables these features and then add disk to downgrade raid. raid resync then avoids write or does trim.

This is the bit that concerns me the most - expecting the user to enable this sort of feature. In many cases they won't making the code pointless. If an optimisation is needed, we should use it automatically. Maybe we can for the first first case. Not sure yet about the second one.

Quoted 2 lines

I understood your concerns. Giving this feature can harm/benefit, depending on different SSDs and workload/usage model, an interface to enable is preferred.

Neil,
any further comments on this? This is a usable feature, I hope we can have some agreements.

Quoted 168 lines · On Wed, 17 Oct 2012 13:11:13 +0800 Shaohua Li <[email protected]> wrote:

Quoted 167 lines · Shaohua Li

Quoted 124 lines · NeilBrown

Quoted 113 lines · On Wed, 19 Sep 2012 13:51:06 +0800 Shaohua Li <[email protected]> wrote:

Quoted 102 lines · NeilBrown

Quoted 90 lines · On Wed, 12 Sep 2012 13:29:41 +0800 Shaohua Li <[email protected]> wrote:

Quoted 68 lines · NeilBrown

Quoted 66 lines · On Tue, 31 Jul 2012 16:12:04 +0800 Shaohua Li <[email protected]> wrote:

2012/7/31 NeilBrown <[email protected]>:

Quoted 49 lines · On Thu, 26 Jul 2012 16:01:50 +0800 Shaohua Li <[email protected]> wrote:

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

Quoted 6 lines

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

It's true, the behavior likes MD_RECOVERY_REQUESTED, ie, read all disks, only do write if two disk data not match. But I hope it works as soon as the array is assembled. Surely we can set in the metadata, but I didn't want to enable it by default, since even with SSD, the read-compare-write isn't optimal some times, because it involves extra IO read and memory compare.

It appears MD_RECOVERY_REQUESTED assumes disks are insync. It doesn't read disk !insync, so it doesn't work for assemble.

Quoted 8 lines

Quoted 4 lines

In my mind, user frozen the sync first (with sync_action setting), and then enable read-compare-write, and finally continue the sync. I can't stop a recovery and set MD_RECOVERY_REQUESTED, so I added a flag. Anyway, please suggest what the preferred way for this.

I guess I just don't find this functionality at all convincing. It isn't clear when anyone would use it, or how they would use it. It seems best not to provide it.

The background is: For SSD, writting a hard formatted disk and fully filled disk, the first case could be 300% faster than the second case depending on SSD firmware and how fragmented the filled disk is. So this isn't a trival performance issue.

The usage model is something like this: one disk is borken, add a new disk to the raid. Currently we copy the whole disk of the first disk to the second. For SSD, if the first disk and second disk have most data identical or most content of the first is 0, the copy is very bad. In this scenario, we can avoid some copy, which will make latter write to the disk faster. Thinking about 300% faster :).

Quoted 9 lines

I don't think we can do this in underlayer. We don't want to do it always. Detecting 0 is very expensive.

And this isn't intrusive to me. We only do the 'copy avoidness' in sync, and sync is rare event.

I hope to convince you this is a useful functionality, then we can discuss the implementation.

Quoted 7 lines

Let's start with "when would someone use it".

You said that you don't want to make it the default. If something isn't a default it will not be used very often, and will only be used at all if it is reasonably easy to use.
So how would someone use this? What would make them choose to use it, and what action would that take to make it happen?

Ok, let me explain the usage model.

Quoted 8 lines

For 'compare and avoid write if equal' case:
1. update SSD firmware. This doesn't change the data, but we need take one disk off from the raid one time.
2. One disk has errors, but these errors don't ruin most of the data (for example, a pcie error)
3. driver/os crash.
In all these cases, two raid disks must be resync, and they have almost identical data. write avoidness will be very helpful for these.

Quoted 1 line

This is all valid, but doesn't explain how it actually happens.

Those are real use cases actually.

Quoted 4 lines

Maybe the rsync should initially do a comparison and write-if-different. If at any time after the first megabyte the fraction of blocks that requires a write exceeds (say) 10%, we switch to "read one, write the others". ??

Not sure, but this will limit the usage. For example, in my case 1, the first disk can be written slightly when the second disk is doing firmware upgrade. The write can be in the first 1M for sure.

Quoted 8 lines

Quoted 3 lines

For 'compare and trim if source disk data is 0' case:
If filesystem support trim, this can be enabled always (or at least we can enable it if used capacity of the disk is less than 80% for example).

One of my concerns about this is that I would expect smart flash drives to actually do this zero-detection internally. Maybe they don't now, but surely they will soon. By the time we get this feature into production there seems a reasonably chance that it won't be needed - at least on new hardware.

Doesn't make sense to me. zero-detection is very expensive. blindly doing it for every request is insane. While my proposal is just doing it for sync, which is a rare scenario.

Quoted 3 lines

And on old hardware I believe that 'discard' isn't particularly fast (as it cannot be queued ... or something). So I would only want to send a discard request if it was very large, though I'm not sure how large is large enough.

Yep, discard speed varies between SSDs. Some are very slow, much slower than write, some SATA SSD discard speed is even slower than harddisk write. While our card is very fast, faster than write.

Quoted 7 lines

Quoted 2 lines

User enables these features and then add disk to downgrade raid. raid resync then avoids write or does trim.

This is the bit that concerns me the most - expecting the user to enable this sort of feature. In many cases they won't making the code pointless. If an optimisation is needed, we should use it automatically. Maybe we can for the first first case. Not sure yet about the second one.

Quoted 6 lines

Quoted 2 lines

I understood your concerns. Giving this feature can harm/benefit, depending on different SSDs and workload/usage model, an interface to enable is preferred.

Neil,
any further comments on this? This is a usable feature, I hope we can have some agreements.

You still haven't answered my main question, which possibly means I haven't asked it very clearly.

You are saying that this new behaviour should not be the default and I think I agree.
So the question is: how it is selected?

You cannot expect the user to explicitly enable it any time a resync or recovery starts that should use this new feature. You must have some automatic, or semi-automatic, way for the feature to be activated, otherwise it will never be used.

I'm not asking "when should the feature be used" - you've answered that question a few time and it really isn't an issue.
The question it "What it the exact process by which the feature is turned on for any particular resync or recovery?"

Quoted 16 lines

Quoted 3 lines

Neil,
any further comments on this? This is a usable feature, I hope we can have some agreements.

You still haven't answered my main question, which possibly means I haven't asked it very clearly.

You are saying that this new behaviour should not be the default and I think I agree.
So the question is: how it is selected?

You cannot expect the user to explicitly enable it any time a resync or recovery starts that should use this new feature. You must have some automatic, or semi-automatic, way for the feature to be activated, otherwise it will never be used.

Quoted 4 lines

I'm not asking "when should the feature be used" - you've answered that question a few time and it really isn't an issue.
The question it "What it the exact process by which the feature is turned on for any particular resync or recovery?"

So you worried about users don't know how to correctly select the feature. An experienced user knows this, the usage scenario I mentioned describes how to do the decision. For example, a resync after system crash should enable the feature. I admit an inexperienced user doesn't know how to select it, but this isn't a big problem to me. There are a lot of tunables in the kernel (even MD), which can significantly impact kernel behavior. These tunables are just for experienced users.

Thanks,
Shaohua

Quoted 29 lines · On Thu, 18 Oct 2012 09:17:35 +0800 Shaohua Li <[email protected]> wrote:

Quoted 16 lines

Quoted 3 lines

Neil,
any further comments on this? This is a usable feature, I hope we can have some agreements.

You still haven't answered my main question, which possibly means I haven't asked it very clearly.

You are saying that this new behaviour should not be the default and I think I agree.
So the question is: how it is selected?

You cannot expect the user to explicitly enable it any time a resync or recovery starts that should use this new feature. You must have some automatic, or semi-automatic, way for the feature to be activated, otherwise it will never be used.

Quoted 4 lines

I'm not asking "when should the feature be used" - you've answered that question a few time and it really isn't an issue.
The question it "What it the exact process by which the feature is turned on for any particular resync or recovery?"

So you worried about users don't know how to correctly select the feature. An experienced user knows this, the usage scenario I mentioned describes how to do the decision. For example, a resync after system crash should enable the feature. I admit an inexperienced user doesn't know how to select it, but this isn't a big problem to me. There are a lot of tunables in the kernel (even MD), which can significantly impact kernel behavior. These tunables are just for experienced users.

Quoted 2 lines

Thanks,
Shaohua

You still aren't answering my question.

What exactly, precisely, specifically, will an "experienced user" do?

Quoted 35 lines · NeilBrown

Quoted 31 lines · On Thu, 18 Oct 2012 09:17:35 +0800 Shaohua Li <[email protected]> wrote:

Quoted 16 lines

Quoted 3 lines

Neil,
any further comments on this? This is a usable feature, I hope we can have some agreements.

You still haven't answered my main question, which possibly means I haven't asked it very clearly.

You are saying that this new behaviour should not be the default and I think I agree.
So the question is: how it is selected?

You cannot expect the user to explicitly enable it any time a resync or recovery starts that should use this new feature. You must have some automatic, or semi-automatic, way for the feature to be activated, otherwise it will never be used.

Quoted 4 lines

I'm not asking "when should the feature be used" - you've answered that question a few time and it really isn't an issue.
The question it "What it the exact process by which the feature is turned on for any particular resync or recovery?"

So you worried about users don't know how to correctly select the feature. An experienced user knows this, the usage scenario I mentioned describes how to do the decision. For example, a resync after system crash should enable the feature. I admit an inexperienced user doesn't know how to select it, but this isn't a big problem to me. There are a lot of tunables in the kernel (even MD), which can significantly impact kernel behavior. These tunables are just for experienced users.

Thanks,
Shaohua

Quoted 3 lines

You still aren't answering my question.

What exactly, precisely, specifically, will an "experienced user" do?

Set something to a sysfs entry to enable the feature (like my RFC patch does to have a new sysfs entry for the feature), and readd disk. resync then does 'only write mismatch data'. Is this what you asked?

Quoted 8 lines

Quoted 8 lines

For 'compare and avoid write if equal' case:
1. update SSD firmware. This doesn't change the data, but we need take one disk off from the raid one time.
2. One disk has errors, but these errors don't ruin most of the data (for example, a pcie error)
3. driver/os crash.
In all these cases, two raid disks must be resync, and they have almost identical data. write avoidness will be very helpful for these.

Quoted 3 lines

Set something to a sysfs entry to enable the feature (like my RFC patch does to have a new sysfs entry for the feature), and readd disk. resync then does 'only write mismatch data'. Is this what you asked?

Yes, that is the sort of thing I was asking for.
When you say "readd disk" I assume you mean to use the --readd option to mdadm.
The only works when there is a bitmap active on the array, so relatively few blocks will be resynced so does it really matter which approach is taken? Always copy, or read-and-test?

Though maybe you really mean to "--add" the device. In that case it would probably make sense to add some other option to mdadm to say "enable read-mostly recovery". I wonder what a good name would be. --minimize-writes ??

Quoted 36 lines · On Thu, 18 Oct 2012 10:01:34 +0800 Shaohua Li <[email protected]> wrote:

Quoted 35 lines · NeilBrown

Quoted 31 lines · On Thu, 18 Oct 2012 09:17:35 +0800 Shaohua Li <[email protected]> wrote:

Quoted 16 lines

Quoted 3 lines

Neil,
any further comments on this? This is a usable feature, I hope we can have some agreements.

You still haven't answered my main question, which possibly means I haven't asked it very clearly.

You are saying that this new behaviour should not be the default and I think I agree.
So the question is: how it is selected?

You cannot expect the user to explicitly enable it any time a resync or recovery starts that should use this new feature. You must have some automatic, or semi-automatic, way for the feature to be activated, otherwise it will never be used.

Quoted 4 lines

I'm not asking "when should the feature be used" - you've answered that question a few time and it really isn't an issue.
The question it "What it the exact process by which the feature is turned on for any particular resync or recovery?"

So you worried about users don't know how to correctly select the feature. An experienced user knows this, the usage scenario I mentioned describes how to do the decision. For example, a resync after system crash should enable the feature. I admit an inexperienced user doesn't know how to select it, but this isn't a big problem to me. There are a lot of tunables in the kernel (even MD), which can significantly impact kernel behavior. These tunables are just for experienced users.

Thanks,
Shaohua

Quoted 7 lines

Quoted 3 lines

You still aren't answering my question.

What exactly, precisely, specifically, will an "experienced user" do?

Set something to a sysfs entry to enable the feature (like my RFC patch does to have a new sysfs entry for the feature), and readd disk. resync then does 'only write mismatch data'. Is this what you asked?

Yes, that is the sort of thing I was asking for.
When you say "readd disk" I assume you mean to use the --readd option to mdadm.
The only works when there is a bitmap active on the array, so relatively few blocks will be resynced so does it really matter which approach is taken? Always copy, or read-and-test?

Though maybe you really mean to "--add" the device. In that case it would probably make sense to add some other option to mdadm to say "enable read-mostly recovery". I wonder what a good name would be. --minimize-writes ??

You earlier gave a list of scenarios in which you thought this would be useful. It was:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

For 'compare and avoid write if equal' case:
1. update SSD firmware. This doesn't change the data, but we need take one disk off from the raid one time.
2. One disk has errors, but these errors don't ruin most of the data (for example, a pcie error)
3. driver/os crash.
In all these cases, two raid disks must be resync, and they have almost identical data. write avoidness will be very helpful for these.

For case '3', it would be a "resync" rather than a "recovery". How would you expect an "advanced user" to choose read-and-test recovery in that case? There is no "readd" command happening.

Quoted 38 lines · NeilBrown

Quoted 36 lines · On Thu, 18 Oct 2012 10:01:34 +0800 Shaohua Li <[email protected]> wrote:

Quoted 35 lines · NeilBrown

Quoted 31 lines · On Thu, 18 Oct 2012 09:17:35 +0800 Shaohua Li <[email protected]> wrote:

Quoted 16 lines

Quoted 3 lines

Neil,
any further comments on this? This is a usable feature, I hope we can have some agreements.

You still haven't answered my main question, which possibly means I haven't asked it very clearly.

You are saying that this new behaviour should not be the default and I think I agree.
So the question is: how it is selected?

You cannot expect the user to explicitly enable it any time a resync or recovery starts that should use this new feature. You must have some automatic, or semi-automatic, way for the feature to be activated, otherwise it will never be used.

Quoted 4 lines

I'm not asking "when should the feature be used" - you've answered that question a few time and it really isn't an issue.
The question it "What it the exact process by which the feature is turned on for any particular resync or recovery?"

So you worried about users don't know how to correctly select the feature. An experienced user knows this, the usage scenario I mentioned describes how to do the decision. For example, a resync after system crash should enable the feature. I admit an inexperienced user doesn't know how to select it, but this isn't a big problem to me. There are a lot of tunables in the kernel (even MD), which can significantly impact kernel behavior. These tunables are just for experienced users.

Thanks,
Shaohua

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

You still aren't answering my question.

What exactly, precisely, specifically, will an "experienced user" do?

Set something to a sysfs entry to enable the feature (like my RFC patch does to have a new sysfs entry for the feature), and readd disk. resync then does 'only write mismatch data'. Is this what you asked?

sorry for the delay.

Quoted 11 lines

Yes, that is the sort of thing I was asking for.
When you say "readd disk" I assume you mean to use the --readd option to mdadm.
The only works when there is a bitmap active on the array, so relatively few blocks will be resynced so does it really matter which approach is taken? Always copy, or read-and-test?

Though maybe you really mean to "--add" the device. In that case it would probably make sense to add some other option to mdadm to say "enable read-mostly recovery". I wonder what a good name would be. --minimize-writes ??

Yep, it's '--add' case. For the '--readd' with bitmap case, bitmap can already avoid a lot of write already. The useage case is something like:
one disk is broken; trim whole disk of a new disk; add the new disk If source disk has a lot of 0 and we only write mismatch data, we can avoid write a lot.

I believe we need such mechanism for '--create' too, if the first disk has some data, but the second disk is empty.

Quoted 13 lines

You earlier gave a list of scenarios in which you thought this would be useful. It was:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

For 'compare and avoid write if equal' case:
1. update SSD firmware. This doesn't change the data, but we need take one disk off from the raid one time.
2. One disk has errors, but these errors don't ruin most of the data (for example, a pcie error)
3. driver/os crash.
In all these cases, two raid disks must be resync, and they have almost identical data. write avoidness will be very helpful for these.

Quoted 3 lines

For case '3', it would be a "resync" rather than a "recovery". How would you expect an "advanced user" to choose read-and-test recovery in that case? There is no "readd" command happening.

If there is bitmap, maybe we don't need do read-and-test, so this one isn't very necessary in current stage. If not, what I suggested is:
1. user suspends resync (write something to a sysfs file)
2. user enables read-and-test (again, write a sysfs file)
3. resume resync

Quoted 87 lines · On Wed, 31 Oct 2012 11:25:33 +0800 Shaohua Li <[email protected]> wrote:

Quoted 38 lines · NeilBrown

Quoted 36 lines · On Thu, 18 Oct 2012 10:01:34 +0800 Shaohua Li <[email protected]> wrote:

Quoted 35 lines · NeilBrown

Quoted 31 lines · On Thu, 18 Oct 2012 09:17:35 +0800 Shaohua Li <[email protected]> wrote:

Quoted 16 lines

Quoted 3 lines

Neil,
any further comments on this? This is a usable feature, I hope we can have some agreements.

You still haven't answered my main question, which possibly means I haven't asked it very clearly.

You are saying that this new behaviour should not be the default and I think I agree.
So the question is: how it is selected?

You cannot expect the user to explicitly enable it any time a resync or recovery starts that should use this new feature. You must have some automatic, or semi-automatic, way for the feature to be activated, otherwise it will never be used.

Quoted 4 lines

I'm not asking "when should the feature be used" - you've answered that question a few time and it really isn't an issue.
The question it "What it the exact process by which the feature is turned on for any particular resync or recovery?"

So you worried about users don't know how to correctly select the feature. An experienced user knows this, the usage scenario I mentioned describes how to do the decision. For example, a resync after system crash should enable the feature. I admit an inexperienced user doesn't know how to select it, but this isn't a big problem to me. There are a lot of tunables in the kernel (even MD), which can significantly impact kernel behavior. These tunables are just for experienced users.

Thanks,
Shaohua

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

You still aren't answering my question.

What exactly, precisely, specifically, will an "experienced user" do?

Set something to a sysfs entry to enable the feature (like my RFC patch does to have a new sysfs entry for the feature), and readd disk. resync then does 'only write mismatch data'. Is this what you asked?

sorry for the delay.

Quoted 11 lines

Yes, that is the sort of thing I was asking for.
When you say "readd disk" I assume you mean to use the --readd option to mdadm.
The only works when there is a bitmap active on the array, so relatively few blocks will be resynced so does it really matter which approach is taken? Always copy, or read-and-test?

Though maybe you really mean to "--add" the device. In that case it would probably make sense to add some other option to mdadm to say "enable read-mostly recovery". I wonder what a good name would be. --minimize-writes ??

Yep, it's '--add' case. For the '--readd' with bitmap case, bitmap can already avoid a lot of write already. The useage case is something like:
one disk is broken; trim whole disk of a new disk; add the new disk If source disk has a lot of 0 and we only write mismatch data, we can avoid write a lot.

I believe we need such mechanism for '--create' too, if the first disk has some data, but the second disk is empty.

Quoted 13 lines

You earlier gave a list of scenarios in which you thought this would be useful. It was:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

For 'compare and avoid write if equal' case:
1. update SSD firmware. This doesn't change the data, but we need take one disk off from the raid one time.
2. One disk has errors, but these errors don't ruin most of the data (for example, a pcie error)
3. driver/os crash.
In all these cases, two raid disks must be resync, and they have almost identical data. write avoidness will be very helpful for these.

Quoted 3 lines

For case '3', it would be a "resync" rather than a "recovery". How would you expect an "advanced user" to choose read-and-test recovery in that case? There is no "readd" command happening.

Quoted 5 lines

If there is bitmap, maybe we don't need do read-and-test, so this one isn't very necessary in current stage. If not, what I suggested is:
1. user suspends resync (write something to a sysfs file)
2. user enables read-and-test (again, write a sysfs file)
3. resume resync

So you are happy for the resync to start doing the wrong thing, and expect the sysadmin to notice, and then take some obscure action to stop it doing the wrong thing and start it doing the right thing.
Certainly possible, but very error prone I would think.

Quoted 189 lines · On Wed, 17 Oct 2012 13:11:13 +0800 Shaohua Li <[email protected]> wrote:

Quoted 169 lines · Shaohua Li

Quoted 167 lines · NeilBrown

Quoted 124 lines · On Wed, 19 Sep 2012 13:51:06 +0800 Shaohua Li <[email protected]> wrote:

Quoted 114 lines · NeilBrown

Quoted 102 lines · On Wed, 12 Sep 2012 13:29:41 +0800 Shaohua Li <[email protected]> wrote:

Quoted 91 lines · NeilBrown

Quoted 68 lines · On Tue, 31 Jul 2012 16:12:04 +0800 Shaohua Li <[email protected]> wrote:

Quoted 67 lines

2012/7/31 NeilBrown <[email protected]>:

Quoted 49 lines · On Thu, 26 Jul 2012 16:01:50 +0800 Shaohua Li <[email protected]> wrote:

Quoted 33 lines

Write has some impacts to SSD:
1. wear out flash. Frequent write can speed out the progress.
2. increase the burden of garbage collection of SSD firmware. If no space left for write, SSD firmware garbage collection will try to free some space.
3. slow down subsequent write. After write SSD to some extents (for example, write the whole disk), subsequent write will slow down significantly (because almost every write invokes garbage collection in such case).

We want to avoid unnecessary write as more as possible. raid sync generally involves a lot of unnecessary write. For example, even two disks don't have any data, we write the second disk for the whole disk size.

To reduce write, we always compare raid disk data and only write mismatch part. This means sync will have extra IO read and memory compare. So this scheme is very bad for hard disk raid and sometimes SSD raid too if mismatch part is majority. But sometimes this can be very helpful to reduce write, in that case, since sync is rare operation, the extra IO/CPU usage is worthy paying. People who want to use the feature should understand the risk first. So this ability is off by default, a sysfs entry can be used to enable it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/md.c    |   41 +++++++++++++++++++++++++++++++
 drivers/md/md.h    |    3 ++
 drivers/md/raid1.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 4 deletions(-)

Index: linux/drivers/md/md.h
===================================================================

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 struct mddev { · line 330

Quoted 6 lines

drivers/md/md.h 2012-07-26 10:36:38.500740552 +0800 renamed from drivers/md/md.h 2012-07-25 13:51:00.353775521 +0800 +3 -0

@@ -325,6 +325,9 @@ struct mddev {
 #define      MD_RECOVERY_FROZEN      9
 
      unsigned long                   recovery;
+#define MD_RECOVERY_MODE_REPAIR              0
+#define MD_RECOVERY_MODE_DISCARD     1
+     unsigned long                   recovery_mode;

You have not documented the meaning of these two flags at all, and I don't feel up to guessing.

The patch looks more complex that it should be. The behaviour you are suggesting is exactly the behaviour you get when MD_RECOVERY_REQUESTED is set, so at most I expect to see a few places where that flag is tested changed to test something else as well.

Quoted 6 lines

How would this be used? It affects resync and resync happens as soon as the array is assembled. So when and how would you set the flag which says "prefer reads to writes"? It seems like it needs to be set in the metadata.

BTW RAID10 already does this - it reads and compares for a normal sync. So maybe just tell people to use raid10 if they want this behaviour?

It's true, the behavior likes MD_RECOVERY_REQUESTED, ie, read all disks, only do write if two disk data not match. But I hope it works as soon as the array is assembled. Surely we can set in the metadata, but I didn't want to enable it by default, since even with SSD, the read-compare-write isn't optimal some times, because it involves extra IO read and memory compare.

It appears MD_RECOVERY_REQUESTED assumes disks are insync. It doesn't read disk !insync, so it doesn't work for assemble.

Quoted 8 lines

Quoted 4 lines

In my mind, user frozen the sync first (with sync_action setting), and then enable read-compare-write, and finally continue the sync. I can't stop a recovery and set MD_RECOVERY_REQUESTED, so I added a flag. Anyway, please suggest what the preferred way for this.

I guess I just don't find this functionality at all convincing. It isn't clear when anyone would use it, or how they would use it. It seems best not to provide it.

The background is: For SSD, writting a hard formatted disk and fully filled disk, the first case could be 300% faster than the second case depending on SSD firmware and how fragmented the filled disk is. So this isn't a trival performance issue.

The usage model is something like this: one disk is borken, add a new disk to the raid. Currently we copy the whole disk of the first disk to the second. For SSD, if the first disk and second disk have most data identical or most content of the first is 0, the copy is very bad. In this scenario, we can avoid some copy, which will make latter write to the disk faster. Thinking about 300% faster :).

Quoted 9 lines

I don't think we can do this in underlayer. We don't want to do it always. Detecting 0 is very expensive.

And this isn't intrusive to me. We only do the 'copy avoidness' in sync, and sync is rare event.

I hope to convince you this is a useful functionality, then we can discuss the implementation.

Quoted 7 lines

Let's start with "when would someone use it".

You said that you don't want to make it the default. If something isn't a default it will not be used very often, and will only be used at all if it is reasonably easy to use.
So how would someone use this? What would make them choose to use it, and what action would that take to make it happen?

Ok, let me explain the usage model.

Quoted 8 lines

For 'compare and avoid write if equal' case:
1. update SSD firmware. This doesn't change the data, but we need take one disk off from the raid one time.
2. One disk has errors, but these errors don't ruin most of the data (for example, a pcie error)
3. driver/os crash.
In all these cases, two raid disks must be resync, and they have almost identical data. write avoidness will be very helpful for these.

Quoted 1 line

This is all valid, but doesn't explain how it actually happens.

Those are real use cases actually.

Quoted 4 lines

Maybe the rsync should initially do a comparison and write-if-different. If at any time after the first megabyte the fraction of blocks that requires a write exceeds (say) 10%, we switch to "read one, write the others". ??

Not sure, but this will limit the usage. For example, in my case 1, the first disk can be written slightly when the second disk is doing firmware upgrade. The write can be in the first 1M for sure.

Quoted 8 lines

Quoted 3 lines

For 'compare and trim if source disk data is 0' case:
If filesystem support trim, this can be enabled always (or at least we can enable it if used capacity of the disk is less than 80% for example).

One of my concerns about this is that I would expect smart flash drives to actually do this zero-detection internally. Maybe they don't now, but surely they will soon. By the time we get this feature into production there seems a reasonably chance that it won't be needed - at least on new hardware.

Doesn't make sense to me. zero-detection is very expensive. blindly doing it for every request is insane. While my proposal is just doing it for sync, which is a rare scenario.

Quoted 3 lines

And on old hardware I believe that 'discard' isn't particularly fast (as it cannot be queued ... or something). So I would only want to send a discard request if it was very large, though I'm not sure how large is large enough.

Yep, discard speed varies between SSDs. Some are very slow, much slower than write, some SATA SSD discard speed is even slower than harddisk write. While our card is very fast, faster than write.

Quoted 7 lines

Quoted 2 lines

User enables these features and then add disk to downgrade raid. raid resync then avoids write or does trim.

This is the bit that concerns me the most - expecting the user to enable this sort of feature. In many cases they won't making the code pointless. If an optimisation is needed, we should use it automatically. Maybe we can for the first first case. Not sure yet about the second one.

Quoted 6 lines

Quoted 2 lines

I understood your concerns. Giving this feature can harm/benefit, depending on different SSDs and workload/usage model, an interface to enable is preferred.

Neil,
any further comments on this? This is a usable feature, I hope we can have some agreements.

You still haven't answered my main question, which possibly means I haven't asked it very clearly.

You are saying that this new behaviour should not be the default and I think I agree.
So the question is: how it is selected?

You cannot expect the user to explicitly enable it any time a resync or recovery starts that should use this new feature. You must have some automatic, or semi-automatic, way for the feature to be activated, otherwise it will never be used.

Quoted 4 lines

I'm not asking "when should the feature be used" - you've answered that question a few time and it really isn't an issue.
The question it "What it the exact process by which the feature is turned on for any particular resync or recovery?"

So you worried about users don't know how to correctly select the feature. An experienced user knows this, the usage scenario I mentioned describes how to do the decision. For example, a resync after system crash should enable the feature. I admit an inexperienced user doesn't know how to select it, but this isn't a big problem to me. There are a lot of tunables in the kernel (even MD), which can significantly impact kernel behavior. These tunables are just for experienced users.

Thanks,
Shaohua
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

You still haven't answered my main question, which possibly means I haven't asked it very clearly.

You are saying that this new behaviour should not be the default and I think I agree.
So the question is: how it is selected?

You cannot expect the user to explicitly enable it any time a resync or recovery starts that should use this new feature. You must have some automatic, or semi-automatic, way for the feature to be activated, otherwise it will never be used.

I'm not asking "when should the feature be used" - you've answered that question a few time and it really isn't an issue.
The question it "What it the exact process by which the feature is turned on for any particular resync or recovery?"

NeilBrown