* RE: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-05 7:56 ` Qu Wenruo
@ 2016-09-06 2:41 ` Zhao Lei
2016-09-06 3:05 ` Jeff Mahoney
2016-09-07 1:38 ` Sean Fu
2 siblings, 0 replies; 15+ messages in thread
From: Zhao Lei @ 2016-09-06 2:41 UTC (permalink / raw)
To: 'Qu Wenruo', 'Sean Fu', dsterba
Cc: clm, anand.jain, fdmanana, linux-btrfs, linux-kernel
Hi, Qu Wenruo
> From: Qu Wenruo [mailto:quwenruo@cn.fujitsu.com]
> Sent: Monday, September 05, 2016 3:57 PM
> To: Zhao Lei <zhaolei@cn.fujitsu.com>; 'Sean Fu' <fxinrong@gmail.com>;
> dsterba@suse.com
> Cc: clm@fb.com; anand.jain@oracle.com; fdmanana@suse.com;
> linux-btrfs@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Btrfs: remove unnecessary code of chunk_root
> assignment in btrfs_read_chunk_tree.
>
>
>
> At 09/05/2016 09:19 AM, Zhao Lei wrote:
> > Hi, Sean Fu
> >
> >> From: Sean Fu [mailto:fxinrong@gmail.com]
> >> Sent: Sunday, September 04, 2016 7:54 PM
> >> To: dsterba@suse.com
> >> Cc: clm@fb.com; anand.jain@oracle.com; fdmanana@suse.com;
> >> zhaolei@cn.fujitsu.com; linux-btrfs@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; Sean Fu <fxinrong@gmail.com>
> >> Subject: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment
> in
> >> btrfs_read_chunk_tree.
> >>
> >> The input argument root is already set with "fs_info->chunk_root".
> >> "chunk_root = fs_info->chunk_root = btrfs_alloc_root(fs_info)" in caller
> >> "open_ctree".
> >> “root->fs_info = fs_info” in "btrfs_alloc_root".
> >>
> > The root argument of this function means "any root".
> > And the function is designed getting chunk root from
> > "any root" in head.
> >
> > Since there is only one caller of this function,
> > and the caller always send chunk_root as root argument in
> > current code, we can remove above conversion,
> > and I suggest renaming root to chunk_root to make it clear,
> > something like:
> >
> > - btrfs_read_chunk_tree(struct btrfs_root *root)
> > + btrfs_read_chunk_tree(struct btrfs_root *chunk_root)
>
> Since root is only used to get fs_info->chunk_root, why not use fs_info
> directly?
>
Good idea.
Thanks
Zhaolei
> Thanks,
> Qu
>
> >
> > Thanks
> > Zhaolei
> >
> >> Signed-off-by: Sean Fu <fxinrong@gmail.com>
> >> ---
> >> fs/btrfs/volumes.c | 2 --
> >> 1 file changed, 2 deletions(-)
> >>
> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >> index 366b335..384a6d2 100644
> >> --- a/fs/btrfs/volumes.c
> >> +++ b/fs/btrfs/volumes.c
> >> @@ -6600,8 +6600,6 @@ int btrfs_read_chunk_tree(struct btrfs_root
> *root)
> >> int ret;
> >> int slot;
> >>
> >> - root = root->fs_info->chunk_root;
> >> -
> >> path = btrfs_alloc_path();
> >> if (!path)
> >> return -ENOMEM;
> >> --
> >> 2.6.2
> >>
> >
> >
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-05 7:56 ` Qu Wenruo
2016-09-06 2:41 ` Zhao Lei
@ 2016-09-06 3:05 ` Jeff Mahoney
2016-09-06 3:13 ` Jeff Mahoney
2016-09-07 1:38 ` Sean Fu
2 siblings, 1 reply; 15+ messages in thread
From: Jeff Mahoney @ 2016-09-06 3:05 UTC (permalink / raw)
To: Qu Wenruo, Zhao Lei, 'Sean Fu', dsterba
Cc: clm, anand.jain, fdmanana, linux-btrfs, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2605 bytes --]
On 9/5/16 3:56 AM, Qu Wenruo wrote:
>
>
> At 09/05/2016 09:19 AM, Zhao Lei wrote:
>> Hi, Sean Fu
>>
>>> From: Sean Fu [mailto:fxinrong@gmail.com]
>>> Sent: Sunday, September 04, 2016 7:54 PM
>>> To: dsterba@suse.com
>>> Cc: clm@fb.com; anand.jain@oracle.com; fdmanana@suse.com;
>>> zhaolei@cn.fujitsu.com; linux-btrfs@vger.kernel.org;
>>> linux-kernel@vger.kernel.org; Sean Fu <fxinrong@gmail.com>
>>> Subject: [PATCH] Btrfs: remove unnecessary code of chunk_root
>>> assignment in
>>> btrfs_read_chunk_tree.
>>>
>>> The input argument root is already set with "fs_info->chunk_root".
>>> "chunk_root = fs_info->chunk_root = btrfs_alloc_root(fs_info)" in caller
>>> "open_ctree".
>>> “root->fs_info = fs_info” in "btrfs_alloc_root".
>>>
>> The root argument of this function means "any root".
>> And the function is designed getting chunk root from
>> "any root" in head.
>>
>> Since there is only one caller of this function,
>> and the caller always send chunk_root as root argument in
>> current code, we can remove above conversion,
>> and I suggest renaming root to chunk_root to make it clear,
>> something like:
>>
>> - btrfs_read_chunk_tree(struct btrfs_root *root)
>> + btrfs_read_chunk_tree(struct btrfs_root *chunk_root)
>
> Since root is only used to get fs_info->chunk_root, why not use fs_info
> directly?
Weird. Exactly this was a part of my fs_info patchset. I guess I need
to go back and check what else is missing.
-Jeff
> Thanks,
> Qu
>
>>
>> Thanks
>> Zhaolei
>>
>>> Signed-off-by: Sean Fu <fxinrong@gmail.com>
>>> ---
>>> fs/btrfs/volumes.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 366b335..384a6d2 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -6600,8 +6600,6 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
>>> int ret;
>>> int slot;
>>>
>>> - root = root->fs_info->chunk_root;
>>> -
>>> path = btrfs_alloc_path();
>>> if (!path)
>>> return -ENOMEM;
>>> --
>>> 2.6.2
>>>
>>
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 827 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-06 3:05 ` Jeff Mahoney
@ 2016-09-06 3:13 ` Jeff Mahoney
2016-09-06 9:58 ` David Sterba
0 siblings, 1 reply; 15+ messages in thread
From: Jeff Mahoney @ 2016-09-06 3:13 UTC (permalink / raw)
To: Qu Wenruo, Zhao Lei, 'Sean Fu', dsterba
Cc: clm, anand.jain, fdmanana, linux-btrfs, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2859 bytes --]
On 9/5/16 11:05 PM, Jeff Mahoney wrote:
> On 9/5/16 3:56 AM, Qu Wenruo wrote:
>>
>>
>> At 09/05/2016 09:19 AM, Zhao Lei wrote:
>>> Hi, Sean Fu
>>>
>>>> From: Sean Fu [mailto:fxinrong@gmail.com]
>>>> Sent: Sunday, September 04, 2016 7:54 PM
>>>> To: dsterba@suse.com
>>>> Cc: clm@fb.com; anand.jain@oracle.com; fdmanana@suse.com;
>>>> zhaolei@cn.fujitsu.com; linux-btrfs@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; Sean Fu <fxinrong@gmail.com>
>>>> Subject: [PATCH] Btrfs: remove unnecessary code of chunk_root
>>>> assignment in
>>>> btrfs_read_chunk_tree.
>>>>
>>>> The input argument root is already set with "fs_info->chunk_root".
>>>> "chunk_root = fs_info->chunk_root = btrfs_alloc_root(fs_info)" in caller
>>>> "open_ctree".
>>>> “root->fs_info = fs_info” in "btrfs_alloc_root".
>>>>
>>> The root argument of this function means "any root".
>>> And the function is designed getting chunk root from
>>> "any root" in head.
>>>
>>> Since there is only one caller of this function,
>>> and the caller always send chunk_root as root argument in
>>> current code, we can remove above conversion,
>>> and I suggest renaming root to chunk_root to make it clear,
>>> something like:
>>>
>>> - btrfs_read_chunk_tree(struct btrfs_root *root)
>>> + btrfs_read_chunk_tree(struct btrfs_root *chunk_root)
>>
>> Since root is only used to get fs_info->chunk_root, why not use fs_info
>> directly?
>
> Weird. Exactly this was a part of my fs_info patchset. I guess I need
> to go back and check what else is missing.
Actually, most of this didn't land. Pretty much anything that's a root
->fs_info conversion is in there.
-Jeff
> -Jeff
>
>
>> Thanks,
>> Qu
>>
>>>
>>> Thanks
>>> Zhaolei
>>>
>>>> Signed-off-by: Sean Fu <fxinrong@gmail.com>
>>>> ---
>>>> fs/btrfs/volumes.c | 2 --
>>>> 1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index 366b335..384a6d2 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -6600,8 +6600,6 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
>>>> int ret;
>>>> int slot;
>>>>
>>>> - root = root->fs_info->chunk_root;
>>>> -
>>>> path = btrfs_alloc_path();
>>>> if (!path)
>>>> return -ENOMEM;
>>>> --
>>>> 2.6.2
>>>>
>>>
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 827 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-06 3:13 ` Jeff Mahoney
@ 2016-09-06 9:58 ` David Sterba
2016-09-06 15:12 ` Jeff Mahoney
0 siblings, 1 reply; 15+ messages in thread
From: David Sterba @ 2016-09-06 9:58 UTC (permalink / raw)
To: Jeff Mahoney
Cc: Qu Wenruo, Zhao Lei, 'Sean Fu',
clm, anand.jain, Filipe Manana, linux-btrfs, linux-kernel
On Mon, Sep 05, 2016 at 11:13:40PM -0400, Jeff Mahoney wrote:
> >> Since root is only used to get fs_info->chunk_root, why not use fs_info
> >> directly?
> >
> > Weird. Exactly this was a part of my fs_info patchset. I guess I need
> > to go back and check what else is missing.
>
> Actually, most of this didn't land. Pretty much anything that's a root
> ->fs_info conversion is in there.
Only half of the patchset has been merged so far because it did not pass
testing, so I bisected to some point. I was about to let you know once
most of 4.9 patches are prepared so there are less merge conflicts.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-06 9:58 ` David Sterba
@ 2016-09-06 15:12 ` Jeff Mahoney
2016-09-07 1:44 ` Sean Fu
2016-09-09 3:08 ` Sean Fu
0 siblings, 2 replies; 15+ messages in thread
From: Jeff Mahoney @ 2016-09-06 15:12 UTC (permalink / raw)
To: dsterba, Qu Wenruo, Zhao Lei, 'Sean Fu',
clm, anand.jain, Filipe Manana, linux-btrfs, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 814 bytes --]
On 9/6/16 5:58 AM, David Sterba wrote:
> On Mon, Sep 05, 2016 at 11:13:40PM -0400, Jeff Mahoney wrote:
>>>> Since root is only used to get fs_info->chunk_root, why not use fs_info
>>>> directly?
>>>
>>> Weird. Exactly this was a part of my fs_info patchset. I guess I need
>>> to go back and check what else is missing.
>>
>> Actually, most of this didn't land. Pretty much anything that's a root
>> ->fs_info conversion is in there.
>
> Only half of the patchset has been merged so far because it did not pass
> testing, so I bisected to some point. I was about to let you know once
> most of 4.9 patches are prepared so there are less merge conflicts.
Ok, thanks. I was going to start the rebase today but I'll hold off
until you're set for 4.9.
-Jeff
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-06 15:12 ` Jeff Mahoney
@ 2016-09-07 1:44 ` Sean Fu
2016-09-09 3:08 ` Sean Fu
1 sibling, 0 replies; 15+ messages in thread
From: Sean Fu @ 2016-09-07 1:44 UTC (permalink / raw)
To: Jeff Mahoney
Cc: dsterba, Qu Wenruo, Zhao Lei, clm, anand.jain, Filipe Manana,
linux-btrfs, linux-kernel
On Tue, Sep 06, 2016 at 11:12:20AM -0400, Jeff Mahoney wrote:
> On 9/6/16 5:58 AM, David Sterba wrote:
> > On Mon, Sep 05, 2016 at 11:13:40PM -0400, Jeff Mahoney wrote:
> >>>> Since root is only used to get fs_info->chunk_root, why not use fs_info
> >>>> directly?
> >>>
> >>> Weird. Exactly this was a part of my fs_info patchset. I guess I need
> >>> to go back and check what else is missing.
> >>
> >> Actually, most of this didn't land. Pretty much anything that's a root
> >> ->fs_info conversion is in there.
> >
> > Only half of the patchset has been merged so far because it did not pass
> > testing, so I bisected to some point. I was about to let you know once
> > most of 4.9 patches are prepared so there are less merge conflicts.
>
> Ok, thanks. I was going to start the rebase today but I'll hold off
> until you're set for 4.9.
Sorry for late reply.
Many thanks to all of you.
>
> -Jeff
>
> --
> Jeff Mahoney
> SUSE Labs
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-06 15:12 ` Jeff Mahoney
2016-09-07 1:44 ` Sean Fu
@ 2016-09-09 3:08 ` Sean Fu
2016-09-09 3:25 ` Jeff Mahoney
1 sibling, 1 reply; 15+ messages in thread
From: Sean Fu @ 2016-09-09 3:08 UTC (permalink / raw)
To: Jeff Mahoney
Cc: dsterba, Qu Wenruo, Zhao Lei, clm, anand.jain, Filipe Manana,
linux-btrfs, linux-kernel
On Tue, Sep 06, 2016 at 11:12:20AM -0400, Jeff Mahoney wrote:
> On 9/6/16 5:58 AM, David Sterba wrote:
> > On Mon, Sep 05, 2016 at 11:13:40PM -0400, Jeff Mahoney wrote:
> >>>> Since root is only used to get fs_info->chunk_root, why not use fs_info
> >>>> directly?
> >>>
> >>> Weird. Exactly this was a part of my fs_info patchset. I guess I need
> >>> to go back and check what else is missing.
> >>
> >> Actually, most of this didn't land. Pretty much anything that's a root
> >> ->fs_info conversion is in there.
> >
> > Only half of the patchset has been merged so far because it did not pass
> > testing, so I bisected to some point. I was about to let you know once
> > most of 4.9 patches are prepared so there are less merge conflicts.
>
> Ok, thanks. I was going to start the rebase today but I'll hold off
> until you're set for 4.9.
>
Hi Jeff, Could you please share your patch? Where can i get it?
I wanna have a look at it.
Thanks
> -Jeff
>
> --
> Jeff Mahoney
> SUSE Labs
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-09 3:08 ` Sean Fu
@ 2016-09-09 3:25 ` Jeff Mahoney
2016-09-09 3:45 ` Sean Fu
2016-09-10 15:58 ` Sean Fu
0 siblings, 2 replies; 15+ messages in thread
From: Jeff Mahoney @ 2016-09-09 3:25 UTC (permalink / raw)
To: dsterba, Qu Wenruo, Zhao Lei, clm, anand.jain, Filipe Manana,
linux-btrfs, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1770 bytes --]
On 9/8/16 11:08 PM, Sean Fu wrote:
> On Tue, Sep 06, 2016 at 11:12:20AM -0400, Jeff Mahoney wrote:
>> On 9/6/16 5:58 AM, David Sterba wrote:
>>> On Mon, Sep 05, 2016 at 11:13:40PM -0400, Jeff Mahoney wrote:
>>>>>> Since root is only used to get fs_info->chunk_root, why not use fs_info
>>>>>> directly?
>>>>>
>>>>> Weird. Exactly this was a part of my fs_info patchset. I guess I need
>>>>> to go back and check what else is missing.
>>>>
>>>> Actually, most of this didn't land. Pretty much anything that's a root
>>>> ->fs_info conversion is in there.
>>>
>>> Only half of the patchset has been merged so far because it did not pass
>>> testing, so I bisected to some point. I was about to let you know once
>>> most of 4.9 patches are prepared so there are less merge conflicts.
>>
>> Ok, thanks. I was going to start the rebase today but I'll hold off
>> until you're set for 4.9.
>>
> Hi Jeff, Could you please share your patch? Where can i get it?
> I wanna have a look at it.
Sure, it's the whole series that starts with this commit:
commit 160ceedfd40085cfb1e08305917fcc24cefdad93
Author: Jeff Mahoney <jeffm@suse.com>
Date: Wed Aug 31 23:55:33 2016 -0400
btrfs: add dynamic debug support
... I still need to do clean up some commits that need merging.
https://git.kernel.org/cgit/linux/kernel/git/jeffm/linux-btrfs.git/log/?h=btrfs-testing/kdave/misc-4.9/root-fsinfo-cleanup
-Jeff
> Thanks
>> -Jeff
>>
>> --
>> Jeff Mahoney
>> SUSE Labs
>>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-09 3:25 ` Jeff Mahoney
@ 2016-09-09 3:45 ` Sean Fu
2016-09-10 15:58 ` Sean Fu
1 sibling, 0 replies; 15+ messages in thread
From: Sean Fu @ 2016-09-09 3:45 UTC (permalink / raw)
To: Jeff Mahoney
Cc: dsterba, Qu Wenruo, Zhao Lei, clm, anand.jain, Filipe Manana,
linux-btrfs, linux-kernel
On Thu, Sep 08, 2016 at 11:25:48PM -0400, Jeff Mahoney wrote:
> On 9/8/16 11:08 PM, Sean Fu wrote:
> > On Tue, Sep 06, 2016 at 11:12:20AM -0400, Jeff Mahoney wrote:
> >> On 9/6/16 5:58 AM, David Sterba wrote:
> >>> On Mon, Sep 05, 2016 at 11:13:40PM -0400, Jeff Mahoney wrote:
> >>>>>> Since root is only used to get fs_info->chunk_root, why not use fs_info
> >>>>>> directly?
> >>>>>
> >>>>> Weird. Exactly this was a part of my fs_info patchset. I guess I need
> >>>>> to go back and check what else is missing.
> >>>>
> >>>> Actually, most of this didn't land. Pretty much anything that's a root
> >>>> ->fs_info conversion is in there.
> >>>
> >>> Only half of the patchset has been merged so far because it did not pass
> >>> testing, so I bisected to some point. I was about to let you know once
> >>> most of 4.9 patches are prepared so there are less merge conflicts.
> >>
> >> Ok, thanks. I was going to start the rebase today but I'll hold off
> >> until you're set for 4.9.
> >>
> > Hi Jeff, Could you please share your patch? Where can i get it?
> > I wanna have a look at it.
>
> Sure, it's the whole series that starts with this commit:
> commit 160ceedfd40085cfb1e08305917fcc24cefdad93
> Author: Jeff Mahoney <jeffm@suse.com>
> Date: Wed Aug 31 23:55:33 2016 -0400
>
> btrfs: add dynamic debug support
>
> ... I still need to do clean up some commits that need merging.
>
> https://git.kernel.org/cgit/linux/kernel/git/jeffm/linux-btrfs.git/log/?h=btrfs-testing/kdave/misc-4.9/root-fsinfo-cleanup
>
Nice work.
Thanks
> -Jeff
>
>
> > Thanks
> >> -Jeff
> >>
> >> --
> >> Jeff Mahoney
> >> SUSE Labs
> >>
> >
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
>
> --
> Jeff Mahoney
> SUSE Labs
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-09 3:25 ` Jeff Mahoney
2016-09-09 3:45 ` Sean Fu
@ 2016-09-10 15:58 ` Sean Fu
2016-09-10 16:01 ` Jeff Mahoney
1 sibling, 1 reply; 15+ messages in thread
From: Sean Fu @ 2016-09-10 15:58 UTC (permalink / raw)
To: Jeff Mahoney
Cc: dsterba, Qu Wenruo, Zhao Lei, clm, anand.jain, Filipe Manana,
linux-btrfs, linux-kernel
On Thu, Sep 08, 2016 at 11:25:48PM -0400, Jeff Mahoney wrote:
> On 9/8/16 11:08 PM, Sean Fu wrote:
> > On Tue, Sep 06, 2016 at 11:12:20AM -0400, Jeff Mahoney wrote:
> >> On 9/6/16 5:58 AM, David Sterba wrote:
> >>> On Mon, Sep 05, 2016 at 11:13:40PM -0400, Jeff Mahoney wrote:
> >>>>>> Since root is only used to get fs_info->chunk_root, why not use fs_info
> >>>>>> directly?
> >>>>>
> >>>>> Weird. Exactly this was a part of my fs_info patchset. I guess I need
> >>>>> to go back and check what else is missing.
> >>>>
> >>>> Actually, most of this didn't land. Pretty much anything that's a root
> >>>> ->fs_info conversion is in there.
> >>>
> >>> Only half of the patchset has been merged so far because it did not pass
> >>> testing, so I bisected to some point. I was about to let you know once
> >>> most of 4.9 patches are prepared so there are less merge conflicts.
> >>
> >> Ok, thanks. I was going to start the rebase today but I'll hold off
> >> until you're set for 4.9.
> >>
> > Hi Jeff, Could you please share your patch? Where can i get it?
> > I wanna have a look at it.
>
> Sure, it's the whole series that starts with this commit:
> commit 160ceedfd40085cfb1e08305917fcc24cefdad93
> Author: Jeff Mahoney <jeffm@suse.com>
> Date: Wed Aug 31 23:55:33 2016 -0400
>
> btrfs: add dynamic debug support
>
> ... I still need to do clean up some commits that need merging.
>
> https://git.kernel.org/cgit/linux/kernel/git/jeffm/linux-btrfs.git/log/?h=btrfs-testing/kdave/misc-4.9/root-fsinfo-cleanup
>
Nice work. Thanks for your explaination.
I have one more question about it.
Although the total text size of this function is not changed, using
fs_info should need one more instruction to get chunk_root field of
fs_info than using chunk_root directly.
Does it cause any performance impact?
Sean
> -Jeff
>
>
> > Thanks
> >> -Jeff
> >>
> >> --
> >> Jeff Mahoney
> >> SUSE Labs
> >>
> >
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
>
> --
> Jeff Mahoney
> SUSE Labs
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-10 15:58 ` Sean Fu
@ 2016-09-10 16:01 ` Jeff Mahoney
0 siblings, 0 replies; 15+ messages in thread
From: Jeff Mahoney @ 2016-09-10 16:01 UTC (permalink / raw)
To: dsterba, Qu Wenruo, Zhao Lei, clm, anand.jain, Filipe Manana,
linux-btrfs, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2436 bytes --]
On 9/10/16 11:58 AM, Sean Fu wrote:
> On Thu, Sep 08, 2016 at 11:25:48PM -0400, Jeff Mahoney wrote:
>> On 9/8/16 11:08 PM, Sean Fu wrote:
>>> On Tue, Sep 06, 2016 at 11:12:20AM -0400, Jeff Mahoney wrote:
>>>> On 9/6/16 5:58 AM, David Sterba wrote:
>>>>> On Mon, Sep 05, 2016 at 11:13:40PM -0400, Jeff Mahoney wrote:
>>>>>>>> Since root is only used to get fs_info->chunk_root, why not use fs_info
>>>>>>>> directly?
>>>>>>>
>>>>>>> Weird. Exactly this was a part of my fs_info patchset. I guess I need
>>>>>>> to go back and check what else is missing.
>>>>>>
>>>>>> Actually, most of this didn't land. Pretty much anything that's a root
>>>>>> ->fs_info conversion is in there.
>>>>>
>>>>> Only half of the patchset has been merged so far because it did not pass
>>>>> testing, so I bisected to some point. I was about to let you know once
>>>>> most of 4.9 patches are prepared so there are less merge conflicts.
>>>>
>>>> Ok, thanks. I was going to start the rebase today but I'll hold off
>>>> until you're set for 4.9.
>>>>
>>> Hi Jeff, Could you please share your patch? Where can i get it?
>>> I wanna have a look at it.
>>
>> Sure, it's the whole series that starts with this commit:
>> commit 160ceedfd40085cfb1e08305917fcc24cefdad93
>> Author: Jeff Mahoney <jeffm@suse.com>
>> Date: Wed Aug 31 23:55:33 2016 -0400
>>
>> btrfs: add dynamic debug support
>>
>> ... I still need to do clean up some commits that need merging.
>>
>> https://git.kernel.org/cgit/linux/kernel/git/jeffm/linux-btrfs.git/log/?h=btrfs-testing/kdave/misc-4.9/root-fsinfo-cleanup
>>
> Nice work. Thanks for your explaination.
> I have one more question about it.
> Although the total text size of this function is not changed, using
> fs_info should need one more instruction to get chunk_root field of
> fs_info than using chunk_root directly.
> Does it cause any performance impact?
I doubt it. The caller had to do the dereference before.
-Jeff
> Sean
>> -Jeff
>>
>>
>>> Thanks
>>>> -Jeff
>>>>
>>>> --
>>>> Jeff Mahoney
>>>> SUSE Labs
>>>>
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>> --
>> Jeff Mahoney
>> SUSE Labs
>>
>
>
>
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-05 7:56 ` Qu Wenruo
2016-09-06 2:41 ` Zhao Lei
2016-09-06 3:05 ` Jeff Mahoney
@ 2016-09-07 1:38 ` Sean Fu
2016-09-07 1:56 ` Qu Wenruo
2 siblings, 1 reply; 15+ messages in thread
From: Sean Fu @ 2016-09-07 1:38 UTC (permalink / raw)
To: Qu Wenruo
Cc: Zhao Lei, dsterba, clm, anand.jain, fdmanana, linux-btrfs, linux-kernel
On Mon, Sep 05, 2016 at 03:56:41PM +0800, Qu Wenruo wrote:
>
>
> At 09/05/2016 09:19 AM, Zhao Lei wrote:
> >Hi, Sean Fu
> >
> >>From: Sean Fu [mailto:fxinrong@gmail.com]
> >>Sent: Sunday, September 04, 2016 7:54 PM
> >>To: dsterba@suse.com
> >>Cc: clm@fb.com; anand.jain@oracle.com; fdmanana@suse.com;
> >>zhaolei@cn.fujitsu.com; linux-btrfs@vger.kernel.org;
> >>linux-kernel@vger.kernel.org; Sean Fu <fxinrong@gmail.com>
> >>Subject: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in
> >>btrfs_read_chunk_tree.
> >>
> >>The input argument root is already set with "fs_info->chunk_root".
> >>"chunk_root = fs_info->chunk_root = btrfs_alloc_root(fs_info)" in caller
> >>"open_ctree".
> >>“root->fs_info = fs_info” in "btrfs_alloc_root".
> >>
> >The root argument of this function means "any root".
> >And the function is designed getting chunk root from
> >"any root" in head.
> >
> >Since there is only one caller of this function,
> >and the caller always send chunk_root as root argument in
> >current code, we can remove above conversion,
> >and I suggest renaming root to chunk_root to make it clear,
> >something like:
> >
> >- btrfs_read_chunk_tree(struct btrfs_root *root)
> >+ btrfs_read_chunk_tree(struct btrfs_root *chunk_root)
>
> Since root is only used to get fs_info->chunk_root, why not use fs_info
> directly?
Sorry for late reply.
chunk_root is processed in btrfs_read_chunk_tree.
Why should we pass fs_info directly to btrfs_read_chunk_tree?
Could you give me more detail?
Many thanks
>
> Thanks,
> Qu
>
> >
> >Thanks
> >Zhaolei
> >
> >>Signed-off-by: Sean Fu <fxinrong@gmail.com>
> >>---
> >> fs/btrfs/volumes.c | 2 --
> >> 1 file changed, 2 deletions(-)
> >>
> >>diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >>index 366b335..384a6d2 100644
> >>--- a/fs/btrfs/volumes.c
> >>+++ b/fs/btrfs/volumes.c
> >>@@ -6600,8 +6600,6 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
> >> int ret;
> >> int slot;
> >>
> >>- root = root->fs_info->chunk_root;
> >>-
> >> path = btrfs_alloc_path();
> >> if (!path)
> >> return -ENOMEM;
> >>--
> >>2.6.2
> >>
> >
> >
> >
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in btrfs_read_chunk_tree.
2016-09-07 1:38 ` Sean Fu
@ 2016-09-07 1:56 ` Qu Wenruo
0 siblings, 0 replies; 15+ messages in thread
From: Qu Wenruo @ 2016-09-07 1:56 UTC (permalink / raw)
To: Zhao Lei, dsterba, clm, anand.jain, fdmanana, linux-btrfs, linux-kernel
At 09/07/2016 09:38 AM, Sean Fu wrote:
> On Mon, Sep 05, 2016 at 03:56:41PM +0800, Qu Wenruo wrote:
>>
>>
>> At 09/05/2016 09:19 AM, Zhao Lei wrote:
>>> Hi, Sean Fu
>>>
>>>> From: Sean Fu [mailto:fxinrong@gmail.com]
>>>> Sent: Sunday, September 04, 2016 7:54 PM
>>>> To: dsterba@suse.com
>>>> Cc: clm@fb.com; anand.jain@oracle.com; fdmanana@suse.com;
>>>> zhaolei@cn.fujitsu.com; linux-btrfs@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; Sean Fu <fxinrong@gmail.com>
>>>> Subject: [PATCH] Btrfs: remove unnecessary code of chunk_root assignment in
>>>> btrfs_read_chunk_tree.
>>>>
>>>> The input argument root is already set with "fs_info->chunk_root".
>>>> "chunk_root = fs_info->chunk_root = btrfs_alloc_root(fs_info)" in caller
>>>> "open_ctree".
>>>> “root->fs_info = fs_info” in "btrfs_alloc_root".
>>>>
>>> The root argument of this function means "any root".
>>> And the function is designed getting chunk root from
>>> "any root" in head.
>>>
>>> Since there is only one caller of this function,
>>> and the caller always send chunk_root as root argument in
>>> current code, we can remove above conversion,
>>> and I suggest renaming root to chunk_root to make it clear,
>>> something like:
>>>
>>> - btrfs_read_chunk_tree(struct btrfs_root *root)
>>> + btrfs_read_chunk_tree(struct btrfs_root *chunk_root)
>>
>> Since root is only used to get fs_info->chunk_root, why not use fs_info
>> directly?
> Sorry for late reply.
> chunk_root is processed in btrfs_read_chunk_tree.
> Why should we pass fs_info directly to btrfs_read_chunk_tree?
> Could you give me more detail?
>
> Many thanks
Normally we should only pass btrfs_root as parameter if it's a
file/log/relocation tree which can't be grabbed directly from fs_info.
For system wide trees, which are already in fs_info, like
fs_info->extent_root/chunk_root/..., we should pass fs_info.
Which is much much safer than passing a btrfs_root.
Careless caller can pass wrong tree and cause undefined behavior.
And such behavior makes caller more aware of what they really want to do.
Cases like just to grab sectorsize/nodesize shouldn't need a full
btrfs_root.
(Jeff's patchset has already done such things quite well)
Thanks,
Qu
>>
>> Thanks,
>> Qu
>>
>>>
>>> Thanks
>>> Zhaolei
>>>
>>>> Signed-off-by: Sean Fu <fxinrong@gmail.com>
>>>> ---
>>>> fs/btrfs/volumes.c | 2 --
>>>> 1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index 366b335..384a6d2 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -6600,8 +6600,6 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
>>>> int ret;
>>>> int slot;
>>>>
>>>> - root = root->fs_info->chunk_root;
>>>> -
>>>> path = btrfs_alloc_path();
>>>> if (!path)
>>>> return -ENOMEM;
>>>> --
>>>> 2.6.2
>>>>
>>>
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread