Re: [RFC][PATCH 3/3] Btrfs: improve truncation of btrfs

[Miao Xie <miaox@cn.fujitsu.com>]

On Fri, 6 Jan 2012 09:50:31 -0500, Josef Bacik wrote:
> On Fri, Jan 06, 2012 at 11:51:16AM +0800, Miao Xie wrote:
>> On thu, 5 Jan 2012 10:15:50 -0500, Josef Bacik wrote:
>>>> +	trans = btrfs_start_transaction(root, 2);
>>>> +	if (IS_ERR(trans))
>>>> +		return PTR_ERR(trans);
>>>>  
>>>>  	/*
>>>>  	 * setattr is responsible for setting the ordered_data_close flag,
>>>> @@ -6621,26 +6585,12 @@ static int btrfs_truncate(struct inode *inode)
>>>>  	 * using truncate to replace the contents of the file will
>>>>  	 * end up with a zero length file after a crash.
>>>>  	 */
>>>> -	if (inode->i_size == 0 && BTRFS_I(inode)->ordered_data_close)
>>>> +	if (newsize == 0 && BTRFS_I(inode)->ordered_data_close)
>>>>  		btrfs_add_ordered_operation(trans, root, inode);
>>
>> Since we have write out all the dirty page, we can drop the following code which is
>> in front of the while loop, and move the first btrfs_start_transaction() into the loop,
>> the logic of btrfs_truncate() will become simpler.
>>
>>>>  	while (1) {
>>>> -		ret = btrfs_block_rsv_refill(root, rsv, min_size);
>>>> -		if (ret) {
>>>> -			/*
>>>> -			 * This can only happen with the original transaction we
>>>> -			 * started above, every other time we shouldn't have a
>>>> -			 * transaction started yet.
>>>> -			 */
>>>> -			if (ret == -EAGAIN)
>>>> -				goto end_trans;
>>>> -			err = ret;
>>>> -			break;
>>>> -		}
>>>> -
>>>
>>> Taking this part out is wrong, we need to have this slack space to account for
>>> any COW that truncate does.  Other than that this looks pretty good.  Thanks,
>>>
>>
>> I think we can take this part out, because we start a new transaction every time we
>> do a truncation, and reserve enough space at that time. See below:
>>
> 
> Ok let me rephrase.  The whole reason I do this is because the reservation stuff
> is tricky, we may not actually use any of this space and so constantly going
> back to reserve it makes us much more likely to fail our truncate() because of
> ENOSPC.  But if we just hold onto a min size and then refill it when we need to
> we lower the risk considerably, so this needs to stay.  Thanks,

I see.
But I think this method is too gingerly, it can not avoid the ENOSPC completely, but it
makes the code more complex. Though dropping this part will make the risk of ENOSPC higher,
it doesn't break the meta-data. so it is enough that reserving space when starting a new
transaction and just return the error number if ENOSPC happens.

Thanks
Miao