From: Neil Brown <hidden> Date: 2004-06-15 06:27:17
On Tuesday June 8, paul.clements@steeleye.com wrote:
Neil,
Here's the latest patch...it supports bitmaps in files as well as block
devices (disks or partitions), contrary to what I had stated in my
previous e-mail. I've tried to address all the issues you've pointed
out, and generally cleaned up and fixed the patch some more...details
below...
This looks a lot better. There are a few little issues that I noticed
on a quick read through:
- last_block_device should return "sector_t", not "unsigned
long". It would be worth checking for other placed that
sector_t might be needed.
- bitmap_checkpage still isn't quite safe. If "hijacked" gets
set while it is allocating a page (unlikely, but possible), it
will exit with both highjacked set and an allocated page, which
isn't right.
- The event comparison
+ sb->events_hi >= refsb->bitmap_events_hi &&
+ sb->events_lo >= refsb->bitmap_events_lo) {
in hot_add_disk is wrong.
I'll try to make time to try the patch out in a week or so to get a
better feel for it.
The changes to mdadm need a bit of work.
You have added "--persistent" and "--non-persistent" flags for
--create. This is wrong.
--create always uses persistent superblocks.
--build makes arrays without persistent superblocks.
I don't think I like --create-bitmap. A bitmap file should always be
created in the context of a particular array (partly so that the size
and uuid can be set correctly). I think I would like to bitmap to be
specified as a "--bitmap=filename" option to --create, --build, or
--grow. I haven't thought this through in great detail yet, but that
is my leaning.
--examine-bitmap is a bit of a pain too. I think I would like
--examine to figure out what it has been given to look at, and report
on whatever it finds.
NeilBrown
From: Paul Clements <hidden> Date: 2004-06-17 17:57:13
Neil Brown wrote:
The changes to mdadm need a bit of work.
You have added "--persistent" and "--non-persistent" flags for
--create. This is wrong.
--create always uses persistent superblocks.
--build makes arrays without persistent superblocks.
Yes, I think --build didn't use to work for raid1, though, which is why
I went the create route. I'll fix this.
I don't think I like --create-bitmap. A bitmap file should always be
created in the context of a particular array (partly so that the size
and uuid can be set correctly). I think I would like to bitmap to be
specified as a "--bitmap=filename" option to --create, --build, or
--grow. I haven't thought this through in great detail yet, but that
is my leaning.
That all sounds fine except for --build. How do we know if we're doing
the initial build or just a rebuild. We need to know this so we know
whether to initialize the bitmap or simply use it. So I think we still
need --create-bitmap for the build command (and then build will just use
the bitmap, not initialize it). For --create I can make it so that the
bitmap is always initialized using the array parameters, so no separate
--create-bitmap will be needed. I'll need to work out the conflict that
this will create with the --chunk option, though, since --chunk is the
array chunk size for --create and --chunk is the bitmap chunk size for
--bitmap-create. Any suggestions?
--examine-bitmap is a bit of a pain too.
-X is usually what I use...that's a lot easier to remember and type...
I think I would like
--examine to figure out what it has been given to look at, and report
on whatever it finds.
Well, that gets into a little bit of black magic when you're talking
about examining a disk or partition, which could conceivably contain
both an md superblock at the end and a bitmap at the front. How do we
know which one it is "supposed" to be? Or do you just want examine to
print information for both? That would seem a little confusing to me
(granted this would only happen when a disk changed roles between bitmap
and array component).
--
Paul
From: Paul Clements <hidden> Date: 2004-06-18 20:48:31
Neil,
responses to your feedback on the kernel code:
Neil Brown wrote:
On Tuesday June 8, paul.clements@steeleye.com wrote:
This looks a lot better. There are a few little issues that I noticed
on a quick read through:
- last_block_device should return "sector_t", not "unsigned
long". It would be worth checking for other placed that
sector_t might be needed.
Yes, there were a couple. I've fixed them now.
- bitmap_checkpage still isn't quite safe. If "hijacked" gets
set while it is allocating a page (unlikely, but possible), it
will exit with both highjacked set and an allocated page, which
isn't right.
Yes, that's right. Fixed.
- The event comparison
+ sb->events_hi >= refsb->bitmap_events_hi &&
+ sb->events_lo >= refsb->bitmap_events_lo) {
in hot_add_disk is wrong.
I think it's OK. Basically, I only record bitmap events while the array
is in sync. As soon as the array goes out of sync, we continue to set
bits in the bitmap, but never clear them. This means that the bitmap is
valid for resyncing any disk that was part of the array at the time it
was last in sync. Does that make sense, or am I missing a corner case?
I'll try to make time to try the patch out in a week or so to get a
better feel for it.
On Tuesday June 8, paul.clements@steeleye.com wrote:
This looks a lot better. There are a few little issues that I noticed
on a quick read through:
- last_block_device should return "sector_t", not "unsigned
long". It would be worth checking for other placed that
sector_t might be needed.
Fixed.
- bitmap_checkpage still isn't quite safe. If "hijacked" gets
set while it is allocating a page (unlikely, but possible), it
will exit with both highjacked set and an allocated page, which
isn't right.
Fixed.
- The event comparison
+ sb->events_hi >= refsb->bitmap_events_hi &&
+ sb->events_lo >= refsb->bitmap_events_lo) {
in hot_add_disk is wrong.
This is still there, see my explanation in the previous email...
The changes to mdadm need a bit of work.
You have added "--persistent" and "--non-persistent" flags for
--create. This is wrong.
--create always uses persistent superblocks.
--build makes arrays without persistent superblocks.
OK, changed. The --build command now also accepts the --bitmap option.
I don't think I like --create-bitmap. A bitmap file should always be
created in the context of a particular array (partly so that the size
and uuid can be set correctly). I think I would like to bitmap to be
specified as a "--bitmap=filename" option to --create, --build, or
--grow. I haven't thought this through in great detail yet, but that
is my leaning.
OK, --create-bitmap remains solely for the --build case, but is no
longer needed for --create. The --create command now does all the array
creation in userspace and simply Assembles the resulting collection of
disks. It also initialises the bitmap correctly for use with the array
(i.e., uuid and size match).
--examine-bitmap is a bit of a pain too. I think I would like
--examine to figure out what it has been given to look at, and report
on whatever it finds.
This also remains as-is for now, pending your reply to my previous emails.
Thanks,
Paul
A quick note for anyone who is interested: this patch misses the
poolinfo structure changes that were made to raid1 between 2.6.6 and
2.6.7. Unfortunately, the mempool interfaces use casts from void *, so
this wasn't caught by the compiler. Trivial changes need to be made to
r1bio_pool_free for the code to work with 2.6.7.
Thanks,
Paul