| Message ID | 20201012065624.80649-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: space-info: fix the wrong trace name for bytes_may_use | expand |
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>
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.
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
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?".
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);
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(-)