On Tue, Sep 15, 2015 at 09:00:31AM +0200, Neil Brown wrote:

This only saves the allocation for two I/O units doesn't it?  So not a
big saveding, but still a good clean-up.

Yes.

quoted

r5l_do_reclaim as a side effect: previous if took the last unit which
isn't checkpointed into account.

It took me a while to see that - so just to be sure I understand.

There is a list of I/O units which have been completely written to the
log and to the RAID.  The last one of these is used as a marker for the
start of the log, so it cannot be freed yet.  All the earlier ones can
be freed.
The previous code added up the sizes of all of the units in this list,
so it incorrectly included that last unit.
Your new code calculates the difference between the start of the first
unit and the start of the last unit, and so correctly excluded the last
unit from the free space calculation.

Did I get that right?

You did - or at very least your understanding matches my understanding of
the old code.

quoted

+		md_wakeup_thread(log->rdev->mddev->thread);
+		wait_event_lock_irq(log->iounit_wait,
+				    r5l_reclaimable_space(log) > reclaimable,
+				    log->io_list_lock);
 	}

This is ringing warning bells....  I'm not sure it's wrong, but I'm not
sure it is right.
I feel that if the thread gets woken and finds that
r5l_reclaimable_space is still too low, it should wake up the thread
again, just in case.
Also, it should test against reclaim_target, not reclaimable, shouldn't
it?

Instead of a wait_event_lock_irq inside a loop, could we just have a
wake_event_*??

wait_event_lock_irq_cmd(log->iounit_wait,
                        r5l_reclaimable_space(log) >= reclaim_target ||
                        (list_empty() && .... list_empty()),
                        log->io_list_lock,
                        md_wakeup_thread(log->rdev->mddev->thread));

??


Otherwise I like it.  Thanks.

I agree with a lot of your points, but I tried to match the old behavior
as close as possible.  I can change it to something closer to your
suggestion if Shaohua agrees.