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

On Monday February 9, Paul.Clements@SteelEye.com wrote:

Neil Brown wrote:

quoted

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

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.

quoted

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

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.

quoted

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

quoted

  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.

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

quoted

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

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.

quoted

- The bitmap_page structure is amusing.

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

quoted

  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.

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.

quoted

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

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

Neil Brown wrote:
 

quoted

quoted

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

quoted

  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.

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

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

Neil Brown wrote:
 

quoted

quoted

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

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.

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