Re: [PATCH 3/3] raid5: allow r5l_io_unit allocations to fail

[NeilBrown <neilb@suse.com>]

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

On Fri, Dec 18 2015, Shaohua Li wrote:

> On Thu, Dec 17, 2015 at 11:09:57PM +0100, Christoph Hellwig wrote:
>> And propagate the error up the stack so we can add the stripe
>> to no_stripes_list and retry our log operation later.  This avoids
>> blocking raid5d due to reclaim, an it allows to get rid of the
>> deadlock-prone GFP_NOFAIL allocation.
>> 
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>  drivers/md/raid5-cache.c | 49 +++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 34 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
>> index e0a605f..ddee884 100644
>> --- a/drivers/md/raid5-cache.c
>> +++ b/drivers/md/raid5-cache.c
>> @@ -287,8 +287,10 @@ static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
>>  	struct r5l_io_unit *io;
>>  	struct r5l_meta_block *block;
>>  
>> -	/* We can't handle memory allocate failure so far */
>> -	io = kmem_cache_zalloc(log->io_kc, GFP_NOIO | __GFP_NOFAIL);
>> +	io = kmem_cache_zalloc(log->io_kc, GFP_ATOMIC);
>> +	if (!io)
>> +		return NULL;
>> +
>>  	io->log = log;
>>  	INIT_LIST_HEAD(&io->log_sibling);
>>  	INIT_LIST_HEAD(&io->stripe_list);
>> @@ -326,8 +328,12 @@ static int r5l_get_meta(struct r5l_log *log, unsigned int payload_size)
>>  	    log->current_io->meta_offset + payload_size > PAGE_SIZE)
>>  		r5l_submit_current_io(log);
>>  
>> -	if (!log->current_io)
>> +	if (!log->current_io) {
>>  		log->current_io = r5l_new_meta(log);
>> +		if (!log->current_io)
>> +			return -ENOMEM;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -372,11 +378,12 @@ static void r5l_append_payload_page(struct r5l_log *log, struct page *page)
>>  	r5_reserve_log_entry(log, io);
>>  }
>>  
>> -static void r5l_log_stripe(struct r5l_log *log, struct stripe_head *sh,
>> +static int r5l_log_stripe(struct r5l_log *log, struct stripe_head *sh,
>>  			   int data_pages, int parity_pages)
>>  {
>>  	int i;
>>  	int meta_size;
>> +	int ret;
>>  	struct r5l_io_unit *io;
>>  
>>  	meta_size =
>> @@ -385,7 +392,10 @@ static void r5l_log_stripe(struct r5l_log *log, struct stripe_head *sh,
>>  		sizeof(struct r5l_payload_data_parity) +
>>  		sizeof(__le32) * parity_pages;
>>  
>> -	r5l_get_meta(log, meta_size);
>> +	ret = r5l_get_meta(log, meta_size);
>> +	if (ret)
>> +		return ret;
>> +
>>  	io = log->current_io;
>>  
>>  	for (i = 0; i < sh->disks; i++) {
>> @@ -415,6 +425,8 @@ static void r5l_log_stripe(struct r5l_log *log, struct stripe_head *sh,
>>  	list_add_tail(&sh->log_list, &io->stripe_list);
>>  	atomic_inc(&io->pending_stripe);
>>  	sh->log_io = io;
>> +
>> +	return 0;
>>  }
>>  
>>  static void r5l_wake_reclaim(struct r5l_log *log, sector_t space);
>> @@ -429,6 +441,7 @@ int r5l_write_stripe(struct r5l_log *log, struct stripe_head *sh)
>>  	int meta_size;
>>  	int reserve;
>>  	int i;
>> +	int ret = 0;
>>  
>>  	if (!log)
>>  		return -EAGAIN;
>> @@ -477,18 +490,24 @@ int r5l_write_stripe(struct r5l_log *log, struct stripe_head *sh)
>>  	mutex_lock(&log->io_mutex);
>>  	/* meta + data */
>>  	reserve = (1 + write_disks) << (PAGE_SHIFT - 9);
>> -	if (r5l_has_free_space(log, reserve))
>> -		r5l_log_stripe(log, sh, data_pages, parity_pages);
>> -	else {
>> -		spin_lock(&log->no_space_stripes_lock);
>> -		list_add_tail(&sh->log_list, &log->no_space_stripes);
>> -		spin_unlock(&log->no_space_stripes_lock);
>> -
>> -		r5l_wake_reclaim(log, reserve);
>> -	}
>> -	mutex_unlock(&log->io_mutex);
>> +	if (!r5l_has_free_space(log, reserve))
>> +		goto err_retry;
>>  
>> +	ret = r5l_log_stripe(log, sh, data_pages, parity_pages);
>> +	if (ret)
>> +		goto err_retry;
>> +
>> +out_unlock:
>> +	mutex_unlock(&log->io_mutex);
>>  	return 0;
>> +
>> +err_retry:
>> +	spin_lock(&log->no_space_stripes_lock);
>> +	list_add_tail(&sh->log_list, &log->no_space_stripes);
>> +	spin_unlock(&log->no_space_stripes_lock);
>> +
>> +	r5l_wake_reclaim(log, reserve);
>> +	goto out_unlock;
>>  }
>
> if the reclaim thread doesn't have anything to reclaim,
> r5l_run_no_space_stripes isn't called. we might miss the retry.

so something like this:
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 18de1fc4a75b..b63878edf7e9 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -596,7 +596,8 @@ static void __r5l_stripe_write_finished(struct r5l_io_unit *io)
 		return;
 	}
 
-	if (r5l_reclaimable_space(log) > log->max_free_space)
+	if (r5l_reclaimable_space(log) > log->max_free_space ||
+	    !list_empty(&log->no_space_stripes))
 		r5l_wake_reclaim(log, 0);
 
 	spin_unlock_irqrestore(&log->io_list_lock, flags);

or is that too simplistic?

>
> I'm a little worrying about the GFP_ATOMIC allocation. In the first try,
> GFP_NOWAIT is better. And on the other hand, why sleep is bad here? We
> could use GFP_NOIO | __GFP_NORETRY, there is no deadlock risk.
>
> In the retry, GFP_NOIO looks better. No deadlock too, since it's not
> called from raid5d (maybe we shouldn't call from reclaim thread if using
> GFP_NOIO, a workqueue is better). Otherwise we could keep retring but do
> nothing.

I did wonder a little bit about that.
GFP_ATOMIC is (__GFP_HIGH)
GFP_NOIO | __GFP_NORETRY is  (__GFP_WAIT | __GFP_NORETRY)

It isn't clear that we need 'HIGH', and WAIT with NORETRY should be OK.
It allows __alloc_pages_direct_reclaim, but only once and never waits
for other IO.

We probably should add __GFP_NOWARN too because we expect occasional
failure.

So

--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -287,7 +287,7 @@ static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
 	struct r5l_io_unit *io;
 	struct r5l_meta_block *block;
 
-	io = kmem_cache_zalloc(log->io_kc, GFP_ATOMIC);
+	io = kmem_cache_zalloc(log->io_kc, GFP_NOIO | __GFP_NORETRY | __GFP_NOWARN);
 	if (!io)
 		return NULL;
 


Thoughts?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]