| Message ID | 4657a485af5665a07682c4d7a5eb14ef241995a5.1606938211.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup error handling in relocation | expand |
On 2020/12/3 上午3:50, Josef Bacik wrote: > I was attempting to reproduce a problem that Zygo hit, but my error > injection wasn't firing for a few of the common calls to > btrfs_should_cancel_balance. This is because the compiler decided to > inline it at these spots. Keep this from happening by explicitly > noinline'ing the function so that error injection will always work. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Oh, I should have added noinline for the error injection I introduced. Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks for the fix. Qu > --- > fs/btrfs/relocation.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 2b30e39e922a..ce935139d87b 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -2617,7 +2617,7 @@ int setup_extent_mapping(struct inode *inode, u64 start, u64 end, > /* > * Allow error injection to test balance cancellation > */ > -int btrfs_should_cancel_balance(struct btrfs_fs_info *fs_info) > +noinline int btrfs_should_cancel_balance(struct btrfs_fs_info *fs_info) > { > return atomic_read(&fs_info->balance_cancel_req) || > fatal_signal_pending(current); >
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 2.12.20 г. 21:50 ч., Josef Bacik wrote: > I was attempting to reproduce a problem that Zygo hit, but my error > injection wasn't firing for a few of the common calls to > btrfs_should_cancel_balance. This is because the compiler decided to > inline it at these spots. Keep this from happening by explicitly > noinline'ing the function so that error injection will always work. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/relocation.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 2b30e39e922a..ce935139d87b 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -2617,7 +2617,7 @@ int setup_extent_mapping(struct inode *inode, u64 start, u64 end, > /* > * Allow error injection to test balance cancellation > */ > -int btrfs_should_cancel_balance(struct btrfs_fs_info *fs_info) > +noinline int btrfs_should_cancel_balance(struct btrfs_fs_info *fs_info) > { > return atomic_read(&fs_info->balance_cancel_req) || > fatal_signal_pending(current); > I'd really like to not pay the cost of non-inlining in case error injection is disabled. How about you introduce a new noinline_for_err define that would add noinline in case error injection is enabled or be optimized away when injection is off. Alternatively, though that would be slightly more work, the ALLOW_ERRO_INJECTION macro can be modified so that all functions that want error injection could be declared as : ALLOW_ERROR_INJECTION(ftdec, fname, _etype) // same body as before // // // noinline ftdec so functions could be defined as : ALLOW_ERROR_INJECTION(int btrfs_should_cancel_balance(struct btrfs_fs_info *fs_info), btrfs_should_cancel_balance, ERRNO) Though that seems a bit unwieldy TBH. I'm making the case that we shouldn't introduce extra overhead when it's not required.
On 12/3/20 4:00 AM, Nikolay Borisov wrote: > > > On 2.12.20 г. 21:50 ч., Josef Bacik wrote: >> I was attempting to reproduce a problem that Zygo hit, but my error >> injection wasn't firing for a few of the common calls to >> btrfs_should_cancel_balance. This is because the compiler decided to >> inline it at these spots. Keep this from happening by explicitly >> noinline'ing the function so that error injection will always work. >> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> >> --- >> fs/btrfs/relocation.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c >> index 2b30e39e922a..ce935139d87b 100644 >> --- a/fs/btrfs/relocation.c >> +++ b/fs/btrfs/relocation.c >> @@ -2617,7 +2617,7 @@ int setup_extent_mapping(struct inode *inode, u64 start, u64 end, >> /* >> * Allow error injection to test balance cancellation >> */ >> -int btrfs_should_cancel_balance(struct btrfs_fs_info *fs_info) >> +noinline int btrfs_should_cancel_balance(struct btrfs_fs_info *fs_info) >> { >> return atomic_read(&fs_info->balance_cancel_req) || >> fatal_signal_pending(current); >> > > I'd really like to not pay the cost of non-inlining in case error injection is disabled. How about you introduce a new noinline_for_err define that would add noinline in case error injection is enabled or be optimized away when injection is off. Alternatively, though that would be slightly more work, the ALLOW_ERRO_INJECTION macro can be modified so that all functions that want error injection could be declared as : > > ALLOW_ERROR_INJECTION(ftdec, fname, _etype) > // same body as before > // > // > // > > noinline ftdec > > > so functions could be defined as : > > > ALLOW_ERROR_INJECTION(int btrfs_should_cancel_balance(struct btrfs_fs_info *fs_info), btrfs_should_cancel_balance, ERRNO) > > Though that seems a bit unwieldy TBH. > > I'm making the case that we shouldn't introduce extra overhead when it's not required. > This is something we could address later on a global scale, but this isn't a hot path function. Alexei had to do something similar for __add_to_page_cache_locked, for now lets stick with this and I'll work with the bpf guys to figure out a reasonable solution. Thanks, Josef
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 2b30e39e922a..ce935139d87b 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -2617,7 +2617,7 @@ int setup_extent_mapping(struct inode *inode, u64 start, u64 end, /* * Allow error injection to test balance cancellation */ -int btrfs_should_cancel_balance(struct btrfs_fs_info *fs_info) +noinline int btrfs_should_cancel_balance(struct btrfs_fs_info *fs_info) { return atomic_read(&fs_info->balance_cancel_req) || fatal_signal_pending(current);
I was attempting to reproduce a problem that Zygo hit, but my error injection wasn't firing for a few of the common calls to btrfs_should_cancel_balance. This is because the compiler decided to inline it at these spots. Keep this from happening by explicitly noinline'ing the function so that error injection will always work. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/relocation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)