Re: [PATCH 02/12] raid5-cache: free I/O units earlier

[Christoph Hellwig <hch@lst.de>]

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.

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

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