[PATCH] btrfs: space-info: fix the wrong trace name for bytes_may_use

[PATCH] btrfs: space-info: fix the wrong trace name for bytes_may_use

[Qu Wenruo]

The trace_name for bytes_may_use should be "may_use", "space_info".

Fixes: f3e75e3805e1 ("btrfs: roll tracepoint into btrfs_space_info_update helper")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/space-info.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
index c3c64019950a..25fe03a53fb8 100644
--- a/fs/btrfs/space-info.h
+++ b/fs/btrfs/space-info.h
@@ -113,7 +113,7 @@ btrfs_space_info_update_##name(struct btrfs_fs_info *fs_info,		\
 	sinfo->name += bytes;						\
 }
 
-DECLARE_SPACE_INFO_UPDATE(bytes_may_use, "space_info");
+DECLARE_SPACE_INFO_UPDATE(bytes_may_use, "may_use");
 DECLARE_SPACE_INFO_UPDATE(bytes_pinned, "pinned");
 
 int btrfs_init_space_info(struct btrfs_fs_info *fs_info);
-- 
2.28.0

Re: [PATCH] btrfs: space-info: fix the wrong trace name for bytes_may_use

[Nikolay Borisov]

On 12.10.20 г. 9:56 ч., Qu Wenruo wrote:
> The trace_name for bytes_may_use should be "may_use", "space_info".
> 
> Fixes: f3e75e3805e1 ("btrfs: roll tracepoint into btrfs_space_info_update helper")
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH] btrfs: space-info: fix the wrong trace name for bytes_may_use

[David Sterba]

On Mon, Oct 12, 2020 at 02:56:24PM +0800, Qu Wenruo wrote:
> The trace_name for bytes_may_use should be "may_use", "space_info".
> 
> Fixes: f3e75e3805e1 ("btrfs: roll tracepoint into btrfs_space_info_update helper")

This patch reduced all calls to the definition and kept the same output,
ie. the 'space_info' so it was correct and the Fixes does not seem to be
right here. You should explain why the 'may_use' is supposed to be
there.

Re: [PATCH] btrfs: space-info: fix the wrong trace name for bytes_may_use

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1306 bytes --]



On 2020/10/14 下午7:44, David Sterba wrote:
> On Mon, Oct 12, 2020 at 02:56:24PM +0800, Qu Wenruo wrote:
>> The trace_name for bytes_may_use should be "may_use", "space_info".
>>
>> Fixes: f3e75e3805e1 ("btrfs: roll tracepoint into btrfs_space_info_update helper")
> 
> This patch reduced all calls to the definition and kept the same output,
> ie. the 'space_info' so it was correct and the Fixes does not seem to be
> right here. You should explain why the 'may_use' is supposed to be
> there.
> 
The next bytes_pinned still uses "pinned" for its name.

And in the DECLARE_SPACE_INFO_UPDATE(), the @trace_name is only passed
to trcae_btrfs_space_reservation(), to indicate the cause of the trace
event.

Under most case, we pass things like "space_info:enospc" for cases we
will going to return -ENOSPC, and for other cases, we pass what we're
reserving for, like in btrfs_delalloc_reserve_metadata().

For the bytes_may_use case, the DECLARE_SPACE_INFO_UPDATE() is
definitely not called for ENOSPC, thus it should what we're reserving for.
Since the declared function has no real context, thus using the
"may_use" looks completely sane to me.

The fixes looks correct, as it looks like a copy-n-paste error from that
commit.

Or did I miss somthing?

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: space-info: fix the wrong trace name for bytes_may_use

[David Sterba]

On Wed, Oct 14, 2020 at 07:59:07PM +0800, Qu Wenruo wrote:
> On 2020/10/14 下午7:44, David Sterba wrote:
> > On Mon, Oct 12, 2020 at 02:56:24PM +0800, Qu Wenruo wrote:
> >> The trace_name for bytes_may_use should be "may_use", "space_info".
> >>
> >> Fixes: f3e75e3805e1 ("btrfs: roll tracepoint into btrfs_space_info_update helper")
> > 
> > This patch reduced all calls to the definition and kept the same output,
> > ie. the 'space_info' so it was correct and the Fixes does not seem to be
> > right here. You should explain why the 'may_use' is supposed to be
> > there.
> > 
> The next bytes_pinned still uses "pinned" for its name.
> 
> And in the DECLARE_SPACE_INFO_UPDATE(), the @trace_name is only passed
> to trcae_btrfs_space_reservation(), to indicate the cause of the trace
> event.
> 
> Under most case, we pass things like "space_info:enospc" for cases we
> will going to return -ENOSPC, and for other cases, we pass what we're
> reserving for, like in btrfs_delalloc_reserve_metadata().
> 
> For the bytes_may_use case, the DECLARE_SPACE_INFO_UPDATE() is
> definitely not called for ENOSPC, thus it should what we're reserving for.
> Since the declared function has no real context, thus using the
> "may_use" looks completely sane to me.
> 
> The fixes looks correct, as it looks like a copy-n-paste error from that
> commit.

The commit dropped the trace_ call and moved it to the update function,
so the string "space_info" made it to
btrfs_space_info_update_bytes_may_use.

                btrfs_space_info_update_bytes_may_use(fs_info, sinfo,
                                                      -num_bytes);
-               trace_btrfs_space_reservation(fs_info, "space_info",
-                                     sinfo->flags, num_bytes, 0);

so there was no copy&paste error.

What I am missing is changelog rationale behind the change from
space_info to may_use. If 'may_use' should do the same as 'pinned', then
please explain it or give hints how to verify that the change is
correct.

  'The trace_name for bytes_may_use should be "may_use", "space_info".'

just begs to ask "why?".