Re: [PATCH 2/5] Btrfs: fix trans block rsv regression

[Liu Bo <bo.li.liu@oracle.com>]

On 09/14/2012 07:15 PM, David Sterba wrote:
> On Fri, Sep 14, 2012 at 04:58:04PM +0800, Liu Bo wrote:
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -306,9 +306,17 @@ static struct btrfs_trans_handle *start_transaction(struct btrfs_root *root,
>>  		WARN_ON(type != TRANS_JOIN && type != TRANS_JOIN_NOLOCK &&
>>  			type != TRANS_JOIN_ONLY);
>>  		h = current->journal_info;
>> -		h->use_count++;
>> -		h->orig_rsv = h->block_rsv;
>> +		if (h->block_rsv) {
>> +			struct btrfs_trans_rsv_item *item;
>> +			item = kmalloc(sizeof(*item), GFP_NOFS);
> 
> I'd rather avoid the kmalloc here and add a list hook into
> btrfs_block_rsv itself (used only for this purpose).
> 
> It also does not increase the failure surface and we don't have to
> handle error conditions from deep callchains.
> 

Actually I placed a list hook at first, but I found the same block_rsv could be inserted into
the list chain twice, which will cause list_head's terrible warnings.

>> +			if (!item)
>> +				return ERR_PTR(-ENOMEM);
>> +			item->rsv = h->block_rsv;
>> +			INIT_LIST_HEAD(&item->list);
>> +			list_add(&item->list, &h->blk_rsv_list);
>> +		}
>>  		h->block_rsv = NULL;
>> +		h->use_count++;
>>  		goto got_it;
>>  	} else if (type == TRANS_JOIN_ONLY) {
>>  		return ERR_PTR(-ENOENT);
>> --- a/fs/btrfs/transaction.h
>> +++ b/fs/btrfs/transaction.h
>> @@ -57,7 +57,6 @@ struct btrfs_trans_handle {
>>  	unsigned long delayed_ref_updates;
>>  	struct btrfs_transaction *transaction;
>>  	struct btrfs_block_rsv *block_rsv;
>> -	struct btrfs_block_rsv *orig_rsv;
>>  	int aborted;
>>  	int adding_csums;
>>  	/*
>> @@ -68,6 +67,12 @@ struct btrfs_trans_handle {
>>  	struct btrfs_root *root;
>>  	struct seq_list delayed_ref_elem;
>>  	struct list_head qgroup_ref_list;
>> +	struct list_head blk_rsv_list;
> 
> Does it refer to chain of orig_rsv's ? Ie. naming it orig_blk_rsv_list
> 

Make sense.

>> +};
>> +
>> +struct btrfs_trans_rsv_item {
>> +	struct btrfs_block_rsv *rsv;
>> +	struct list_head list;
> 
> Generally, for such 'list of single pointers' structs I'd evaluate the
> possibility of embedding the hook inside the struct, the overhead
> (memory, processing) is not desirable.
> 

See the above, plz.

thanks,
liubo


>>  };
>>  
>>  struct btrfs_pending_snapshot {