Re: [PATCH 1/3] btrfs: add a comment describing block-rsvs

[Nikolay Borisov <nborisov@suse.com>]

On 4.02.20 г. 11:30 ч., Qu Wenruo wrote:
> 
> 
> On 2020/2/4 上午4:44, Josef Bacik wrote:
>> This is a giant comment at the top of block-rsv.c describing generally
>> how block rsvs work.  It is purely about the block rsv's themselves, and
>> nothing to do with how the actual reservation system works.
> 
> Such comment really helps!
> 
> Although it looks like there are too many words but too little ascii
> arts or graphs.
> Not sure if it's really easy to read.
> 
> And some questions inlined below.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>  fs/btrfs/block-rsv.c | 81 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 81 insertions(+)
>>
>> diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
>> index d07bd41a7c1e..54380f477f80 100644
>> --- a/fs/btrfs/block-rsv.c
>> +++ b/fs/btrfs/block-rsv.c
>> @@ -6,6 +6,87 @@
>>  #include "space-info.h"
>>  #include "transaction.h"
>>


<snip>

>> + *
>> + *   We go to modify the tree for our operation, we allocate a tree block, which
>> + *   calls btrfs_use_block_rsv(), and subtracts nodesize from
>> + *   block_rsv->reserved.
>> + *
>> + *   We finish our operation, we subtract our original reservation from ->size,
>> + *   and then we subtract ->size from ->reserved if there is an excess and free
>> + *   the excess back to the space info, by reducing space_info->bytes_may_use by
>> + *   the excess amount.
> 
> So I find the workflow can be expressed like this using timeline (?) graph:
> 
> +--- Reserve:
> |    Entrance: btrfs_block_rsv_add(), btrfs_block_rsv_refill()
> |
> |    Calculate needed bytes by btrfs_calc*(), then add the needed space
> |    to our ->size and our ->reserved.
> |    This also contributes to space_info->bytes_may_use.
> |
> +--- Use:
> |    Entrance: btrfs_use_block_rsv()
> |
> |    We're allocating a tree block, will subtracts nodesize from
> |    block_rsv->reserved.
> |
> +--- Finish:
>      Entrance: btrfs_block_rsv_release()
> 
>      we subtract our original reservation from ->size,
>      and then we subtract ->size from ->reserved if there is an excess
>      and free the excess back to the space info, by reducing
>      space_info->bytes_may_use by the excess amount.

I find this graphic helpful. Also IMO it's important to explicitly state
that ->size is based on an overestimation, whereas the space subtracted
from ->reserved is always based on real usage, hence we can have a case
where we end up with  excess space that can be returned.

Over reservation is mentioned in the BLOCK_RSV_GLOBAL paragraph but I
think it should be here and can be removed from there.
> 
>> + *
>> + *   In some cases we may return this excess to the global block reserve or
>> + *   delayed refs reserve if either of their ->size is greater than their
>> + *   ->reserved.
>> + *
> 
> Types of block_rsv:
> 
>> + * BLOCK_RSV_TRANS, BLOCK_RSV_DELOPS, BLOCK_RSV_CHUNK
>> + *   These behave normally, as described above, just within the confines of the
>> + *   lifetime of ther particular operation (transaction for the whole trans
>> + *   handle lifetime, for example).
>> + *
>> + * BLOCK_RSV_GLOBAL
>> + *   This has existed forever, with diminishing degrees of importance.
>> + *   Currently it exists to save us from ourselves.  We definitely over-reserve
>> + *   space most of the time, but the nature of COW is that we do not know how
>> + *   much space we may need to use for any given operation.  This is
>> + *   particularly true about the extent tree.  Modifying one extent could
>> + *   balloon into 1000 modifications of the extent tree, which we have no way of
>> + *   properly predicting.  To cover this case we have the global reserve act as
>> + *   the "root" space to allow us to not abort the transaciton when things are
nit: s/transaciton/transaction
>> + *   very tight.  As such we tend to treat this space as sacred, and only use it
>> + *   if we are desparate.  Generally we should no longer be depending on its
nit: s/desparate/desperate

>> + *   space, and if new use cases arise we need to address them elsewhere.
> 
> Although we all know global rsv is really important for essential tree
> updates, can we make it a little simpler?
> It looks too long to read though.

The 2nd sentence of the paragraph can be removed. Also it can be
mentioned that globalrsv is used for other trees apart from extent i.e
chunk/csum ones. Also isn't it used to ensure progress of unlink() ?

> 
> I guess we don't need to put all related info here.
> Maybe just mentioning the usage of each type is enough?
> (Since the reader will still go greping for more details)
> 
> This also applies to the remaining types.


I disagree, those comment provide glimpses of the problem that
necessitated having block rsv in the first place. It's good to read this
before diving into the code.

<snip>