Re: [PATCH v2] dm thin: Fix bug wrt FUA request completion

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/15/19 3:54 PM, Joe Thornber wrote:
> Ack.
> 
> Thanks for this I was under the mistaken impression that FUA requests got split 
> by core dm into separate payload and PREFLUSH requests.
> 
> I've audited dm-cache and that looks ok.
> 
> How did you test this patch?  That missing bio_list_init() in V1 must
> have caused memory corruption?
> 
> - Joe

Hi Joe,

bio_list_init() initializes the bio list's head and tail pointers to
NULL and pool_create() allocates the struct pool structure using
kzalloc() so the bio list was implicitly correctly initialized and no
memory corruption occurred.

Nikos

> 
> 
> On Fri, Feb 15, 2019 at 01:21:38AM +0200, Nikos Tsironis wrote:
>> When provisioning a new data block for a virtual block, either because
>> the block was previously unallocated or because we are breaking sharing,
>> if the whole block of data is being overwritten the bio that triggered
>> the provisioning is issued immediately, skipping copying or zeroing of
>> the data block.
>>
>> When this bio completes the new mapping is inserted in to the pool's
>> metadata by process_prepared_mapping(), where the bio completion is
>> signaled to the upper layers.
>>
>> This completion is signaled without first committing the metadata. If
>> the bio in question has the REQ_FUA flag set and the system crashes
>> right after its completion and before the next metadata commit, then the
>> write is lost despite the REQ_FUA flag requiring that I/O completion for
>> this request is only signaled after the data has been committed to
>> non-volatile storage.
>>
>> Fix this by deferring the completion of overwrite bios, with the REQ_FUA
>> flag set, after the metadata has been committed.
>>
>> Signed-off-by: Nikos Tsironis <ntsironis@xxxxxxxxxxx>
>> ---
>>  drivers/md/dm-thin.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 50 insertions(+), 5 deletions(-)
>>
>> Changes in v2:
>>   - Add missing bio_list_init() in pool_create()
>>
>> v1: https://www.redhat.com/archives/dm-devel/2019-February/msg00064.html
>>
>> diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
>> index ca8af21bf644..e83b63608262 100644
>> --- a/drivers/md/dm-thin.c
>> +++ b/drivers/md/dm-thin.c
>> @@ -257,6 +257,7 @@ struct pool {
>>  
>>  	spinlock_t lock;
>>  	struct bio_list deferred_flush_bios;
>> +	struct bio_list deferred_flush_completions;
>>  	struct list_head prepared_mappings;
>>  	struct list_head prepared_discards;
>>  	struct list_head prepared_discards_pt2;
>> @@ -956,6 +957,39 @@ static void process_prepared_mapping_fail(struct dm_thin_new_mapping *m)
>>  	mempool_free(m, &m->tc->pool->mapping_pool);
>>  }
>>  
>> +static void complete_overwrite_bio(struct thin_c *tc, struct bio *bio)
>> +{
>> +	struct pool *pool = tc->pool;
>> +	unsigned long flags;
>> +
>> +	/*
>> +	 * If the bio has the REQ_FUA flag set we must commit the metadata
>> +	 * before signaling its completion.
>> +	 */
>> +	if (!bio_triggers_commit(tc, bio)) {
>> +		bio_endio(bio);
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Complete bio with an error if earlier I/O caused changes to the
>> +	 * metadata that can't be committed, e.g, due to I/O errors on the
>> +	 * metadata device.
>> +	 */
>> +	if (dm_thin_aborted_changes(tc->td)) {
>> +		bio_io_error(bio);
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Batch together any bios that trigger commits and then issue a
>> +	 * single commit for them in process_deferred_bios().
>> +	 */
>> +	spin_lock_irqsave(&pool->lock, flags);
>> +	bio_list_add(&pool->deferred_flush_completions, bio);
>> +	spin_unlock_irqrestore(&pool->lock, flags);
>> +}
>> +
>>  static void process_prepared_mapping(struct dm_thin_new_mapping *m)
>>  {
>>  	struct thin_c *tc = m->tc;
>> @@ -988,7 +1022,7 @@ static void process_prepared_mapping(struct dm_thin_new_mapping *m)
>>  	 */
>>  	if (bio) {
>>  		inc_remap_and_issue_cell(tc, m->cell, m->data_block);
>> -		bio_endio(bio);
>> +		complete_overwrite_bio(tc, bio);
>>  	} else {
>>  		inc_all_io_entry(tc->pool, m->cell->holder);
>>  		remap_and_issue(tc, m->cell->holder, m->data_block);
>> @@ -2317,7 +2351,7 @@ static void process_deferred_bios(struct pool *pool)
>>  {
>>  	unsigned long flags;
>>  	struct bio *bio;
>> -	struct bio_list bios;
>> +	struct bio_list bios, bio_completions;
>>  	struct thin_c *tc;
>>  
>>  	tc = get_first_thin(pool);
>> @@ -2328,26 +2362,36 @@ static void process_deferred_bios(struct pool *pool)
>>  	}
>>  
>>  	/*
>> -	 * If there are any deferred flush bios, we must commit
>> -	 * the metadata before issuing them.
>> +	 * If there are any deferred flush bios, we must commit the metadata
>> +	 * before issuing them or signaling their completion.
>>  	 */
>>  	bio_list_init(&bios);
>> +	bio_list_init(&bio_completions);
>> +
>>  	spin_lock_irqsave(&pool->lock, flags);
>>  	bio_list_merge(&bios, &pool->deferred_flush_bios);
>>  	bio_list_init(&pool->deferred_flush_bios);
>> +
>> +	bio_list_merge(&bio_completions, &pool->deferred_flush_completions);
>> +	bio_list_init(&pool->deferred_flush_completions);
>>  	spin_unlock_irqrestore(&pool->lock, flags);
>>  
>> -	if (bio_list_empty(&bios) &&
>> +	if (bio_list_empty(&bios) && bio_list_empty(&bio_completions) &&
>>  	    !(dm_pool_changed_this_transaction(pool->pmd) && need_commit_due_to_time(pool)))
>>  		return;
>>  
>>  	if (commit(pool)) {
>> +		bio_list_merge(&bios, &bio_completions);
>> +
>>  		while ((bio = bio_list_pop(&bios)))
>>  			bio_io_error(bio);
>>  		return;
>>  	}
>>  	pool->last_commit_jiffies = jiffies;
>>  
>> +	while ((bio = bio_list_pop(&bio_completions)))
>> +		bio_endio(bio);
>> +
>>  	while ((bio = bio_list_pop(&bios)))
>>  		generic_make_request(bio);
>>  }
>> @@ -2954,6 +2998,7 @@ static struct pool *pool_create(struct mapped_device *pool_md,
>>  	INIT_DELAYED_WORK(&pool->no_space_timeout, do_no_space_timeout);
>>  	spin_lock_init(&pool->lock);
>>  	bio_list_init(&pool->deferred_flush_bios);
>> +	bio_list_init(&pool->deferred_flush_completions);
>>  	INIT_LIST_HEAD(&pool->prepared_mappings);
>>  	INIT_LIST_HEAD(&pool->prepared_discards);
>>  	INIT_LIST_HEAD(&pool->prepared_discards_pt2);
>> -- 
>> 2.11.0
>>
> 
> --
> dm-devel mailing list
> dm-devel@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/dm-devel
> 

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel



[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux