md: persistent (file-backed) bitmap and async writes

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.

Background
==========
This patch is an adjunct to Peter T. Breuer's raid1 bitmap code (fr1 v2.14, ftp://oboe.it.uc3m.es/pub/Programs/fr1-2.14.tgz). The code was originally written for 2.4 (I have patches vs. 2.4.19/20 Red Hat and SuSE kernels, if anyone is interested). The 2.4 version of this patch has undergone extensive alpha, beta, and stress testing, including a WAN setup where a 500MB partition was mirrored across the U.S. The 2.6 version of the patch remains as close to the 2.4 version as possible, while still allowing it to function properly in the 2.6 kernel. The 2.6 code has also been tested quite a bit and is fairly stable.

Features
========

Persistent Bitmap
-----------------
The bitmap tracks which blocks are out of sync between the primary and secondary disk in a raid1 array (in raid5, the bitmap would indicate which stripes need to be rebuilt). The bitmap is stored in memory (for speed) and on disk (for persistence, so that a full resync is never needed, even after a failure or reboot).

There is a kernel daemon that periodically (lazily) clears bits in the bitmap file (this reduces the number and frequency of disk writes to the bitmap file).

The bitmap can also be rescaled -- i.e., change the amount of data that each bit represents. This allows for increased efficiency at the cost of reduced bitmap granularity.

Currently, the bitmap code has been implemented only for raid1, but it could easily be leveraged by other raid drivers (namely raid5 and raid6) by adding a few calls to the bitmap routines in the appropriate places.

Asynchronous Writes
-------------------
The asynchronous write capability allows the raid1 driver to function more efficiently in data replication environments (i.e., where the secondary disk is remote). Asynchronous writes allow us to overcome high network latency by filling the network pipe.

Modifications to mdadm
----------------------
I have modified Neil's mdadm tool to allow it to configure the additional bitmap and async parameters. The attached patch is against the 1.2 mdadm release. Briefly, the new options are:

Creation:

mdadm -C /dev/md0 -l 1 -n 2 --persistent --async=512 --bitmap=/tmp/bitmap_md0_file,4096,5 /dev/xxx /dev/yyy

This creates a raid1 array with:

* 2 disks
* a persistent superblock
* asynchronous writes enabled (maximum of 512 outstanding writes)
* bitmap enabled (using the file /tmp/bitmap_md0_file)
* a bitmap chunksize of 4k (bitmap chunksize determines how much data each bitmap bit represents)
* the bitmap daemon set to wake up every 5 seconds to clear bits in the bitmap file (if needed)
* /dev/xxx as the primary disk
* /dev/yyy as the backup disk (when asynchronous writes are enabled, the second disk in the array is labelled as a "backup", indicating that it is remote, and thus no reads will be issued to the device)

Assembling:

mdadm -A /dev/md0 --bitmap=/tmp/bitmap_md0_file /dev/xxx /dev/yyy

This assembles an existing array and configures it to use a bitmap file. The bitmap file pathname is not stored in the array superblock, and so must be specified every time the array is assembled.

Details:

mdadm -D /dev/md0

This will display information about /dev/md0, including some additional information about the bitmap and async parameters.

I've also added some information to the /proc/mdstat file:

# cat /proc/mdstat
Personalities : [raid1] 
md1 : active raid1 loop0[0] loop1[1](B)
      39936 blocks [2/2] [UU]
      async: 0/256 outstanding writes
      bitmap: 1/1 pages (15 cached) [64KB], 64KB chunk, file:
/tmp/bitmap_md1

unused devices: <none>

More details on the design and implementation can be found in Section 3 of my 2003 OLS Paper:
http://archive.linuxsymposium.org/ols2003/Proceedings/All-Reprints/Reprint-Clements-OLS2003.pdf

Patch Location
==============

Finally, the patches are available here:

kernel patch vs. 2.6.2-rc2-bk3
------------------------------
http://parisc-linux.org/~jejb/md_bitmap/md_bitmap_2_30_2_6_2_RC2_BK3_RELEASE.diff

mdadm patch vs. 1.2.0
---------------------
http://parisc-linux.org/~jejb/md_bitmap/mdadm_1_2_0.diff

So if you're interested, please review, test, ask questions, etc. Any feedback is welcome.

Thanks,
Paul

I've uploaded a patch against the latest mdadm (1.5.0):

http://parisc-linux.org/~jejb/md_bitmap/mdadm_1_5_0.diff

Thanks,
Paul

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

...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...

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?

Yes.

Yes, that would be nice. I'll think about this some and maybe submit it as a follow-on patch.

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).

Yes, unfortunately...

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.

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?

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'll see what I can do about this. Should be easy enough to wait for the writes either to complete or error out.

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.

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.

I like to keep my readers entertained... :)

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.

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.

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

Two would certainly be better than 1.
Splitting off the "resync" stuff probably isn't necessary, but the 'hotrepair' seems conceptually quite separate and it would help to see the patch and the justification separately, but I won't push it.

It would seem to make sense to store the "chunk size" in the bitmap file.
In fact, if the bitmap file had a small header containing:
magic number
array uuid
event count
bitmap chunk size
Then you could rescale the bitmap without touching the superblock and would at the same time solve the problem with possibly getting an out-of-date bitmap.

When you change the structure size, you change the ioctl number so you can differentiate requests based just on the ioctl.
so you would need to define a "struct mda_array_info_s_old" and have
#define GET_ARRAY_INFO _IOR (MD_MAJOR, 0x11, mdu_array_info_t)
#define GET_ARRAY_INFO_OLD _IOR (MD_MAJOR, 0x11, struct mdu_array_info_s_old)

and handle each ioctl separately (or translate one into the other early in md_ioctl).

You would need an "unplug" concept.
When make_request is given a write request, it puts the request on a queue and schedules an bitmap update. You set mddev->queue->unplug_fn to a function which flushes the outstanding bitmap updates, and then moves the queue of outstanding write requests to somewhere that raid1d can pick them up and initiate them.

True it isn't a great loss. It just seemed amusing that you squeezed two 16bit counters into the pointer (seeming to make best use of available memory) and then wasted 31 bits.
If you put the "highjacked" flag in the counter (not in the pointer) as e.g. the lsb, doing all real counting by 2s, it should be straight forward.

Ahh, I see. That's why you need both.
I guess you could create the array "the old way", stop it, and then re-assemble it and pass down the fd then.
If you kept the bitmap parameters in the head of the bitmap file rather than in the superblock, you could avoid having to change kernel-side array-creation altogether.

I'm not actually a big fan of having the kernel create the array. You might note that for new style superblocks, the kernel will not create the array at all.
You have to have user-space write out the initial superblocks, and then just have the kernel assemble the array.

I would advocate doing the same thing for the bitmap superblock (if you choose to have one in the bitmap file) - i.e. userspace sets it up and passes it to the kernel, and the kernel extracts the information it needs.

NeilBrown

I was thinking of leaving the original ioctls alone and just adding two new ioctls that would handle only the new parameters, something like:

#define GET_EXTRA_ARRAY_INFO _IOR (MD_MAJOR, 0x15,
mdu_extra_array_info_t)
#define SET_EXTRA_ARRAY_INFO _IOW (MD_MAJOR, 0x2b,
mdu_extra_array_info_t)

/* The extra array info structure is flexible, to allow for future changes */
#define EXTRA_ARRAY_INFO_VERSION 1

typedef struct mdu_extra_array_info_s {
__u32 extra_info_version; /* EXTRA_ARRAY_INFO_VERSION */
__u32 async_max_writes; /* Max outstanding async writes (0 =
sync) */
__u32 bitmap_fd; /* The bitmap file descriptor (in
only) */
__u32 bitmap_chunksize; /* The bitmap
chunksize */
__u32 bitmap_daemon_sleep; /* The bitmap daemon sleep
period */
char bitmap_path[256]; /* The bitmap filename (out
only) */

        char pad[4096-256-20];     /* Allow for future additions, empty

now */
} mdu_extra_array_info_t;

So for older kernels, the ioctls would just error out and mdadm would ignore that, and the array would be run using only the old features. This will also allow me to get rid of the SET_BITMAP_FILE ioctl and just use the SET_EXTRA_ARRAY_INFO ioctl for Assembling.

Sound OK?

--
Paul

That sounds good, as it would lend some more foolproofing to the bitmap file operations. The only problem being that we wouldn't be able to support bitmaps without backing files (in-memory only bitmaps). I guess if we don't think that's a useful feature we could do this.

We could also add this bitmap file superblock as a follow-on patch. The code that reads in the bitmap file would simply look for the magic string in the superblock to determine if the bitmap file is a flat file (old style) or contains a superblock (new style).

--
Paul

And here is the next set of patches. Tested thoroughly against 2.6.2-rc2-bk3 and compile tested against 2.6.3-rc2.

I've split the kernel code into two patches. The async write patch is dependent on the bitmap patch. The bitmap patch includes the new GET/SET_EXTRA_ARRAY_INFO ioctls and a mempool for the bitmap_update structures (no more -ENOMEM in the I/O request path).

bitmap patch vs. 2.6.3-rc2
--------------------------
http://parisc-linux.org/~jejb/md_bitmap/md_bitmap_2_30_2_6_3_RC2.diff

async write patch vs. 2.6.3-rc2
-------------------------------
http://parisc-linux.org/~jejb/md_bitmap/md_async_2_30_2_6_3_RC2.diff

mdadm patch vs. 1.5.0
---------------------
http://parisc-linux.org/~jejb/md_bitmap/mdadm_1_5_0-2.diff

For simplicity's sake, the mdadm patch includes both the bitmap and async code (it's a fairly small patch anyway, and probably wouldn't have benefited much from a split).

--
Paul

I finally had a chance to look at this properly -- or atleast at the "bitmap" patch.

There is a lot in there that I don't like. I was just going to rip out the bits that I wasn't happy with and produce a minimal patch that I was happy with and which did something useful, but I ran into problems with the file io in bitmap.c. It is just simply wrong, and needs a substantial rewrite.

Using "generic_file_write" is wrong. It is a service routine for filesystems to call. Not an interface for clients of a filesystem to call.

You should look at loop.c. It seems to be closer to correct. You need to see the files as an "address_space" and use address_space operations to access it.
->readpage to read a page
->prepare_write / ->commit_write to write a page (or maybe
write_page, I'm not sure)
->sync_page to sync a patch

Apart from that....

The more I think about it, the less I like extra fields being added to the superblock.
I think the bitmap file should have a header with
magic
uuid (which must match raid array)
events counter (which must also match)
chunk size
other configurables.

Then to assemble an array with a bitmap you create the bitmap file, and pass it down with a new ioctl.

If the array has a persistent superblock, md check the header against the superblock, otherwise it just trusts you.

You don't need any ioctls to get or set extra information about the array. Just read the header off the bitmap file.

I'm not 100% sure what R1BIO_NotUptodate is supposed to do, but I'm fairly sure it does it wrongly, and I don't like the name (as it seems similar to "R1BIO_Uptodate", but is clearly a lot different.

So, if you can produce a patch which *only* adds a persistent bitmap in a file, uses it to record which blocks are not in-sync, and optimises resync using the bitmap, and which uses address_space operations for fileio, then I will see if it is acceptable, and we can then start adding bits like hot-repair and async-write etc on top of that.

NeilBrown

Thanks for looking this over...

Agreed. I'm pretty unfamiliar with some of these interfaces, so I was not sure which ones were the correct ones to use. After looking over the interfaces a bit more and with some guidance from a couple of experts, I have a much better idea about what to do...

I'm already using read_cache_page(). Didn't realize that it would actually extend the file for me automatically. Given that it can do that, the generic read and write stuff can just be thrown out completely.

Yes, these combined with read_cache_page are all I need...

OK, fair enough. I'll look at adding a header to the bitmap file and I'll axe the superblock fields and the new ioctl I created.

...and maybe a version field and some padding to allow for future changes.

This all means no more in-memory-only bitmaps, but I think that's not really a critical feature anyway.

Right. Probably a lot simpler than the existing method.

Yep.

Yep.

Yes, looking back at this, there were a couple of errors in the way that bit was handled. I've corrected those and renamed the bit to R1BIO_Degraded. I think this is a better name as it's closer to what the bit really signifies. If the bit is set, it means:

1) either we've got a degraded array (only 1 disk), or
2) we've gotten a write failure (and we're about to degrade the array)

In either case, we will not clear the bitmap.

I'll work on that and get it out as soon as I can...

Thanks again,
Paul

OK, so here it is. The changes from the previous patch are, basically:

1) The patch is a little less intrusive now. The diffs against md/raid1 are smaller as more of the bitmap handling code is now isolated in the bitmap.c and bitmap.h files. Also, the diff against mdadm, while now a little larger, is less intrusive as I've created a bitmap.c file in the mdadm sources, as well.

2) The bitmap file I/O routines have been totally rewritten and are now much simpler. We now use read_cache_page, prepare_write/commit_write (to extend the file at init time, if necessary), and bmap/submit_bh (to "sync" the file pages). The use of bmap and submit_bh is due to a limitation in jbd that allows only one ongoing transaction per process (see comment in bitmap.c regarding current->journal_info).

3) The bitmap file now has a superblock containing various configuration information. The bitmap superblock is checked against the md superblock when the array is assembled.

Options have been added to mdadm to create and examine bitmap files, and also to specify the bitmap file to be used with an array:

Create a bitmap file:
--------------------

# mdadm --create-bitmap=65536,3,580480 /tmp/bitmap --force

(this creates a bitmap file with a chunksize of 64KB, a 3 second bitmap daemon sleep period, and a bitmap (initially dirtied) which is appropriate for use with an array of 580480 blocks)

Examine a bitmap file:
---------------------

# mdadm --examine-bitmap /tmp/bitmap (or just: mdadm -X /tmp/bitmap)
        Filename : /tmp/bitmap
           Magic : 6d746962
         Version : 2
            UUID : 997cb579.99d31d20.3014cae8.4bb4bf9d
          Events : 5
           State : OK
       Chunksize : 4 KB
          Daemon : 5s flush period
      Array Size : 580480 (566.88 MiB 594.41 MB)
          Bitmap : 145120 bits (chunks), 145120 dirty (100.0%)

(in addition, mdadm -D <array> gives the bitmap information if a bitmap is attached to the array)

Create or Assemble array using a bitmap:
---------------------------------------

# mdadm -C /dev/md2 -n 2 -l 1 --bitmap=/tmp/bitmap /dev/sda5 missing

# mdadm -A /dev/md2 --bitmap=/tmp/bitmap /dev/sda5 /dev/sdb5

Patch Location:
--------------

Patch vs. linux 2.6.5-rc2
=========================
http://parisc-linux.org/~jejb/md_bitmap/md_bitmap_2_31_2_6_5_RC2.diff

Patch vs. mdadm 1.5.0
=====================
http://parisc-linux.org/~jejb/md_bitmap/mdadm_1_5_0-bitmap.diff

(no async write patch has been generated, these patches contain only the bitmap code)

Notes:
-----

1) an is_create flag was added to do_md_run to tell bitmap_create whether we are creating or just assembling the array -- this is necessary since 0.90 superblocks do not have a UUID until one is generated randomly at array creation time, therefore, we must set the bitmap UUID equal to this newly generated array UUID when the array is created

2) bitmap.h was not included in the mdadm patch, but a link (or copy) must be made to the kernel's include/linux/raid/bitmap.h file in order to build mdadm now

3) code was added to mdadm to allow creation of arrays with non-persistent superblocks (also, device size calculation with non-persistent superblocks was fixed)

4) a fix was made to the hot_remove code to allow a faulty device to be removed

5) various typo and minor bug fixes were also included in the patches

Since there hasn't been any negative feedback, I assume that this version is acceptable for inclusion in mainline/-mm?

Thanks,
Paul

"Also sprach Paul Clements:"

whew ... since I just got out of a %^&% grant appication spin, and can breathe now, you prompted me to take a look.

First thing I notice is md.o is replaced with md-mod.o. Why? That's gratuitous breakage of every poor admin's scripts that up till now has been doing an insmod md, or has had a "md" in /etc/modules.

Owwww ....

Is the reason perhaps that md.c is no longer the whole story? So that there is extra linking? I guess so. Why not put the extra stuff in a separate module and leave md.o be?

I'll not comment on the bitmap.c file (I still need to figure out ow it can possibly work :-). But I like the comments in the file! Looks comprehensible and very neat. Perhaps a summary of what's in it could be put up top ... or is that likely to get out of step? Perhaps.

The changes to md.c ... look very benign. I see you add a function, export a function previously static ... add bitmap sb update to the sb update fn and export it.

I can't be sure from the diff alone, but it looks as though a bitmap is always created with bitmap_create, and there is no way to stop it? Is that right? I'd need to see more context in the diff to figure it out. Perhaps the calls to bintmmap_create and bitmap_destroy don't do anything unless the sb is marked as "wanting"

get_bitmap_file seems to be intended to talk to a user, but it's static! Are you going to use it in an ioctl? If so, surely the ioctl part should do the copy_to_user, and this function should not?

It's also go a kmalloc of a buf inside, and I would have thought you had loadsa potential bufferspace lying around in the superblocks you have in memory ... oh well, it's independent this way.

There's a tiny change

- if (rdev->raid_disk >= 0)
+ if (rdev->raid_disk >= 0 && !rdev->faulty)

                  goto busy;

which needs more context for me to judge! So we don't pretend we're busy if what we're busy with is faulty? Where are we? It must be a diskop of some kind.

The code following ...

+
+ /* check the sb on the new disk to see whether the bitmap can be used
+ * to resync it (if not, we dirty the bitmap to cause a full resync) */
+ if (mddev->bitmap) {
+ int bitmap_invalidate = 1;
+ mdk_rdev_t *rdev0 = list_entry(mddev->disks.next,
+ mdk_rdev_t, same_set);

looks entirely opaque to me! Loads of if's.

There's an addition of set_bitmap_file as an allowed diskop when we aren't initialised yet - entirely harmless and correct.

Ah .. here I think we have the ioctl for get_bitmap_file. Yes, but I would have prefered the copy_to_user here, not in the cllaed routine.

Maybe set_bitmap_file does the same "trick"?

There's some status report output reformatting which is hard to figure. Maybe only a \n added? Probably because you follow with a bitmap status report when there is one. Shouldn't this bitmap report be a function in the bitmap.c file? Or would that be all cyclic in terms of dependencies?

Here there is something I don't get:

  -       if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
  +       /* we don't use the checkpoint if there's a bitmap */
  +       if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery) && !mddev->bitmap)

Are you trying to avoid the speed throttling problem here? I guess the checkpoints are where the speed is recalculated too.

When I patched md.c here to get around the throttle (it doesn't know that some of the synced blocks are "virtual", thanks to the bitmap code), I did it by adding an extra array of "discounted blocks" that the sync code could increment every time it skipped something. Then I changed the throttle speed calculation in md.c to discount the discounted blocks each time, and only consider the remainder, which had been "really" written. So the throttle acted on the real i/o speed, not the virtual speed.

But for printout, I used the virtual speed.

Here's another bit that needs explanation:

- blk_run_queues();
+ if (need_sync)
+ blk_run_queues();

And here ...

+ /* don't worry about speed limits unless we do I/O */
+ if (!need_sync)
+ continue;

Are you saying that in the corner case when the sync at since the previous checkpoint has been entirely "virtual", we just dive right back round again? What's this "need_sync" *exactly*? When do you think we "need" a sync?

Hurrr .. I see changes to raid1.c next. I'll consider them after a stiff drink.

In summary, the md.c changes look almost certainly benign to me, with just a couple of one-liners that I would need tolook at harder. I would be grateful if you maybe comment on them here (or even in the code).

Perhaps commenting the patch would be appropriate? Can one add text between hunks? One can do it before the diff starts.

Peter

On Wednesday March 31, [email protected] wrote:
and less that a month later I replied .... I've been busy and had two weeks leave in there. Sorry.

Maybe:
mdadm --create-bitmap --chunksize=64 --delay=3 --size==580480 ???
while more verbose, it is also easier to remember and more in-keeping with the rest of mdadm's syntax.

I think this is the wrong approach. You are justifying a design error by reference to a previous design error.
I think user-space should be completely responsible for creating the bitmap file including setting the UUID.
Either
1/ add the bitmap after the array has been created.
or
2/ Create the array in user-space and just get the kernel to
assemble it (this is what I will almost certainly do in mdadm once I get around to supporting version 1 superblocks).

please, Please, PLEASE, keep separate patches separate. It makes them much easier to review, and hence makes acceptance much more likely.

In particular, your fix in 4 is, I think, wrong. I agree that there is a problem here. I don't think your fix is correct.

But, onto the patch.

1/ Why bitmap_alloc_page instead of just kmalloc?
If every kernel subsystem kept it's own private cache of memory in case of desperate need, then there would be a lot of memory
wastage. Unless you have evidence that times when you need to
allocate a bitmap are frequently times when there is excessive memory pressure, I think you should just trust kmalloc. On the other hand, if you have reason to believe that the services of kmalloc are substantially suboptimal for your needs, you should explain why in a comment.

2/There is a race in bitmap_checkpage.
I assume that the required postcondition is that (if the arguments are valid) either bitmap->bp[page].hijacked or bitmap->bp[page].map but not both. If this is the case, then

+ if ((mappage = bitmap_alloc_page(bitmap)) == NULL) {
+ PRINTK("%s: bitmap map page allocation failed, hijacking\n",
+ bmname(bitmap));
+ /* failed - set the hijacked flag so that we can use the
+ * pointer as a counter */
+ spin_lock_irqsave(&bitmap->lock, flags);
+ bitmap->bp[page].hijacked = 1;
+ goto out_unlock;
+ }

should become:

+ if ((mappage = bitmap_alloc_page(bitmap)) == NULL) {
+ PRINTK("%s: bitmap map page allocation failed, hijacking\n",
+ bmname(bitmap));
+ /* failed - set the hijacked flag so that we can use the
+ * pointer as a counter */
+ spin_lock_irqsave(&bitmap->lock, flags);
+ if (!bitmap->bp[page].map) bitmap->bp[page].hijacked = 1;
+ goto out_unlock;
+ }

as someone else could have allocated a bitmap while we were trying and failing.

3/ Your bmap_page / sync_page is very filesystem-specific.

bmap_page sets page->private to a linked-list of buffers. However page->private "belongs" to the address-space that the page is in, which means the filesystem.
I don't know if any filesystems use page->private for anything other than a list of buffers, but they certainly could if they wanted to, and if they did, this code would suddenly break.

I think that if you have your heart set on being able to store the bitmap in a file, that using a loop-back mount would be easiest. But if you don't want to do that, at least use the correct interface. Referring to the code in loop.c would help.

Another alternative would be to use the approach that swapfile uses. i.e. create a list of extents using bmap information, and then do direct IO to the device using this extent information. swapfile chooses to ignore any extent that is less that a page. You might not want to do that, but you wouldn't have to.

4/ I would avoid extending the file in the kernel. It is too easy
to do that in user space. Just require that the file is the correct size.

5/ I find it very confusing that you kmap a page, and then leave it
to some function that you call to kunmap the page (unmap_put_page or sync_put_page). It looks very unbalanced. I would much rather see the kunmap in the same function as the kmap.

6/ It seems odd that bitmap_set_state calls unmap_put_page instead
of sync_put_page. Surely you want to sync the superblock at this point.
7/ The existence of bitmap_get_state confuses me. Why not store the
state in the bitmap structure in bitmap_read_sb??

8/ calling md_force_spare(.., 1) to force a sync is definitely
wrong. You appear to be assuming a raid1 array with exactly two devices. Can't you just set recovery_cp to 0, or maybe just set a flag somewhere and test it in md_check_recovery??

9/ why don't you just pass "%s_bitmap" as the thread name to
md_register_thread ? As far as I can tell, it would have exactly the same effect as the current code without requiring a kmalloc.

10/
+static void bitmap_stop_daemon(struct bitmap *bitmap) +{
+ mdk_thread_t *daemon;
+ unsigned long flags;
+
+ spin_lock_irqsave(&bitmap->lock, flags);
+ if (!bitmap->daemon) {
+ spin_unlock_irqrestore(&bitmap->lock, flags);
+ return;
+ }
+ daemon = bitmap->daemon;
+ bitmap->daemon = NULL;
+ spin_unlock_irqrestore(&bitmap->lock, flags);
+ md_unregister_thread(daemon); /* destroy the thread */
+}

would look better as:

+static void bitmap_stop_daemon(struct bitmap *bitmap) +{
+ mdk_thread_t *daemon;
+ unsigned long flags;
+
+ spin_lock_irqsave(&bitmap->lock, flags);
+ daemon = bitmap->daemon;
+ bitmap->daemon = NULL;
+ spin_unlock_irqrestore(&bitmap->lock, flags);
+ if (bitmap->daemon)
+ md_unregister_thread(daemon); /* destroy the thread */
+}

11/ md_update_sb needs to be called with the mddev locked, and I
think there are times when you call it without the lock. I would prefer it if you left it 'static' and just set the sb_dirty flag. raid[156]d will the update date it soon enough.

12/ The tests in hot_add_disk to see if the newly added device is
sufficiently up-to-date that the bitmap can be used to get it the rest of the way up-to-date must be wrong as they don't contain any reference to 'events'.
You presumably want to be able to fail/remove a drive and then re-add it and not require a full resync.
For this to work, you need to record an event number when the bitmap switches from "which blocks on active drives are not in-sync" to "which blocks active drives have changed since a drive failed", and when you incorporate a new device, only allow it to be synced based on the bitmap if the event counter is at least as new as the saved one (plus the checks you currently have).

13/ The test
+ } else if (atomic_read(&mddev->bitmap_file->f_count) > 2) {
in set_bitmap_file is half-hearted at best.
What you probably want is "deny_write_access".
Or just check that the file is owned by root and isn't world writable.

The check against the uuid in the header should be enough to ensure that operator error doesn't result in the one file being used for two arrays.

14/ In md_do_sync, I think you should move "cond_reshed()" above

@@ -3312,6 +3506,10 @@ static void md_do_sync(mddev_t *mddev)
goto out;
}

+ /* don't worry about speed limits unless we do I/O */
+ if (!need_sync)
+ continue;
+
/*
* this loop exits only if either when we are slower than
* the 'hard' speed limit, or the system was IO-idle for

to make sure that mdX_sync doesn't steal too much time before allowing a reschedule.

and the worst part about it is that the code doesn't support what I would think would be the most widely used and hence most useful case, and that is to store the bitmap in the 60K of space after the superblock.

If you had implemented that first, then you could have avoided all the mucking about with files, which seems to be a problematic area, and probably had working and accepted code by now. Then adding support for separate files could have been layered on top.
That would have meant that each piece of submitted code was substantially smaller and hence easier to review.

Anyway, enough for now. I'll try to review your next patch with a little less delay, but it is a substantial effort, and that makes it easy to put off.

NeilBrown

Ahh, I noticed you'd been absent from the mailing lists for a bit...

Anyway, thanks for taking the time to review this...

OK...and it will probably change slightly if we're not doing bitmaps in files...more on that below...

I agree, I don't like it either, but there is no clean solution that I can think of...

We can't do this because the initial resync would have started.

I'd gladly support version 1 superblocks, but currently the tools and kernel support for them is not complete.

So in order to have working code, right now, unfortunately, my hack is a necessary evil. If there's a cleaner way to make this work, I'm certainly open to suggestions.

Yes, I should have cleaned the patch up a bit...sorry about that...

Could you explain what's wrong with it? I'll be glad to alter the patch if there's a better way to fix this problem.

That was part of Peter's original patch and I never bothered to change it. Admittedly, there probably is not a valid reason for having the cache. I'll remove it and just use kmalloc. If we find we need it later, it can be added back...

Yes, you're right.

XFS uses page->private differently...

We could not do the prepare_write/commit_write (as loop does) because of the current->journal_info limitation in jbd/ext3 (i.e., a single process cannot have two jbd transactions ongoing at a time, even though the two transactions are for different filesystems). In order to work around that limitation, we would have had to create a separate thread to do the bitmap writes, which is complex and probably too slow to be an acceptable solution.

I now agree with you that (due to the aforementioned limitations) bitmap files are not going to work. I think, at least for now, we'll go with a bitmap located on a device at a certain offset. So, for a bitmap located on the md partition itself, we specify an offset of sb_offset+4096. For a standalone bitmap device/partition (or loopback mount of a file, as you suggested) we give a 0 offset.

OK. That's easy enough to change.

This is code that was necessary in 2.4, where it was harder to trigger a resync. I think this can be cleaned up for 2.6.

OK

I think that will be OK, as long as it doesn't open up a window for things to get into an inconsistent state if there's a crash.

Yes, the current code assumes only two partitions, and thus does not do this extra checking. I'll look at adding that.

I'll look at doing this.

As mentioned above, the next patch will support this configuration (as well as standalone bitmap devices/partitions).

Hopefully, the next patch will be more to your liking (and much smaller, too...)

--
Paul

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...

Patches available at:

bitmap patch for 2.6.6:
http://dsl2.external.hp.com/~jejb/md_bitmap/md_bitmap_2_32_2_6_6.diff

mdadm patch against v1.6.0:
http://dsl2.external.hp.com/~jejb/md_bitmap/mdadm_1_6_0-bitmap.diff

(the normal parisc-linux.org URLs are not working right now, for some reason...)

Thanks again,
Paul

OK, this is done. I reused the existing --chunk, --delay, and --size options. The bitmap "file" can be either a file (e.g., /tmp/my_bitmap) or a block device (e.g., /dev/sdd10).

I could not find another way to make this work with the existing code, so this remains as is.

I've removed all the other bug fixes, spelling corrections, etc.

This has been changed to simply kmalloc/kfree.

I've basically rewritten the bitmap file read/write code to be exactly like loop.c. This includes the current limitation of needing an extra write thread in which to perform the bitmap file writes (again, due to the jbd current->journal_info problem).

The file will no longer be extended in the kernel.

This has been cleaned up and simplified.

There's now an explicit bitmap_update_sb where needed.

bitmap_get_state is gone...the state now gets recorded in bitmap->flags and read from there

This has been updated to the 2.6 style of kicking off recovery (setting MD_RECOVERY_NEEDED and waking up the recovery thread).

Right, fixed.

Agreed. Actually, since the bitmap state is no longer in the md superblock, this is completely unneeded and has been removed.

Right. The extra check and event fields in the superblock have been added.

Done.

Unfortunately, this type of setup performs rather abysmally (generally, about a 5-10x slowdown in write performance). If you think about what is happening to the disk head, it becomes clear why. In fact, having the intent log anywhere on the same physical media as the array components gives very bad performance. For this reason, I have not taken extra steps to support this configuration. If anyone is curious, this type of setup can be tested using device mapper (but not loop, because loop does not have the correct sync semantics) to map that area of the disk as a separate device and use it as a bitmap.

Thanks. I've penciled it into my diary to look through this on Tuesday 15th
NeilBrown

Unfortunately, these dsl2 URLs aren't working today - I should have grabbed the patches earlier. I'll check again later in the day.

I guess that means you (or someone) needs to write some code. Creating the array, including allocating the UUID, in userspace is trivial. Just make some superblocks and write them into the right
location in each device. Then assemble the array as you would any
pre-existing array.

Ahhh... this explains a comment you made some time ago where I thought performance would be pretty poor and you said it wasn't. I was imagining the bitmap in the same device as the array and you weren't.

I think I mentioned at the time that this could be addressed by using "plugging".

When a write request arrives for a block that isn't flagged as "dirty", we flag it 'dirty' and put the request on a queue. When an "unplug" request is made, we flush all updates to the "dirty" bitmap, and then release the queued write requests. There could still be a noticeable performance hit, but it should be substantially less than with the current code.

Everything else you mentioned sounds good. When I manage to get a copy of the patch I'll comment further.

NeilBrown

I'm sorry, that's my fault. dsl2 is being decomissioned ... it was a crappy x86 box, we're replacing it with a parisc one.

The correct URL should have www.parisc-linux.org in place of dsl2.external.hp.com

James

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

Yes, I think --build didn't use to work for raid1, though, which is why I went the create route. I'll fix this.

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?

-X is usually what I use...that's a lot easier to remember and type...

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

Neil,

responses to your feedback on the kernel code:

Yes, there were a couple. I've fixed them now.

Yes, that's right. Fixed.

OK, that sounds great.

Thanks,
Paul

Hi Neil,

Here's the next round of patches:

kernel patch against 2.6.7:
--------------------------
http://www.parisc-linux.org/~jejb/md_bitmap/md_bitmap_2_32_2_6_7.diff

mdadm patch against 1.6.0:
-------------------------
http://www.parisc-linux.org/~jejb/md_bitmap/md_bitmap_2_32_2_6_7.diff

Details below...

Fixed.

Fixed.

OK, changed. The --build command now also accepts the --bitmap option.

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).

This also remains as-is for now, pending your reply to my previous emails.

Thanks,
Paul

[snip]

err, this should be:

http://www.parisc-linux.org/~jejb/md_bitmap/mdadm_1_6_0-bitmap-2.diff

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

Neil,

I've implemented the improvements that you've suggested below, and preliminary tests are showing some very good results!

The latest patches are available at:

http://www.parisc-linux.org/~jejb/md_bitmap/

Further details below...

Thanks,
Paul

Yes, that's quite true. In fact, in my simple write performance tests I used to see a slowdown of around 30% with the bitmap file...now, the difference is not even measurable! (This is with the bitmap file located on a dedicated disk, in order to reduce disk head movement, which tends to degrade performance).

I've implemented more or less what you've described above in this latest patch.

Yes, getting rid of the bitmap_update structure was a very good idea. The code is much cleaner without that...

This is definitely heading in the right direction. However I think you have missed some of the subtleties of plugging.

Plugging should be seen as entirely the responsibility of each device. Each device will plug it's own queue if or when it thinks that is a good idea, and will possibly unplug it spontaneously. Thus if you ever call generic_make_request, you have to assume that the request could get services at any moment.

The only interest that clients of a device have in plugging is that they may request the device to unplug, in case it still has some old requests in a plugged queue.

When a raid1 write request comes, your code passes that request to generic_make_request, and schedules a bitmap update for later. Presumably you assume that the underlying devices are or will-be plugged and only get unplugged when ->unplug_fn is called. However this isn't true.

What you need to do is:
In the WRITE branch of make_request, schedule the bitmap update as you currently do, but *don't* call generic_make_request on the bio. Instead, you should add the bio to a queue using ->bi_next for linkage.

Then in raid1_unplug, you should:
Call bitmap_unplug
(this waits for the bitmaps to be safe)
Walk through the queue of bio and submit them with
make_request. As the bit(s) will already be marked dirty, the request will go straight through.
Finally call unplug_slaves.

That should then have the bitmap updates happening before the data updates, and should cluster bitmap updates better.

The next step would be to stop write_page from being synchronous - I'm not comfortable about ->unplug-fn having to wait for disk io.

This looks to be quite awkward as there is not call-back that happens when writeback on a page is completed. You need to have a kernel thread to wait for the pages to complete. I would get the bitmap_daemon to do this.

write_page puts the page on a queue and wakes up bitmap_write_daemon bitmap_write_daemon calls prepare_write/commit_write and puts the page on another queue and wakes up bitmap_daemon.

bitmap_daemon whenever it is woken up:
- checks if any clean bitmaps needs to be written and puts them on the queue for bitmap_write_daemon
- checks if there is a page in the queue from bitmap_write_daemon and, if there is, waits for it (wait_on_page_writeback). When that completes, put the page and decrease a count, and go round the loop.
- if the count of pending bitmap updates hits zero, bitmap_daemon walks the queue of pending write requests of the array (as I suggested above could be done in raid1_unplug) and submits them.

Does that make sense?

I would suggest doing the first version first (leaving write_page synchronous), and then have a go at making write_page synchronous.

NeilBrown

OK, here we go again. The patch has been updated to 2.6.9-rc2 and mdadm 1.7.0, and I've implemented the suggestions you've made below...

Patches: http://parisc-linux.org/~jejb/md_bitmap/

Thanks,
Paul

----

[snip]

OK, I've fixed that.

Well, it still does have to wait for the bitmap writes to complete before letting the regular writes go, but I see your point about the writes being submitted simultaneously (nearly) rather than being completely serialized.

OK, this is what I've done...there didn't seem to be a better place to do it.

Certainly does, and seems to work...

Well, I ran some simple write performance benchmarks today and I was stunned to find the bitmap performance was terrible. After debugging a while, I realized the problem was having the bitmap daemon do the wait_on_page_writeback...if the daemon got caught at the wrong time, it could take quite a while to come back around and do the writeback. So, I added yet another thread, the bitmap writeback daemon (whose sole purpose is to wait for page writebacks) and now things are running excellently. My simple benchmarks, which previously showed about a 25-30% slowdown when using a bitmap vs. not using one, now show only a 4% slowdown, which is pretty close to the margin of error in the test itself. Check out the new patch here:

http://parisc-linux.org/~jejb/md_bitmap/md_bitmap_2_37_2_6_9_rc2.diff

Thanks again,
Paul

Further comments.

bitmap_events
1/ You have inserted bitmap_event_hi/lo *before* recovery_cp, thus
moving recovery_cp, and thus breaking backwards comparability.
2/ The test in hot_add_disk:
+ if (refsb && sb && uuid_equal(sb, refsb) &&
+ sb->events_hi >= refsb->bitmap_events_hi &&
+ sb->events_lo >= refsb->bitmap_events_lo) {
+ bitmap_invalidate = 0;
is wrong. The events count must be compared as a 64bit
number. e.g. it is only meaningful to compare events_lo if both events_hi are equal.

pending_bio_list
1/ Do you really need a separate pending_bio_lock, or would
the current device_lock be adequate to the task.
2/ I think there can be a race with new requests being added to
this list while bitmap_unplug is running in unplug_slaves. I think you should "bio_get_list" before calling bitmap_unplug, So that you only then submit requests that were made definitely *before* the call the bitmap_unplug. This would have the added advantage that you don't need to keep claiming and dropping pending_bio_lock.

random damage
1/ You have changed r1bio_pool_free without it being a useful
change.
2/ You have removed "static" from sync_page_io for no apparent
reason.
3/ You declare and set start_sector_nr, and never use it.

I have removed the "random damage" bit and merged that patch into my collection. I would really appreciate it if any further changes could be sent as incremental patches, as reading through a large patch like that takes a fair bit of effort.

I'll hopefully be finishing up some modification to mdadm this week. I'll aim to include support for bitmaps as part of that (based on your patch, but probably with a number of changes to match changes in mdadm).

NeilBrown

Yes. I guess when recovery_cp came along I failed to notice that...

Probably so...especially with the following change...

Yes, that would make sense.

Yes, I certainly understand that. I appreciate all the effort you've put into reviewing and giving suggestions. Further patches will be on top of the current bitmap patch.

I will send out an incremental patch to fix the issues above, probably in a couple of days.

Thanks,
Paul

Neil,

patch to fix the issues mentioned below has been tested and is attached. Should apply on top of 2.6.9-rc2 + md_bitmap.

--
Paul

diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.6.9-rc2-BITMAP/drivers/md/md.c linux-2.6.9-rc2-BITMAP-NEW/drivers/md/md.c
@@ -2375,8 +2375,9 @@ static int hot_add_disk(mddev_t * mddev,
 		if (rdev->sb_loaded)
 			sb = (mdp_super_t *)page_address(rdev->sb_page);
 		if (refsb && sb && uuid_equal(sb, refsb) &&
-		    sb->events_hi >= refsb->bitmap_events_hi &&
-		    sb->events_lo >= refsb->bitmap_events_lo) {
+		    (sb->events_hi > refsb->bitmap_events_hi ||
+		    (sb->events_hi == refsb->bitmap_events_hi &&
+		    sb->events_lo >= refsb->bitmap_events_lo))) {
 			bitmap_invalidate = 0;
 		} else if (!mddev->persistent) { /* assume bitmap is valid */
 			bitmap_invalidate = 0;

diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.6.9-rc2-BITMAP/drivers/md/raid1.c linux-2.6.9-rc2-BITMAP-NEW/drivers/md/raid1.c
@@ -455,18 +455,21 @@ static void unplug_slaves(mddev_t *mddev
 	struct bio *bio;
 	unsigned long flags;
 
+	/* pull writes off the pending queue and (later) submit them */
+	spin_lock_irqsave(&conf->device_lock, flags);
+	bio = bio_list_get(&conf->pending_bio_list);
+	spin_unlock_irqrestore(&conf->device_lock, flags);
+
 	/* flush any pending bitmap writes to disk before proceeding w/ I/O */
 	if (bitmap_unplug(mddev->bitmap) != 0)
 		printk("%s: bitmap file write failed!\n", mdname(mddev));
 
-	/* pull writes off the pending queue and submit them */
-	spin_lock_irqsave(&conf->pending_bio_lock, flags);
-	while ((bio = bio_list_pop(&conf->pending_bio_list))) {
-		spin_unlock_irqrestore(&conf->pending_bio_lock, flags);
+	while (bio) { /* submit pending writes */
+		struct bio *next = bio->bi_next;
+		bio->bi_next = NULL;
 		generic_make_request(bio);
-		spin_lock_irqsave(&conf->pending_bio_lock, flags);
+		bio = next;
 	}
-	spin_unlock_irqrestore(&conf->pending_bio_lock, flags);
 
 	spin_lock_irqsave(&conf->device_lock, flags);
 	for (i=0; i<mddev->raid_disks; i++) {

@@ -666,9 +669,9 @@ static int make_request(request_queue_t 
 
 		atomic_inc(&r1_bio->remaining);
 		/* queue the write...it will be submitted when we unplug */
-		spin_lock_irqsave(&conf->pending_bio_lock, flags);
+		spin_lock_irqsave(&conf->device_lock, flags);
 		bio_list_add(&conf->pending_bio_list, mbio);
-		spin_unlock_irqrestore(&conf->pending_bio_lock, flags);
+		spin_unlock_irqrestore(&conf->device_lock, flags);
 	}
 
 	if (atomic_dec_and_test(&r1_bio->remaining)) {

@@ -965,9 +968,9 @@ static void sync_request_write(mddev_t *
 				"while resyncing!\n", mdname(mddev), err);
 
 		/* queue the write...it will be submitted when we unplug */
-		spin_lock_irqsave(&conf->pending_bio_lock, flags);
+		spin_lock_irqsave(&conf->device_lock, flags);
 		bio_list_add(&conf->pending_bio_list, wbio);
-		spin_unlock_irqrestore(&conf->pending_bio_lock, flags);
+		spin_unlock_irqrestore(&conf->device_lock, flags);
 	}
 
 	if (atomic_dec_and_test(&r1_bio->remaining)) {

@@ -1307,7 +1310,6 @@ static int run(mddev_t *mddev)
 	init_waitqueue_head(&conf->wait_idle);
 	init_waitqueue_head(&conf->wait_resume);
 
-	conf->pending_bio_lock = SPIN_LOCK_UNLOCKED;
 	bio_list_init(&conf->pending_bio_list);
 
 	if (!conf->working_disks) {

diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.6.9-rc2-BITMAP/include/linux/raid/md_p.h linux-2.6.9-rc2-BITMAP-NEW/include/linux/raid/md_p.h
@@ -133,17 +133,20 @@ typedef struct mdp_superblock_s {
 	__u32 events_lo;	/*  8 low-order of superblock update count    */
 	__u32 cp_events_hi;	/*  9 high-order of checkpoint update count   */
 	__u32 cp_events_lo;	/* 10 low-order of checkpoint update count    */
-	__u32 bitmap_events_hi;	/* 11 high-order of bitmap update count       */
-	__u32 bitmap_events_lo;	/* 12 low-order of bitmap update count        */
 #else
 	__u32 events_lo;	/*  7 low-order of superblock update count    */
 	__u32 events_hi;	/*  8 high-order of superblock update count   */
 	__u32 cp_events_lo;	/*  9 low-order of checkpoint update count    */
 	__u32 cp_events_hi;	/* 10 high-order of checkpoint update count   */
-	__u32 bitmap_events_lo;	/* 11 low-order of bitmap update count        */
+#endif
+	__u32 recovery_cp;	/* 11 recovery checkpoint sector count	      */
+#ifdef __BIG_ENDIAN
 	__u32 bitmap_events_hi;	/* 12 high-order of bitmap update count       */
+	__u32 bitmap_events_lo;	/* 13 low-order of bitmap update count        */
+#else
+	__u32 bitmap_events_lo;	/* 12 low-order of bitmap update count        */
+	__u32 bitmap_events_hi;	/* 13 high-order of bitmap update count       */
 #endif
-	__u32 recovery_cp;	/* 13 recovery checkpoint sector count	      */
 	__u32 gstate_sreserved[MD_SB_GENERIC_STATE_WORDS - 14];
 
 	/*

diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.6.9-rc2-BITMAP/include/linux/raid/raid1.h linux-2.6.9-rc2-BITMAP-NEW/include/linux/raid/raid1.h
@@ -37,7 +37,6 @@ struct r1_private_data_s {
 
 	/* queue pending writes and submit them on unplug */
 	struct bio_list		pending_bio_list;
-	spinlock_t		pending_bio_lock;
 
 	/* for use when syncing mirrors: */
 

Hi Paul,
I've been looking through the bitmap code again and found the handling of resync a bit hard to wrap my brain around. The handling of the counter here seems a bit non-obvious and possibly fragile.

I would like to make a change, but thought I should bounce it off you first.

I would like to change the "RESYNC_MASK" bit to mean:
At least one block in this chunk is out-of-sync.

Then:
- When we read the bitmap from disk we set this bit and the SHADOW_MASK, but leave the counter at zero.
- When we get a failed write, we set this bit, but still decrement the counter.
- When we are performing a resync, we periodically clear the bit on recently completed chunks.
- We only clear the SHADOW_MASK and on-disk bit when the counter hits zero *and* this bit is clear.

I would find this approach a lot easier to understand. Are you OK with it?

Also, I would like to move the bitmap_testbit in md_do_sync down into the personality. This should make life easier for other personalities like raid10 which use a very different approach for resync than for recovery.

NeilBrown

Hi Neil,

OK

I assume that this also means that the counter will get incremented before each read-for-resync is submitted. Perhaps you've already considered that?

It sounds reasonable. Probably a little simpler too...

OK, that sounds fine.

--
Paul