| Message ID | 20190806082201.22683-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: transaction: Commit transaction more frequently for BPF | expand |
On Tue, Aug 06, 2019 at 04:22:01PM +0800, Qu Wenruo 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> > --- > fs/btrfs/transaction.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index 248d535bb14d..2e758957126e 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" > @@ -781,10 +782,18 @@ void btrfs_throttle(struct btrfs_fs_info *fs_info) > wait_current_trans(fs_info); > } > Please put a comment here what's the purpose of the function otherwise the people who like to delete code will find it and you know what will happen next. > +static noinline bool btrfs_need_trans_pressure(struct btrfs_trans_handle *trans) > +{ > + return false; > +} > +ALLOW_ERROR_INJECTION(btrfs_need_trans_pressure, TRUE); > + > 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; > > @@ -845,6 +854,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
On 6.08.19 г. 11:22 ч., Qu Wenruo 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> > --- > fs/btrfs/transaction.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index 248d535bb14d..2e758957126e 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" > @@ -781,10 +782,18 @@ void btrfs_throttle(struct btrfs_fs_info *fs_info) > wait_current_trans(fs_info); > } > > +static noinline bool btrfs_need_trans_pressure(struct btrfs_trans_handle *trans) > +{ > + return false; > +} > +ALLOW_ERROR_INJECTION(btrfs_need_trans_pressure, TRUE); > + > 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; > > @@ -845,6 +854,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); I'd rather have this gated behind CONFIG_BTRFS_DEBUG. > if (lock && READ_ONCE(cur_trans->state) == TRANS_STATE_BLOCKED) { > if (throttle) > return btrfs_commit_transaction(trans); >
On 2019/8/6 下午11:23, Nikolay Borisov wrote: > > > On 6.08.19 г. 11:22 ч., Qu Wenruo 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> >> --- >> fs/btrfs/transaction.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c >> index 248d535bb14d..2e758957126e 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" >> @@ -781,10 +782,18 @@ void btrfs_throttle(struct btrfs_fs_info *fs_info) >> wait_current_trans(fs_info); >> } >> >> +static noinline bool btrfs_need_trans_pressure(struct btrfs_trans_handle *trans) >> +{ >> + return false; >> +} >> +ALLOW_ERROR_INJECTION(btrfs_need_trans_pressure, TRUE); >> + >> 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; >> >> @@ -845,6 +854,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); > > I'd rather have this gated behind CONFIG_BTRFS_DEBUG. Then we need both CONFIG_BTRFS_DEBUG and CONFIG_BPF_KPROBE_OVERRIDE to enable this feature. And for CONFIG_BTRFS_DEBUG not enabled case, that function would not be used at all (if you also gate the one in should_end_transaction()). Not sure if extra hassle is really worthy for such a simple debug feature. Thanks, Qu > > >> if (lock && READ_ONCE(cur_trans->state) == TRANS_STATE_BLOCKED) { >> if (throttle) >> return btrfs_commit_transaction(trans); >>
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index 248d535bb14d..2e758957126e 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" @@ -781,10 +782,18 @@ void btrfs_throttle(struct btrfs_fs_info *fs_info) wait_current_trans(fs_info); } +static noinline bool btrfs_need_trans_pressure(struct btrfs_trans_handle *trans) +{ + return false; +} +ALLOW_ERROR_INJECTION(btrfs_need_trans_pressure, TRUE); + 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; @@ -845,6 +854,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);
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> --- fs/btrfs/transaction.c | 11 +++++++++++ 1 file changed, 11 insertions(+)