* Re: [PATCH] f2fs: fix indefinite loop scanning for free nid
@ 2020-08-18 9:55 ` Sahitya Tummala
0 siblings, 0 replies; 10+ messages in thread
From: Sahitya Tummala @ 2020-08-18 9:55 UTC (permalink / raw)
To: Chao Yu; +Cc: Jaegeuk Kim, linux-f2fs-devel, linux-kernel, stummala
On Tue, Aug 18, 2020 at 04:29:05PM +0800, Chao Yu wrote:
> On 2020/8/14 16:05, Sahitya Tummala wrote:
> >If the sbi->ckpt->next_free_nid is not NAT block aligned and if there
> >are free nids in that NAT block between the start of the block and
> >next_free_nid, then those free nids will not be scanned in scan_nat_page().
> >This results into mismatch between nm_i->available_nids and the sum of
> >nm_i->free_nid_count of all NAT blocks scanned. And nm_i->available_nids
> >will always be greater than the sum of free nids in all the blocks.
> >Under this condition, if we use all the currently scanned free nids,
> >then it will loop forever in f2fs_alloc_nid() as nm_i->available_nids
> >is still not zero but nm_i->free_nid_count of that partially scanned
> >NAT block is zero.
> >
> >Fix this to align the nm_i->next_scan_nid to the first nid of the
> >corresponding NAT block.
> >
> >Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> >---
> > fs/f2fs/node.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> >diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> >index 9bbaa26..d615e59 100644
> >--- a/fs/f2fs/node.c
> >+++ b/fs/f2fs/node.c
> >@@ -2402,6 +2402,8 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
> > if (IS_ERR(page)) {
> > ret = PTR_ERR(page);
> > } else {
> >+ if (nid % NAT_ENTRY_PER_BLOCK)
> >+ nid = NAT_BLOCK_OFFSET(nid) * NAT_ENTRY_PER_BLOCK;
>
> How about moving this logic to the beginning of __f2fs_build_free_nids(),
> after nid reset?
>
Sure, I will move it.
> BTW, it looks we can add unlikely in this judgment condition?
But it may not be an unlikely as it can happen whenever checkpoint is done,
based on the next available free nid in function next_free_nid(), which can happen
quite a few times, right?
Hitting the loop forever issue condition due to this could be a rare/difficult to
reproduce but this check itself may not be unlikely in my opinion.
Thanks,
>
> Thanks,
>
> > ret = scan_nat_page(sbi, page, nid);
> > f2fs_put_page(page, 1);
> > }
> >
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: fix indefinite loop scanning for free nid
2020-08-18 9:55 ` Sahitya Tummala
@ 2020-08-18 10:01 ` Chao Yu
-1 siblings, 0 replies; 10+ messages in thread
From: Chao Yu @ 2020-08-18 10:01 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: Jaegeuk Kim, linux-kernel, linux-f2fs-devel
On 2020/8/18 17:55, Sahitya Tummala wrote:
> On Tue, Aug 18, 2020 at 04:29:05PM +0800, Chao Yu wrote:
>> On 2020/8/14 16:05, Sahitya Tummala wrote:
>>> If the sbi->ckpt->next_free_nid is not NAT block aligned and if there
>>> are free nids in that NAT block between the start of the block and
>>> next_free_nid, then those free nids will not be scanned in scan_nat_page().
>>> This results into mismatch between nm_i->available_nids and the sum of
>>> nm_i->free_nid_count of all NAT blocks scanned. And nm_i->available_nids
>>> will always be greater than the sum of free nids in all the blocks.
>>> Under this condition, if we use all the currently scanned free nids,
>>> then it will loop forever in f2fs_alloc_nid() as nm_i->available_nids
>>> is still not zero but nm_i->free_nid_count of that partially scanned
>>> NAT block is zero.
>>>
>>> Fix this to align the nm_i->next_scan_nid to the first nid of the
>>> corresponding NAT block.
>>>
>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>> ---
>>> fs/f2fs/node.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index 9bbaa26..d615e59 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -2402,6 +2402,8 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
>>> if (IS_ERR(page)) {
>>> ret = PTR_ERR(page);
>>> } else {
>>> + if (nid % NAT_ENTRY_PER_BLOCK)
>>> + nid = NAT_BLOCK_OFFSET(nid) * NAT_ENTRY_PER_BLOCK;
>>
>> How about moving this logic to the beginning of __f2fs_build_free_nids(),
>> after nid reset?
>>
>
> Sure, I will move it.
>
>> BTW, it looks we can add unlikely in this judgment condition?
>
> But it may not be an unlikely as it can happen whenever checkpoint is done,
> based on the next available free nid in function next_free_nid(), which can happen
> quite a few times, right?
Oh, yes, I missed that place, please ignore that suggestion.. :)
Thanks,
>
> Hitting the loop forever issue condition due to this could be a rare/difficult to
> reproduce but this check itself may not be unlikely in my opinion.
>
> Thanks,
>
>>
>> Thanks,
>>
>>> ret = scan_nat_page(sbi, page, nid);
>>> f2fs_put_page(page, 1);
>>> }
>>>
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] f2fs: fix indefinite loop scanning for free nid
@ 2020-08-18 10:01 ` Chao Yu
0 siblings, 0 replies; 10+ messages in thread
From: Chao Yu @ 2020-08-18 10:01 UTC (permalink / raw)
To: Sahitya Tummala; +Cc: Jaegeuk Kim, linux-f2fs-devel, linux-kernel
On 2020/8/18 17:55, Sahitya Tummala wrote:
> On Tue, Aug 18, 2020 at 04:29:05PM +0800, Chao Yu wrote:
>> On 2020/8/14 16:05, Sahitya Tummala wrote:
>>> If the sbi->ckpt->next_free_nid is not NAT block aligned and if there
>>> are free nids in that NAT block between the start of the block and
>>> next_free_nid, then those free nids will not be scanned in scan_nat_page().
>>> This results into mismatch between nm_i->available_nids and the sum of
>>> nm_i->free_nid_count of all NAT blocks scanned. And nm_i->available_nids
>>> will always be greater than the sum of free nids in all the blocks.
>>> Under this condition, if we use all the currently scanned free nids,
>>> then it will loop forever in f2fs_alloc_nid() as nm_i->available_nids
>>> is still not zero but nm_i->free_nid_count of that partially scanned
>>> NAT block is zero.
>>>
>>> Fix this to align the nm_i->next_scan_nid to the first nid of the
>>> corresponding NAT block.
>>>
>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>> ---
>>> fs/f2fs/node.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index 9bbaa26..d615e59 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -2402,6 +2402,8 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
>>> if (IS_ERR(page)) {
>>> ret = PTR_ERR(page);
>>> } else {
>>> + if (nid % NAT_ENTRY_PER_BLOCK)
>>> + nid = NAT_BLOCK_OFFSET(nid) * NAT_ENTRY_PER_BLOCK;
>>
>> How about moving this logic to the beginning of __f2fs_build_free_nids(),
>> after nid reset?
>>
>
> Sure, I will move it.
>
>> BTW, it looks we can add unlikely in this judgment condition?
>
> But it may not be an unlikely as it can happen whenever checkpoint is done,
> based on the next available free nid in function next_free_nid(), which can happen
> quite a few times, right?
Oh, yes, I missed that place, please ignore that suggestion.. :)
Thanks,
>
> Hitting the loop forever issue condition due to this could be a rare/difficult to
> reproduce but this check itself may not be unlikely in my opinion.
>
> Thanks,
>
>>
>> Thanks,
>>
>>> ret = scan_nat_page(sbi, page, nid);
>>> f2fs_put_page(page, 1);
>>> }
>>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix indefinite loop scanning for free nid
2020-08-18 9:55 ` Sahitya Tummala
@ 2020-08-18 10:04 ` Sahitya Tummala
-1 siblings, 0 replies; 10+ messages in thread
From: Sahitya Tummala @ 2020-08-18 10:04 UTC (permalink / raw)
To: Chao Yu; +Cc: Jaegeuk Kim, linux-kernel, linux-f2fs-devel
On Tue, Aug 18, 2020 at 03:25:47PM +0530, Sahitya Tummala wrote:
> On Tue, Aug 18, 2020 at 04:29:05PM +0800, Chao Yu wrote:
> > On 2020/8/14 16:05, Sahitya Tummala wrote:
> > >If the sbi->ckpt->next_free_nid is not NAT block aligned and if there
> > >are free nids in that NAT block between the start of the block and
> > >next_free_nid, then those free nids will not be scanned in scan_nat_page().
> > >This results into mismatch between nm_i->available_nids and the sum of
> > >nm_i->free_nid_count of all NAT blocks scanned. And nm_i->available_nids
> > >will always be greater than the sum of free nids in all the blocks.
> > >Under this condition, if we use all the currently scanned free nids,
> > >then it will loop forever in f2fs_alloc_nid() as nm_i->available_nids
> > >is still not zero but nm_i->free_nid_count of that partially scanned
> > >NAT block is zero.
> > >
> > >Fix this to align the nm_i->next_scan_nid to the first nid of the
> > >corresponding NAT block.
> > >
> > >Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > >---
> > > fs/f2fs/node.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > >diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > >index 9bbaa26..d615e59 100644
> > >--- a/fs/f2fs/node.c
> > >+++ b/fs/f2fs/node.c
> > >@@ -2402,6 +2402,8 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
> > > if (IS_ERR(page)) {
> > > ret = PTR_ERR(page);
> > > } else {
> > >+ if (nid % NAT_ENTRY_PER_BLOCK)
> > >+ nid = NAT_BLOCK_OFFSET(nid) * NAT_ENTRY_PER_BLOCK;
> >
> > How about moving this logic to the beginning of __f2fs_build_free_nids(),
> > after nid reset?
> >
>
> Sure, I will move it.
>
> > BTW, it looks we can add unlikely in this judgment condition?
>
> But it may not be an unlikely as it can happen whenever checkpoint is done,
> based on the next available free nid in function next_free_nid(), which can happen
> quite a few times, right?
>
> Hitting the loop forever issue condition due to this could be a rare/difficult to
> reproduce but this check itself may not be unlikely in my opinion.
>
Sorry, I was wrong above. During CP we update only ckpt->next_free_nid but not
the nm_i->next_free_nid, which is done only once during boot up.
So yes, I will mark it as unlikely conditiona.
Thanks,
> Thanks,
>
> >
> > Thanks,
> >
> > > ret = scan_nat_page(sbi, page, nid);
> > > f2fs_put_page(page, 1);
> > > }
> > >
>
> --
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] f2fs: fix indefinite loop scanning for free nid
@ 2020-08-18 10:04 ` Sahitya Tummala
0 siblings, 0 replies; 10+ messages in thread
From: Sahitya Tummala @ 2020-08-18 10:04 UTC (permalink / raw)
To: Chao Yu; +Cc: Jaegeuk Kim, linux-f2fs-devel, linux-kernel, stummala
On Tue, Aug 18, 2020 at 03:25:47PM +0530, Sahitya Tummala wrote:
> On Tue, Aug 18, 2020 at 04:29:05PM +0800, Chao Yu wrote:
> > On 2020/8/14 16:05, Sahitya Tummala wrote:
> > >If the sbi->ckpt->next_free_nid is not NAT block aligned and if there
> > >are free nids in that NAT block between the start of the block and
> > >next_free_nid, then those free nids will not be scanned in scan_nat_page().
> > >This results into mismatch between nm_i->available_nids and the sum of
> > >nm_i->free_nid_count of all NAT blocks scanned. And nm_i->available_nids
> > >will always be greater than the sum of free nids in all the blocks.
> > >Under this condition, if we use all the currently scanned free nids,
> > >then it will loop forever in f2fs_alloc_nid() as nm_i->available_nids
> > >is still not zero but nm_i->free_nid_count of that partially scanned
> > >NAT block is zero.
> > >
> > >Fix this to align the nm_i->next_scan_nid to the first nid of the
> > >corresponding NAT block.
> > >
> > >Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> > >---
> > > fs/f2fs/node.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > >diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > >index 9bbaa26..d615e59 100644
> > >--- a/fs/f2fs/node.c
> > >+++ b/fs/f2fs/node.c
> > >@@ -2402,6 +2402,8 @@ static int __f2fs_build_free_nids(struct f2fs_sb_info *sbi,
> > > if (IS_ERR(page)) {
> > > ret = PTR_ERR(page);
> > > } else {
> > >+ if (nid % NAT_ENTRY_PER_BLOCK)
> > >+ nid = NAT_BLOCK_OFFSET(nid) * NAT_ENTRY_PER_BLOCK;
> >
> > How about moving this logic to the beginning of __f2fs_build_free_nids(),
> > after nid reset?
> >
>
> Sure, I will move it.
>
> > BTW, it looks we can add unlikely in this judgment condition?
>
> But it may not be an unlikely as it can happen whenever checkpoint is done,
> based on the next available free nid in function next_free_nid(), which can happen
> quite a few times, right?
>
> Hitting the loop forever issue condition due to this could be a rare/difficult to
> reproduce but this check itself may not be unlikely in my opinion.
>
Sorry, I was wrong above. During CP we update only ckpt->next_free_nid but not
the nm_i->next_free_nid, which is done only once during boot up.
So yes, I will mark it as unlikely conditiona.
Thanks,
> Thanks,
>
> >
> > Thanks,
> >
> > > ret = scan_nat_page(sbi, page, nid);
> > > f2fs_put_page(page, 1);
> > > }
> > >
>
> --
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 10+ messages in thread