On Sat, Nov 19 2016, Song Liu wrote:
> RMW of r5c write back cache uses an extra page to store old data for
> prexor. handle_stripe_dirtying() allocates this page by calling
> alloc_page(). However, alloc_page() may fail.
>
> To handle alloc_page() failures, this patch adds a small mempool
> in r5l_log. When alloc_page fails, the stripe is added to a waiting
> list. Then, these stripes get pages from the mempool (from work queue).
>
> Signed-off-by: Song Liu <songliubraving@xxxxxx>
> ---
> drivers/md/raid5-cache.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++-
> drivers/md/raid5.c | 34 +++++++++++-----
> drivers/md/raid5.h | 6 +++
> 3 files changed, 130 insertions(+), 10 deletions(-)
This looks *way* more complex that I feel comfortable with. I cannot
see any obvious errors, but I find it hard to be confident there aren't
any, or that errors won't creep in.
We already have mechanisms for delaying stripes. It would be nice to
re-use those.
Ideally, when using a mempool, a single allocation from the mempool
should provide all you need for a single transaction. For that to work
in this case, each object in the mempool would need to hold raid_disks-1
pages. However in many cases we don't need that many, usually fewer
than raid_disks/2. So it would be wasteful or clumsy, or both.
So I would:
- preallocate a single set of spare pages, and store them, one each, in
the disk_info structures.
- add a flag to cache_state to record if these pages are in use.
- if alloc_page() fails and test_and_set() on the bit succeeds, then
use pages from disk_info
- if alloc_page() fails and test_and_set() fails, set STRIPE_DELAYED
- raid5_activate_delayed() doesn't active stripes if the flag is set,
showing that the spare pages are in use.
I think that would be much simpler, and should be just as effective.
Thanks,
NeilBrown
Attachment:
signature.asc
Description: PGP signature
