Re: [PATCH] md/r5cache: handle alloc_page failure

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]

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@fb.com>
> ---
>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]