* [PATCH 1/1] btrfs: tests: remove unnecessary oom message @ 2021-06-17 8:30 Zhen Lei 2021-06-17 20:35 ` David Sterba 0 siblings, 1 reply; 6+ messages in thread From: Zhen Lei @ 2021-06-17 8:30 UTC (permalink / raw) To: Chris Mason, Josef Bacik, David Sterba, linux-btrfs; +Cc: Zhen Lei Fixes scripts/checkpatch.pl warning: WARNING: Possible unnecessary 'out of memory' message Remove it can help us save a bit of memory. Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- fs/btrfs/tests/extent-io-tests.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c index 99b791b931d7..7655672752ad 100644 --- a/fs/btrfs/tests/extent-io-tests.c +++ b/fs/btrfs/tests/extent-io-tests.c @@ -393,7 +393,6 @@ static int test_eb_bitmaps(u32 sectorsize, u32 nodesize) bitmap = kmalloc(nodesize, GFP_KERNEL); if (!bitmap) { - test_err("couldn't allocate test bitmap"); ret = -ENOMEM; goto out; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] btrfs: tests: remove unnecessary oom message 2021-06-17 8:30 [PATCH 1/1] btrfs: tests: remove unnecessary oom message Zhen Lei @ 2021-06-17 20:35 ` David Sterba 2021-06-18 2:33 ` Leizhen (ThunderTown) 0 siblings, 1 reply; 6+ messages in thread From: David Sterba @ 2021-06-17 20:35 UTC (permalink / raw) To: Zhen Lei; +Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs On Thu, Jun 17, 2021 at 04:30:53PM +0800, Zhen Lei wrote: > Fixes scripts/checkpatch.pl warning: > WARNING: Possible unnecessary 'out of memory' message > > Remove it can help us save a bit of memory. Well, we have a few more messages in tests regarding failed memory allocations. Though I've never seen one in practice, I think it's not a big deal to have that one here as well. The failures in the testsuite are intentionally verbose and saving a few bytes in optional development feature hardly bothers anyone. Where bytes can be saved are error messages for the same type of error, that I've implemented in the past, see file fs/btrfs/tests/btrfs-tests.c array test_error that maps enums to strings. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] btrfs: tests: remove unnecessary oom message 2021-06-17 20:35 ` David Sterba @ 2021-06-18 2:33 ` Leizhen (ThunderTown) 2021-06-18 5:58 ` Qu Wenruo 0 siblings, 1 reply; 6+ messages in thread From: Leizhen (ThunderTown) @ 2021-06-18 2:33 UTC (permalink / raw) To: dsterba, Chris Mason, Josef Bacik, David Sterba, linux-btrfs On 2021/6/18 4:35, David Sterba wrote: > On Thu, Jun 17, 2021 at 04:30:53PM +0800, Zhen Lei wrote: >> Fixes scripts/checkpatch.pl warning: >> WARNING: Possible unnecessary 'out of memory' message >> >> Remove it can help us save a bit of memory. > > Well, we have a few more messages in tests regarding failed memory > allocations. Though I've never seen one in practice, I think it's not > a big deal to have that one here as well. The failures in the testsuite > are intentionally verbose and saving a few bytes in optional development > feature hardly bothers anyone. The calltrace of the OOM message contains all the information printed by test_err() here. I don't think anyone wants to see a bunch of unhelpful tips when locating an OOM problem. > > Where bytes can be saved are error messages for the same type of error, It also saves a dozen bytes of binary code. > that I've implemented in the past, see file fs/btrfs/tests/btrfs-tests.c > array test_error that maps enums to strings. As mentioned above, I don't think these "no memory" strings are necessary, unless the rest of the test can continue to run healthy. Otherwise, no one trusts the test results in the OOM situation. They're going to locate the OOM problem first, and these information are pointless. > > . > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] btrfs: tests: remove unnecessary oom message 2021-06-18 2:33 ` Leizhen (ThunderTown) @ 2021-06-18 5:58 ` Qu Wenruo 2021-06-18 6:05 ` Qu Wenruo 0 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2021-06-18 5:58 UTC (permalink / raw) To: Leizhen (ThunderTown), dsterba, Chris Mason, Josef Bacik, David Sterba, linux-btrfs On 2021/6/18 上午10:33, Leizhen (ThunderTown) wrote: > > > On 2021/6/18 4:35, David Sterba wrote: >> On Thu, Jun 17, 2021 at 04:30:53PM +0800, Zhen Lei wrote: >>> Fixes scripts/checkpatch.pl warning: >>> WARNING: Possible unnecessary 'out of memory' message >>> >>> Remove it can help us save a bit of memory. >> >> Well, we have a few more messages in tests regarding failed memory >> allocations. Though I've never seen one in practice, I think it's not >> a big deal to have that one here as well. The failures in the testsuite >> are intentionally verbose and saving a few bytes in optional development >> feature hardly bothers anyone. > > The calltrace of the OOM message contains all the information printed by > test_err() here. I don't think anyone wants to see a bunch of unhelpful tips > when locating an OOM problem. This only get enabled for btrfs developers, in production environment would enable CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y. Thus this error message are only for btrfs developers. And I'm 100% sure you won't need to investigate such OOM problem, nor even see it. > >> >> Where bytes can be saved are error messages for the same type of error, > > It also saves a dozen bytes of binary code. It won't make any different as you won't enable that config. Thanks, Qu > >> that I've implemented in the past, see file fs/btrfs/tests/btrfs-tests.c >> array test_error that maps enums to strings. > > As mentioned above, I don't think these "no memory" strings are necessary, > unless the rest of the test can continue to run healthy. Otherwise, no one trusts > the test results in the OOM situation. They're going to locate the OOM problem > first, and these information are pointless. > >> >> . >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] btrfs: tests: remove unnecessary oom message 2021-06-18 5:58 ` Qu Wenruo @ 2021-06-18 6:05 ` Qu Wenruo 2021-06-23 20:41 ` David Sterba 0 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2021-06-18 6:05 UTC (permalink / raw) To: Leizhen (ThunderTown), dsterba, Chris Mason, Josef Bacik, David Sterba, linux-btrfs On 2021/6/18 下午1:58, Qu Wenruo wrote: > > > On 2021/6/18 上午10:33, Leizhen (ThunderTown) wrote: >> >> >> On 2021/6/18 4:35, David Sterba wrote: >>> On Thu, Jun 17, 2021 at 04:30:53PM +0800, Zhen Lei wrote: >>>> Fixes scripts/checkpatch.pl warning: >>>> WARNING: Possible unnecessary 'out of memory' message >>>> >>>> Remove it can help us save a bit of memory. >>> >>> Well, we have a few more messages in tests regarding failed memory >>> allocations. Though I've never seen one in practice, I think it's not >>> a big deal to have that one here as well. The failures in the testsuite >>> are intentionally verbose and saving a few bytes in optional development >>> feature hardly bothers anyone. >> >> The calltrace of the OOM message contains all the information printed by >> test_err() here. I don't think anyone wants to see a bunch of >> unhelpful tips >> when locating an OOM problem. > > This only get enabled for btrfs developers, in production environment > would enable CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y. > > Thus this error message are only for btrfs developers. > > And I'm 100% sure you won't need to investigate such OOM problem, nor > even see it. > >> >>> >>> Where bytes can be saved are error messages for the same type of error, >> >> It also saves a dozen bytes of binary code. > > It won't make any different as you won't enable that config. > > Thanks, > Qu >> >>> that I've implemented in the past, see file fs/btrfs/tests/btrfs-tests.c >>> array test_error that maps enums to strings. >> >> As mentioned above, I don't think these "no memory" strings are >> necessary, >> unless the rest of the test can continue to run healthy. Otherwise, no >> one trusts >> the test results in the OOM situation. They're going to locate the OOM >> problem >> first, and these information are pointless. > And nope, it's not only OOM can cause the selftest to fail, but also error injection. I guess you never ran error injection tests for filesystems. Under most case, we inject error with specific call chain, but sometimes without any call chain specification, error injection may find some corner cases we're unaware of. If by chance the injected memory allocation failure happens during selftest, there will be *NO* OOM dump at all. In that case, have a good luck investigating why selftest fails. Your words make sense for common path, but next time before sending such patches to earn your KPI, please dig a little deeper to understand the context. Thanks, Qu >>> >>> . >>> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] btrfs: tests: remove unnecessary oom message 2021-06-18 6:05 ` Qu Wenruo @ 2021-06-23 20:41 ` David Sterba 0 siblings, 0 replies; 6+ messages in thread From: David Sterba @ 2021-06-23 20:41 UTC (permalink / raw) To: Qu Wenruo Cc: Leizhen (ThunderTown), dsterba, Chris Mason, Josef Bacik, David Sterba, linux-btrfs On Fri, Jun 18, 2021 at 02:05:59PM +0800, Qu Wenruo wrote: > On 2021/6/18 下午1:58, Qu Wenruo wrote: > > On 2021/6/18 上午10:33, Leizhen (ThunderTown) wrote: > >> On 2021/6/18 4:35, David Sterba wrote: > >>> On Thu, Jun 17, 2021 at 04:30:53PM +0800, Zhen Lei wrote: > >>>> Fixes scripts/checkpatch.pl warning: > >>>> WARNING: Possible unnecessary 'out of memory' message > >>>> > >>>> Remove it can help us save a bit of memory. > >>> > >>> Well, we have a few more messages in tests regarding failed memory > >>> allocations. Though I've never seen one in practice, I think it's not > >>> a big deal to have that one here as well. The failures in the testsuite > >>> are intentionally verbose and saving a few bytes in optional development > >>> feature hardly bothers anyone. > >> > >> The calltrace of the OOM message contains all the information printed by > >> test_err() here. I don't think anyone wants to see a bunch of > >> unhelpful tips > >> when locating an OOM problem. > > > > This only get enabled for btrfs developers, in production environment > > would enable CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y. > > > > Thus this error message are only for btrfs developers. > > > > And I'm 100% sure you won't need to investigate such OOM problem, nor > > even see it. > > > >>> Where bytes can be saved are error messages for the same type of error, > >> > >> It also saves a dozen bytes of binary code. > > > > It won't make any different as you won't enable that config. > > > >>> that I've implemented in the past, see file fs/btrfs/tests/btrfs-tests.c > >>> array test_error that maps enums to strings. > >> > >> As mentioned above, I don't think these "no memory" strings are > >> necessary, > >> unless the rest of the test can continue to run healthy. Otherwise, no > >> one trusts > >> the test results in the OOM situation. They're going to locate the OOM > >> problem > >> first, and these information are pointless. > > > And nope, it's not only OOM can cause the selftest to fail, but also > error injection. > > I guess you never ran error injection tests for filesystems. > > Under most case, we inject error with specific call chain, but sometimes > without any call chain specification, error injection may find some > corner cases we're unaware of. > > If by chance the injected memory allocation failure happens during > selftest, there will be *NO* OOM dump at all. Yeah, a hard OOM won't probably happen and the allocations can fail for other valid reasons. The error message in the logs, with the ERROR message level is clear and helpful. Saving a few bytes here does not make much sense. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-23 20:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-17 8:30 [PATCH 1/1] btrfs: tests: remove unnecessary oom message Zhen Lei 2021-06-17 20:35 ` David Sterba 2021-06-18 2:33 ` Leizhen (ThunderTown) 2021-06-18 5:58 ` Qu Wenruo 2021-06-18 6:05 ` Qu Wenruo 2021-06-23 20:41 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).