On 2019/8/16 下午5:33, Filipe Manana wrote:
> On Thu, Aug 15, 2019 at 9:36 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> Btrfs has btrfs_end_transaction_throttle() which could try to commit
>> transaction when needed.
>>
>> However under most cases btrfs_end_transaction_throttle() won't really
>> commit transaction, due to the hard timing requirement.
>>
>> Now introduce a new error injection point, btrfs_need_trans_pressure(),
>> to allow btrfs_should_end_transaction() to return 1 and
>> btrfs_end_transaction_throttle() to fallback to
>> btrfs_commit_transaction().
>>
>> With such more aggressive transaction commit, we can dig deeper into
>> cases like snapshot drop.
>> Now each reference drop of btrfs_drop_snapshot() will lead to a
>> transaction commit, allowing dm-logwrites to catch more details, other
>> than one big transaction dropping everything.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> Changelog:
>> v2:
>> - Add comment to explain why this function is needed
>> ---
>>  fs/btrfs/transaction.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index 3f6811cdf803..8c5471b01d03 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/pagemap.h>
>>  #include <linux/blkdev.h>
>>  #include <linux/uuid.h>
>> +#include <linux/error-injection.h>
>>  #include "ctree.h"
>>  #include "disk-io.h"
>>  #include "transaction.h"
>> @@ -749,10 +750,25 @@ void btrfs_throttle(struct btrfs_fs_info *fs_info)
>>         wait_current_trans(fs_info);
>>  }
>>
>> +/*
>> + * This function is to allow BPF to override the return value so that we can
>> + * make btrfs to commit transaction more aggressively.
>> + *
>> + * It's a debug only feature, mainly used with dm-log-writes to catch more details
>> + * of transient operations like balance and subvolume drop.
> 
> Transient? I think you mean long running operations that can span
> multiple transactions.

Nope, really transient details.

E.g catching subvolume dropping for each drop_progress update. While
under most one transaction can contain multiple drop_progress update or
even the whole tree just get dropped in one transaction.

> 
>> + */
>> +static noinline bool btrfs_need_trans_pressure(struct btrfs_trans_handle *trans)
>> +{
>> +       return false;
>> +}
>> +ALLOW_ERROR_INJECTION(btrfs_need_trans_pressure, TRUE);
> 
> So, I'm not sure if it's really a good idea to have such specific
> things like this.
> Has this proven useful already? I.e., have you already found any bug using this?

Not exactly.
I have observed a case where btrfs check gives false alert on missing a
backref of a dropped tree.

Originally planned to use this feature to catch the exact update, but
the problem is, with this pressure, we need an extra ioctl to wait the
full subvolume drop to finish.

Or we will get the fs unmounted before we really go to DROP_REFERENCE
phase, thus dm-log-writes gets nothing interesting.

Thanks,
Qu

> 
> I often add such similar things myself for testing and debugging, but
> because they are so specific, or ugly or verbose, I keep them to
> myself.
> 
> Allowing the return value of should_end_transaction() to be
> overridden, using the same approach, would be more generic for
> example.
> 
> Thanks.
> 
>> +
>>  static int should_end_transaction(struct btrfs_trans_handle *trans)
>>  {
>>         struct btrfs_fs_info *fs_info = trans->fs_info;
>>
>> +       if (btrfs_need_trans_pressure(trans))
>> +               return 1;
>>         if (btrfs_check_space_for_delayed_refs(fs_info))
>>                 return 1;
>>
>> @@ -813,6 +829,8 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
>>
>>         btrfs_trans_release_chunk_metadata(trans);
>>
>> +       if (throttle && btrfs_need_trans_pressure(trans))
>> +               return btrfs_commit_transaction(trans);
>>         if (lock && READ_ONCE(cur_trans->state) == TRANS_STATE_BLOCKED) {
>>                 if (throttle)
>>                         return btrfs_commit_transaction(trans);
>> --
>> 2.22.0
>>
> 
>