Neil Brown wrote: > On Friday February 13, Paul.Clements@SteelEye.com wrote: > I finally had a chance to look at this properly -- or atleast at the > "bitmap" patch. Thanks for looking this over... > 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. 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... > 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 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. > ->prepare_write / ->commit_write to write a page Yes, these combined with read_cache_page are all I need... > The more I think about it, the less I like extra fields being added > to the superblock. 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. > 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. ...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. > Then to assemble an array with a bitmap you create the bitmap file, > and pass it down with a new ioctl. Right. Probably a lot simpler than the existing method. > If the array has a persistent superblock, md check the header against > the superblock, otherwise it just trusts you. Yep. > You don't need any ioctls to get or set extra information about the > array. Just read the header off the bitmap file. Yep. > 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. 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. > 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. I'll work on that and get it out as soon as I can... Thanks again, Paul - To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
