On Thursday January 29, Paul.Clements@SteelEye.com wrote:
Description
===========
This patch provides the md driver with the ability to track
resync/rebuild progress with a bitmap. It also gives the raid1 driver
the ability to perform asynchronous writes (i.e., writes are
acknowledged before they actually reach the secondary disk). The bitmap
and asynchronous write capabilities are primarily useful when raid1 is
employed in data replication (e.g., with a remote disk served over nbd
as the secondary device). However, the bitmap is also useful for
reducing resync/rebuild times with ordinary (local) raid1, raid5, and
raid6 arrays.
Thanks for this.
There is obviously some good functionality here, but I do have a few
issues with the patch and the code that I would like to see addressed
first.
- The patch seems to add several separate, though related, pieces of
functionality. It would be a lot easier to review if they were
separate.
There is (at least):
- "intent-loging" in the bitmaps.
- optimising resync using the intents.
- async writes to non-primary devices
- 'hotrepair'
- bitmap storage. I gather these are stored on a separate device
(in a separate filesystem). It would be nice to be able to store
them on the raid array directly so the optimised-resync works on a
root-filesytem device as well. There is nearly 64k after the
superblock that can be used. I think there should be an option to
store the bitmap on all the (primary) devices after the superblock.
(This could be added later - a separate patch)
- A question - can the bitmap be "rescaled" online, or only offline?
(not that I particularly want on-line rescaling) How is it achieved?
- ioctl - By changing the size of mdu_array_info_t you have changed
the GET_ARRAY_INFO ioctl, so you lose backwards compatibility with
user-space tools. If you want to return more info with
GET_ARRAY_INFO (which is reasonable) we need to handle both the old
and new mdu_array_info_t sizes. Ditto for SET_ARRAY_INFO
- superblock. If you have an array with a bitmap attached, boot an
old kernel that doesn't understand bitmaps, use the array for a
while (after a full sync) have an unclean shutdown, and then boot
back into the new kernel, the out-of-date bitmap will be used to
optimise resync - incorrectly. Is this possibility handled at all?
- I don't see the point of the new "array_size" personality method.
Surely the size of the bitmap depends on the device size
(mddev->size), rather than the array_size - after all, that is what
md_do_sync iterates over.
- I don't think it is right for "stop" to return EBUSY if there are
outstanding async writes. It should either wait for the writes to
complete, or abort them (or both, with a timeout).
- Write requests tend to come in bursts. So when you get a burst of
write requests, it would be most efficient to set all the on-disk
bits for all requests in the burst, flush all blocks which have
changed and then release the burst of write requests. If I read the
code correctly, you do one bitmap-block-write for every bit that
gets set.
- returning -ENOMEM in the make_request function is a no-no. You need
to either cope with a kmalloc failure, or use a mempool or similar
to guarantee some memory either now or soon.
- The bitmap_page structure is amusing. On memory failure, you
highjack the "*map" pointer to form 2 16bit counters, and spend a
whole 32bit "unsigned int" to flag if this is the case or not.
Would it be better to use one bit of "counter" as the highjack flag,
and thereby save a substantial fraction of the size of 'bitmap_page'?
- What is the relationship between the 'bitmap_fd' field in
mdu_array_info_s, and the SET_BITMAP_FILE ioctl? They both seem to
set the bitmap file.
Finally, do you have any measurements of the performance impact of the
intent logging on various workloads?
That'll do for now.
NeilBrown
Neil Brown wrote:
On Thursday January 29, Paul.Clements@SteelEye.com wrote:
quoted
Description
===========
This patch provides the md driver with the ability to track
resync/rebuild progress with a bitmap. It also gives the raid1 driver
the ability to perform asynchronous writes (i.e., writes are
acknowledged before they actually reach the secondary disk). The bitmap
and asynchronous write capabilities are primarily useful when raid1 is
employed in data replication (e.g., with a remote disk served over nbd
as the secondary device). However, the bitmap is also useful for
reducing resync/rebuild times with ordinary (local) raid1, raid5, and
raid6 arrays.
Thanks for this.
...and thanks for your feedback. There are some really good
ideas/suggestions in here. A couple of them I had thought about, but
wavered on which way to implement them. I'll respond to each of your
points below...
- The patch seems to add several separate, though related, pieces of
functionality. It would be a lot easier to review if they were
separate.
There is (at least):
- "intent-loging" in the bitmaps.
- optimising resync using the intents.
- async writes to non-primary devices
- 'hotrepair'
I can fairly easily split the bitmap code from the async write code.
Splitting the smaller bitmap pieces from the rest of the bitmap code,
however, is a little more difficult. Also, the "hotrepair" and "resync"
parts would be so small, that I'm not sure it would be worth it. Would
it be alright if I split it into two patches and see what that looks
like?
- bitmap storage. I gather these are stored on a separate device
(in a separate filesystem).
Yes.
It would be nice to be able to store
them on the raid array directly so the optimised-resync works on a
root-filesytem device as well. There is nearly 64k after the
superblock that can be used. I think there should be an option to
store the bitmap on all the (primary) devices after the superblock.
(This could be added later - a separate patch)
Yes, that would be nice. I'll think about this some and maybe submit it
as a follow-on patch.
- A question - can the bitmap be "rescaled" online, or only offline?
(not that I particularly want on-line rescaling) How is it achieved?
The bitmap file can be rescaled offline by simply expanding or
collapsing the bits as necessary, and then writing the enlarged or
shrunken bitmap file. When the array is started (with a new bitmap chunk
size), everything will work correctly. (The bitmap file can also be
truncated, in which case the driver will assume all the bits to be dirty
and perform a full resync).
- ioctl - By changing the size of mdu_array_info_t you have changed
the GET_ARRAY_INFO ioctl, so you lose backwards compatibility with
user-space tools.
Yes, unfortunately...
If you want to return more info with
GET_ARRAY_INFO (which is reasonable) we need to handle both the old
and new mdu_array_info_t sizes. Ditto for SET_ARRAY_INFO
OK, I'll code this up and include it in the next patch. Some type of
versioning scheme (like what you've done with the superblock) would
probably do the trick.
- superblock. If you have an array with a bitmap attached, boot an
old kernel that doesn't understand bitmaps, use the array for a
while (after a full sync) have an unclean shutdown, and then boot
back into the new kernel, the out-of-date bitmap will be used to
optimise resync - incorrectly. Is this possibility handled at all?
No, I guess I was assuming that a persistent bitmap could always be
trusted, but in this particular situation, that's not the case. I guess
I'll have to add bitmap event counters to the superblock?
- I don't see the point of the new "array_size" personality method.
Surely the size of the bitmap depends on the device size
(mddev->size), rather than the array_size - after all, that is what
md_do_sync iterates over.
Yikes...thanks for catching that. It got lost in the translation between
2.4 and 2.6 (size no longer comes directly from the superblock). I'll be
happy to get rid of that bit of ugliness from the patch.
- I don't think it is right for "stop" to return EBUSY if there are
outstanding async writes. It should either wait for the writes to
complete, or abort them (or both, with a timeout).
I'll see what I can do about this. Should be easy enough to wait for the
writes either to complete or error out.
- Write requests tend to come in bursts. So when you get a burst of
write requests, it would be most efficient to set all the on-disk
bits for all requests in the burst, flush all blocks which have
changed and then release the burst of write requests. If I read the
code correctly, you do one bitmap-block-write for every bit that
gets set.
That would be a very nice optimization. It would also be pretty
involved. If you'd like I can look at doing this as a follow-on patch.
- returning -ENOMEM in the make_request function is a no-no. You need
to either cope with a kmalloc failure, or use a mempool or similar
to guarantee some memory either now or soon.
Yes, I opted for the simple route here, but I think you're right; a
mempool really is required. I'll code that up and include it in the next
patch.
- The bitmap_page structure is amusing.
I like to keep my readers entertained... :)
highjack the "*map" pointer to form 2 16bit counters, and spend a
whole 32bit "unsigned int" to flag if this is the case or not.
Would it be better to use one bit of "counter" as the highjack flag,
and thereby save a substantial fraction of the size of 'bitmap_page'?
In all seriousness though, I opted for the extra flag because I didn't
want to get into figuring out which bits of a pointer could be used as
flags and which could not, across all the various 32 and 64 bit
architectures...it was easier and less risky just to use a couple more
bytes of memory. I suppose if pages are guaranteed to be aligned on some
boundary, the lowest bit could be used as a flag without any risk of
misinterpretation? At any rate, wasting 4 bytes per page didn't seem
like a terrible loss.
- What is the relationship between the 'bitmap_fd' field in
mdu_array_info_s, and the SET_BITMAP_FILE ioctl? They both seem to
set the bitmap file.
Yes, they are the same. mdadm opens the bitmap file and passes down a
file descriptor (bitmap_fd). SET_BITMAP_FILE is used in the case of
Assembling an array (where SET_ARRAY_INFO is called with a NULL array
info pointer), while the bitmap_fd is simply passed down inside the
array info struct in the Create case.
Finally, do you have any measurements of the performance impact of the
intent logging on various workloads?
No, I don't have any exact numbers, but I haven't observed a slowdown in
the tests I've performed. However, most of my tests were data
replication setups using async writes, which tends to mitigate any
slowdown caused by the bitmap.
--
Paul