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

[Paul Clements <paul.clements@steeleye.com>]

Neil Brown wrote:
>>Paul Clements wrote:

>>itself. Check out the new patch here:
>>
>>http://parisc-linux.org/~jejb/md_bitmap/md_bitmap_2_37_2_6_9_rc2.diff
>>

> Further comments.
> 
> bitmap_events
>   1/  You have inserted bitmap_event_hi/lo *before* recovery_cp, thus
>       moving recovery_cp, and thus breaking backwards comparability.

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

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

Yes, that is broken.

> pending_bio_list
>   1/ Do you really need a separate pending_bio_lock, or would
>      the current device_lock be adequate to the task.

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

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

Yes, that would make sense.

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

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