Re: [ANNOUNCE][PATCH 2.6] md: persistent (file-backed) bitmap and async writes

[Paul Clements <Paul.Clements@SteelEye.com>]

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